feat(roster-assistant): catalog snapshot collector + GET /roster/assistant/catalog #1218

Merged

charles merged 1 commit from code-lead/1211 into main 2026-05-15 20:18:36 +00:00

Conversation 2 Commits 1 Files changed 7 +679 -1

code-lead commented 2026-05-15 18:59:04 +00:00

Collaborator

Copy link

Summary

Implements the catalog producer the Agents Roster AI Assistant feature uses to constrain its proposals to skills, MCPs, tools, and model tiers that actually exist in the deployment.

• collectCatalog() (domain/roster-assistant/catalog.ts) enumerates every section through the existing registries — getSkillLibrary() for skills, resolveMcpServers() at the global ladder layer for configured MCPs, config/recommended-catalog.json for known-but-unconfigured MCPs with setup_required, FORGE_TOOLS_ALLOWLIST + built-in SDK tools (Bash/Write/Edit flagged sensitive per spec), and the shared MODEL_TIERS const.
• GET /roster/assistant/catalog returns the snapshot as JSON. Read-only, session-gated (same gate as the other /api/* GETs).
• Memoised in-process for 5 minutes. Invalidated from the agent-config MCP write path (afterMutation when kind === 'mcp') and from the skill-library cache reset.
• hash is a SHA-256 of the snapshot with the hash field itself excluded. Arrays are sorted and object literals are built with a fixed key order so JSON.stringify is deterministic — the FE drift detector at accept time gets a stable identifier.

Closes #1211

Test plan

• just qa clean (typecheck + lint + format + 3382 tests + sql-layer-check + paraglide-check + i18n-string-check + flow-schema-check all pass)
• Unit tests in catalog.test.ts cover: shape (every section + sensitive-tool flagging + every FORGE allowlist entry + penpot wildcard only when configured + recommended merging + missing file → empty recommended set), hash stability (no-op call, MCP added, configured set changed, order-independent), caching + invalidation (TTL memoisation, invalidate forces rebuild, override bypasses cache).
• Integration test in roster-assistant.test.ts drives the route through handleRequest end-to-end (status, content type, body shape, cache HIT).

🤖 Generated with Claude Code

## Summary Implements the catalog producer the Agents Roster AI Assistant feature uses to constrain its proposals to skills, MCPs, tools, and model tiers that actually exist in the deployment. - `collectCatalog()` (`domain/roster-assistant/catalog.ts`) enumerates every section through the existing registries — `getSkillLibrary()` for skills, `resolveMcpServers()` at the global ladder layer for configured MCPs, `config/recommended-catalog.json` for known-but-unconfigured MCPs with `setup_required`, `FORGE_TOOLS_ALLOWLIST` + built-in SDK tools (Bash/Write/Edit flagged sensitive per spec), and the shared `MODEL_TIERS` const. - `GET /roster/assistant/catalog` returns the snapshot as JSON. Read-only, session-gated (same gate as the other `/api/*` GETs). - Memoised in-process for 5 minutes. Invalidated from the agent-config MCP write path (`afterMutation` when `kind === 'mcp'`) and from the skill-library cache reset. - `hash` is a SHA-256 of the snapshot with the hash field itself excluded. Arrays are sorted and object literals are built with a fixed key order so `JSON.stringify` is deterministic — the FE drift detector at accept time gets a stable identifier. Closes #1211 ## Test plan - [x] `just qa` clean (typecheck + lint + format + 3382 tests + sql-layer-check + paraglide-check + i18n-string-check + flow-schema-check all pass) - [x] Unit tests in `catalog.test.ts` cover: shape (every section + sensitive-tool flagging + every FORGE allowlist entry + penpot wildcard only when configured + recommended merging + missing file → empty recommended set), hash stability (no-op call, MCP added, configured set changed, order-independent), caching + invalidation (TTL memoisation, invalidate forces rebuild, override bypasses cache). - [x] Integration test in `roster-assistant.test.ts` drives the route through `handleRequest` end-to-end (status, content type, body shape, cache HIT). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

code-lead self-assigned this 2026-05-15 18:59:04 +00:00

code-lead added 1 commit 2026-05-15 18:59:04 +00:00

feat(roster-assistant): catalog snapshot collector + GET /roster/assistant/catalog ... cd231f27e6

Adds the catalog producer the Agents Roster AI Assistant feature uses to
constrain its proposals to skills, MCPs, tools, and model tiers that
actually exist in the deployment. The collector reads every section
through the existing registries (skill library, MCP resolver, forge
allowlist, shared MODEL_TIERS) so the catalog never drifts from the
runtime. Configured MCPs come from the global ladder view (built-in
defaults + operator-added rows); known-but-unconfigured MCPs come from
config/recommended-catalog.json with configured=false so the assistant
can suggest them with setup_required.

The snapshot is memoised in-process for 5 minutes and busted from the
agent-config MCP write path (afterMutation when kind === 'mcp') and the
skill library cache reset. hash is a SHA-256 of the snapshot contents
(without the hash field itself); arrays are sorted and object literals
are built with a fixed key order so JSON.stringify is deterministic and
the FE drift detector at accept time gets a stable identifier.

Closes #1211

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

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

reviewer requested changes 2026-05-15 19:06:09 +00:00

Dismissed

reviewer left a comment

Copy link

Code Review — PR #1218: catalog snapshot collector + GET /roster/assistant/catalog

Overall this is a well-designed, thoroughly documented, and well-tested piece of work. The hash-stability guarantee, cache-invalidation contract, and multi-source MCP enumeration are all done correctly. Two issues need addressing before merge.

---

❌ 1. Circular dependency: catalog.ts ↔ skill-library.ts (must fix)

catalog.ts imports getSkillLibrary from skill-library.ts:

import { getSkillLibrary } from "../skills/skill-library";

And skill-library.ts now imports back into the feature that depends on it:

import { invalidateCatalogSnapshot } from "../roster-assistant/catalog";

This creates a circular module graph. ESM live-bindings mean it works at runtime today, but it's an architectural layering violation (a utility domain module importing from a higher-level feature module), creates hidden initialization-order sensitivity, and will confuse any future reader who tries to understand the skills layer in isolation.

The fix is simple — the coupling is unnecessary. catalog.test.ts already calls both resets explicitly in beforeEach:

resetSkillLibraryCacheForTest();
invalidateCatalogSnapshot();

That's the right place for it. resetSkillLibraryCacheForTest should only reset the skill library. Remove the invalidateCatalogSnapshot() call from it (and the import). The PR comment itself acknowledges "Production never calls this" — there's no production justification for the coupling.

---

❌ 2. No negative auth test on GET /roster/assistant/catalog (must fix)

roster-assistant.test.ts only exercises the happy path. There is no test verifying that a request without a valid session cookie is rejected. Every other session-gated endpoint in the test suite includes a 401 / redirect case. Add:

test("returns 401 without a session cookie", async () => {
  const res = await handleRequest(req("/roster/assistant/catalog", { withSession: false }), "127.0.0.1");
  expect(res.status).toBe(401);
});

The req helper already has the withSession opt, so this is a one-liner to add.

---

⚠️ 3. Silent Zod validation failure in readRecommendedMcps (minor, consider fixing)

When recommendedCatalogSchema.safeParse fails, the function silently returns []:

const result = recommendedCatalogSchema.safeParse(parsed);
if (!result.success) return [];

An operator who ships a malformed config/recommended-catalog.json (e.g. missing a required field after an upgrade) will see the recommended section silently empty with no indication why. A console.warn with result.error.message would make this much easier to diagnose. The ENOENT path correctly returns [] without logging (expected); schema failures deserve a warning.

---

ℹ️ 4. BUILTIN_TOOLS drift risk (informational)

The comment says the list is "kept in lockstep with DefaultMcpRegistry.allowedTools" but there's no automated enforcement. If a future SDK update adds a new built-in tool, the catalog will silently omit it. Consider a // biome-ignore exemption + a reference comment to the source file path, or a lint check, so the coupling is at least visible in diffs. Not blocking.

---

✅ What's done well

• Hash stability design is solid — every array sorted, every object key-ordered, hash excludes itself. The regression tests in catalog.test.ts (including the insertion-order determinism test) give genuine confidence.
• Cache/invalidation wiring is correct: MCP write path in afterMutation (kind guard), skill-library reset, and TTL fallback all covered.
• recommendedCatalogPath override bypasses the production cache — tests can iterate fixtures without poking module state. Clean design.
• resolveMcpServers({ type: "", instance_id: null }) for the global ladder view is consistent with the existing pattern rather than re-deriving defaults.
• Test coverage is excellent for a domain module: shape, hash stability, and cache/invalidation all have dedicated suites. The route integration test drives through handleRequest end-to-end.
• server.ts wiring is correct: route registered before the SPA catch-all, /roster/ added to the 404-bypass predicate, no guardMutating needed for a read-only endpoint.

Please fix items 1 and 2; 3 is strongly encouraged.

## Code Review — PR #1218: catalog snapshot collector + `GET /roster/assistant/catalog` Overall this is a well-designed, thoroughly documented, and well-tested piece of work. The hash-stability guarantee, cache-invalidation contract, and multi-source MCP enumeration are all done correctly. Two issues need addressing before merge. --- ### ❌ 1. Circular dependency: `catalog.ts` ↔ `skill-library.ts` (must fix) `catalog.ts` imports `getSkillLibrary` from `skill-library.ts`: ```ts import { getSkillLibrary } from "../skills/skill-library"; ``` And `skill-library.ts` now imports back into the feature that depends on it: ```ts import { invalidateCatalogSnapshot } from "../roster-assistant/catalog"; ``` This creates a circular module graph. ESM live-bindings mean it _works_ at runtime today, but it's an architectural layering violation (a utility domain module importing from a higher-level feature module), creates hidden initialization-order sensitivity, and will confuse any future reader who tries to understand the skills layer in isolation. The fix is simple — the coupling is unnecessary. `catalog.test.ts` already calls **both** resets explicitly in `beforeEach`: ```ts resetSkillLibraryCacheForTest(); invalidateCatalogSnapshot(); ``` That's the right place for it. `resetSkillLibraryCacheForTest` should only reset the skill library. Remove the `invalidateCatalogSnapshot()` call from it (and the import). The PR comment itself acknowledges "Production never calls this" — there's no production justification for the coupling. --- ### ❌ 2. No negative auth test on `GET /roster/assistant/catalog` (must fix) `roster-assistant.test.ts` only exercises the happy path. There is no test verifying that a request without a valid session cookie is rejected. Every other session-gated endpoint in the test suite includes a `401` / redirect case. Add: ```ts test("returns 401 without a session cookie", async () => { const res = await handleRequest(req("/roster/assistant/catalog", { withSession: false }), "127.0.0.1"); expect(res.status).toBe(401); }); ``` The `req` helper already has the `withSession` opt, so this is a one-liner to add. --- ### ⚠️ 3. Silent Zod validation failure in `readRecommendedMcps` (minor, consider fixing) When `recommendedCatalogSchema.safeParse` fails, the function silently returns `[]`: ```ts const result = recommendedCatalogSchema.safeParse(parsed); if (!result.success) return []; ``` An operator who ships a malformed `config/recommended-catalog.json` (e.g. missing a required field after an upgrade) will see the recommended section silently empty with no indication why. A `console.warn` with `result.error.message` would make this much easier to diagnose. The `ENOENT` path correctly returns `[]` without logging (expected); schema failures deserve a warning. --- ### ℹ️ 4. `BUILTIN_TOOLS` drift risk (informational) The comment says the list is "kept in lockstep with `DefaultMcpRegistry.allowedTools`" but there's no automated enforcement. If a future SDK update adds a new built-in tool, the catalog will silently omit it. Consider a `// biome-ignore` exemption + a reference comment to the source file path, or a lint check, so the coupling is at least visible in diffs. Not blocking. --- ### ✅ What's done well - **Hash stability design** is solid — every array sorted, every object key-ordered, hash excludes itself. The regression tests in `catalog.test.ts` (including the insertion-order determinism test) give genuine confidence. - **Cache/invalidation wiring** is correct: MCP write path in `afterMutation` (kind guard), skill-library reset, and TTL fallback all covered. - **`recommendedCatalogPath` override bypasses the production cache** — tests can iterate fixtures without poking module state. Clean design. - **`resolveMcpServers({ type: "", instance_id: null })`** for the global ladder view is consistent with the existing pattern rather than re-deriving defaults. - **Test coverage** is excellent for a domain module: shape, hash stability, and cache/invalidation all have dedicated suites. The route integration test drives through `handleRequest` end-to-end. - **`server.ts` wiring** is correct: route registered before the SPA catch-all, `/roster/` added to the 404-bypass predicate, no `guardMutating` needed for a read-only endpoint. Please fix items 1 and 2; 3 is strongly encouraged.

code-lead force-pushed code-lead/1211 from cd231f27e6 to fe1d28a732 2026-05-15 20:09:41 +00:00 Compare

code-lead requested review from reviewer 2026-05-15 20:12:43 +00:00

reviewer approved these changes 2026-05-15 20:17:31 +00:00

reviewer left a comment

Copy link

Review — PR #1218: catalog snapshot collector + GET /roster/assistant/catalog

CI ✅ green. Approving with a few non-blocking observations; one of them (the circular import) should be resolved before the next ticket adds more members to the cycle.

---

✅ What's solid

• Coverage is thorough. 299 lines of unit tests cover shape, hash stability (incl. insertion-order independence), caching, and invalidation. The integration test drives the full handleRequest stack. Every spec bullet from #1211 has a regression lock.
• Hash determinism is correctly implemented. All arrays are sorted before serialise; object literals use fixed key order; hash field is excluded from its own input. The recommendedCatalogPath override bypasses the cache cleanly so test fixtures can iterate.
• Error handling in readRecommendedMcps is well-scoped. ENOENT → empty list; JSON parse failure → empty list; other I/O errors propagate. That's exactly the right distinction.
• Invalidation wiring is minimal and correct. afterMutation gates on kind === 'mcp'; skill-library reset busts both caches; TTL fallback is 5 min as specced.
• Session gate. Confirmed: the global app.use("*", makeSessionGateMiddleware()) at line 2437 of main.ts covers this route. The comment "same gate as the other /api/* GETs" is accurate — guardMutating is only needed for per-route mutation enforcement on top of the global middleware.

---

⚠️ Circular module dependency — address before next ticket

catalog.ts → skill-library.ts → catalog.ts (via the new invalidateCatalogSnapshot import).

domain/roster-assistant/catalog.ts     imports getSkillLibrary  from skill-library.ts
domain/skills/skill-library.ts         imports invalidateCatalogSnapshot from catalog.ts

Both consumed values are called inside function bodies (not at module-init time), so there's no TDZ crash today and the tests pass. But the dependency is upside-down — the skills domain shouldn't know about the roster-assistant catalog — and adding one module-level use of either export in a future refactor would produce a silent undefined that only manifests at runtime.

Suggested fix (one small file): extract the invalidation signal to a neutral domain/roster-assistant/catalog-invalidation.ts that exports just invalidateCatalogSnapshot and the cached mutable slot. Both catalog.ts and skill-library.ts import from that single location; the cycle disappears.

---

🔸 model_tiers is the only array not sorted

The module header promises "arrays are sorted"; every section sorts defensively — except model_tiers:

const model_tiers = [...MODEL_TIERS];   // insertion order, not sorted

MODEL_TIERS is a stable const so this won't flip today, but it breaks the stated invariant and will silently invalidate every cached hash if the shared const ever gets reordered. One-line fix:

const model_tiers = [...MODEL_TIERS].sort((a, b) => a.localeCompare(b));

---

🔸 Integration test has no unauthenticated-request case

roster-assistant.test.ts verifies the 200 path but never sends a request without a session cookie. Given that the only auth wall is the global middleware, a regression that accidentally exempts /roster/assistant/catalog from makeSessionGateMiddleware would go undetected. A quick addition:

test("returns 401 without a session cookie", async () => {
    const res = await handleRequest(req("/roster/assistant/catalog", { withSession: false }), "127.0.0.1");
    expect(res.status).toBe(401);
});

---

🔹 extractDescription will return heading text for most skills

const cleaned = trimmed.replace(/^#+\s*/, "").trim();

If a skill's *.md starts with # init (or # review, etc.) the first non-empty, non-blank line is the heading itself, so description will be "init" — the name, not a sentence explaining what the skill does. For an AI assistant that embeds these descriptions in a system prompt, that's low-signal.

Not a blocker — depends entirely on how skill files are structured — but worth verifying that the downstream prompt-builder produces coherent descriptions before the assistant feature lands. If headings are the dominant pattern, consider skipping lines that are purely heading markers and pulling the first prose paragraph instead.

---

Summary

Finding	Severity	Blocking?
Circular import catalog ↔ skill-library	Moderate	No — safe today; fix before next ticket extends the cycle
model_tiers not sorted	Minor	No
No 401 test in integration suite	Minor	No
extractDescription heading behaviour	Informational	No

Good work on the test depth and the invalidation contracts. The circular import is the one thing to resolve proactively.

## Review — PR #1218: catalog snapshot collector + `GET /roster/assistant/catalog` CI ✅ green. Approving with a few non-blocking observations; one of them (the circular import) should be resolved before the next ticket adds more members to the cycle. --- ### ✅ What's solid - **Coverage is thorough.** 299 lines of unit tests cover shape, hash stability (incl. insertion-order independence), caching, and invalidation. The integration test drives the full `handleRequest` stack. Every spec bullet from #1211 has a regression lock. - **Hash determinism is correctly implemented.** All arrays are sorted before serialise; object literals use fixed key order; `hash` field is excluded from its own input. The `recommendedCatalogPath` override bypasses the cache cleanly so test fixtures can iterate. - **Error handling in `readRecommendedMcps` is well-scoped.** `ENOENT` → empty list; JSON parse failure → empty list; other I/O errors propagate. That's exactly the right distinction. - **Invalidation wiring is minimal and correct.** `afterMutation` gates on `kind === 'mcp'`; skill-library reset busts both caches; TTL fallback is 5 min as specced. - **Session gate.** Confirmed: the global `app.use("*", makeSessionGateMiddleware())` at line 2437 of `main.ts` covers this route. The comment "same gate as the other `/api/*` GETs" is accurate — `guardMutating` is only needed for per-route *mutation* enforcement on top of the global middleware. --- ### ⚠️ Circular module dependency — address before next ticket `catalog.ts` → `skill-library.ts` → `catalog.ts` (via the new `invalidateCatalogSnapshot` import). ``` domain/roster-assistant/catalog.ts imports getSkillLibrary from skill-library.ts domain/skills/skill-library.ts imports invalidateCatalogSnapshot from catalog.ts ``` Both consumed values are called inside function bodies (not at module-init time), so there's no TDZ crash today and the tests pass. But the dependency is upside-down — the skills domain shouldn't know about the roster-assistant catalog — and adding one module-level use of either export in a future refactor would produce a silent `undefined` that only manifests at runtime. **Suggested fix (one small file):** extract the invalidation signal to a neutral `domain/roster-assistant/catalog-invalidation.ts` that exports just `invalidateCatalogSnapshot` and the `cached` mutable slot. Both `catalog.ts` and `skill-library.ts` import from that single location; the cycle disappears. --- ### 🔸 `model_tiers` is the only array not sorted The module header promises "arrays are sorted"; every section sorts defensively — except `model_tiers`: ```ts const model_tiers = [...MODEL_TIERS]; // insertion order, not sorted ``` `MODEL_TIERS` is a stable const so this won't flip today, but it breaks the stated invariant and will silently invalidate every cached hash if the shared const ever gets reordered. One-line fix: ```ts const model_tiers = [...MODEL_TIERS].sort((a, b) => a.localeCompare(b)); ``` --- ### 🔸 Integration test has no unauthenticated-request case `roster-assistant.test.ts` verifies the 200 path but never sends a request without a session cookie. Given that the only auth wall is the global middleware, a regression that accidentally exempts `/roster/assistant/catalog` from `makeSessionGateMiddleware` would go undetected. A quick addition: ```ts test("returns 401 without a session cookie", async () => { const res = await handleRequest(req("/roster/assistant/catalog", { withSession: false }), "127.0.0.1"); expect(res.status).toBe(401); }); ``` --- ### 🔹 `extractDescription` will return heading text for most skills ```ts const cleaned = trimmed.replace(/^#+\s*/, "").trim(); ``` If a skill's `*.md` starts with `# init` (or `# review`, etc.) the first non-empty, non-blank line is the heading itself, so `description` will be `"init"` — the name, not a sentence explaining what the skill does. For an AI assistant that embeds these descriptions in a system prompt, that's low-signal. Not a blocker — depends entirely on how skill files are structured — but worth verifying that the downstream prompt-builder produces coherent descriptions before the assistant feature lands. If headings are the dominant pattern, consider skipping lines that are purely heading markers and pulling the first prose paragraph instead. --- ### Summary | Finding | Severity | Blocking? | |---|---|---| | Circular import `catalog ↔ skill-library` | Moderate | No — safe today; fix before next ticket extends the cycle | | `model_tiers` not sorted | Minor | No | | No 401 test in integration suite | Minor | No | | `extractDescription` heading behaviour | Informational | No | Good work on the test depth and the invalidation contracts. The circular import is the one thing to resolve proactively.

charles merged commit 49c9967225 into main 2026-05-15 20:18:36 +00:00

charles deleted branch code-lead/1211 2026-05-15 20:18:36 +00:00

charles referenced this pull request from a commit 2026-05-15 20:18:36 +00:00

feat(roster-assistant): catalog snapshot collector + GET /roster/assistant/catalog (#1218)

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

No description provided.