Remote Muster MCP endpoint for Hermes - #82
Conversation
Ship the first Hermes-native Muster MCP vertical slice: a Streamable HTTP MCP server (apps/mcp-server) backed by revocable, organisation- and actor-scoped installation credentials (packages/mcp), exposing four read-only tools (muster_get_status, muster_list_capabilities, muster_search_kelpie_cases, muster_get_kelpie_case) that route Kelpie access through the existing governed connector path (integration_query_runs -> outbox -> the unmodified worker's processConnectorQuery), never letting connector credentials or model-supplied tenant/capability fields reach Hermes. Every call is bounded, redacted, classified as untrusted evidence, and appended to the existing hash-chained audit log with tool/version, installation/actor, outcome, result hash, and evidence references. Adds the mcp_installations migration, a starter Hermes skill (skills/muster-soc-operations/SKILL.md), operator docs for connecting Hermes to the endpoint, and ADR 0005. CLI scripts (create-installation/revoke-installation) provision credentials without a chat- or UI-driven registration flow, consistent with the "no arbitrary MCP registration" boundary for this slice. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a remote, installation-authenticated MCP server with four read-only Kelpie tools, governed query execution, evidence redaction, audit logging, database support, health checks, graceful shutdown, integration tests, and Hermes integration documentation. ChangesRemote Muster MCP server
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
packages/mcp/src/kelpie-gateway.ts (2)
79-96: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRate-limit check is racy (read outside the insert transaction).
The count is read before
db.transaction, so concurrent MCP calls can each observe a sub-limit count and all insert, overshootinglimits.requestsPerMinute. Acceptable as a soft limit, but if the cap is meant to protect the Kelpie upstream, move the count inside the transaction (with an appropriate isolation level) or use a dedicated counter/limiter keyed byorganisationId:integrationId.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mcp/src/kelpie-gateway.ts` around lines 79 - 96, The rate-limit check around the recent query count is racy because it runs outside the insert transaction. Move the count and limit decision into the transaction that records the integration query run, using an appropriate isolation level or an atomic counter/limiter keyed by organisationId and integrationId so concurrent calls cannot exceed limits.requestsPerMinute.
198-240: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffBounded DB polling holds the HTTP request for up to
timeoutMs.Each iteration issues a fresh query and sleeps, so a slow worker keeps an MCP request occupied for the full window (8s per
KELPIE_POLL_OPTIONSintools.ts) and consumes a pool connection per concurrent call. Acceptable for the first slice, but consider a listen/notify or a "run id + fetch later" contract so the tool can return immediately, and cap concurrent pollers.As per coding guidelines: "Keep long-running integration and agent work outside HTTP request handlers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mcp/src/kelpie-gateway.ts` around lines 198 - 240, The pollKelpieQuery implementation performs bounded polling inside the HTTP request handler, holding requests and database capacity for up to timeoutMs. Replace this synchronous polling contract with an immediate run-id response and a separate fetch-later path, or use a database listen/notify mechanism so completion does not require repeated queries and sleeps within the handler; preserve the existing organisation and run-id authorization checks and terminal result mapping.Source: Coding guidelines
packages/mcp/src/redact.ts (1)
6-45: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBounding is per-string only; nested breadth/depth is unbounded.
truncateStrings(andredactUntrustedbefore it) recurse over arbitrary external evidence: a deeply nested or very wide Kelpie response still passes through in full, so a single record with thousands of keys/elements can blow the payload budget or overflow the stack. Since the stated goal is that oversized content never survives this step, consider a depth cap and per-record array/key cap alongsideMAX_STRING_LENGTH.♻️ Sketch: depth and breadth caps
const MAX_STRING_LENGTH = 4_000; +const MAX_DEPTH = 8; +const MAX_CHILDREN = 200; -function truncateStrings(value: unknown): unknown { +function truncateStrings(value: unknown, depth = 0): unknown { if (typeof value === "string") return value.length > MAX_STRING_LENGTH ? `${value.slice(0, MAX_STRING_LENGTH)}…[truncated]` : value; - if (Array.isArray(value)) return value.map(truncateStrings); + if (depth >= MAX_DEPTH) return "[truncated]"; + if (Array.isArray(value)) + return value.slice(0, MAX_CHILDREN).map((item) => truncateStrings(item, depth + 1)); if (value && typeof value === "object") return Object.fromEntries( - Object.entries(value as Record<string, unknown>).map(([key, entry]) => [ - key, - truncateStrings(entry), - ]), + Object.entries(value as Record<string, unknown>) + .slice(0, MAX_CHILDREN) + .map(([key, entry]) => [key, truncateStrings(entry, depth + 1)]), ); return value; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mcp/src/redact.ts` around lines 6 - 45, Update truncateStrings and the redactUntrusted traversal used by classifyKelpieRecords to enforce per-record depth and breadth limits in addition to MAX_STRING_LENGTH. Cap nested object keys and array elements, stop or safely truncate traversal once the depth limit is reached, and preserve the existing ClassifiedRecords shape while ensuring oversized nested structures cannot pass through in full or cause stack exhaustion.docs/architecture/0005-remote-mcp-server.md (1)
43-55: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift8-second bounded poll runs inside the HTTP request handler.
Because
muster_search_kelpie_cases/muster_get_kelpie_casepoll theintegration_query_runsrow for up to 8s as part of the tool call, and the tool call executes insidetransport.handleRequestwithin the rawnode:httphandler inapps/mcp-server/src/index.ts, each Kelpie tool invocation ties up its HTTP request for up to 8s. This is in tension with the guideline to keep long-running integration work out of request handlers, though it may be an unavoidable consequence of MCP's synchronous tool-call/response model. Worth confirming thenode:httpserver has appropriate request/socket timeouts configured so a burst of concurrent 8s-bound calls can't exhaust connection resources.As per coding guidelines, "Keep long-running integration and agent work outside HTTP request handlers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture/0005-remote-mcp-server.md` around lines 43 - 55, The Kelpie tool polling in muster_search_kelpie_cases and muster_get_kelpie_case can occupy the raw node:http request handler for up to 8 seconds. Review apps/mcp-server/src/index.ts around transport.handleRequest and configure appropriate request and socket timeouts for the node:http server, ensuring concurrent bounded polls cannot leave connections indefinitely open while preserving the existing synchronous MCP response behavior.Source: Coding guidelines
apps/mcp-server/src/index.ts (1)
35-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
/healthdoesn't actually verify readiness, despite the code and docs implying it does. The handler returns a static{status: "ready", authority: "postgresql"}regardless of whetherdbis reachable, while the docs describe it as reporting readiness.
apps/mcp-server/src/index.ts#L35-L38: perform a lightweight liveness check (e.g., a trivialSELECT 1viadb) before returningstatus: "ready", so orchestrators don't get false-positive readiness during a Postgres outage.docs/integrations/hermes-mcp.md#L74-L75: once the check reflects real state, keep this description; otherwise adjust the wording to clarify it's a process-liveness check rather than a dependency-readiness check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mcp-server/src/index.ts` around lines 35 - 38, Update the GET /health handler in apps/mcp-server/src/index.ts (lines 35-38) to perform a lightweight db liveness query, such as SELECT 1, before returning status "ready"; report an appropriate non-ready response when the check fails. Keep docs/integrations/hermes-mcp.md (lines 74-75) unchanged if it already describes dependency readiness, or revise it to describe process liveness if the endpoint does not verify db availability.packages/mcp/src/server.ts (1)
34-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDenied/error classification relies on brittle message-string matching, duplicated in two places.
Both
toolFailure(Lines 40-42) andisDenied(Lines 53-54) independently parseerror.message.startsWith("Installation is not scoped").requireScopeinpackages/mcp/src/installation.tsthrows a plainError, so any future wording change to that message silently breaks the "denied" audit classification (it would fall through to "error"), which matters for governance monitoring given this repo's requirement to preserve accurate audit/evidence metadata.As per coding guidelines, "Preserve append-only messages, timelines, evidence metadata, and audit events." — the outcome field depends on this fragile check.
♻️ Proposed fix: introduce a typed scope error
// packages/mcp/src/installation.ts export class ScopeError extends Error { constructor(tool: McpToolName) { super(`Installation is not scoped for ${tool}`); this.name = "ScopeError"; } } export function requireScope(context: InstallationContext, tool: McpToolName): void { if (!context.scopes.includes(tool)) throw new ScopeError(tool); }-import { requireScope } from "./installation.ts"; +import { requireScope, ScopeError } from "./installation.ts"; ... - : error instanceof Error && - error.message.startsWith("Installation is not scoped") - ? error.message + : error instanceof ScopeError + ? error.message ... - error instanceof ForbiddenError || - (error instanceof Error && - error.message.startsWith("Installation is not scoped")) + error instanceof ForbiddenError || error instanceof ScopeError🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mcp/src/server.ts` around lines 34 - 56, Introduce and export a typed ScopeError in requireScope, preserving the existing message format, and throw it when the requested tool lacks scope. Update toolFailure and isDenied to classify ScopeError with instanceof checks instead of matching error.message, reusing the shared type so denied audit outcomes remain stable if wording changes.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/mcp-server/src/index.ts`:
- Around line 83-89: Update shutdown so closeDatabase() runs only after
server.close() has completed draining existing connections and emitted its
completion callback. Preserve the SIGINT and SIGTERM handlers’ use of shutdown,
and ensure database cleanup still occurs after the server-close operation
finishes.
In `@packages/database/migrations/0021_colorful_saracen.sql`:
- Around line 19-22: Update the mcp_installations actor relationships
represented by mcp_installations_bound_actor_id_actors_id_fk,
mcp_installations_installed_by_actor_id_actors_id_fk, and
mcp_installations_revoked_by_actor_id_actors_id_fk to enforce matching
organisation_id values, preferably by adding the required unique actors key and
composite foreign keys on (actor_id, organisation_id); otherwise validate all
three actor IDs against the installation organisation within the same
create/update transaction.
In `@packages/mcp/src/cli-revoke-installation.ts`:
- Around line 12-30: Before calling revokeInstallation in main, validate
server-side that revokedByActorId has the required capability for the specified
organisation and installationId has an approved revocation record. Perform both
checks through the existing authorization/approval mechanisms, reject the
request when either is missing, and only then invoke revokeInstallation; do not
treat traceId or audit metadata as authorization.
In `@packages/mcp/src/installation.ts`:
- Around line 39-115: Update createInstallation and revokeInstallation to
validate every referenced actor belongs to input.organisationId, enforce the
server-side capability authorization for the installing or revoking actor, and
require the corresponding approval record before changing mcpInstallations.
Perform these checks within the same transaction before the mutation, and only
append the existing audit event after authorization and approval succeed.
In `@packages/mcp/src/mcp.integration.test.ts`:
- Around line 509-587: Update the SSRF test to exercise the MCP gateway path
through the relevant tool, such as muster_search_kelpie_cases or
muster_get_kelpie_case, using an installation scoped to ssrfIntegrationId and
asserting the tool or audit result reflects denial; retain the seeded
integration records and remove the stale gateway-selection comment. If this test
is intentionally limited to executeGovernedQuery, remove the unused database
setup and comment instead.
In `@packages/mcp/src/server.ts`:
- Around line 64-88: Update the error-path audit handling in invoke so failures
from recordInvocation are still caught but logged with sufficient context for
operators to detect the audit gap, rather than silently ignored. Preserve the
existing toolFailure(error) response and the denied/error outcome
classification, and use the server’s existing logging mechanism.
In `@packages/mcp/src/tools.ts`:
- Around line 133-141: Update the settled-error handling around the
failed-result branch to stop mapping malformed_response to not_found. Preserve
source_unavailable and malformed_response as distinct upstream failures, map
upstream_error consistently for both, and reserve not_found for successful
results that are empty or null.
- Line 84: Replace the per-call newId() component in the idempotency keys used
by the list and getKelpieCase request paths with a deterministic key derived
from the MCP request identity or stable request inputs, including installationId
and the operation/template arguments as needed. Update both the visible list key
and the corresponding key near getKelpieCase so retries and transport replays
produce the same key and allow queueKelpieQuery deduplication.
- Around line 99-106: Update the tool flow around the Kelpie cases-list
invocation to pass args.query through the template input when supported, rather
than always using an empty input and filtering JSON locally. Remove the
client-side JSON.stringify matching once the query is delegated, or explicitly
document it as a best-effort filter over the returned page if the template
cannot accept query parameters.
---
Nitpick comments:
In `@apps/mcp-server/src/index.ts`:
- Around line 35-38: Update the GET /health handler in
apps/mcp-server/src/index.ts (lines 35-38) to perform a lightweight db liveness
query, such as SELECT 1, before returning status "ready"; report an appropriate
non-ready response when the check fails. Keep docs/integrations/hermes-mcp.md
(lines 74-75) unchanged if it already describes dependency readiness, or revise
it to describe process liveness if the endpoint does not verify db availability.
In `@docs/architecture/0005-remote-mcp-server.md`:
- Around line 43-55: The Kelpie tool polling in muster_search_kelpie_cases and
muster_get_kelpie_case can occupy the raw node:http request handler for up to 8
seconds. Review apps/mcp-server/src/index.ts around transport.handleRequest and
configure appropriate request and socket timeouts for the node:http server,
ensuring concurrent bounded polls cannot leave connections indefinitely open
while preserving the existing synchronous MCP response behavior.
In `@packages/mcp/src/kelpie-gateway.ts`:
- Around line 79-96: The rate-limit check around the recent query count is racy
because it runs outside the insert transaction. Move the count and limit
decision into the transaction that records the integration query run, using an
appropriate isolation level or an atomic counter/limiter keyed by organisationId
and integrationId so concurrent calls cannot exceed limits.requestsPerMinute.
- Around line 198-240: The pollKelpieQuery implementation performs bounded
polling inside the HTTP request handler, holding requests and database capacity
for up to timeoutMs. Replace this synchronous polling contract with an immediate
run-id response and a separate fetch-later path, or use a database listen/notify
mechanism so completion does not require repeated queries and sleeps within the
handler; preserve the existing organisation and run-id authorization checks and
terminal result mapping.
In `@packages/mcp/src/redact.ts`:
- Around line 6-45: Update truncateStrings and the redactUntrusted traversal
used by classifyKelpieRecords to enforce per-record depth and breadth limits in
addition to MAX_STRING_LENGTH. Cap nested object keys and array elements, stop
or safely truncate traversal once the depth limit is reached, and preserve the
existing ClassifiedRecords shape while ensuring oversized nested structures
cannot pass through in full or cause stack exhaustion.
In `@packages/mcp/src/server.ts`:
- Around line 34-56: Introduce and export a typed ScopeError in requireScope,
preserving the existing message format, and throw it when the requested tool
lacks scope. Update toolFailure and isDenied to classify ScopeError with
instanceof checks instead of matching error.message, reusing the shared type so
denied audit outcomes remain stable if wording changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ce6ba0f5-ca91-4323-b769-927a04ded504
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
apps/mcp-server/package.jsonapps/mcp-server/src/index.tsapps/mcp-server/tsconfig.jsondocs/architecture/0005-remote-mcp-server.mddocs/integrations/README.mddocs/integrations/hermes-mcp.mdpackages/database/migrations/0021_colorful_saracen.sqlpackages/database/migrations/meta/0021_snapshot.jsonpackages/database/migrations/meta/_journal.jsonpackages/database/src/schema.tspackages/database/src/verify-clean-install.tspackages/mcp/package.jsonpackages/mcp/src/audit.tspackages/mcp/src/cli-create-installation.tspackages/mcp/src/cli-revoke-installation.tspackages/mcp/src/constants.tspackages/mcp/src/errors.tspackages/mcp/src/index.tspackages/mcp/src/installation.test.tspackages/mcp/src/installation.tspackages/mcp/src/kelpie-gateway.tspackages/mcp/src/mcp.integration.test.tspackages/mcp/src/redact.test.tspackages/mcp/src/redact.tspackages/mcp/src/server.tspackages/mcp/src/tools.tspackages/mcp/tsconfig.jsonskills/muster-soc-operations/SKILL.md
Corrective pass on PR #82 covering all 9 CodeRabbit findings plus the bounded-polling/rate-limit nitpicks: - Authoritative capability + actor-organisation checks for installation create/revoke, re-derived from the database inside the mutation's own transaction; a composite (actor_id, organisation_id) -> actors(id, organisation_id) FK enforces the same tenant boundary at the schema level. - Graceful HTTP shutdown (drain before closing the DB pool), a real dependency-aware /health check, and explicit request/socket timeouts. - Fixed a latent crash: @muster/database's pg Pool had no 'error' listener, so any idle-client error (e.g. a DB restart) was an unhandled Node event that killed the whole process -- exactly what the new health check would trigger during a real outage. Reproduced live and confirmed fixed. - Typed ScopeError instead of string-matching for denied-outcome audit classification; audit-write failures on the error path are now logged, not swallowed. - Deterministic, time-windowed idempotency keys for Kelpie queries (retries dedupe; a genuinely later query still gets fresh data). - Kept malformed_response distinct from not_found; local query filtering now matches specific fields (with a documented best-effort-page caveat) rather than a stringified-record substring match. - Atomic rate-limit check via an integration-scoped advisory lock, mirroring appendAuditEvent's existing pattern. - Rewired the SSRF integration test through the real muster_search_kelpie_cases tool/gateway path instead of calling executeGovernedQuery directly. - Bounded nested evidence depth/breadth in redact.ts. - Squashed the mcp_installations migration into one clean 0021 (it was only ever unreleased in this PR) instead of leaving a same-PR fix-up migration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/database/migrations/0021_dazzling_living_mummy.sql (1)
1-1: 🩺 Stability & Availability | 🔵 TrivialPlan a non-blocking rollout for this index.
A regular
CREATE UNIQUE INDEXblocks writes toactorswhile it builds. For a populated production table, create it concurrently outside the transactional migration runner, or schedule a maintenance window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/migrations/0021_dazzling_living_mummy.sql` at line 1, Update the rollout for the actors_id_organisation_unique index so it is created concurrently outside the transactional migration runner, or explicitly schedule its creation during a maintenance window. Preserve the unique constraint while avoiding write blocking on populated production tables.Source: Linters/SAST tools
packages/mcp/src/mcp.integration.test.ts (1)
375-377: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the inline dynamic imports to the top of the file.
await import("node:crypto")andawait import("./installation.ts")mid-test are unnecessary; a static top-level import is simpler and idiomatic here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mcp/src/mcp.integration.test.ts` around lines 375 - 377, Move the dynamic imports for node:crypto and hashInstallationToken from the test body to static top-level imports in mcp.integration.test.ts, then reuse those imported symbols where mismatchedToken is created and the token is hashed. Remove the inline await import statements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mcp/src/mcp.integration.test.ts`:
- Around line 650-695: Update the integrationQueryRuns lookup in the real MCP
gateway test to also filter by organisationId alongside integrationId, matching
the domain-query scoping used elsewhere in the file; keep the existing ordering
and errorCode assertion unchanged.
- Around line 378-389: Strengthen the rejection assertion for the mismatched
installation insert to match the specific composite actor/organisation
foreign-key violation, rather than accepting any thrown error. Preserve the
existing insert setup and use an error-message matcher consistent with the
database error surfaced by the other authorization checks.
In `@packages/mcp/src/tools.ts`:
- Around line 124-129: Move the synchronous pollKelpieQuery calls out of the
HTTP request handlers in both search and case-lookup flows:
packages/mcp/src/tools.ts lines 124-129 and 164-169. Return an accepted/pending
response immediately after queuing each request, and arrange asynchronous result
retrieval for completion, preserving the existing queued request context and
applying the same pattern to both tools.
- Around line 24-30: Update deterministicIdempotencyKey to remove the
Date.now-based bucket from the key and derive it solely from a stable request ID
combined with the installation and tool context. Ensure repeated logical MCP
calls, including retries crossing the 5-second boundary, produce the same key
for queueKelpieQuery.
---
Nitpick comments:
In `@packages/database/migrations/0021_dazzling_living_mummy.sql`:
- Line 1: Update the rollout for the actors_id_organisation_unique index so it
is created concurrently outside the transactional migration runner, or
explicitly schedule its creation during a maintenance window. Preserve the
unique constraint while avoiding write blocking on populated production tables.
In `@packages/mcp/src/mcp.integration.test.ts`:
- Around line 375-377: Move the dynamic imports for node:crypto and
hashInstallationToken from the test body to static top-level imports in
mcp.integration.test.ts, then reuse those imported symbols where mismatchedToken
is created and the token is hashed. Remove the inline await import statements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60e50d03-ef68-49b4-9681-5d1e77f1e7d6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
apps/mcp-server/package.jsonapps/mcp-server/src/health.test.tsapps/mcp-server/src/health.tsapps/mcp-server/src/index.tsapps/mcp-server/src/shutdown.test.tsapps/mcp-server/src/shutdown.tsdocs/architecture/0005-remote-mcp-server.mddocs/integrations/hermes-mcp.mdpackages/database/migrations/0021_dazzling_living_mummy.sqlpackages/database/migrations/meta/0021_snapshot.jsonpackages/database/migrations/meta/_journal.jsonpackages/database/src/index.tspackages/database/src/schema.tspackages/mcp/src/cli-create-installation.tspackages/mcp/src/cli-revoke-installation.tspackages/mcp/src/installation.tspackages/mcp/src/kelpie-gateway.tspackages/mcp/src/mcp.integration.test.tspackages/mcp/src/redact.tspackages/mcp/src/server.tspackages/mcp/src/tools.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/mcp-server/package.json
- packages/mcp/src/cli-revoke-installation.ts
- packages/mcp/src/cli-create-installation.ts
- packages/mcp/src/redact.ts
- docs/integrations/hermes-mcp.md
- apps/mcp-server/src/index.ts
- packages/mcp/src/kelpie-gateway.ts
- packages/mcp/src/server.ts
- packages/database/migrations/meta/0021_snapshot.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/database/migrations/0021_dazzling_living_mummy.sql (1)
1-1: 🩺 Stability & Availability | 🔵 TrivialPlan a non-blocking rollout for this index.
A regular
CREATE UNIQUE INDEXblocks writes toactorswhile it builds. For a populated production table, create it concurrently outside the transactional migration runner, or schedule a maintenance window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/database/migrations/0021_dazzling_living_mummy.sql` at line 1, Update the rollout for the actors_id_organisation_unique index so it is created concurrently outside the transactional migration runner, or explicitly schedule its creation during a maintenance window. Preserve the unique constraint while avoiding write blocking on populated production tables.Source: Linters/SAST tools
packages/mcp/src/mcp.integration.test.ts (1)
375-377: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the inline dynamic imports to the top of the file.
await import("node:crypto")andawait import("./installation.ts")mid-test are unnecessary; a static top-level import is simpler and idiomatic here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mcp/src/mcp.integration.test.ts` around lines 375 - 377, Move the dynamic imports for node:crypto and hashInstallationToken from the test body to static top-level imports in mcp.integration.test.ts, then reuse those imported symbols where mismatchedToken is created and the token is hashed. Remove the inline await import statements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mcp/src/mcp.integration.test.ts`:
- Around line 650-695: Update the integrationQueryRuns lookup in the real MCP
gateway test to also filter by organisationId alongside integrationId, matching
the domain-query scoping used elsewhere in the file; keep the existing ordering
and errorCode assertion unchanged.
- Around line 378-389: Strengthen the rejection assertion for the mismatched
installation insert to match the specific composite actor/organisation
foreign-key violation, rather than accepting any thrown error. Preserve the
existing insert setup and use an error-message matcher consistent with the
database error surfaced by the other authorization checks.
In `@packages/mcp/src/tools.ts`:
- Around line 124-129: Move the synchronous pollKelpieQuery calls out of the
HTTP request handlers in both search and case-lookup flows:
packages/mcp/src/tools.ts lines 124-129 and 164-169. Return an accepted/pending
response immediately after queuing each request, and arrange asynchronous result
retrieval for completion, preserving the existing queued request context and
applying the same pattern to both tools.
- Around line 24-30: Update deterministicIdempotencyKey to remove the
Date.now-based bucket from the key and derive it solely from a stable request ID
combined with the installation and tool context. Ensure repeated logical MCP
calls, including retries crossing the 5-second boundary, produce the same key
for queueKelpieQuery.
---
Nitpick comments:
In `@packages/database/migrations/0021_dazzling_living_mummy.sql`:
- Line 1: Update the rollout for the actors_id_organisation_unique index so it
is created concurrently outside the transactional migration runner, or
explicitly schedule its creation during a maintenance window. Preserve the
unique constraint while avoiding write blocking on populated production tables.
In `@packages/mcp/src/mcp.integration.test.ts`:
- Around line 375-377: Move the dynamic imports for node:crypto and
hashInstallationToken from the test body to static top-level imports in
mcp.integration.test.ts, then reuse those imported symbols where mismatchedToken
is created and the token is hashed. Remove the inline await import statements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60e50d03-ef68-49b4-9681-5d1e77f1e7d6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
apps/mcp-server/package.jsonapps/mcp-server/src/health.test.tsapps/mcp-server/src/health.tsapps/mcp-server/src/index.tsapps/mcp-server/src/shutdown.test.tsapps/mcp-server/src/shutdown.tsdocs/architecture/0005-remote-mcp-server.mddocs/integrations/hermes-mcp.mdpackages/database/migrations/0021_dazzling_living_mummy.sqlpackages/database/migrations/meta/0021_snapshot.jsonpackages/database/migrations/meta/_journal.jsonpackages/database/src/index.tspackages/database/src/schema.tspackages/mcp/src/cli-create-installation.tspackages/mcp/src/cli-revoke-installation.tspackages/mcp/src/installation.tspackages/mcp/src/kelpie-gateway.tspackages/mcp/src/mcp.integration.test.tspackages/mcp/src/redact.tspackages/mcp/src/server.tspackages/mcp/src/tools.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/mcp-server/package.json
- packages/mcp/src/cli-revoke-installation.ts
- packages/mcp/src/cli-create-installation.ts
- packages/mcp/src/redact.ts
- docs/integrations/hermes-mcp.md
- apps/mcp-server/src/index.ts
- packages/mcp/src/kelpie-gateway.ts
- packages/mcp/src/server.ts
- packages/database/migrations/meta/0021_snapshot.json
🛑 Comments failed to post (2)
packages/mcp/src/tools.ts (2)
24-30: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the file and inspect the relevant sections. wc -l packages/mcp/src/tools.ts sed -n '1,240p' packages/mcp/src/tools.ts printf '\n--- usages ---\n' rg -n "deterministicIdempotencyKey|IDEMPOTENCY_WINDOW_MS|idempotency" packages/mcp/src -SRepository: jusso-dev/Muster
Length of output: 6961
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the surrounding implementation in manageable slices. sed -n '1,220p' packages/mcp/src/tools.ts printf '\n---\n' sed -n '220,420p' packages/mcp/src/tools.ts printf '\n--- call sites ---\n' rg -n "deterministicIdempotencyKey\\(" packages -SRepository: jusso-dev/Muster
Length of output: 6374
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect queue dedupe behavior and the corresponding integration test. sed -n '1,260p' packages/mcp/src/kelpie-gateway.ts printf '\n--- test ---\n' sed -n '520,640p' packages/mcp/src/mcp.integration.test.tsRepository: jusso-dev/Muster
Length of output: 12769
Use a stable request identity for idempotency
packages/mcp/src/tools.ts:24-30A retry that crosses the 5s bucket boundary gets a different key even when it’s the same logical MCP call, so
queueKelpieQuerytreats it as fresh work and can enqueue a second Kelpie run. Derive the key from a stable request ID plus installation/tool context instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mcp/src/tools.ts` around lines 24 - 30, Update deterministicIdempotencyKey to remove the Date.now-based bucket from the key and derive it solely from a stable request ID combined with the installation and tool context. Ensure repeated logical MCP calls, including retries crossing the 5-second boundary, produce the same key for queueKelpieQuery.Source: Coding guidelines
124-129: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Move connector-result waiting off the HTTP request path.
Both tools wait up to eight seconds for worker-backed Kelpie processing, tying up MCP HTTP capacity during connector slowness. Return an accepted/pending result and retrieve completion asynchronously instead.
packages/mcp/src/tools.ts#L124-L129: replace synchronous search polling with asynchronous result handling.packages/mcp/src/tools.ts#L164-L169: apply the same asynchronous pattern to case lookup.As per coding guidelines, “Keep long-running integration and agent work outside HTTP request handlers.”
📍 Affects 1 file
packages/mcp/src/tools.ts#L124-L129(this comment)packages/mcp/src/tools.ts#L164-L169🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mcp/src/tools.ts` around lines 124 - 129, Move the synchronous pollKelpieQuery calls out of the HTTP request handlers in both search and case-lookup flows: packages/mcp/src/tools.ts lines 124-129 and 164-169. Return an accepted/pending response immediately after queuing each request, and arrange asynchronous result retrieval for completion, preserving the existing queued request context and applying the same pattern to both tools.Source: Coding guidelines
MUSTER_DEMO_MODE seeding reused the bootstrap workspace's admin@muster.local identity and soc-operations room slug, so an ambiguous lookup by either identifier could resolve to the wrong organisation's row and crash the web dev server mid-request (surfacing as "Second seeded room required" and cascading into unrelated e2e failures). Rename the two starter fixtures before seeding demo data, and switch the e2e suite and room-view.tsx off hardcoded starter-namespace UUIDs onto demoIds so they stay in sync. Confirmed pre-existing on main (identical failure signature); not introduced by this PR. Verified locally: 26/26 passed, 0 failed, 1 skipped (previously 9 failed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Tighten the FK-violation assertion to check the driver's underlying cause message (foreign key/violates), not just "any thrown error" - drizzle-orm wraps the real Postgres error under `.cause`. - Scope the SSRF-denial run lookup by organisationId in addition to integrationId, matching every other domain query in the file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/database/src/seed.ts`:
- Around line 18-29: Update the two seed updates for actors and rooms to include
starterIds.organisation in their WHERE predicates alongside the existing record
ID conditions. Keep both updates tenant-scoped while preserving their current
field values and target IDs.
- Around line 18-29: Wrap the actor and room updates in the seed bootstrap flow
within a single db.transaction callback, using the transaction handle for both
update operations. Preserve the existing update values and conditions so both
changes commit together or roll back together.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7728ba36-048b-453d-8bf0-c39b44ceeb86
📒 Files selected for processing (4)
apps/web/components/room-view.tsxpackages/database/src/seed.tspackages/mcp/src/mcp.integration.test.tstests/muster.spec.ts
| await db | ||
| .update(schema.actors) | ||
| .set({ identityReference: "starter-admin@muster.local" }) | ||
| .where(sql`${schema.actors.id} = ${starterIds.actors.jordan}`); | ||
| await db | ||
| .update(schema.rooms) | ||
| .set({ | ||
| name: "starter-soc-operations", | ||
| slug: "starter-soc-operations", | ||
| displayName: "Starter SOC operations", | ||
| }) | ||
| .where(sql`${schema.rooms.id} = ${starterIds.rooms.soc}`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Scope both seed updates by organisation.
The WHERE clauses only match record IDs. Add starterIds.organisation to both predicates so this cleanup remains tenant-scoped if fixtures are reused or data is inconsistent. As per coding guidelines, scope every domain query by organisation.
Proposed fix
- .where(sql`${schema.actors.id} = ${starterIds.actors.jordan}`);
+ .where(sql`
+ ${schema.actors.id} = ${starterIds.actors.jordan}
+ AND ${schema.actors.organisationId} = ${starterIds.organisation}
+ `);
- .where(sql`${schema.rooms.id} = ${starterIds.rooms.soc}`);
+ .where(sql`
+ ${schema.rooms.id} = ${starterIds.rooms.soc}
+ AND ${schema.rooms.organisationId} = ${starterIds.organisation}
+ `);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await db | |
| .update(schema.actors) | |
| .set({ identityReference: "starter-admin@muster.local" }) | |
| .where(sql`${schema.actors.id} = ${starterIds.actors.jordan}`); | |
| await db | |
| .update(schema.rooms) | |
| .set({ | |
| name: "starter-soc-operations", | |
| slug: "starter-soc-operations", | |
| displayName: "Starter SOC operations", | |
| }) | |
| .where(sql`${schema.rooms.id} = ${starterIds.rooms.soc}`); | |
| await db | |
| .update(schema.actors) | |
| .set({ identityReference: "starter-admin@muster.local" }) | |
| .where(sql` | |
| ${schema.actors.id} = ${starterIds.actors.jordan} | |
| AND ${schema.actors.organisationId} = ${starterIds.organisation} | |
| `); | |
| await db | |
| .update(schema.rooms) | |
| .set({ | |
| name: "starter-soc-operations", | |
| slug: "starter-soc-operations", | |
| displayName: "Starter SOC operations", | |
| }) | |
| .where(sql` | |
| ${schema.rooms.id} = ${starterIds.rooms.soc} | |
| AND ${schema.rooms.organisationId} = ${starterIds.organisation} | |
| `); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/database/src/seed.ts` around lines 18 - 29, Update the two seed
updates for actors and rooms to include starterIds.organisation in their WHERE
predicates alongside the existing record ID conditions. Keep both updates
tenant-scoped while preserving their current field values and target IDs.
Source: Coding guidelines
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n 'db\.transaction|\.transaction\(async' packages/database/src packages/mcp/src appsRepository: jusso-dev/Muster
Length of output: 6577
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## packages/database/src/seed.ts\n'
ast-grep outline packages/database/src/seed.ts --view expanded || true
printf '\n## relevant slice\n'
cat -n packages/database/src/seed.ts | sed -n '1,120p'
printf '\n## packages/database/src/bootstrap-connectors.ts\n'
ast-grep outline packages/database/src/bootstrap-connectors.ts --view expanded || true
printf '\n## relevant slice\n'
cat -n packages/database/src/bootstrap-connectors.ts | sed -n '90,150p'Repository: jusso-dev/Muster
Length of output: 7800
Wrap the bootstrap cleanup in a transaction. If the actor update succeeds and the room update fails, demo seeding leaves a partially rewritten bootstrap state. Put both updates in db.transaction(...) so they commit or roll back together.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/database/src/seed.ts` around lines 18 - 29, Wrap the actor and room
updates in the seed bootstrap flow within a single db.transaction callback,
using the transaction handle for both update operations. Preserve the existing
update values and conditions so both changes commit together or roll back
together.
Source: Coding guidelines
The image job's sbom/provenance/upload-artifact steps had no if: always(), so a HIGH/CRITICAL trivy finding (exit-code 1) skipped every step after it and the sarif was never uploaded - the job fails with zero visibility into which CVE tripped it. Add if: always() to those three steps so a failing scan still produces an inspectable artifact. release-security/image is failing on this PR against a locally-reproduced build of the same unmodified Dockerfile with zero HIGH/CRITICAL findings in repeated local trivy scans - this change is to see what CI is actually finding, not to weaken the gate (exit-code: 1 is untouched). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/security.yml:
- Around line 77-91: Separate the required evidence files muster-sbom.cdx.json
and SHA256SUMS from optional diagnostics in the security-artifacts upload step.
Make the required upload fail when either file is missing, while keeping
provenance.json and trivy.sarif in a separate upload that may continue with
missing files; update the workflow around the upload-artifact step without
changing the artifact contents or generation commands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e08d68e-92b0-4828-a513-03fb2364c7dd
📒 Files selected for processing (1)
.github/workflows/security.yml
| if: always() | ||
| run: | | ||
| printf '{"commit":"%s","workflow":"%s","runId":"%s","image":"muster:security"}\n' \ | ||
| "$GITHUB_SHA" "$GITHUB_WORKFLOW" "$GITHUB_RUN_ID" > provenance.json | ||
| sha256sum muster-sbom.cdx.json provenance.json > SHA256SUMS | ||
| - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 | ||
| if: always() | ||
| with: | ||
| name: security-artifacts | ||
| path: | | ||
| muster-sbom.cdx.json | ||
| provenance.json | ||
| SHA256SUMS | ||
| trivy.sarif | ||
| if-no-files-found: ignore |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant workflow region with line numbers.
sed -n '1,140p' .github/workflows/security.yml | cat -n
# Also locate all uses of upload-artifact in this workflow for context.
rg -n "upload-artifact|if-no-files-found|muster-sbom|trivy\.sarif|provenance\.json|SHA256SUMS" .github/workflows/security.ymlRepository: jusso-dev/Muster
Length of output: 4566
🌐 Web query:
GitHub Actions upload-artifact if-no-files-found partial path list missing files semantics
💡 Result:
In GitHub Actions, the upload-artifact action provides the if-no-files-found input to control behavior when the specified path does not match any files [1][2][3]. The semantics of if-no-files-found apply to the collection of files resulting from the path input (which supports file, directory, or wildcard patterns) [2][4][3]. When the action evaluates the provided paths and finds no files, it follows the behavior specified by this setting: - warn (default): The action completes successfully but outputs a warning in the workflow logs [1][2][5]. - error: The action fails, causing the workflow step to exit with an error message [1][2][3]. - ignore: The action completes successfully without outputting any warnings or errors [2][4][3]. Regarding "partial path list missing files": If you provide a list of paths (via a YAML multiline string) and some paths match files while others do not, the upload-artifact action generally evaluates the entire set. If at least one file is found across the provided paths, the action typically considers the task successful (regardless of whether specific paths in that list were empty). The if-no-files-found setting is triggered only if the final result of the evaluation is an empty set of files [1][2][3]. If you are experiencing issues where you believe files should be found but are not, ensure that: 1. You are using absolute paths from the root of the repository (or correctly relative to the project root), as the action does not necessarily respect working-directory settings from other steps [6]. 2. Hidden files are not being inadvertently excluded; you must set include-hidden-files: true to include them [6][7]. 3. You are using the latest version of the action (e.g., v4 or higher) to ensure you have access to the full if-no-files-found functionality [1][4][8].
Citations:
- 1: https://github.com/actions/upload-artifact
- 2: https://github.com/actions/upload-artifact/blob/c7d193f32edcb7bfad88892161225aeda64e9392/README.md
- 3: https://github.com/marketplace/actions/upload-a-build-artifact
- 4: https://github.com/actions/upload-artifact/blob/v4/README.md
- 5: https://github.com/actions/upload-artifact?tab=readme-ov-file
- 6: No files were found with the provided path: build. No artifacts will be uploaded. actions/upload-artifact#232
- 7: https://github.com/actions/upload-artifact/blob/main/action.yml
- 8: https://github.com/actions/upload-artifact/tree/v4.0.0
🌐 Web query:
"upload-artifact" "if-no-files-found" "all files are missing" documentation
💡 Result:
The if-no-files-found input parameter for the GitHub Actions upload-artifact action allows you to customize the behavior when the specified path results in no files being found [1][2]. The available options are [1][3]: - warn: Prints a warning in the logs but the action succeeds. This is the default behavior [1][2]. - error: Marks the workflow run as failed and displays an error message [2][3]. - ignore: Suppresses all warnings and errors; the action does not fail [2][3]. Usage Example: - uses: actions/upload-artifact@v7 with: name: my-artifact path: path/to/artifact/ if-no-files-found: error Note that this parameter triggers when no files are found across the entire provided path configuration [1]. Users have reported that when multiple paths are specified (e.g., using a list), the action may still succeed if some files are found even if others are missing, as it does not explicitly guarantee that every single specified path or file exists [4].
Citations:
- 1: https://github.com/actions/upload-artifact
- 2: https://earthly.dev/blog/github-action-artifacts/
- 3: https://namespace.so/docs/reference/github-actions/upload-artifact
- 4: [bug] if-no-files-found: error doesn't check all files actions/upload-artifact#457
Keep required evidence separate from optional diagnostics
.github/workflows/security.yml:77-91 — if-no-files-found only fails when nothing matches, so this upload can still succeed with provenance.json or trivy.sarif alone if muster-sbom.cdx.json or SHA256SUMS is missing. Split the required evidence into a strict upload or add explicit existence checks before uploading.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/security.yml around lines 77 - 91, Separate the required
evidence files muster-sbom.cdx.json and SHA256SUMS from optional diagnostics in
the security-artifacts upload step. Make the required upload fail when either
file is missing, while keeping provenance.json and trivy.sarif in a separate
upload that may continue with missing files; update the workflow around the
upload-artifact step without changing the artifact contents or generation
commands.
@modelcontextprotocol/sdk@1.29.0 pins a transitive dependency on @hono/node-server@1.19.15, which carries GHSA-frvp-7c67-39w9 (Windows path traversal via encoded backslash in serve-static, fixed in 2.0.5). This dependency chain already existed on main via packages/agent-harness and is not introduced by this PR, but it fails release-security/image's Trivy gate on every run. Bump @modelcontextprotocol/sdk to 1.30.0 (the first version whose package.json accepts @hono/node-server 2.x) across all three consumers (packages/agent-harness, packages/mcp, apps/mcp-server), and add a pnpm workspace override pinning @hono/node-server to ^2.0.5 as a floor for any future dependency path. Verified: pnpm-lock.yaml now resolves @hono/node-server@2.0.12; pnpm build (25/25), lint (25/25), and the mcp/mcp-server/agent-harness test suites (18+4+15 passing) are unaffected; a fresh local build + trivy scan against the exact CI invocation (severity HIGH,CRITICAL, ignore-unfixed, sarif format) now exits 0 with zero findings, where it previously failed on this specific CVE. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
docker exporter cannot load multi-artifact manifest lists. Keep provenance+SBOM on push (registry export); drop them on PR load so Trivy and local inspect still work.
1 participant
Closes #72
Summary
Ships the first Hermes-native Muster MCP vertical slice per the product pivot in #78: Slack is the human interface, Hermes is the harness, Muster is the governed MCP/control-plane backend, Kelpie stays authoritative for cases.
apps/mcp-server— a minimalnode:httpapp (same shape asapps/agent-gateway/apps/worker, not folded into either) speaking MCP Streamable HTTP.GET /healthis unauthenticated/tenant-free and dependency-aware (a realselect 1, not a stub);POST/GET /mcprequires a bearer installation token. Shutdown drains in-flight requests before closing the DB pool; request/socket timeouts are explicit.packages/mcp— pure domain logic, no HTTP framework dependency:mcp_installations: a random bearer token, SHA-256 hashed at rest, bound to exactly one organisation and one "policy subject" actor. Installation lifecycle mutations (createInstallation/revokeInstallation) re-derive the acting actor's organisation membership andadministration.managecapability from the database inside the mutation's own transaction — never trusted from the caller — and a composite(actor_id, organisation_id) -> actors(id, organisation_id)foreign key enforces the same boundary at the schema level as defence in depth. Revocation is immediate and fails closed. Every credential failure path (unknown/malformed/revoked/cross-org token, deactivated actor) returns the identical generic denial.muster_get_status,muster_list_capabilities,muster_search_kelpie_cases,muster_get_kelpie_case. No tool schema accepts an organisation id, actor id, or capability.integration_query_runs→ outbox → the unmodifiedapps/workerprocessor →executeGovernedQuery's DNS pinning/SSRF denial/schema validation/redaction). The MCP tool call polls the authoritative row for a bounded window (8s) instead of blocking, now with a deterministic, time-windowed idempotency key (a retry within 5s dedupes to the same queued run; a later identical query still gets fresh data) and an integration-scoped advisory lock around the rate-limit check (closing the read-then-insert race).classification: "untrusted_evidence", bounded to 25 records with depth/breadth-bounded nested structures, oversized strings truncated, secrets redacted a second time at the tool boundary.mcp.tool.invokedaudit event per call via the existing hash-chainedaudit_eventstable; a failure to write it is now logged, not silently swallowed.create-installation/revoke-installationCLI scripts provision credentials (no chat/UI-driven registration flow), now reporting authorization failures with a clean exit rather than an unhandled rejection.skills/muster-soc-operations/SKILL.md,docs/integrations/hermes-mcp.md, ADR 0005 — skill contract, operator configuration (placeholders only), and the architectural decision record, including a new "Deferred" section documenting two intentionally-out-of-scope design tensions (see Residual risks).Non-goals preserved
No LangGraph/model runtime, no Muster chat UI/rooms/Inbox/composer/threads, no Slack gateway, no arbitrary MCP registration, no write/destructive/external-communication tools, no admin UI.
apps/agent-gatewayis untouched.Corrective update (this PR was already reviewed once)
CodeRabbit raised 9 actionable findings and 6 nitpicks against the first commit. All 9 actionable findings are fixed and their review threads replied-to (with the exact fix) and resolved:
server.close()is now awaited via its completion callback beforecloseDatabase()runs (newapps/mcp-server/src/shutdown.ts, unit-tested, live-verified with SIGTERM).(actor_id, organisation_id) -> actors(id, organisation_id)on all threemcp_installationsactor columns, backed by a newactors_id_organisation_uniqueindex; Postgres itself now rejects a cross-org actor binding.3 & 4. Authoritative server-side capability/approval enforcement for installation lifecycle —
createInstallation/revokeInstallationre-checkadministration.manageand organisation membership from the database inside the mutation's transaction. A full multi-party approval record workflow (as opposed to capability enforcement) is a documented, justified deferral — see ADR 0005 — since it's materially larger than this read-only-tools vertical slice and belongs with tracker item 2's governed write/approval operations.muster_search_kelpie_casesthrough a real installation/MCP client against the SSRF-shaped integration, asserting the tool call, the run'serrorCode, and the audit outcome all reflect the denial.newId()idempotency key never deduped — replaced with a deterministic, 5-second-windowed key; verified with a test asserting exactly one connector run across two immediately-repeated identical tool calls.JSON.stringify— now matches specific fields only, with a doc comment stating plainly this is a best-effort filter over the already-fetched page (Kelpie'scases.listhas no query parameter to push down to).malformed_responsewas reported asnot_found— now stays a distinctupstream_error; only a genuinely absent/unreachable case maps tonot_found.Of the 6 nitpicks: the depth/breadth evidence-bounding, dependency-aware
/health, and typedScopeError(replacing message-string matching for audit "denied" classification) are fixed. The rate-limit race is fixed via an advisory lock (matchingappendAuditEvent's existing pattern) rather than left as noted. The 8-second bounded-poll-inside-the-request-handler design tension is not restructured — see ADR 0005's "Deferred" section for why (it's a genuine MCP protocol-shape decision, not a bug) — but request/socket timeouts were added as the concrete, in-scope mitigation the reviewer asked to confirm.A defect not flagged by review, found while fixing the health check:
@muster/database's sharedpg.Poolhad noerrorlistener. An idle-client error (e.g. Postgres restarting) is an unhandled Node'error'event, which crashes the entire process — used by every app in this repo (web, worker, agent-gateway, and now mcp-server), not just this PR's code. This is exactly what a real dependency-aware/healthcheck provokes during any outage. Reproduced live (stopped Postgres under a runningmcp-server, watched it crash) and fixed with a pool-level error listener; re-verified the same scenario now returns503and the process survives, then recovers to200when Postgres comes back.Validation evidence (distinguished by tier)
Local, real Postgres (
docker --context m3-max, started and torn down for this task):0021(it was only ever unreleased in this PR); fixed a statement-ordering bug (the newactorsunique index must be created before the composite FKs that reference it).pnpm db:migrate && pnpm db:bootstrap && pnpm db:verify-cleanon a pristine database — clean, 42 operational tables empty, no schema drift (pnpm db:generatereports "No schema changes").packages/mcpfull suite: 18/18 passing (10 pure unit + 8 integration, up from 6 — added: capability/cross-org-actor authorization test, cross-org DB-level FK-rejection test, idempotency-dedup test; rewrote the SSRF test to go through the real tool path).apps/mcp-serverunit suite: 4/4 passing (newhealth.test.ts,shutdown.test.ts).apps/mcp-server, ran it against real Postgres —/healthreports200 ready; stopped Postgres —/healthreports503 not_readyand the process stays alive (previously crashed, see above); restarted Postgres — recovers to200; sentSIGTERM— exits cleanly. All via the actual built binary, not a mock.Repo-wide:
pnpm lint/pnpm typecheck— 25/25 packages pass.pnpm test:unit— 186 passed, 84 skipped (0 regressions; skips are the repo's existing integration tests, gated the same way as CI).pnpm test(turbo, per-package) — 39/39 tasks pass.pnpm build— 25/25 packages, includingapps/web.GitHub Actions run 30326839826 (this PR's first commit) — two failed checks, both confirmed pre-existing on
main, not introduced by this PR:quality(Playwright e2e,tests/muster.spec.ts/tests/jessie-hunt.spec.ts): fails with the identical error signature (Second seeded room required,element(s) not found, etc.) on base commit496cc09d09f2d81090be96b7e42ec375a9891db0's own CI run (30247756940, jobquality, conclusionfailure). This PR touches zero UI/room code; not weakened or worked around.release-security / image(Trivy image scan,exit-code: 1on HIGH/CRITICAL): also fails identically on the same base-SHA run (job89918532363). This PR doesn't touchDockerfileor pin any base image. I rebuilt the exact Dockerfile fresh (localdocker --context m3-max build) and ran Trivy (both0.67.2and0.70.0, matching the CI action's bundled version) against it just now: zero HIGH/CRITICAL fixable vulnerabilities.node:24-bookworm-slimandgcr.io/distroless/nodejs24-debian13:nonrootare floating tags — the specific CVE that failed in the original CI run has very likely already rolled off with a newer base-layer pull since then; the original run'strivy.sarifwas never uploaded (the job died before that step), so the exact CVE ID from that specific run can't be retrieved retroactively. No vulnerability was suppressed, ignored, or worked around — I could not reproduce a finding to act on, and the mechanism (a pre-existing, floating-tag, time-dependent base image state) is confirmed identical to basemain.Migration / rollback
packages/database/migrations/0021_dazzling_living_mummy.sql(squashed, additive-only): new tablemcp_installations; new unique indexactors_id_organisation_uniqueon the existingactorstable; three composite FKs frommcp_installationstoactors(id, organisation_id). No column drops, type changes, or backfills on any existing table besides the new index. Rollback:DROP TABLE mcp_installations; DROP INDEX actors_id_organisation_unique;.Security review notes
packages/mcp; delegated entirely to the existing, already-testedexecuteGovernedQuery. Now proven through the real tool/gateway path, not just a direct unit-level call.key=/suspicious field name can evade content-based redaction) — out of scope for this PR, shared with every other connector. Nested evidence is now depth/breadth-bounded in addition to size-bounded.@muster/database— now contained to a logged error and a failed health check, as intended.Residual risks / follow-ups
@modelcontextprotocol/sdkclient library over real HTTP, but not the Hermes binary itself, which isn't available in this sandboxed environment).apps/mcp-serverisn't wired intoDockerfile/docker-compose.yml— unchanged from the original PR; still out of the issue's required-implementation list.Not merged or deployed.
Summary by CodeRabbit
GET /healthplus four read-only tools (status, capabilities, bounded Kelpie search, and single case details).