feat(crews): a Crew thread's provider approvals and questions reach the Inbox API - #308
bryantderosier wants to merge 7 commits into
Conversation
…he Inbox API Captains and seats run while the person is elsewhere, but their providers' tool approvals and user-input questions only rendered inline in the thread's composer, so a Crew stalled on a prompt nobody saw. A J5-owned CrewRuntimeRequestService reads the pending approvals and questions on every live Crew's Captain and seat threads straight from their projections, with the same selection the composer uses, and answers them through runtime-request.respond. Two J5 routes serve it: a list under read scope and an answer under operate scope. An answer to a request that already resolved, including one answered inline on another device, is refused with 409 and changes nothing. A thread outside every live Crew is never listed or answered, and a retired Crew's requests leave the list. The web Inbox section is #264. Closes #263 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: Jacksondr5/j5code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: Comment |
Found in the seeded dev-server pass: a refused answer read "Failed to dispatch orchestration command runtime-request.respond (<command id>)", hiding why. The refusal now carries the orchestrator's own reason, such as "Runtime request ... is resolved" or "Provider session ... was not found". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The Crew runtime request tests this branch adds still called NodeSqliteClient.layerMemory(), which the sync replaced with NodeSqliteClient.layer({ filename: ":memory:" }).
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…nbox lists From review on #308: - A thread read goes through getThreadProjectionIfPresent: only a genuine not-found or an archived thread reads as absent. A store failure or a listLive SQL failure now propagates, so list returns 500 instead of an empty Inbox and respond no longer reports a live request as not found. - respond looks the request up through the same selector list uses, so auth_refresh, dynamic_tool_call, and a question without its item are never answered, and a request that is not live is refused with the way out. - A dispatch refusal with the decider's string reason is a 409; any other dispatch failure is a new CrewRuntimeRequestDispatchError the route logs as a 500. - list reads a Crew's threads concurrently. - The selector moves to J5-owned packages/shared/src/j5/crewRuntimeRequests.ts and matches the composer's derivePendingThreadRequests (an approval is answerable only when live; a question answered by message reads as message; multiSelect defaults to false). A client-runtime test runs both selectors over every branch and fails if they drift. - The test mocks fail the way the real services do. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…rest inline With #308's selector matching the composer, a request can be listed that the Inbox cannot answer: a question answered by sending a message, or one that is no longer resumable. The Inbox now offers answers only for live requests and points to the thread for the rest, and the composer keeps those requests inline instead of handing them to an Inbox that cannot answer them. FORK.md case 46 records the shared selector and the client-runtime test that holds it to derivePendingThreadRequests. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@tyler-barton-horizon Thanks for the careful review. All eight points were real, and I've addressed each one in its thread and resolved it. In short:
CI is running on both branches now. |
| | `packages/effect-codex-app-server/src/schema.test.ts` | N | G | | ||
| | `packages/effect-codex-app-server/test/fixtures/codex-app-server-mock-peer.ts` | R | G | | ||
| | `packages/shared/package.json` | N | 43 | | ||
| | `packages/shared/package.json` | N | 43, 46 | |
There was a problem hiding this comment.
[Review panel: Opus 5.5 + Astra] FORK.md: this row points at a case that doesn't exist in this PR.
#308 changes the packages/shared/package.json row to 43, 46, but case 46's text only arrives in #309. If #308 lands alone, the row cites a missing case. Worse, per the known collision: if #302 lands between #308 and #309 and takes case 46, this row then points at the Captains case. Nobody will notice, because #309's renumbering pass only touches rows #309 itself edits.
Fix: give #308 its own short case covering what it adds to upstream-owned files (the ./j5/crewRuntimeRequests export in packages/shared/package.json and the parity test's dependency on derivePendingThreadRequests), and have #309 extend that case for ChatView.tsx. Alternatively, if you take the simplification in my other comment, the shared/package.json edit goes away and so does this row change.
| const liveCrewThreads = crews.listLive().pipe( | ||
| Effect.map((live) => | ||
| live.flatMap((crew): Array<CrewThread> => [ | ||
| { threadId: crew.captainThreadId, crew, seat: null }, |
There was a problem hiding this comment.
[Review panel: Opus 5.5 + Astra] A Captain's request is listed once for every live Crew it commands.
A Captain can command several live Crews at once (docs/j5/product/features/crews.md:29, "One Captain, many Crews"). liveCrewThreads adds the Captain's thread once per Crew, so with two live Crews under one Captain, list returns every pending Captain request twice, differing only in crewInstanceId/crewName. In #309 this means:
- two cards with the same
itemKey(env:thread:request), which gives duplicate React keys and a shared answer draft; - the bell counts the request twice;
- answering one card resolves the request, and answering the other gets a 409.
Fix: emit each Captain thread once (dedupe by threadId). A Captain's request isn't about any single Crew, so either list it with the first Crew or drop the Crew name for Captain rows. Add a test with two live Crews under one Captain that asserts one item.
|
Decision (Jackson, 2026-09-26): slim #308/#309 to seat approvals only. The Captain is the thread the person watches by design, so the Captain's approvals and questions stay inline in its own thread. Seats are what the person isn't watching, and seats will no longer have native question tools; they ask their Captain instead (#325). What's left for the Inbox is seat approvals. For #308 that means:
Related: custom seats will default to Full access and the roster card will warn about seats with limited access (#326), so seat approvals should be the exception rather than the norm. |
Important
#309 is based on #308, so #308 merges first. Neither has migrations or ordering against the other Crews PRs.
Problem
When a thread is a Crew Captain or a live seat, every approval or question its provider raises (tool approvals, and user-input questions like Claude's AskUserQuestion or Codex's request_user_input) renders only in that thread's composer. Captains and seats run while I'm elsewhere, so those prompts sit unseen and the Crew stalls (#263). My rule: anything a Captain or seat needs from me goes to my Inbox; the initial roster card is the one exception.
What I changed
apps/server/src/j5/a2a/CrewRuntimeRequestService.ts(new):listreads the pending requests on every live Crew's Captain and seat threads straight from their projections.pendingCrewThreadRequestsdoes the selection, mirroring client-runtime'sderivePendingThreadRequests, which the server can't import: pending only, questions with their item, and noauth_refreshordynamic_tool_call.respondchecks that the thread belongs to a live Crew and the request is still pending, then dispatchesruntime-request.respond.apps/server/src/j5/a2a/CrewRuntimeRequestsHttp.ts(new):POST /api/j5/a2a/crews/runtime-requests(read scope) and.../respond(operate scope). 404 for anything outside a live Crew, 409 once resolved, 400 for the wrong answer kind.packages/contracts/src/j5.ts:CrewRuntimeRequestItem, the list response, and the respond request and response; both paths are added toJ5_API_PATHS.j5/a2a/runtimeLayer.ts, the routes inJ5AuthenticatedRoutes.ts, and the two aggregate tests gain a service mock.After Tyler's review
getThreadProjectionIfPresent, so only a real not-found or an archived thread reads as absent. A store orlistLivefailure now propagates:listreturns a logged 500 instead of an empty Inbox, andrespondno longer calls a live request not found.respondlooks requests up through the same selector aslist. Hidden kinds and questions without their item are never answered, and a request that isn'tliveis refused with the way out.CrewRuntimeRequestDispatchError, a logged 500.listreads a Crew's threads concurrently. The narrower read is Crew request polls read only pending runtime requests #324.packages/shared/src/j5/crewRuntimeRequests.tsand matchesderivePendingThreadRequests.packages/client-runtime/src/j5/crewRuntimeRequests.test.tsruns both over every branch and fails if they drift.Why this shape
I serve this as its own Inbox source rather than as ledger Exchanges:
HumanInboxService.answerwrites an A2A reply, not a provider response. I read live state from the projection rather than copying it into a table, so there's nothing to keep in sync and a resolved or retired request just stops appearing. Each answer gets a fresh command id, so a second answer is refused by the request's own state instead of being silently deduplicated.Invariants
Surfaces
packages/contracts)Out of scope
Upgrade and data
None. No table, no migration.
Verification
CrewRuntimeRequestService.test.tsuses the real Crew store and a thread mock that resolves a request once, as the orchestrator does. It covers:CrewRuntimeRequestsHttp.test.ts: readers list, readers get 403 on answer, operators 200 then 409, plus 404 and 400.434e1f47ef, found in feat(crews): answer a Crew thread's provider approvals and questions from the Inbox #309's seeded dev-server pass, where the refusal only named the command id).vp test run apps/server/src/j5: 686 passed at open; the J5 a2a suite passes after the fix. Server typecheck clean.Review focus
respondreads, checks pending, then dispatches, and the orchestrator rechecks pending. Is the 409 mapping right for every dispatch error, or should some be 500?listwalks every live Crew's threads on each poll; there are usually a handful, but check the cost assumption.Closes #263
Claude Opus 5.5 via Claude Code
🤖 Generated with Claude Code