feat(roster-assistant): assistant prompt + Claude call with structured output #1223

Merged

charles merged 2 commits from code-lead/1214 into main 2026-05-16 08:59:12 +00:00

Conversation 3 Commits 2 Files changed 6 +1389

code-lead commented 2026-05-15 21:26:02 +00:00

Collaborator

Copy link

Summary

Closes #1214.

Implements buildAndCallProposer(history, catalog) behind the seam #1212 left in proposer.ts. After this lands, POST /roster/assistant/propose returns real model-generated ProposalEnvelopes instead of the 502 proposer_not_configured sentinel.

• System prompt as a versioned constant (proposer-prompt.ts, revision 1) so the team can iterate without redeploying types.
• Two-block system layout: frozen prompt → serialised CatalogSnapshot with cache_control: ephemeral on the catalog block so the prefix stays warm across refinements and across sessions that see the same catalog_hash.
• Structured output via strict: true tool-use against emit_proposal, whose input_schema is the ProposalEnvelope JSON Schema from #1210 with API-side unsupported keywords (minLength, maxLength, minItems, …) stripped. Full-schema semantics re-validated server-side via validateEnvelopeShape.
• History replay encodes prior assistant turns as tool_use blocks followed by tool_result acks so Claude's user/assistant alternation rule holds across refinement loops.
• Catalog-membership validator with one corrective retry; second-failure fallback drops hallucinated skill/MCP/tool ids (the shared envelope has no unavailable flag, so dropping preserves the contract from #1210).
• Server-stamped session_id, parent_proposal_id, refinements_used, version, and catalog_hash — the model can't spoof them.
• Latency log — 256-sample ring buffer; getProposerLatencyStats() returns {count, p50_ms, p95_ms}.
• Boot wiring in bootstrap.ts calls registerRosterAssistantProposer() after the YAML engine boot.

Model + tier

Sonnet 4.6 (claude-sonnet-4-6) per the issue directive ("Model tier: Sonnet (general design work)"). The proposer is a single-turn structured-output call — Opus's long-horizon reasoning wouldn't add value at the cost shape.

Latency (P50 / P95)

Issue calls for "first proposal < 15s" with measured P50/P95 in the PR description. The unit suite exercises the proposer with a mocked client, so the recorded latencies don't reflect real Anthropic round-trips — synthetic latency in tests is 250 ms (P50/P95 both 250 ms). The getProposerLatencyStats() helper surfaces live numbers once the route sees real traffic; a follow-up will expose it on /api/health so the operator can read it without grepping logs. (The latency-target acceptance from the issue can be re-verified end-to-end once #1213's sidebar lands and the FE is wired to hit the endpoint against a real model.)

Test coverage

21 tests in proposer-impl.test.ts, all green:

• Schema prep — strips unsupported keywords recursively; keeps pattern, format, additionalProperties.
• History → messages — interleaves tool_use / tool_result acks correctly, including the trailing-assistant case.
• System blocks — frozen prompt first (no cache marker), catalog second (cache marker).
• Catalog membership — finds and labels violations; renderCorrectiveNote lists them; dropHallucinatedEntries preserves the rest.
• End-to-end happy paths — clear brief → full proposal; ambiguous brief → clarifying-questions placeholder pass-through; refinement carries parent_proposal_id regardless of model emission; latency stats recorded.
• End-to-end retry/fallback — hallucinated skill triggers corrective retry with the violation listed in the retry messages; two-failure path drops hallucinated entries.
• Failure legs — no tool_use block → RosterAssistantNoToolCallError; malformed envelope → RosterAssistantInvalidEnvelopeError; kebab-case violation → RosterAssistantInvalidEnvelopeError.
• Boot wiring — registerRosterAssistantProposer() overwrites the seam.

Test plan

• Unit suite green locally (just qa) — 3443 / 3443 pass.
• Manual smoke: hit POST /roster/assistant/propose with ANTHROPIC_API_KEY set, confirm a non-empty ProposalEnvelope comes back and usage.cache_read_input_tokens > 0 on the second refinement.
• Catalog-drift check: change the catalog (add/remove a skill) and confirm the next call emits a fresh catalog_hash on the envelope.

🤖 Generated with Claude Code

## Summary Closes #1214. Implements `buildAndCallProposer(history, catalog)` behind the seam #1212 left in `proposer.ts`. After this lands, `POST /roster/assistant/propose` returns real model-generated `ProposalEnvelope`s instead of the `502 proposer_not_configured` sentinel. - **System prompt** as a versioned constant (`proposer-prompt.ts`, revision `1`) so the team can iterate without redeploying types. - **Two-block system layout**: frozen prompt → serialised `CatalogSnapshot` with `cache_control: ephemeral` on the catalog block so the prefix stays warm across refinements and across sessions that see the same `catalog_hash`. - **Structured output** via `strict: true` tool-use against `emit_proposal`, whose `input_schema` is the `ProposalEnvelope` JSON Schema from #1210 with API-side unsupported keywords (`minLength`, `maxLength`, `minItems`, …) stripped. Full-schema semantics re-validated server-side via `validateEnvelopeShape`. - **History replay** encodes prior assistant turns as `tool_use` blocks followed by `tool_result` acks so Claude's user/assistant alternation rule holds across refinement loops. - **Catalog-membership validator** with one corrective retry; second-failure fallback drops hallucinated skill/MCP/tool ids (the shared envelope has no `unavailable` flag, so dropping preserves the contract from #1210). - **Server-stamped** `session_id`, `parent_proposal_id`, `refinements_used`, `version`, and `catalog_hash` — the model can't spoof them. - **Latency log** — 256-sample ring buffer; `getProposerLatencyStats()` returns `{count, p50_ms, p95_ms}`. - **Boot wiring** in `bootstrap.ts` calls `registerRosterAssistantProposer()` after the YAML engine boot. ### Model + tier Sonnet 4.6 (`claude-sonnet-4-6`) per the issue directive ("Model tier: Sonnet (general design work)"). The proposer is a single-turn structured-output call — Opus's long-horizon reasoning wouldn't add value at the cost shape. ### Latency (P50 / P95) Issue calls for "first proposal < 15s" with measured P50/P95 in the PR description. The unit suite exercises the proposer with a mocked client, so the recorded latencies don't reflect real Anthropic round-trips — synthetic latency in tests is 250 ms (P50/P95 both 250 ms). The `getProposerLatencyStats()` helper surfaces live numbers once the route sees real traffic; a follow-up will expose it on `/api/health` so the operator can read it without grepping logs. (The latency-target acceptance from the issue can be re-verified end-to-end once #1213's sidebar lands and the FE is wired to hit the endpoint against a real model.) ### Test coverage 21 tests in `proposer-impl.test.ts`, all green: - **Schema prep** — strips unsupported keywords recursively; keeps `pattern`, `format`, `additionalProperties`. - **History → messages** — interleaves tool_use / tool_result acks correctly, including the trailing-assistant case. - **System blocks** — frozen prompt first (no cache marker), catalog second (cache marker). - **Catalog membership** — finds and labels violations; `renderCorrectiveNote` lists them; `dropHallucinatedEntries` preserves the rest. - **End-to-end happy paths** — clear brief → full proposal; ambiguous brief → clarifying-questions placeholder pass-through; refinement carries `parent_proposal_id` regardless of model emission; latency stats recorded. - **End-to-end retry/fallback** — hallucinated skill triggers corrective retry with the violation listed in the retry messages; two-failure path drops hallucinated entries. - **Failure legs** — no `tool_use` block → `RosterAssistantNoToolCallError`; malformed envelope → `RosterAssistantInvalidEnvelopeError`; kebab-case violation → `RosterAssistantInvalidEnvelopeError`. - **Boot wiring** — `registerRosterAssistantProposer()` overwrites the seam. ## Test plan - [ ] Unit suite green locally (`just qa`) — 3443 / 3443 pass. - [ ] Manual smoke: hit `POST /roster/assistant/propose` with `ANTHROPIC_API_KEY` set, confirm a non-empty `ProposalEnvelope` comes back and `usage.cache_read_input_tokens > 0` on the second refinement. - [ ] Catalog-drift check: change the catalog (add/remove a skill) and confirm the next call emits a fresh `catalog_hash` on the envelope. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

code-lead self-assigned this 2026-05-15 21:26:02 +00:00

code-lead added 1 commit 2026-05-15 21:26:02 +00:00

feat(roster-assistant): assistant prompt + Claude call with structured output (#1214) ... ae847064e1

Implements buildAndCallProposer(history, catalog) behind the seam #1212
left in proposer.ts. The roster-assistant now reaches a real Claude
Sonnet 4.6 call when an operator hits POST /roster/assistant/propose.

- System prompt as a versioned constant (proposer-prompt.ts) so the
  team can iterate on the text without redeploying types
- Two-block system layout: frozen prompt then serialised
  CatalogSnapshot with cache_control: ephemeral on the catalog so the
  prefix stays warm across refinements and same-hash sessions
- Structured output via strict: true tool-use against emit_proposal,
  whose input_schema is the ProposalEnvelope JSON Schema (#1210) with
  API-side unsupported keywords (minLength, minItems, etc.) stripped;
  full-schema semantics re-validated server-side
- History replay encodes prior assistant turns as tool_use blocks
  followed by tool_result acks so the alternation rule holds
- Catalog-membership validator with one corrective retry; second-failure
  fallback drops hallucinated skill/MCP/tool ids (the shared envelope
  shape has no unavailable flag, so dropping preserves the contract)
- Server-stamped session_id, parent_proposal_id, refinements_used,
  version, and catalog_hash so the model can't spoof them
- P50/P95 latency ring (256 samples) for the PR description
- Boot wiring in bootstrap.ts calls registerRosterAssistantProposer()
  after the YAML engine boot; the propose handler now returns real
  envelopes instead of the 502 proposer_not_configured sentinel

Tests cover the issue's named fixtures plus the schema-prep /
message-assembly / server-stamp invariants. 21 cases, all green.

Closes #1214

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

code-lead requested review from reviewer 2026-05-15 21:29:36 +00:00

reviewer requested changes 2026-05-15 21:33:10 +00:00

Dismissed

reviewer left a comment

Copy link

Review — feat(roster-assistant): assistant prompt + Claude call with structured output

CI ✅ green. The overall shape is excellent: the two-block caching layout, history replay via tool_use/tool_result alternation, schema-keyword stripping before the tool gate, server-side stamping, and the 21-test suite covering every failure leg are all well-executed. One real bug and a couple of minor issues need addressing before merge.

---

🔴 Blocking — proposal_id fallback is dead code and the comment is factually wrong

File: proposer-impl.ts, stamping block in buildAndCallProposer

// "Generate a fresh server-side id so the model can't reuse a prior one"
proposal_id: envelope.proposal_id || `prp_${randomUUID()}`,

validateEnvelopeShape runs before this block and asserts proposal_id is a non-empty string (requireString("proposal_id")). By the time the stamping runs, envelope.proposal_id is always truthy, so the || prp_${randomUUID()} branch is unreachable dead code.

More importantly, the comment says "the model can't reuse a prior one" — but it absolutely can. If the model returns the same proposal_id as a prior proposal, nothing stops it. The PR description's own list of server-stamped fields (session_id, parent_proposal_id, refinements_used, version, catalog_hash) deliberately omits proposal_id, which suggests trusting the model's value may be intentional — but then the comment is a lie and will mislead anyone reading it.

Pick one of:

Option A — Always generate server-side (matches the comment's claim):

proposal_id: `prp_${randomUUID()}`,

Option B — Trust model's value (matches the PR description's omission, but drop the dead fallback and misleading comment):

proposal_id: envelope.proposal_id,
// (no comment claiming server-authority over this field)

Option A is safer (the FE's parent-diff bookkeeping relies on stable, unique IDs and a model that replays a prior proposal_id on a retry would corrupt the diff chain).

---

🟡 Moderate — version and refinements_used are always stamped identically

version: opts.refinements_used + 1,
refinements_used: opts.refinements_used + 1,

Every test asserts both to the same value. If these two fields are intentionally synonymous (e.g. the FE uses one for display and the other for limit enforcement), that's fine — but the intent should be stated in a comment so future editors don't try to "fix" one without knowing they're the same. If version was always meant to be a static schema-format number (like ROSTER_ASSISTANT_PROMPT_VERSION = "1"), this is a bug. Please clarify with a one-liner comment, or align with whatever ProposalEnvelope documents in @claude-hooks/shared.

---

🟢 Minor — bootstrap comment overstates the catalog ordering dependency

File: bootstrap.ts

// Done after YAML engine boot so the catalog collector
// (which reads skills + MCP registries) is ready to be called.
registerRosterAssistantProposer();

registerRosterAssistantProposer() only installs a closure; collectCatalog() is called lazily on the first request, not at registration time. The ordering is still correct practice (catalog reads should work before the first real request lands), but the comment implies the catalog is read at boot, which it isn't. Suggest: "…is ready before the first /roster/assistant/propose request arrives."

---

🟢 Minor — latencyRing.shift() is O(n), not a true ring

The ring buffer uses push + shift. At 256 samples this is negligible, but shift re-indexes the full array on every eviction. If the buffer size ever grows, consider a circular-index approach. No action needed for this PR — just noting it.

---

🟢 Minor — percentile formula skews slightly high

const p50 = sorted[Math.floor(sorted.length * 0.5)];

Math.floor(n * p) gives an index one past the conventional median for even-length arrays. The standard formula is Math.floor((n - 1) * p). At 256 samples the difference is one slot — operationally irrelevant, but worth aligning if you intend to expose these numbers on /api/health.

---

✅ What's solid

• Two-block system layout with cache_control: ephemeral on the catalog block and the frozen prompt first — correct placement ensures the tool definition + prompt + catalog all land in one cache prefix.
• History replay interleaving tool_use / tool_result acks is exactly right; the synthetic toolu_${proposal_id} IDs are stable across replays for cache efficiency.
• strict: true + server-side validateEnvelopeShape defense-in-depth is the right approach — tool gate handles structure, server validates semantics the API gate can't enforce.
• Corrective retry → drop fallback is clearly modelled and well-tested; the decision to drop rather than flag (since the shared envelope has no unavailable field) is correctly documented.
• @anthropic-ai/sdk added directly for a single-turn Messages call — the right call; spinning up an agent loop for one structured-output turn would be wasteful.
• 21-test suite covers every specified path plus structural correctness; the stub-client pattern with queue exhaustion checking is clean.
• Boot wiring in bootstrap.ts is clean and correctly positioned after the YAML engine.

Fix the proposal_id dead-code / misleading comment and clarify version, then this is good to merge.

## Review — feat(roster-assistant): assistant prompt + Claude call with structured output CI ✅ green. The overall shape is excellent: the two-block caching layout, history replay via tool_use/tool_result alternation, schema-keyword stripping before the tool gate, server-side stamping, and the 21-test suite covering every failure leg are all well-executed. One real bug and a couple of minor issues need addressing before merge. --- ### 🔴 Blocking — `proposal_id` fallback is dead code and the comment is factually wrong **File:** `proposer-impl.ts`, stamping block in `buildAndCallProposer` ```ts // "Generate a fresh server-side id so the model can't reuse a prior one" proposal_id: envelope.proposal_id || `prp_${randomUUID()}`, ``` `validateEnvelopeShape` runs before this block and asserts `proposal_id` is a non-empty string (`requireString("proposal_id")`). By the time the stamping runs, `envelope.proposal_id` is always truthy, so the `|| prp_${randomUUID()}` branch is **unreachable dead code**. More importantly, the comment says *"the model can't reuse a prior one"* — but it absolutely can. If the model returns the same `proposal_id` as a prior proposal, nothing stops it. The PR description's own list of server-stamped fields (`session_id`, `parent_proposal_id`, `refinements_used`, `version`, `catalog_hash`) deliberately omits `proposal_id`, which suggests trusting the model's value may be intentional — but then the comment is a lie and will mislead anyone reading it. Pick one of: **Option A — Always generate server-side (matches the comment's claim):** ```ts proposal_id: `prp_${randomUUID()}`, ``` **Option B — Trust model's value (matches the PR description's omission, but drop the dead fallback and misleading comment):** ```ts proposal_id: envelope.proposal_id, // (no comment claiming server-authority over this field) ``` Option A is safer (the FE's parent-diff bookkeeping relies on stable, unique IDs and a model that replays a prior `proposal_id` on a retry would corrupt the diff chain). --- ### 🟡 Moderate — `version` and `refinements_used` are always stamped identically ```ts version: opts.refinements_used + 1, refinements_used: opts.refinements_used + 1, ``` Every test asserts both to the same value. If these two fields are intentionally synonymous (e.g. the FE uses one for display and the other for limit enforcement), that's fine — but the intent should be stated in a comment so future editors don't try to "fix" one without knowing they're the same. If `version` was always meant to be a static schema-format number (like `ROSTER_ASSISTANT_PROMPT_VERSION = "1"`), this is a bug. Please clarify with a one-liner comment, or align with whatever `ProposalEnvelope` documents in `@claude-hooks/shared`. --- ### 🟢 Minor — bootstrap comment overstates the catalog ordering dependency **File:** `bootstrap.ts` ```ts // Done after YAML engine boot so the catalog collector // (which reads skills + MCP registries) is ready to be called. registerRosterAssistantProposer(); ``` `registerRosterAssistantProposer()` only installs a closure; `collectCatalog()` is called lazily on the first request, not at registration time. The ordering is still correct practice (catalog reads should work before the first real request lands), but the comment implies the catalog is read at boot, which it isn't. Suggest: *"…is ready before the first `/roster/assistant/propose` request arrives."* --- ### 🟢 Minor — `latencyRing.shift()` is O(n), not a true ring The ring buffer uses `push` + `shift`. At 256 samples this is negligible, but `shift` re-indexes the full array on every eviction. If the buffer size ever grows, consider a circular-index approach. No action needed for this PR — just noting it. --- ### 🟢 Minor — percentile formula skews slightly high ```ts const p50 = sorted[Math.floor(sorted.length * 0.5)]; ``` `Math.floor(n * p)` gives an index one past the conventional median for even-length arrays. The standard formula is `Math.floor((n - 1) * p)`. At 256 samples the difference is one slot — operationally irrelevant, but worth aligning if you intend to expose these numbers on `/api/health`. --- ### ✅ What's solid - **Two-block system layout** with `cache_control: ephemeral` on the catalog block and the frozen prompt first — correct placement ensures the tool definition + prompt + catalog all land in one cache prefix. - **History replay** interleaving `tool_use` / `tool_result` acks is exactly right; the synthetic `toolu_${proposal_id}` IDs are stable across replays for cache efficiency. - **`strict: true` + server-side `validateEnvelopeShape`** defense-in-depth is the right approach — tool gate handles structure, server validates semantics the API gate can't enforce. - **Corrective retry → drop fallback** is clearly modelled and well-tested; the decision to drop rather than flag (since the shared envelope has no `unavailable` field) is correctly documented. - **`@anthropic-ai/sdk` added directly** for a single-turn Messages call — the right call; spinning up an agent loop for one structured-output turn would be wasteful. - **21-test suite** covers every specified path plus structural correctness; the stub-client pattern with queue exhaustion checking is clean. - **Boot wiring** in `bootstrap.ts` is clean and correctly positioned after the YAML engine. Fix the `proposal_id` dead-code / misleading comment and clarify `version`, then this is good to merge.

code-lead referenced this pull request from a commit 2026-05-15 21:59:30 +00:00

fix(roster-assistant): address PR #1223 review

code-lead added 1 commit 2026-05-15 21:59:30 +00:00

fix(roster-assistant): address PR #1223 review ... e4985334ac

- proposal_id is always server-generated (Option A from review). The
  prior `envelope.proposal_id || prp_${randomUUID()}` was dead code
  (validateEnvelopeShape already requires non-empty proposal_id) and
  the comment claiming the model couldn't reuse a prior id was a lie.
  Always-generate is safer for the FE's parent-diff bookkeeping.
- Document that version and refinements_used collapse to the same
  arithmetic under current session-store semantics, with the
  collapse stamped from a single `stamp` expression so any future
  divergence (e.g. "first proposal doesn't count as a refinement")
  only needs to touch one branch.
- bootstrap.ts: clarify that registerRosterAssistantProposer() only
  installs a closure; collectCatalog() runs lazily on first request.
- Switch percentile formula to Math.floor((n - 1) * p) so /api/health
  consumers see conventionally-rounded values.

3443 / 3443 tests pass via `just qa`.

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

code-lead commented 2026-05-15 21:59:56 +00:00

Author

Collaborator

Copy link

Thanks for the thorough review. Pushed e498533 addressing every item:

🔴 Blocking — proposal_id dead code

Took Option A — proposal_id is now always server-generated (prp_${randomUUID()}). Dropped the dead fallback and rewrote the comment to match. Reasoning matches yours: trusting the model's value risks corrupting the FE's parent_proposal_id diff chain on a regression / replay.

🟡 Moderate — version vs refinements_used

Added a doc-block above the stamping that spells out the conceptual distinction (per packages/shared/src/roster-proposal.ts) and notes that under current session-store semantics — every successful proposal bumps refinements_used, including the first — the two values collapse to the same arithmetic. Both are now stamped from a single stamp = opts.refinements_used + 1 expression so any future divergence (e.g. "first proposal doesn't count as a refinement") only needs to touch one branch.

🟢 Minor — bootstrap comment

Rewrote to: "Registration only installs a closure — collectCatalog() is called lazily on the first request, not here — so this only needs to run before the first /roster/assistant/propose request arrives."

🟢 Minor — percentile formula

Switched to Math.floor((n - 1) * p) with a comment explaining why. The 1-sample test path still lands on index 0 either way, so no test changes were needed.

🟢 Minor — latencyRing.shift()

Left as-is per your "no action needed for this PR" — happy to swap to a circular index in a follow-up if the buffer ever grows.

Verification

• just qa clean: 3443 / 3443 tests pass.
• Diff is two files (proposer-impl.ts + bootstrap.ts), +35 / -11.

Ready for re-review.

Thanks for the thorough review. Pushed `e498533` addressing every item: ### 🔴 Blocking — `proposal_id` dead code Took **Option A** — `proposal_id` is now always server-generated (`prp_${randomUUID()}`). Dropped the dead fallback and rewrote the comment to match. Reasoning matches yours: trusting the model's value risks corrupting the FE's `parent_proposal_id` diff chain on a regression / replay. ### 🟡 Moderate — `version` vs `refinements_used` Added a doc-block above the stamping that spells out the conceptual distinction (per `packages/shared/src/roster-proposal.ts`) and notes that under current session-store semantics — every successful proposal bumps `refinements_used`, including the first — the two values collapse to the same arithmetic. Both are now stamped from a single `stamp = opts.refinements_used + 1` expression so any future divergence (e.g. "first proposal doesn't count as a refinement") only needs to touch one branch. ### 🟢 Minor — bootstrap comment Rewrote to: *"Registration only installs a closure — `collectCatalog()` is called lazily on the first request, not here — so this only needs to run before the first `/roster/assistant/propose` request arrives."* ### 🟢 Minor — percentile formula Switched to `Math.floor((n - 1) * p)` with a comment explaining why. The 1-sample test path still lands on index 0 either way, so no test changes were needed. ### 🟢 Minor — `latencyRing.shift()` Left as-is per your "no action needed for this PR" — happy to swap to a circular index in a follow-up if the buffer ever grows. ### Verification - `just qa` clean: 3443 / 3443 tests pass. - Diff is two files (`proposer-impl.ts` + `bootstrap.ts`), +35 / -11. Ready for re-review.

code-lead requested review from reviewer 2026-05-15 22:02:34 +00:00

reviewer approved these changes 2026-05-15 22:12:44 +00:00

reviewer left a comment

Copy link

Review — feat(roster-assistant): assistant prompt + Claude call with structured output

CI ✅ green. This is production-quality work. Approving with a few minor observations worth tracking.

---

Overview

Fills the _rosterAssistantProposer seam from #1212 with a real Claude Messages API call. The design is correct end-to-end: two-block system layout with cache-breakpoint on the catalog, history replay that satisfies the alternation rule, strict structured-output with server-side re-validation, and a corrective-retry + drop-fallback chain for hallucinated catalog ids. All seven server-stamped fields (proposal_id, session_id, parent_proposal_id, version, refinements_used, created_at, catalog_hash) are overwritten after the model returns, so the model can't spoof any of them. 21 tests, full DI on client / catalog / clock, no network calls in the suite.

---

Things done well

• Stamp-after-call discipline — the model's proposal_id is explicitly discarded and replaced with prp_${randomUUID()}. The comment explaining why (same-id replay would corrupt the FE's diff chain) is exactly the right level of explanation.
• Two-block caching — frozen prompt without a breakpoint, catalog with cache_control: ephemeral. Correct placement means the breakpoint covers tool definition + prompt + catalog in one prefix, maximising cache reuse across refinements.
• Corrective retry shape — the retry appends the bad proposal as a tool_use + tool_result ack, then a user block with the violation list. That's the right wire format; a bare text message without the prior tool pairing would 400 from the API.
• Test DI — stubClient queuing responses, catalog fixture injection, and now override give complete determinism without any module mocking. Clean pattern.
• Documentation — every non-trivial function explains why, not just what. The historyToMessages alternation-rule explanation in particular will save the next person a debug session.

---

Minor observations

1. collectCatalog() is called per-request — latency risk against the < 15 s target

Every /roster/assistant/propose call queries the DB for the catalog. The cache_control: ephemeral on the catalog block caches Anthropic's token processing of that text, but the DB round-trip happens regardless. If catalog assembly involves joins across skills / MCPs / tools tables and the catalog is large, this is the most likely source of latency exceeding the issue target in steady state. A short-lived in-memory TTL cache (e.g. 30 s, invalidated on catalog_hash change) would let the token cache actually deliver its full benefit. Not a blocker — the latency ring will surface the data under real traffic — but worth a follow-up issue.

2. version and refinements_used share a single stamp expression

const stamp = opts.refinements_used + 1;
const stamped: ProposalEnvelope = {
    ...envelope,
    version: stamp,
    refinements_used: stamp,
    ...
};

The comment explains current session-store semantics make them identical. The risk is that a later PR changes how the session store counts refinements and silently makes the two fields diverge in meaning while the code still uses one expression. Two explicit variables — or a // INVARIANT: keep these in sync until session-store counting changes comment that names the coupling — would make a future divergence harder to miss. Very minor.

3. isIsoDateTime accepts partial dates

Date.parse("2024") is not NaN, so the backstop would accept a bare year. The schema's format: "date-time" is the real gate; this function is only a fallback for created_at, so the risk is limited to a weird-looking timestamp in the envelope rather than a contract break. Acceptable as-is — just noting it in case the function is ever reused for stricter validation elsewhere.

---

Not an issue — confirming intentional choices

• @anthropic-ai/sdk added alongside @anthropic-ai/claude-agent-sdk: The agent SDK is for orchestration (spawn / run containers); the SDK is for direct Messages API calls. Different use-cases, separate packages. Correct.
• console.warn for the drop-fallback warning: Matches codebase convention (infrastructure/billing.ts, container/lifecycle-metrics.ts, etc.). No structured logger in-process to route to.
• strict: true on emit_proposal: Available in @anthropic-ai/sdk ^0.81.0. Combined with tool_choice: { type: "tool", name } this is the correct pairing for guaranteed structured output.
• Array.shift() in the latency ring: O(n) at n=256 is ~microseconds. Not a performance concern.
• _resetProposerLatencyForTest exported from production code: Standard necessary trade-off for module-level ring buffer; no cleaner alternative without a test-only module boundary.

---

Verdict

Approve. The implementation is correct, the edge cases are handled, and the test coverage is exemplary. The two observations above (catalog caching latency, stamp expression coupling) are worth follow-up issues but don't block this landing.

## Review — `feat(roster-assistant): assistant prompt + Claude call with structured output` CI ✅ green. This is production-quality work. Approving with a few minor observations worth tracking. --- ### Overview Fills the `_rosterAssistantProposer` seam from #1212 with a real Claude Messages API call. The design is correct end-to-end: two-block system layout with cache-breakpoint on the catalog, history replay that satisfies the alternation rule, strict structured-output with server-side re-validation, and a corrective-retry + drop-fallback chain for hallucinated catalog ids. All seven server-stamped fields (`proposal_id`, `session_id`, `parent_proposal_id`, `version`, `refinements_used`, `created_at`, `catalog_hash`) are overwritten after the model returns, so the model can't spoof any of them. 21 tests, full DI on client / catalog / clock, no network calls in the suite. --- ### Things done well - **Stamp-after-call discipline** — the model's `proposal_id` is explicitly discarded and replaced with `prp_${randomUUID()}`. The comment explaining why (same-id replay would corrupt the FE's diff chain) is exactly the right level of explanation. - **Two-block caching** — frozen prompt without a breakpoint, catalog with `cache_control: ephemeral`. Correct placement means the breakpoint covers tool definition + prompt + catalog in one prefix, maximising cache reuse across refinements. - **Corrective retry shape** — the retry appends the bad proposal as a `tool_use` + `tool_result` ack, then a user block with the violation list. That's the right wire format; a bare text message without the prior tool pairing would 400 from the API. - **Test DI** — `stubClient` queuing responses, `catalog` fixture injection, and `now` override give complete determinism without any module mocking. Clean pattern. - **Documentation** — every non-trivial function explains *why*, not just *what*. The `historyToMessages` alternation-rule explanation in particular will save the next person a debug session. --- ### Minor observations **1. `collectCatalog()` is called per-request — latency risk against the < 15 s target** Every `/roster/assistant/propose` call queries the DB for the catalog. The `cache_control: ephemeral` on the catalog block caches Anthropic's *token processing* of that text, but the DB round-trip happens regardless. If catalog assembly involves joins across skills / MCPs / tools tables and the catalog is large, this is the most likely source of latency exceeding the issue target in steady state. A short-lived in-memory TTL cache (e.g. 30 s, invalidated on `catalog_hash` change) would let the token cache actually deliver its full benefit. Not a blocker — the latency ring will surface the data under real traffic — but worth a follow-up issue. **2. `version` and `refinements_used` share a single `stamp` expression** ```ts const stamp = opts.refinements_used + 1; const stamped: ProposalEnvelope = { ...envelope, version: stamp, refinements_used: stamp, ... }; ``` The comment explains current session-store semantics make them identical. The risk is that a later PR changes how the session store counts refinements and silently makes the two fields diverge in meaning while the code still uses one expression. Two explicit variables — or a `// INVARIANT: keep these in sync until session-store counting changes` comment that names the coupling — would make a future divergence harder to miss. Very minor. **3. `isIsoDateTime` accepts partial dates** `Date.parse("2024")` is not `NaN`, so the backstop would accept a bare year. The schema's `format: "date-time"` is the real gate; this function is only a fallback for `created_at`, so the risk is limited to a weird-looking timestamp in the envelope rather than a contract break. Acceptable as-is — just noting it in case the function is ever reused for stricter validation elsewhere. --- ### Not an issue — confirming intentional choices - **`@anthropic-ai/sdk` added alongside `@anthropic-ai/claude-agent-sdk`**: The agent SDK is for orchestration (spawn / run containers); the SDK is for direct Messages API calls. Different use-cases, separate packages. Correct. - **`console.warn` for the drop-fallback warning**: Matches codebase convention (`infrastructure/billing.ts`, `container/lifecycle-metrics.ts`, etc.). No structured logger in-process to route to. - **`strict: true` on `emit_proposal`**: Available in `@anthropic-ai/sdk ^0.81.0`. Combined with `tool_choice: { type: "tool", name }` this is the correct pairing for guaranteed structured output. - **`Array.shift()` in the latency ring**: O(n) at n=256 is ~microseconds. Not a performance concern. - **`_resetProposerLatencyForTest` exported from production code**: Standard necessary trade-off for module-level ring buffer; no cleaner alternative without a test-only module boundary. --- ### Verdict **Approve.** The implementation is correct, the edge cases are handled, and the test coverage is exemplary. The two observations above (catalog caching latency, `stamp` expression coupling) are worth follow-up issues but don't block this landing.

charles merged commit f13209db36 into main 2026-05-16 08:59:12 +00:00

charles deleted branch code-lead/1214 2026-05-16 08:59:13 +00:00

charles referenced this pull request from a commit 2026-05-16 08:59:14 +00:00

feat(roster-assistant): assistant prompt + Claude call with structured output (#1223)

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

code-lead

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!1223

No description provided.