feat(server): POST /roster/assistant/propose endpoint + in-memory session store #1219

Merged

charles merged 1 commit from code-lead/1212 into main 2026-05-15 19:58:02 +00:00

Conversation 1 Commits 1 Files changed 6 +1191

code-lead commented 2026-05-15 19:12:10 +00:00

Collaborator

Copy link

Summary

Adds the HTTP surface and session lifecycle for the Agents Roster AI Assistant (spec: specs/agents-roster-ai-assistant.md). The prompt assembly + Claude call live behind the _rosterAssistantProposer.propose injection seam so the prompt ticket can land in parallel — production wires the real builder on top of this; tests inject a stub.

• In-memory session store (domain/roster-assistant/session-store.ts): per-session conversation history, refinements_used counter, last catalog_hash; idle GC at >1h and a FIFO capacity cap so a misbehaving caller can't grow the resident set without bound.
• POST /roster/assistant/propose handler: validates message (≤4000 chars), creates / resumes the session, calls the injected proposer, validates the returned envelope shape, records the proposal, and tacks on a soft_cap_exceeded: true flag past DEFAULT_REFINEMENTS_SOFT_CAP.
• Failure paths never mutate session history: 400 on bad input, 404 on unknown session, 502 on proposer throw / malformed envelope — the trailing user message is rolled back (fresh-session: drop the whole session) before returning.
• 35 tests across the store + the handler covering: new-session creation, refinement with parent_proposal_id wiring, unknown-session 404, oversized-message 400, proposer-throw 502, default-unconfigured 502, malformed-envelope 502, soft-cap flag wiring, idle-GC, capacity eviction.

Closes #1212.

Test plan

• bun test apps/server/src/domain/roster-assistant apps/server/src/http/handlers/roster-assistant-propose.test.ts → 35 pass
• just qa (typecheck + lint + format + 3397 tests + sql-layer + paraglide + i18n + flow-schema) — all green
• Verify the route 401s on non-loopback unauthenticated callers (covered by the existing guardMutating test path in session-gate.test.ts)
• Smoke against the local dev service once the prompt-builder ticket lands

🤖 Generated with Claude Code

## Summary Adds the HTTP surface and session lifecycle for the **Agents Roster AI Assistant** (spec: `specs/agents-roster-ai-assistant.md`). The prompt assembly + Claude call live behind the `_rosterAssistantProposer.propose` injection seam so the prompt ticket can land in parallel — production wires the real builder on top of this; tests inject a stub. - **In-memory session store** (`domain/roster-assistant/session-store.ts`): per-session conversation history, `refinements_used` counter, last `catalog_hash`; idle GC at >1h and a FIFO capacity cap so a misbehaving caller can't grow the resident set without bound. - **POST /roster/assistant/propose** handler: validates `message` (≤4000 chars), creates / resumes the session, calls the injected proposer, validates the returned envelope shape, records the proposal, and tacks on a `soft_cap_exceeded: true` flag past `DEFAULT_REFINEMENTS_SOFT_CAP`. - **Failure paths** never mutate session history: 400 on bad input, 404 on unknown session, 502 on proposer throw / malformed envelope — the trailing user message is rolled back (fresh-session: drop the whole session) before returning. - **35 tests** across the store + the handler covering: new-session creation, refinement with `parent_proposal_id` wiring, unknown-session 404, oversized-message 400, proposer-throw 502, default-unconfigured 502, malformed-envelope 502, soft-cap flag wiring, idle-GC, capacity eviction. Closes #1212. ## Test plan - [x] `bun test apps/server/src/domain/roster-assistant apps/server/src/http/handlers/roster-assistant-propose.test.ts` → 35 pass - [x] `just qa` (typecheck + lint + format + 3397 tests + sql-layer + paraglide + i18n + flow-schema) — all green - [ ] Verify the route 401s on non-loopback unauthenticated callers (covered by the existing `guardMutating` test path in `session-gate.test.ts`) - [ ] Smoke against the local dev service once the prompt-builder ticket lands 🤖 Generated with [Claude Code](https://claude.com/claude-code)

code-lead added the

area:agents

type:user-story

labels 2026-05-15 19:12:10 +00:00

code-lead added 1 commit 2026-05-15 19:12:10 +00:00

feat(server): POST /roster/assistant/propose endpoint + in-memory session store (#1212) ... 949786fa95

Adds the HTTP surface and session lifecycle for the Agents Roster AI Assistant.
The prompt-builder + Claude call live behind the `_rosterAssistantProposer.propose`
injection seam so the prompt ticket can land in parallel.

- In-memory session store with idle GC (>1h) and FIFO capacity cap; tracks
  conversation history, `refinements_used`, and last `catalog_hash`.
- POST /roster/assistant/propose: validates `message` (≤4000 chars), creates
  or resumes a session, calls the injected proposer, records the validated
  envelope, and adds a `soft_cap_exceeded` flag past `DEFAULT_REFINEMENTS_SOFT_CAP`.
- Error handling: 400 on bad input, 404 on unknown session, 502 on proposer
  failure or malformed envelope — failure paths roll back the trailing user
  message so history isn't polluted.
- 35 tests covering new session, refinement (parent_proposal_id wired),
  unknown session 404, oversized message 400, proposer failure 502, the
  default-unconfigured 502 path, and soft-cap behaviour.

Closes #1212.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

code-lead requested review from reviewer 2026-05-15 19:54:14 +00:00

reviewer approved these changes 2026-05-15 19:57:34 +00:00

reviewer left a comment

Copy link

Review — PR #1219: POST /roster/assistant/propose + in-memory session store

APPROVED with a couple of notes worth acting on before the prompt-builder ticket lands.

---

Overview

Solid implementation of the HTTP surface + session lifecycle layer for the Roster AI Assistant (#1212). The injection-seam pattern mirrors the established _architectForgeFactory convention cleanly, the rollback logic on failure is correct and well-tested, and the session store has the right safety properties (defensive snapshot copies, FIFO cap with TTL-first eviction, inline sweep to avoid background timer management). AC coverage against the spec is good for the scope of this ticket — the latency/catalog-accuracy ACs are correctly deferred to the prompt-builder ticket.

---

Issues

1. Silent recordProposal ?? session fallback — orphaned session on 200 (minor, worth fixing before GA)

In roster-assistant-propose.ts:

session = recordProposal(session.id, envelope) ?? session;

recordProposal returns null when the session id isn't in the store — which can happen in this (unlikely, but real) sequence:

1. Request A calls createSession() → session X is the FIFO-oldest resident.
2. await _rosterAssistantProposer.propose(...) yields the event loop.
3. Request B concurrently calls createSession() at capacity=256 → evictIfOverCapacity() walks from the FIFO front → evicts session X.
4. Request A resumes, recordProposal(X.id, ...) returns null, ?? session silently falls back to the stale snapshot.
5. Handler returns 200 with a valid envelope whose session_id no longer exists in the store.
6. The next refinement call from the FE with that session_id → 404.

The ?? session was clearly intended as a defensive fallback for "shouldn't happen", but at capacity=256 with concurrent traffic it can. The fix is straightforward — log a warning and return a 502 rather than silently lying to the caller:

const recorded = recordProposal(session.id, envelope);
if (!recorded) {
    // Session was evicted by a concurrent createSession at capacity.
    // The envelope is valid but we can't persist the turn — returning
    // a 200 would give the FE an orphaned session_id that 404s on
    // the next refinement. Treat as a transient server error.
    console.warn(`[roster-assistant] session ${session.id} evicted mid-flight — returning 503`);
    return errorBody("session evicted during proposal; retry as a new session", "proposer_failed", 503);
}
session = recorded;

No test covers this path at the moment.

2. lastAssistantProposalId — unnecessarily widened signature + internal cast (nit)

function lastAssistantProposalId(history: ReadonlyArray<{ role: string }>): string | null {
    for (let i = history.length - 1; i >= 0; i--) {
        const entry = history[i] as { role: string; proposal?: { proposal_id?: unknown } };

The function only ever receives session.history which is ReadonlyArray<RosterAssistantHistoryEntry>. Taking the narrower type directly would eliminate the as cast and let TypeScript verify the proposal access without needing a structural assertion:

function lastAssistantProposalId(history: ReadonlyArray<RosterAssistantHistoryEntry>): string | null {
    for (let i = history.length - 1; i >= 0; i--) {
        const entry = history[i];
        if (entry.role === "assistant") return entry.proposal.proposal_id;
    }
    return null;
}

This is a nit since the cast is safe in practice, but removing it makes the narrowing explicit to the compiler.

---

Observations (no action needed)

• isWellFormedEnvelope is intentionally shallow — the comment explains this correctly. Full AJV validation is the proposer's responsibility; the handler's check is a defence-in-depth discriminator guard. ✓

• peekSession touch-semantics test ("getSession touches last_touched_at; peekSession does not") doesn't strongly falsify the no-touch guarantee without a time mock — two consecutive calls in the same tick give a last_touched_at difference of 0 ms, which satisfies ≥ 0 whether or not peekSession touches. This is fine for now since the behaviour is simple to reason about by inspection, but worth noting if the GC logic ever gets more sophisticated.

• Error messages forwarded to the client in 502 (body.error contains err.message): acceptable for an operator-only loopback-gated endpoint, and the tests assert on the message content which locks down the behaviour. Just be mindful when the prompt-builder ticket wires real Claude errors in — surface-level model error text leaking to the FE could be surprising.

• current_roster: unknown is correctly scoped to v1 per the spec ("not used in v1 since scope is new-fleet only"). The design note in RosterAssistantProposerInput makes the intent clear for the prompt-builder implementer.

• /roster/ in the not-found wildcard — the catch-all exclusion in server.ts is correct; unregistered /roster/xxx paths will 404 cleanly rather than falling through to whatever the default handler gives.

---

AC check (this ticket's scope)

AC	Status
POST /roster/assistant/propose exists, guarded by guardMutating	✅
New session created server-side on first call	✅
session_id threaded across refinements	✅
parent_proposal_id wired from prior proposal	✅
message validated (non-empty, ≤ 4000 chars)	✅
Unknown session_id → 404	✅
Proposer failure → 502, history not mutated	✅
Fresh session dropped (not orphaned) on first-call failure	✅
Malformed envelope → 502	✅
soft_cap_exceeded: true past soft cap	✅
Idle GC (1h TTL)	✅
Capacity cap (256 FIFO)	✅
35 tests, all passing per PR description	✅

The two ACs that touch the real model path (latency < 15s, catalog-only skills/MCPs) are correctly left to the prompt-builder ticket.

---

Good work — the rollback logic in particular is exactly right and the test for the first-call failure / session-drop path is the kind of subtle case that often gets missed. The recordProposal ?? session silent fallback is the main thing worth tidying up before the full feature goes live.

## Review — PR #1219: POST /roster/assistant/propose + in-memory session store **APPROVED** with a couple of notes worth acting on before the prompt-builder ticket lands. --- ### Overview Solid implementation of the HTTP surface + session lifecycle layer for the Roster AI Assistant (#1212). The injection-seam pattern mirrors the established `_architectForgeFactory` convention cleanly, the rollback logic on failure is correct and well-tested, and the session store has the right safety properties (defensive snapshot copies, FIFO cap with TTL-first eviction, inline sweep to avoid background timer management). AC coverage against the spec is good for the scope of this ticket — the latency/catalog-accuracy ACs are correctly deferred to the prompt-builder ticket. --- ### Issues #### 1. Silent `recordProposal ?? session` fallback — orphaned session on 200 (minor, worth fixing before GA) In `roster-assistant-propose.ts`: ```ts session = recordProposal(session.id, envelope) ?? session; ``` `recordProposal` returns `null` when the session id isn't in the store — which can happen in this (unlikely, but real) sequence: 1. Request A calls `createSession()` → session X is the FIFO-oldest resident. 2. `await _rosterAssistantProposer.propose(...)` yields the event loop. 3. Request B concurrently calls `createSession()` at capacity=256 → `evictIfOverCapacity()` walks from the FIFO front → evicts session X. 4. Request A resumes, `recordProposal(X.id, ...)` returns `null`, `?? session` silently falls back to the stale snapshot. 5. Handler returns **200** with a valid envelope whose `session_id` no longer exists in the store. 6. The next refinement call from the FE with that `session_id` → **404**. The `?? session` was clearly intended as a defensive fallback for "shouldn't happen", but at capacity=256 with concurrent traffic it can. The fix is straightforward — log a warning and return a 502 rather than silently lying to the caller: ```ts const recorded = recordProposal(session.id, envelope); if (!recorded) { // Session was evicted by a concurrent createSession at capacity. // The envelope is valid but we can't persist the turn — returning // a 200 would give the FE an orphaned session_id that 404s on // the next refinement. Treat as a transient server error. console.warn(`[roster-assistant] session ${session.id} evicted mid-flight — returning 503`); return errorBody("session evicted during proposal; retry as a new session", "proposer_failed", 503); } session = recorded; ``` No test covers this path at the moment. #### 2. `lastAssistantProposalId` — unnecessarily widened signature + internal cast (nit) ```ts function lastAssistantProposalId(history: ReadonlyArray<{ role: string }>): string | null { for (let i = history.length - 1; i >= 0; i--) { const entry = history[i] as { role: string; proposal?: { proposal_id?: unknown } }; ``` The function only ever receives `session.history` which is `ReadonlyArray<RosterAssistantHistoryEntry>`. Taking the narrower type directly would eliminate the `as` cast and let TypeScript verify the `proposal` access without needing a structural assertion: ```ts function lastAssistantProposalId(history: ReadonlyArray<RosterAssistantHistoryEntry>): string | null { for (let i = history.length - 1; i >= 0; i--) { const entry = history[i]; if (entry.role === "assistant") return entry.proposal.proposal_id; } return null; } ``` This is a nit since the cast is safe in practice, but removing it makes the narrowing explicit to the compiler. --- ### Observations (no action needed) - **`isWellFormedEnvelope` is intentionally shallow** — the comment explains this correctly. Full AJV validation is the proposer's responsibility; the handler's check is a defence-in-depth discriminator guard. ✓ - **`peekSession` touch-semantics test** (`"getSession touches last_touched_at; peekSession does not"`) doesn't strongly falsify the no-touch guarantee without a time mock — two consecutive calls in the same tick give a `last_touched_at` difference of 0 ms, which satisfies `≥ 0` whether or not `peekSession` touches. This is fine for now since the behaviour is simple to reason about by inspection, but worth noting if the GC logic ever gets more sophisticated. - **Error messages forwarded to the client in 502** (`body.error` contains `err.message`): acceptable for an operator-only loopback-gated endpoint, and the tests assert on the message content which locks down the behaviour. Just be mindful when the prompt-builder ticket wires real Claude errors in — surface-level model error text leaking to the FE could be surprising. - **`current_roster: unknown`** is correctly scoped to v1 per the spec ("not used in v1 since scope is new-fleet only"). The design note in `RosterAssistantProposerInput` makes the intent clear for the prompt-builder implementer. - **`/roster/` in the not-found wildcard** — the catch-all exclusion in `server.ts` is correct; unregistered `/roster/xxx` paths will 404 cleanly rather than falling through to whatever the default handler gives. --- ### AC check (this ticket's scope) | AC | Status | |---|---| | `POST /roster/assistant/propose` exists, guarded by `guardMutating` | ✅ | | New session created server-side on first call | ✅ | | `session_id` threaded across refinements | ✅ | | `parent_proposal_id` wired from prior proposal | ✅ | | `message` validated (non-empty, ≤ 4000 chars) | ✅ | | Unknown `session_id` → 404 | ✅ | | Proposer failure → 502, history not mutated | ✅ | | Fresh session dropped (not orphaned) on first-call failure | ✅ | | Malformed envelope → 502 | ✅ | | `soft_cap_exceeded: true` past soft cap | ✅ | | Idle GC (1h TTL) | ✅ | | Capacity cap (256 FIFO) | ✅ | | 35 tests, all passing per PR description | ✅ | The two ACs that touch the real model path (latency < 15s, catalog-only skills/MCPs) are correctly left to the prompt-builder ticket. --- Good work — the rollback logic in particular is exactly right and the test for the first-call failure / session-drop path is the kind of subtle case that often gets missed. The `recordProposal ?? session` silent fallback is the main thing worth tidying up before the full feature goes live.

charles merged commit 006c059893 into main 2026-05-15 19:58:02 +00:00

charles deleted branch code-lead/1212 2026-05-15 19:58:02 +00:00

charles referenced this pull request from a commit 2026-05-15 19:58:04 +00:00

feat(server): POST /roster/assistant/propose endpoint + in-memory session store (#1219)

Sign in to join this conversation.

Reviewers

No reviewers

reviewer

Labels

Clear labels   

area:agents

Agent types, pool scheduling, per-instance config

  

area:dashboard

Dashboard UI and observability surfaces

  

area:database

DB layer — schema, migrations, ORM, raw SQL

  

area:design

UI/UX mockup work — routes to designer agent

  

area:design-review

Design review dispatch — routes to design-reviewer agent

  

area:flows

Flow runner — YAML loader, executor, op registry, expression eval

  

area:infra

Deployment, isolation, containers, systemd units

  

area:meta

Tracking, scaffolding, project setup

  

area:security

Security — routes to reviewer-security (opus)

  

area:sessions

Session-id store, Claude SDK resume logic

  

area:webhook

Forgejo webhook routing and handlers

  

area:workdir

Clone cache, worktrees, git identity

  

security

Security-sensitive issue

  

type:bug

Bug

  

type:chore

Chore

  

type:meta

Tracking or decisions, not implementation work

  

type:user-story

User story

No labels

area:agents

area:dashboard

area:database

area:design

area:design-review

area:flows

area:infra

area:meta

area:security

area:sessions

area:webhook

area:workdir

security

type:bug

type:chore

type:meta

type:user-story

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

charles/agent-hooks!1219

No description provided.