Review + integrate all open PRs (#29, #31, #33, #34) and issues (#26, #32) - #35
Merged
Merged
Conversation
…, deadlocks, merge/broadcast accuracy ## Summary Six independent fixes found while stress-testing the ensemble plugin's teammate lifecycle under real, extended multi-teammate sessions: a re-engagement gap on completed teammates, two stall-detection gaps on a teammate's first action, an orphaned-team reconciliation gap plus a deadlock its own fix introduced, a false-success report from `team_merge`, and a missing chatty-detection wire on `team_broadcast`. Each was found via live reproduction against a real running server, not inferred from code reading alone. ### `team_message` cannot re-engage a teammate that already reported completion Ensemble intentionally locks a teammate from being woken again once it reports completion to the lead (prevents a courtesy-reply ping-pong loop), but exposed no way to deliberately re-engage one for legitimate follow-on work — e.g. a later stage of an extended exchange with the same teammate. Adds a lead-only `force: true` param on `team_message` that bypasses the lock. The natural busy transition already resets the completion flag on the teammate's next idle, so `force` doesn't need its own separate reset path. ### Stall detection has zero coverage for a teammate's first action `isTimeStalled` requires at least one prior step-finish event to establish a baseline — a teammate blocked on its first (or only) long-running tool call never accumulates that baseline before a hard timeout, leaving the most likely stall pattern in practice (a long build, test, or file operation as a first action) completely undetected. Fixed in two passes: first wiring `recordBusyStart()` from the `ready/error -> busy` status-transition event, then discovering live that a freshly spawned member is inserted directly as `status='busy'` with no transition event to hook — so `recordBusyStart()` is now also called directly at spawn time. ### Orphaned-team reconciliation, and the deadlock its own fix introduced A team stays `active` forever if its lead session is deleted outside `team_cleanup` (e.g. via the host application's own session UI), blocking `team_create`/`team_cleanup` for that name indefinitely with no recovery path. Adds a periodic sweep plus an on-demand check in `team_create`'s duplicate-name check (the periodic sweep alone only runs at plugin init, missing the case where the bug is hit mid-session). The first version of this fix awaited the sweep synchronously during plugin init; its liveness check makes an HTTP call back to the same server, which — before the server finishes bootstrapping — hangs indefinitely and takes the whole instance down with it. Fixed by making the sweep fire-and-forget, matching the existing pattern for non-critical recovery passes, plus a bounded timeout on the liveness check itself as defense in depth. ### `recoverStaleMembers`'s own pre-existing instance of the same deadlock class Once the above was found, `recoverStaleMembers` — which predates this batch of fixes entirely — turned out to have the identical shape: awaited synchronously at init, with an unbounded `session.abort()` call back to the same server. It hadn't fired in practice only because its query happens to filter on a genuinely-busy member at restart time, not because of any structural guard. Fixed with a bounded timeout on the abort call, kept synchronous (unlike the fix above) because a later init step depends on this function completing first. ### `team_merge` reports success when there was nothing to merge A teammate that writes a file but never commits leaves its branch with zero new commits. `git merge --squash` against such a branch is a legitimate no-op that `team_merge` reported as a successful merge regardless — technically accurate, actively misleading: nothing was captured, and the uncommitted work is permanently lost the moment the worktree is removed, with no signal at that exact point. Now checks commit count before merging: zero commits with a dirty worktree produces an explicit warning naming the worktree path and leaves the branch/ worktree in place for manual recovery instead of attempting a merge or a delete. ### `team_broadcast` invisible to chatty-teammate detection The chatty-detection rate limiter tracks peer-message activity via a call that `team_message`'s handler makes but `team_broadcast`'s handler never did — broadcasting to the entire team, arguably the most "chatty" action possible, was completely invisible to the rate limiter. ## Tests added New and updated tests across `team-spawn`, `team-message`, `team-create`, `team-merge`, `recovery`, and `progress` covering: force re-engagement of a completed teammate (and rejection when a non-lead attempts it), stall-baseline recording at both the transition-event and spawn-time hook points, orphaned-team reconciliation via both the periodic sweep and the on-demand check, the fire-and-forget + bounded-timeout behavior for both deadlock-prone recovery passes, honest-vs-misleading `team_merge` reporting across the zero-commit/ dirty-worktree and zero-commit/clean-worktree cases, and broadcast activity now registering with the chatty-detection tracker. ## Verification - `bun run typecheck` — clean - `bun test` — 715 pass, 0 fail - `bun run build` — clean Each fix was additionally live-reverified against a real running server and real spawned teammates reproducing the exact failure mode described above, not just against the added unit tests.
…2,3,4,6 + schema) Based on bugfix/teammate-lifecycle-robustness (this PR) — needs ProgressTracker.recordBusyStart()/hasReportedCompletion() from that branch; does not stand alone on main. - schema.ts: Migration 10 (this branch's migration count differs from local-integration's), additive last_nudged_at INTEGER on team_member. No CHECK constraint touched — dedicated test asserts the 5-literal status enum string is unchanged. - watchdog.ts: checkStalled()/checkChatty() write last_nudged_at at nudge-decision time; both now gate on hasReportedCompletion() before nudging, with a distinguishable skip-path log line; checkStalled() defers markReported() until promptAsync's delivery is confirmed (its .then(), not unconditionally before the call) so a failed delivery no longer permanently orphans the stall state. - index.ts: idle-without-report nudge's previously-silent promptAsync .catch() now logs a structured failure, matching this branch's other nudge paths. - team-status.ts: the 30s-per-team throttle no longer returns a hardcoded 'no changes' string unconditionally — computes a cheap per-team MAX(time_updated/time_created) marker and only short-circuits when nothing actually changed. 726/726 tests (up from this branch's own 720), typecheck clean, build clean. Companion dashboard-layer change (exposes last_nudged_at as lastNudgedAt in /api/state) is a separate commit against feat/dashboard-state-version-clean — that branch depends on this one's migration and should merge after it.
… (backend half) Based on this PR's branch -- needs no additional dependency beyond what's already here. session.status already has a "retry" case in handleSessionStatusEvent (fires a toast) -- the payload was thrown away immediately after. This persists it instead: - Migration 10: four additive, nullable columns on team_member (retry_until, retry_attempt, retry_provider, retry_message). No CHECK constraint change, no new status/execution_status literal. - handleSessionStatusEvent gains an optional retryPayload param; the existing retry branch persists the four columns in addition to the existing toast, not instead of it. status/execution_status untouched. - No hardcoded rate-limit/throttle language: retry_message prefers action.message, falls back to the generic status.message. - retry_until is a TTL; "currently retrying" is a derived, read-time boolean, never a stored enum -- no clear-write anywhere. - team-status.ts's status line gets an additive annotation for a retrying member, same pattern as the existing nudged-timestamp one. status.next resolved as an absolute epoch-ms timestamp (verified against the opencode server bundle's session.retry.scheduled handler and the built-in TUI's own countdown math). Non-goals: no watchdog/stall-detection changes -- checkStalled()/ ProgressTracker.isTimeStalled() never see this signal, by design. 727/727 tests, typecheck/build clean. Companion change surfacing this via /api/state and the dashboard UI is a separate commit against hueyexe#33.
Adds a bare-integer version field to the /api/state payload and gives buildState() a real return type (EnsembleDashboardState) instead of an inline anonymous shape. Bump ENSEMBLE_STATE_VERSION on any breaking change; additive fields don't require a bump. No changes to teammate-lifecycle logic. Motivated by an external consumer (an OpenCode sidebar TUI plugin) that polls this endpoint and needs exact-match version detection to degrade gracefully instead of crashing on shape drift.
Additive field -- team already carries lead_session_id in the DB schema (NOT NULL since migration 1), buildState() just never selected or exposed it. Lets an external consumer scope teams to "created by this specific session" rather than only "this project directory" -- the current sidebar TUI plugin's project-only scoping still mixes multiple sessions' teams together within one project. No ENSEMBLE_STATE_VERSION bump -- purely additive per the version field's own documented policy (field removed/renamed/retyped needs a bump, new field doesn't).
…api/state Depends on the last_nudged_at column added by the watchdog/nudge-staleness fix (separate PR) — this branch's tests will fail standalone until that migration is present. Confirmed: only failure mode is 'no such column: last_nudged_at' (11 fail/10 errors, all traceable to the missing column, nothing else broken). Merge order: land the watchdog PR first, or rebase this on top of it.
… + UI Depends on the retry_until/retry_attempt/retry_provider/retry_message columns added by the companion backend PR (against hueyexe#31) -- this branch's tests will fail standalone until that migration is present. Confirmed: the only failure mode is the retry_* columns not existing, nothing else broken. - dashboard.ts: additive isRetrying (derived, read-time TTL boolean -- never a stored enum), retryUntil, retryAttempt, retryProvider, retryMessage on the member row in /api/state. No version bump needed (additive-only, same reasoning as the earlier lastNudgedAt field). - dashboard-js-render.ts: additive "retrying" chip on agent cards and the member drawer, generic label + attempt number, only the actual provider message text if present -- no hardcoded rate-limit/throttle wording anywhere in this file either. Merge order: land the backend PR first, or rebase this on top of it.
Two deterministic causes made task status drift from reality: 1. team_spawn's claim_task was a no-op — it only appended a sentence to the teammate context prompt and never touched the DB, so the task stayed pending/unassigned while a teammate actively worked it (and another teammate could claim it). It now atomically claims the task (only if still pending and unassigned, so it never steals work) and the context message reflects whether the claim succeeded. 2. A teammate's in_progress tasks were never released when the teammate reached a terminal state, leaving them stuck in_progress forever with a dead assignee. With more tasks than teammates, a departed teammate stranded work that nobody reclaimed — matching the intermittent report. Add releaseMemberTasks() and call it at every terminal-death path: - team_shutdown force/idle abort (preserveAndAbort) - graceful shutdown completion (handleSessionStatusEvent shutdown transition) - watchdog timeout - crash recovery (recoverStaleMembers) - team_spawn rollback on delivery failure releaseMemberTasks only resets in_progress tasks, so it is idempotent and safe to call from overlapping paths. Completed/cancelled/blocked tasks and other members' tasks are untouched.
The lead can now change a teammate's provider/model without shutting it down, respawning, and re-assigning tasks. Pass 'model' (provider/model) to team_message targeting a teammate: - Lead-only, validated as 'provider/model' before any DB write. - 'text' is optional in this mode: omit it to change the model only; include it to also deliver a message on the new model. - Rejects unknown and terminal (shutdown/error) members. Also fixes a latent bug that made this feature meaningless on its own: team_member.model was only applied on the spawn promptAsync call, so later deliveries silently fell back to the session default. A new shared helper getMemberModel() is now threaded into every teammate-directed promptAsync delivery so the stored model actually sticks on subsequent turns: - team_message peer delivery - index.ts wake-peer and nudge-idle deliveries - watchdog stall and chatty nudges - recovery redelivered messages parseModelId is extracted to src/member-model.ts and shared with team_spawn (no behavioral change to spawn). getMemberModel returns undefined when unset/malformed, so delivery paths behave identically to before for teams that never update a model. No new tool, no schema migration.
Two dashboard bugs:
1. A freshly-spawned teammate's card was stuck showing
'[status 00m] [starting]' the entire time it was working, and only
corrected once it went idle. Cause: team_spawn inserts the member as
busy/starting, but the busy status-event handler only advanced
execution_status to 'running' (and bumped time_updated) when
transitioning from ready/error. A member that was already busy at
spawn never got that update, so execution_status stayed 'starting'
and time_updated was frozen until the first idle. Handle the
busy/starting -> running case explicitly (without resetting
reported_to_lead, since it is not a re-activation). This also lets
the active-task line render correctly during the initial busy window.
2. Every entry in the agent detail drawer's activity timeline showed
'19:00:00' (epoch 0 in UTC-5). Cause: parseMessageParts read
info.time as a string (new Date(info.time)), but the SDK shape is
{ created: number }. new Date({...}) -> NaN -> JSON null -> the
frontend rendered new Date(0). Add parseMessageTime() to read
info.time.created, tolerating numeric-epoch and ISO-string shapes,
and never returning NaN.
Tests added for both: hooks busy/starting transition and
parseMessageParts SDK object-shape timestamp.
This was referenced Aug 25, 2026
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
3 participants
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.
Summary
This branch is a full review-and-integration pass over every currently-open PR (#29, #31, #33, #34) and every currently-open issue (#26, #32). Each contribution was reviewed against a real running OpenCode server, reworked where needed, extended where the review surfaced gaps, and landed as a coherent set of commits. Merging this supersedes all four open PRs and closes both open issues.
36 files changed: +2111 / −104 across src and test, no external dependencies added.
Reviewed from open PRs
PR #31 — Teammate lifecycle robustness (
4feb15d,ab4d3bc)Six fixes found by live stress-testing of the lifecycle, plus a follow-up hardening commit on top of the review:
team_messagecan now re-engage a teammate stuck incompleted, closing the re-engagement gapready→busystatus-event for a fresh spawn), so a member stalled before its first step-finish is visible tocheckStalled()team_mergeno longer reports false success when the merge silently failsteam_broadcastnow wires chatty-detection like other send pathslast_nudged_atschema, nudges visible in/api/state)PR #33 — Versioned
/api/statefor external consumers (e8be872,4bab276,d4fc294,95714e8)Landed the typed/versioned state contract, then extended it additively during review:
buildState()returns an exported, documented type; response carries aversionfield so external consumers can detect payload drift instead of crashing on itleadSessionId(already persisted in the DB, never selected) so consumers can scope teams to the session that created themlastNudgedAtand provider-retry signals exposed in both the API payload and dashboard UIPR #34 — Task status + visualization fixes (
276d2bc,f0bc580)src/tasks.tscentralizes claim/release/reassign, so tasks return to the board when a member stalls, errors, or is shut down instead of staying pinned to a dead assignee[starting]with a frozen timestamp — busy transitions bump execution status and time even when the member was inserted already-busyPR #29 — Nullish agent normalization ✅ verified
The core fix (
agent: null | undefined→"build"instead of hitting the NOT NULL constraint) was already merged via #30. This branch retains and extends its regression coverage intest/tools/team-spawn.test.ts; nothing further required.Closes open issues
Issue #26 — Update agent parameters in-place (
009e868)New
src/member-model.ts:team_messageaccepts{ model: "provider/model", to: "<member>" }and hot-swaps that teammate's model without respawning or re-assigning tasks. Includes permission-safe plumbing throughtool.execute.before, DB persistence of the override, and tests.Closes #26
Issue #32 — Dashboard shows 08:00:00 for historical timestamps (
f0bc580)OpenCode v1 exposes message time as an object (
{ created: number }) rather than a bare number; the activity view parsed the object as a number, fell back to epoch-0 rendering, and produced identical08:00:00stamps for historical entries. Timestamp extraction now handles both shapes.Fixes #32
Also in this branch
621387f): newsrc/notify.tsgives any client a direct channel to wake and message the lead;team_spawn's failure rollback path now uses it, so lead notification survives cases where the old in-band message path could not firefb5f7d2): session.status retry signals are persisted (backend half) and surfaced in the dashboard, so provider flapping is distinguishable from genuine stallsec6a737) and doc dedup (89038ea) left over from the integration workTesting
bun run typecheck && bun test && bun run build— all green