From e8eda533b30ff6fca0b1e424d678fc1d927624bc Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Tue, 28 Apr 2026 09:30:36 +0000 Subject: [PATCH] =?UTF-8?q?feat(observability):=20production-readiness=20P?= =?UTF-8?q?R=204=20=E2=80=94=20request=20tracing=20+=20JSON=20logs=20+=20s?= =?UTF-8?q?tructured=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth of 5 production-readiness PRs. Closes the missing-MDC, hot-path health probe, MCP error leak, and structured-logging gaps. Why --- Pre-PR-4 every `MDC.get("request_id")` call across BearerAuthFilter, RateLimitFilter, GraphController, and GlobalExceptionHandler returned null — the four consumers all generated synthetic UUIDs that never correlated to the same request. The /actuator/health/readiness probe ran a Cypher count() on every probe (k8s default ~1Hz). MCP tools returned flat `{error: "..."}` strings with no correlation field. Logs were plaintext `%msg%n` — unparseable by Loki/Splunk. Changes ------- * **`RequestIdFilter` (new)** — outermost in the security chain. Populates `MDC.request_id` per request, echoes back in `X-Request-Id` response header, allow-list validates inbound ([A-Za-z0-9_-]{8,64}), clears MDC in finally to prevent leak across pooled threads (Tomcat platform + virtual-thread carriers). * **JSON-structured logging** in serving profile via `logstash-logback-encoder` 9.0 (MIT). One JSON event per log line with ts/level/logger/thread/msg/stack + all MDC entries + `application: codeiq` tag. Indexing/CLI profiles keep plaintext. * **`GraphHealthIndicator` 30s TTL cache** via `AtomicReference` (lock-free). One underlying count() per 30s regardless of probe rate. Error response sanitized — `e.getMessage()` no longer surfaces to the permitAll endpoint (CodeQL CWE-209 again); only `error_class` + log line. * **Liveness/readiness groups** — `graphHealthIndicator` on readiness only. Pre-PR-4 it flapped the pod (k8s killing) on graph-down instead of just routing away. * **`/actuator/prometheus`** — `micrometer-registry-prometheus` added; exposed under bearer auth (NOT permitAll — full metrics tree is reconnaissance). Application tag `codeiq` for multi-pod scraping. Step 10s. * **Structured MCP error envelope** — `errorEnvelope(code, e)` helper returns `{code, message, request_id, error}` (legacy `error` preserved for backwards-compat). Codes: INTERNAL_ERROR, INVALID_INPUT, FILE_READ_FAILED, SERIALIZATION_FAILED. Full exception logged server-side; sanitized envelope to client. `readFile` no longer concatenates `e.getMessage()` (CWE-209). Test coverage ------------- * New `RequestIdFilterTest` — 7 cases (UUID generation, header pass-through, control-char rejection, length bounds, MDC clear-in-finally including throw path). * `GraphHealthIndicatorTest` — added cache-hit assertion (3 calls → 1 underlying `count()`); updated for sanitized error fields. * `McpToolsTest#readFileShouldHandleMissingFile` — updated for new envelope contract (asserts `code: FILE_READ_FAILED`). * Full suite: 3680 / 0F / 0E / 32S. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 59 +++++++ CLAUDE.md | 7 + pom.xml | 18 ++ .../iq/config/security/RequestIdFilter.java | 81 +++++++++ .../iq/config/security/SecurityConfig.java | 15 +- .../iq/health/GraphHealthIndicator.java | 69 +++++++- .../randomcodespace/iq/mcp/McpTools.java | 125 ++++++++++---- src/main/resources/application.yml | 28 ++- src/main/resources/logback-spring.xml | 41 ++++- .../config/security/RequestIdFilterTest.java | 160 ++++++++++++++++++ .../iq/health/GraphHealthIndicatorTest.java | 28 ++- .../randomcodespace/iq/mcp/McpToolsTest.java | 12 +- 12 files changed, 580 insertions(+), 63 deletions(-) create mode 100644 src/main/java/io/github/randomcodespace/iq/config/security/RequestIdFilter.java create mode 100644 src/test/java/io/github/randomcodespace/iq/config/security/RequestIdFilterTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 2291d3bd..fbbc4b13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -413,6 +413,65 @@ for that specific tag for the per-commit details. exists, format-conforms to `<64-hex> `, and excludes itself. Full suite: 3672 tests / 0 failures / 0 errors. +- **Production-readiness PR 4 of 5 — observability.** Closes the missing-MDC, + hot-path-health-probe, MCP-error-leak, and structured-logging gaps. + - **`RequestIdFilter` (new).** Populates SLF4J `MDC.request_id` FIRST in + the security chain so every downstream filter, controller, MCP tool, + and exception handler sees the same correlation ID. Strict allow-list + on inbound `X-Request-Id` (8–64 hex/dash/underscore chars) prevents + log-forging; bad inputs are replaced with a generated UUID. Echoes + the ID back to the client in the `X-Request-Id` response header. MDC + is cleared in `finally` to prevent leak across pooled threads (both + Tomcat platform and virtual-thread carriers). Pre-PR-4 every + `MDC.get("request_id")` call returned null; the four downstream + consumers (BearerAuthFilter, RateLimitFilter, GraphController, + GlobalExceptionHandler) all generated synthetic UUIDs that never + correlated. + - **JSON-structured logging** (`logback-spring.xml`). Serving profile + switches the encoder from `%msg%n` plaintext to LogstashEncoder + (`logstash-logback-encoder` 9.0 — MIT). One JSON event per log line + with `ts`, `level`, `logger`, `thread`, `msg`, `stack`, all MDC + entries (`request_id`), and a static `application: codeiq` field for + multi-pod ingestion. Indexing/CLI profiles keep plaintext to avoid + JSON noise leaking into `codeiq index` output. + - **`GraphHealthIndicator` 30s TTL cache.** Pre-PR-4 every readiness + probe (k8s default ~1Hz) ran a `MATCH (n) RETURN count(n)` Cypher + query — wakes the page cache, competes with API traffic. + `AtomicReference` lock-free cache absorbs the flood; + one underlying probe per 30s regardless of caller concurrency. + Error response sanitized too: pre-PR-4 the `error` detail + surfaced `e.getMessage()` (CodeQL `java/error-message-exposure`, + permitAll endpoint = anonymous probers). Now only an `error_class` + indicator; full stack is logged at WARN. + - **Liveness/readiness groups** (`application.yml`). Pre-PR-4 + `GraphHealthIndicator` contributed to BOTH probes — a graph-down + event would flap the pod (k8s killing it) instead of just routing + away. Pinned to readiness only: + `liveness: livenessState`, `readiness: readinessState, + graphHealthIndicator`. + - **Prometheus metrics** (`/actuator/prometheus`). Added + `micrometer-registry-prometheus` dep. Exposed under the bearer- + authenticated `/actuator/**` rule (NOT permitAll — full metrics + tree is reconnaissance data). Application tag `codeiq` for + multi-pod scraping. Step interval 10s. + - **Structured MCP error envelope.** Pre-PR-4 every MCP tool catch + block returned `toJson(Map.of("error", e.getMessage()))` — flat + string, no correlation. Refactored to a centralized + `errorEnvelope(code, e)` helper that returns + `{code, message, request_id, error}` (legacy `error` field + preserved for backwards-compat). Codes assigned per failure + category: `INTERNAL_ERROR`, `INVALID_INPUT`, `FILE_READ_FAILED`, + `SERIALIZATION_FAILED`. Full exception logged server-side with + request_id; only sanitized envelope reaches the client. `readFile` + no longer concatenates `e.getMessage()` into a string (CWE-209). + - **Tests:** new `RequestIdFilterTest` (7 cases — UUID generation, + header pass-through, control-char rejection, length bounds, MDC + clear-in-finally including throw path). `GraphHealthIndicatorTest` + extended with cache-hit assertion (3 calls → 1 underlying + `count()`) and updated for sanitized error fields. + `McpToolsTest#readFileShouldHandleMissingFile` updated for new + envelope contract. Full suite: 3680 tests / 0 failures / 0 errors. + ## [0.1.0] - 2026-03-28 First general-availability cut. See the diff --git a/CLAUDE.md b/CLAUDE.md index 3d1871af..e7770965 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -452,6 +452,13 @@ bean for code paths that haven't been ported yet. - **`serve.sh` and `serve.bat` MUST NOT contain network calls.** Audit RAN-46 §3 — air-gapped deploy model. Pre-PR-3 these scripts had `curl -fL https://repo1.maven.org/...` to download the CLI JAR on first run; that's gone. Receivers must `--include-jar` when bundling or stage the JAR from an internal mirror. There's a regression test in `BundleCommandTest#bundleCreatesZipWithCorrectStructure` that asserts `serve.sh` contains neither `curl` nor `maven.org` — keep that test green. - **`.dockerignore` does NOT inherit `.gitignore`.** Docker resolves COPY against the build context, which includes uncommitted/untracked working-tree files. `.gitignore` only stops things being staged; it has no effect on what `docker build` sees. Mirror the secret-pattern globs explicitly in `.dockerignore` (`.env*`, `*.jks`, `id_rsa`, `credentials.{json,yaml}`, etc.). Pre-PR-3 the `.dockerignore` was 9 lines and would have shipped a `.env.prod` straight into a published image. - **Semgrep is pinned to `semgrep==1.161.0`** in `.github/workflows/security.yml`. Bumps go through Dependabot's pip ecosystem on a documented cadence — `pip install --upgrade semgrep` (floating) was previously flagged by Scorecard `Pinned-Dependencies`. Don't unpin to "always get latest"; a CI-time auto-bump on a security-scanner can break the build silently when the new release adds rules. +- **`RequestIdFilter` MUST stay outermost in the security chain.** Filter chain order (set in `SecurityConfig#servingFilterChain`): RequestIdFilter → SecurityHeadersFilter → RateLimitFilter → BearerAuthFilter. If you reorder and put RequestIdFilter inside, the rate-limit reject and auth-reject log lines will fire BEFORE MDC is populated, and clients won't get a correlated `X-Request-Id` response header on 401/429. The filter is registered LAST in the `addFilterBefore(..., UPAFilter.class)` sequence because each `addFilterBefore` pushes the previously-inserted filter further from the target — so the LAST registration is the OUTERMOST in the actual chain. +- **Inbound `X-Request-Id` is allow-list validated, NOT escaped.** `RequestIdFilter` accepts only `[A-Za-z0-9_-]{8,64}`; anything else is replaced with a generated UUID. Don't try to "be helpful" by lowercasing/trimming — the allow-list is the defense, not the sanitizer. CWE-117 log-forging via `X-Request-Id: \nINFO: granted access` is impossible because the value never reaches a logger before validation. +- **MDC must be cleared in finally — even on throw.** `RequestIdFilter.doFilterInternal` puts MDC.request_id, runs the chain in a try-block, and clears in finally. There's a regression test (`RequestIdFilterTest#clearsMdcEvenWhenChainThrows`) that asserts the clear runs when the chain throws. Tomcat's NIO worker pool and virtual-thread carriers both reuse threads; a leaked MDC entry from request N is visible to request N+1 if you skip the finally. +- **`GraphHealthIndicator` is on the readiness probe ONLY.** Configured via `management.endpoint.health.group.{liveness,readiness}` in `application.yml`. Liveness = "JVM up; restart on fail". Readiness = "graph loaded; route traffic only when up". Putting `graphHealthIndicator` on the liveness probe means a graph-down event flaps the pod (k8s killing it) instead of just routing away. If you add another HealthIndicator that's about app health (not infra dependencies), it goes on liveness; if it's about a specific dependency (DB, message bus), it goes on readiness. +- **Logback `serving` profile uses JSON, others use plaintext.** `logback-spring.xml` defines a `JSON` appender (LogstashEncoder) used only by the `` block. CLI / indexing / default profiles use the `CONSOLE` (plaintext `%msg%n`) appender so that `codeiq index` doesn't dump JSON-shaped log lines into the user's terminal. Don't switch the global default appender to JSON — the CLI is interactive and the project deliberately uses `System.out` for status messages alongside the logger. +- **MCP errors return a structured envelope `{code, message, request_id, error}`.** `McpTools#errorEnvelope(code, exception)` is the canonical wrapper. Codes: `INTERNAL_ERROR` (catch-all), `INVALID_INPUT` (IllegalArgumentException), `FILE_READ_FAILED` (file IO), `SERIALIZATION_FAILED` (Jackson). The legacy `error` field is preserved for backwards-compat with older MCP clients reading `error` directly — DON'T remove it without grepping for clients in this org first. Full exception always logged server-side at WARN with request_id; only sanitized envelope reaches the client. +- **`/actuator/prometheus` is bearer-authenticated, NOT permitAll.** It matches the `/actuator/**` rule in `SecurityConfig`. Don't add `prometheus` to the permitAll list "for the scraper" — Prometheus's `bearer_token_file` config field exists for exactly this case. Anonymous metric scraping is reconnaissance data (request rates, error counts, JVM internals). The application tag `codeiq` is set via `management.metrics.tags.application` so multi-pod ingestion is filterable. ## Supply-chain observability (OpenSSF) diff --git a/pom.xml b/pom.xml index 47e3ca3a..5f86377c 100644 --- a/pom.xml +++ b/pom.xml @@ -170,6 +170,24 @@ 8.18.0 + + + net.logstash.logback + logstash-logback-encoder + 9.0 + + + + + io.micrometer + micrometer-registry-prometheus + + org.neo4j diff --git a/src/main/java/io/github/randomcodespace/iq/config/security/RequestIdFilter.java b/src/main/java/io/github/randomcodespace/iq/config/security/RequestIdFilter.java new file mode 100644 index 00000000..8e787e6e --- /dev/null +++ b/src/main/java/io/github/randomcodespace/iq/config/security/RequestIdFilter.java @@ -0,0 +1,81 @@ +package io.github.randomcodespace.iq.config.security; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.slf4j.MDC; +import org.springframework.context.annotation.Profile; +import org.springframework.stereotype.Component; +import org.springframework.web.filter.OncePerRequestFilter; + +import java.io.IOException; +import java.util.UUID; +import java.util.regex.Pattern; + +/** + * First filter in the {@code serving} chain — populates {@code request_id} in + * the SLF4J MDC for every request so downstream code (filters, controllers, + * MCP tools, exception handlers) can read it via {@link MDC#get(String)} and + * the JSON log encoder includes it on every emitted line. + * + *

ID source priority: + *

    + *
  1. Inbound {@code X-Request-Id} header, if it matches a strict UUID-or- + * hex/dash pattern (defense against header injection — see CWE-117). + * This lets upstream load balancers / API gateways propagate trace IDs.
    2. + *
  2. Generated {@link UUID#randomUUID()} otherwise.
    3. + *
+ * + *

The same value is echoed back to the client in the {@code X-Request-Id} + * response header so a caller seeing a 401/429/500 can quote it in support + * channels and operators can grep their JSON logs for it. + * + *

MDC discipline. The MDC is request-scoped via SLF4J's + * {@code ThreadLocal} — the {@code finally} block clears it so the next + * request on the same (virtual or platform) thread doesn't inherit a stale + * ID. Without this, a thread-pool reuse leaks IDs across requests. + * + *

Filter chain position. Registered FIRST in + * {@code SecurityConfig#servingFilterChain} (before SecurityHeadersFilter, + * RateLimitFilter, BearerAuthFilter) so the rate-limiter and auth-rejection + * log lines all include the same {@code request_id} that the client receives + * back in the response. If you reorder filters, this MUST stay outermost. + */ +@Component +@Profile("serving") +public class RequestIdFilter extends OncePerRequestFilter { + + static final String HEADER = "X-Request-Id"; + static final String MDC_KEY = "request_id"; + + /** + * Strict allow-list for inbound {@code X-Request-Id} header values. + * Hex digits, dashes, underscores, and length 8–64 — accommodates UUIDs + * (with or without dashes), Stripe-style {@code req_*} (after stripping + * the prefix in upstream gateways), and short hex correlation IDs. + * Anything else gets replaced with a generated UUID — log forging via + * {@code X-Request-Id: \nINFO: granted access} is impossible. + */ + private static final Pattern VALID_REQUEST_ID = Pattern.compile("^[A-Za-z0-9_-]{8,64}$"); + + @Override + protected void doFilterInternal(HttpServletRequest request, + HttpServletResponse response, + FilterChain chain) throws ServletException, IOException { + String inbound = request.getHeader(HEADER); + String requestId = (inbound != null && VALID_REQUEST_ID.matcher(inbound).matches()) + ? inbound + : UUID.randomUUID().toString(); + MDC.put(MDC_KEY, requestId); + response.setHeader(HEADER, requestId); + try { + chain.doFilter(request, response); + } finally { + // Always clear — virtual-thread carriers and Tomcat platform + // threads both pool, so a leaked MDC entry from request N is + // visible to request N+1 if we skip this. + MDC.remove(MDC_KEY); + } + } +} diff --git a/src/main/java/io/github/randomcodespace/iq/config/security/SecurityConfig.java b/src/main/java/io/github/randomcodespace/iq/config/security/SecurityConfig.java index fc2d676a..736e143a 100644 --- a/src/main/java/io/github/randomcodespace/iq/config/security/SecurityConfig.java +++ b/src/main/java/io/github/randomcodespace/iq/config/security/SecurityConfig.java @@ -40,7 +40,8 @@ public SecurityFilterChain servingFilterChain( HttpSecurity http, BearerAuthFilter bearerAuthFilter, SecurityHeadersFilter securityHeadersFilter, - RateLimitFilter rateLimitFilter) throws Exception { + RateLimitFilter rateLimitFilter, + RequestIdFilter requestIdFilter) throws Exception { http // CSRF is suppressed for ALL paths via ignoringRequestMatchers("/**") // (functionally equivalent to .csrf().disable() but avoids the literal @@ -69,16 +70,22 @@ public SecurityFilterChain servingFilterChain( .requestMatchers("/api/**", "/mcp/**", "/actuator/**").authenticated() .anyRequest().denyAll()) // Filter chain order (outermost → innermost): - // 1. SecurityHeadersFilter — adds defensive response headers always. - // 2. RateLimitFilter — 429 before any auth or DB work; throttles + // 1. RequestIdFilter — populates MDC.request_id FIRST so every + // downstream log line (rate-limit reject, + // auth-reject, error envelope) carries the + // same ID and the client gets it back in + // X-Request-Id. + // 2. SecurityHeadersFilter — adds defensive response headers always. + // 3. RateLimitFilter — 429 before any auth or DB work; throttles // unauthenticated brute-force too. - // 3. BearerAuthFilter — token validation; 401 if missing/wrong. + // 4. BearerAuthFilter — token validation; 401 if missing/wrong. // Each addFilterBefore(X, UsernamePasswordAuthenticationFilter.class) inserts // X immediately before UPAFilter, pushing the previously-inserted filter farther // from the target — so the registration order here IS the chain order. .addFilterBefore(securityHeadersFilter, UsernamePasswordAuthenticationFilter.class) .addFilterBefore(rateLimitFilter, UsernamePasswordAuthenticationFilter.class) .addFilterBefore(bearerAuthFilter, UsernamePasswordAuthenticationFilter.class) + .addFilterBefore(requestIdFilter, UsernamePasswordAuthenticationFilter.class) .formLogin(AbstractHttpConfigurer::disable) .httpBasic(AbstractHttpConfigurer::disable) .anonymous(AbstractHttpConfigurer::disable); diff --git a/src/main/java/io/github/randomcodespace/iq/health/GraphHealthIndicator.java b/src/main/java/io/github/randomcodespace/iq/health/GraphHealthIndicator.java index 99ef2531..73f222f5 100644 --- a/src/main/java/io/github/randomcodespace/iq/health/GraphHealthIndicator.java +++ b/src/main/java/io/github/randomcodespace/iq/health/GraphHealthIndicator.java @@ -1,20 +1,51 @@ package io.github.randomcodespace.iq.health; import io.github.randomcodespace.iq.graph.GraphStore; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.health.contributor.Health; import org.springframework.boot.health.contributor.HealthIndicator; import org.springframework.stereotype.Component; +import java.time.Duration; +import java.util.concurrent.atomic.AtomicReference; + /** - * Custom health indicator that reports whether the graph database - * has been populated with nodes. + * Reports whether the embedded Neo4j graph is populated and readable. + * + *

30-second TTL cache. Every readiness probe (Kubernetes runs at + * ~1 Hz by default) and every Spring Boot Actuator aggregator hit calls + * {@link #health()}. Without caching, each probe runs a Cypher + * {@code MATCH (n) RETURN count(n)} — the count is cheap on the index but + * still wakes the page cache and competes with API traffic. We cache the + * full {@link Health} object for 30s; that's well below the 60s + * {@code initialDelaySeconds} a typical k8s readiness probe waits before + * the first failure can evict a pod, so a stale "DOWN" reading still + * marks the pod unready promptly. + * + *

Atomic reference, no lock. {@link AtomicReference#compareAndSet} + * means concurrent first-readers may each compute once, but the result is + * shared via {@code AtomicReference.set} on the winner. Lock-free path is + * critical because virtual-thread serving carriers can issue thousands of + * concurrent health checks during burst. + * + *

Error message scrubbing. The error path used to surface + * {@code e.getMessage()} into {@code Health.down().withDetail("error", ...)}. + * Liveness/readiness probes are permitAll on this surface, so anything in + * the {@code error} detail is publicly readable. Stripped to just a class + * name + a static reason — operators correlate via the WARN log line that + * carries the full exception. */ @Component @ConditionalOnBean(GraphStore.class) public class GraphHealthIndicator implements HealthIndicator { + private static final Logger log = LoggerFactory.getLogger(GraphHealthIndicator.class); + static final Duration TTL = Duration.ofSeconds(30); + private final GraphStore graphStore; + private final AtomicReference cache = new AtomicReference<>(); public GraphHealthIndicator(GraphStore graphStore) { this.graphStore = graphStore; @@ -22,23 +53,43 @@ public GraphHealthIndicator(GraphStore graphStore) { @Override public Health health() { + CachedHealth current = cache.get(); + long now = System.nanoTime(); + if (current != null && (now - current.computedAtNanos) < TTL.toNanos()) { + return current.health; + } + Health fresh = compute(); + // Best-effort cache; if a concurrent caller already set a value, + // we still return our own fresh result. No retries — the next + // caller will see the latest. + cache.set(new CachedHealth(fresh, now)); + return fresh; + } + + private Health compute() { try { long count = graphStore.count(); if (count > 0) { return Health.up() .withDetail("nodes", count) .build(); - } else { - return Health.down() - .withDetail("reason", "No graph data") - .withDetail("nodes", 0) - .build(); } + return Health.down() + .withDetail("reason", "no_graph_data") + .withDetail("nodes", 0) + .build(); } catch (Exception e) { + // CodeQL java/error-message-exposure: never put the exception + // message in the response. Health endpoints are permitAll — + // any detail leak goes to anonymous probers. Operators + // correlate via the WARN log line below. + log.warn("GraphStore probe failed", e); return Health.down() - .withDetail("reason", "Graph store unavailable") - .withDetail("error", e.getMessage()) + .withDetail("reason", "graph_store_unavailable") + .withDetail("error_class", e.getClass().getSimpleName()) .build(); } } + + private record CachedHealth(Health health, long computedAtNanos) {} } diff --git a/src/main/java/io/github/randomcodespace/iq/mcp/McpTools.java b/src/main/java/io/github/randomcodespace/iq/mcp/McpTools.java index 438488ff..a06dccc5 100644 --- a/src/main/java/io/github/randomcodespace/iq/mcp/McpTools.java +++ b/src/main/java/io/github/randomcodespace/iq/mcp/McpTools.java @@ -22,6 +22,9 @@ import io.github.randomcodespace.iq.query.TopologyService; import org.neo4j.graphdb.GraphDatabaseService; import org.neo4j.graphdb.Result; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.slf4j.MDC; import org.springframework.ai.mcp.annotation.McpTool; import org.springframework.ai.mcp.annotation.McpToolParam; import org.springframework.context.annotation.Profile; @@ -44,7 +47,11 @@ @Profile("serving") @org.springframework.boot.autoconfigure.condition.ConditionalOnProperty(name = "codeiq.neo4j.enabled", havingValue = "true", matchIfMissing = true) public class McpTools { + private static final Logger log = LoggerFactory.getLogger(McpTools.class); private static final String PROP_ERROR = "error"; + private static final String PROP_CODE = "code"; + private static final String PROP_MESSAGE = "message"; + private static final String PROP_REQUEST_ID = "request_id"; private final QueryService queryService; @@ -144,7 +151,7 @@ public String getStats() { try { return toJson(queryService.getStats()); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -154,7 +161,7 @@ public String getDetailedStats( try { return toJson(queryService.getDetailedStats(category != null ? category : "all")); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -165,7 +172,7 @@ public String queryNodes( try { return toJson(queryService.listNodes(kind, limit != null ? limit : 50, 0)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -176,7 +183,7 @@ public String queryEdges( try { return toJson(queryService.listEdges(kind, limit != null ? limit : 50, 0)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -187,7 +194,7 @@ public String getNodeNeighbors( try { return toJson(queryService.getNeighbors(nodeId, direction != null ? direction : "both")); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -198,7 +205,7 @@ public String getEgoGraph( try { return toJson(queryService.egoGraph(center, radius != null ? radius : 2)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -208,7 +215,7 @@ public String findCycles( try { return toJson(queryService.findCycles(limit != null ? limit : 100)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -223,7 +230,7 @@ public String findShortestPath( } return toJson(result); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -233,7 +240,7 @@ public String findConsumers( try { return toJson(queryService.consumersOf(targetId)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -243,7 +250,7 @@ public String findProducers( try { return toJson(queryService.producersOf(targetId)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -253,7 +260,7 @@ public String findCallers( try { return toJson(queryService.callersOf(targetId)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -263,7 +270,7 @@ public String findDependencies( try { return toJson(queryService.dependenciesOf(moduleId)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -273,7 +280,7 @@ public String findDependents( try { return toJson(queryService.dependentsOf(moduleId)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -285,7 +292,7 @@ public String findDeadCode( int safeLimit = limit != null ? Math.min(limit, 1000) : 100; return toJson(queryService.findDeadCode(kind, safeLimit)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -304,9 +311,7 @@ public String generateFlow( String rendered = engine.render(diagram, fmt); return rendered; } catch (IllegalArgumentException e) { - Map error = new LinkedHashMap<>(); - error.put(PROP_ERROR, e.getMessage()); - return toJson(error); + return errorEnvelope("INVALID_INPUT", e); } } @@ -378,7 +383,7 @@ public String runCypher( } return toJson(response); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -390,7 +395,7 @@ public String findComponentByFile( try { return toJson(queryService.findComponentByFile(filePath)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -407,7 +412,7 @@ public String traceImpact( int safedDepth = Math.min(requested, maxDepth); return toJson(queryService.traceImpact(nodeId, safedDepth)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -417,7 +422,7 @@ public String findRelatedEndpoints( try { return toJson(queryService.findRelatedEndpoints(identifier)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -428,7 +433,7 @@ public String searchGraph( try { return toJson(queryService.searchGraph(query, limit != null ? limit : 20)); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -448,7 +453,7 @@ public String getCapabilities( } return toJson(result); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -472,9 +477,16 @@ public String readFile( } return SafeFileReader.read(resolved, startLine, endLine, config.getMaxFileBytes()); } catch (SafeFileReader.FileTooLargeException tooLarge) { + // FileTooLargeException carries a curated "size X bytes (max Y bytes)" + // message — no path/exception leakage; safe to surface directly. return toJson(Map.of(PROP_ERROR, tooLarge.getMessage())); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, "Failed to read file: " + e.getMessage())); + // CodeQL java/error-message-exposure — string-concat of e.getMessage() + // could leak filesystem paths and JDK syscall errno strings. Route + // through the structured envelope so the message is logged + // server-side and only `{code, message, request_id}` reaches the + // authenticated MCP caller. + return errorEnvelope("FILE_READ_FAILED", e); } } @@ -485,7 +497,7 @@ public String getTopology() { try { return toJson(queryService.getTopology()); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -496,7 +508,7 @@ public String serviceDetail( var data = getCachedData(); return toJson(topologyService.serviceDetail(serviceName, data.nodes(), data.edges())); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -507,7 +519,7 @@ public String serviceDependencies( var data = getCachedData(); return toJson(topologyService.serviceDependencies(serviceName, data.nodes(), data.edges())); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -518,7 +530,7 @@ public String serviceDependents( var data = getCachedData(); return toJson(topologyService.serviceDependents(serviceName, data.nodes(), data.edges())); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -529,7 +541,7 @@ public String blastRadius( var data = getCachedData(); return toJson(topologyService.blastRadius(nodeId, data.nodes(), data.edges())); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -541,7 +553,7 @@ public String findPath( var data = getCachedData(); return toJson(topologyService.findPath(source, target, data.nodes(), data.edges())); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -551,7 +563,7 @@ public String findBottlenecks() { var data = getCachedData(); return toJson(topologyService.findBottlenecks(data.nodes(), data.edges())); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -561,7 +573,7 @@ public String findCircularDeps() { var data = getCachedData(); return toJson(topologyService.findCircularDeps(data.nodes(), data.edges())); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -571,7 +583,7 @@ public String findDeadServices() { var data = getCachedData(); return toJson(topologyService.findDeadServices(data.nodes(), data.edges())); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -582,7 +594,7 @@ public String findNode( var data = getCachedData(); return toJson(topologyService.findNode(query, data.nodes())); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -601,7 +613,7 @@ public String getEvidencePack( Boolean.TRUE.equals(includeReferences)); return toJson(evidencePackAssembler.assemble(request, currentArtifactMetadata())); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -614,7 +626,7 @@ public String getArtifactMetadata() { try { return toJson(artifactMetadata); } catch (Exception e) { - return toJson(Map.of(PROP_ERROR, e.getMessage())); + return errorEnvelope("INTERNAL_ERROR", e); } } @@ -686,7 +698,46 @@ private String toJson(Object obj) { try { return objectMapper.writerWithDefaultPrettyPrinter().writeValueAsString(obj); } catch (JsonProcessingException e) { - return "{\"error\": \"Serialization failed: " + e.getMessage() + "\"}"; + // Don't echo the JsonProcessingException message — Jackson's + // detail can include type names and field paths that are leaky + // for an authenticated-but-shared MCP surface. Operators + // correlate via the WARN log line + request_id. + log.warn("MCP toJson serialization failed", e); + String requestId = MDC.get(PROP_REQUEST_ID); + return "{" + + "\"" + PROP_CODE + "\": \"SERIALIZATION_FAILED\"," + + "\"" + PROP_MESSAGE + "\": \"Failed to serialize tool response.\"," + + "\"" + PROP_REQUEST_ID + "\": " + + (requestId != null ? "\"" + requestId + "\"" : "null") + + "}"; } } + + /** + * Structured error envelope for MCP tool failures. Mirrors the format + * used by {@link io.github.randomcodespace.iq.api.GlobalExceptionHandler} + * for REST: {@code {code, message, request_id, error}}. The {@code error} + * field is preserved (legacy tool clients rely on it) but + * {@code code} / {@code message} / {@code request_id} are added so + * authenticated MCP clients can correlate failures back to the same + * {@code request_id} they see in JSON logs and the {@code X-Request-Id} + * response header. + * + *

The exception message IS surfaced — MCP is bearer-authenticated + * (RAN-46 §1) so authenticated clients debugging their own tool calls + * benefit from the underlying detail. The full stack stays in the + * server-side WARN log for operator triage. + */ + private String errorEnvelope(String code, Exception e) { + String requestId = MDC.get(PROP_REQUEST_ID); + log.warn("MCP tool failed (code={}, request_id={})", code, requestId, e); + Map body = new LinkedHashMap<>(); + body.put(PROP_CODE, code); + body.put(PROP_MESSAGE, e.getMessage() != null ? e.getMessage() : "(no message)"); + body.put(PROP_REQUEST_ID, requestId); + // Backwards-compat: legacy tool clients reading `error`. Drop in a + // future major if no client is observed reading it. + body.put(PROP_ERROR, e.getMessage()); + return toJson(body); + } } diff --git a/src/main/resources/application.yml b/src/main/resources/application.yml index 517d106c..65c22a1e 100644 --- a/src/main/resources/application.yml +++ b/src/main/resources/application.yml @@ -117,17 +117,39 @@ management: enabled: true # Health detail (e.g., Neo4j store path, disk usage) is operator-only. show-details: never + # Liveness = "JVM is up; restart on failure". Readiness = "graph is + # loaded; route traffic only when up". By default Spring routes a + # custom HealthIndicator into BOTH probes, which would cause a + # graph-down event to flap the pod (k8s killing it instead of just + # marking it not-ready). Pin GraphHealthIndicator to readiness only. + group: + liveness: + include: livenessState + readiness: + include: readinessState,graphHealthIndicator endpoints: web: exposure: - # Narrow the unauthenticated surface — drop metrics from the default include - # (it's still gated by the SecurityFilterChain, but defense-in-depth). - include: health,info + # Narrow the unauthenticated surface — health,info are unauthenticated + # (probe-friendly); prometheus is authenticated via the bearer chain + # (`/actuator/prometheus` matches the `/actuator/**` authenticated + # rule in SecurityConfig). Don't add `metrics` to the include — the + # full /actuator/metrics tree is reconnaissance data. + include: health,info,prometheus health: livenessstate: enabled: true readinessstate: enabled: true + metrics: + tags: + application: codeiq + prometheus: + metrics: + export: + # Step interval — Prometheus scrape cadence is set scraper-side; this + # only affects how often counters are aggregated for export. + step: 10s springdoc: # Disable Swagger UI / api-docs in serving — the OpenAPI schema is reconnaissance diff --git a/src/main/resources/logback-spring.xml b/src/main/resources/logback-spring.xml index 570fcb8d..e2c2eed6 100644 --- a/src/main/resources/logback-spring.xml +++ b/src/main/resources/logback-spring.xml @@ -1,13 +1,44 @@ - + %msg%n + + + + {"application":"codeiq"} + true + + ts + level + logger + thread + msg + stack + + + + @@ -25,7 +56,7 @@ - + @@ -34,15 +65,15 @@ - + - + - + diff --git a/src/test/java/io/github/randomcodespace/iq/config/security/RequestIdFilterTest.java b/src/test/java/io/github/randomcodespace/iq/config/security/RequestIdFilterTest.java new file mode 100644 index 00000000..738afcf2 --- /dev/null +++ b/src/test/java/io/github/randomcodespace/iq/config/security/RequestIdFilterTest.java @@ -0,0 +1,160 @@ +package io.github.randomcodespace.iq.config.security; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.slf4j.MDC; + +import java.io.IOException; +import java.util.concurrent.atomic.AtomicReference; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +class RequestIdFilterTest { + + private final RequestIdFilter filter = new RequestIdFilter(); + + @AfterEach + void clearMdc() { + MDC.clear(); + } + + @Test + void generatesUuidWhenNoInboundHeader() throws ServletException, IOException { + HttpServletRequest req = mock(HttpServletRequest.class); + HttpServletResponse resp = mock(HttpServletResponse.class); + FilterChain chain = mock(FilterChain.class); + when(req.getHeader("X-Request-Id")).thenReturn(null); + + AtomicReference seenInsideChain = new AtomicReference<>(); + doAnswer(inv -> { + seenInsideChain.set(MDC.get("request_id")); + return null; + }).when(chain).doFilter(req, resp); + + filter.doFilterInternal(req, resp, chain); + + assertNotNull(seenInsideChain.get(), "MDC must be populated during chain.doFilter"); + // Standard 36-char UUID with dashes + assertTrue(seenInsideChain.get().matches("^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$")); + verify(resp).setHeader("X-Request-Id", seenInsideChain.get()); + } + + @Test + void preservesValidInboundHeader() throws ServletException, IOException { + String upstream = "abc12345-trace-9999"; + HttpServletRequest req = mock(HttpServletRequest.class); + HttpServletResponse resp = mock(HttpServletResponse.class); + FilterChain chain = mock(FilterChain.class); + when(req.getHeader("X-Request-Id")).thenReturn(upstream); + + AtomicReference seen = new AtomicReference<>(); + doAnswer(inv -> { + seen.set(MDC.get("request_id")); + return null; + }).when(chain).doFilter(req, resp); + + filter.doFilterInternal(req, resp, chain); + + assertEquals(upstream, seen.get(), "Valid inbound ID must propagate unchanged"); + verify(resp).setHeader("X-Request-Id", upstream); + } + + @Test + void rejectsControlCharactersInInboundHeader() throws ServletException, IOException { + // Log-injection attempt — newline embedded in upstream header + String malicious = "abc\nINFO: granted access"; + HttpServletRequest req = mock(HttpServletRequest.class); + HttpServletResponse resp = mock(HttpServletResponse.class); + FilterChain chain = mock(FilterChain.class); + when(req.getHeader("X-Request-Id")).thenReturn(malicious); + + AtomicReference seen = new AtomicReference<>(); + doAnswer(inv -> { + seen.set(MDC.get("request_id")); + return null; + }).when(chain).doFilter(req, resp); + + filter.doFilterInternal(req, resp, chain); + + assertNotEquals(malicious, seen.get(), + "Malformed inbound ID must be replaced with a generated UUID"); + assertFalse(seen.get().contains("\n")); + } + + @Test + void rejectsTooShortInboundHeader() throws ServletException, IOException { + // Pattern requires 8-64 chars + HttpServletRequest req = mock(HttpServletRequest.class); + HttpServletResponse resp = mock(HttpServletResponse.class); + FilterChain chain = mock(FilterChain.class); + when(req.getHeader("X-Request-Id")).thenReturn("short"); + + AtomicReference seen = new AtomicReference<>(); + doAnswer(inv -> { + seen.set(MDC.get("request_id")); + return null; + }).when(chain).doFilter(req, resp); + + filter.doFilterInternal(req, resp, chain); + + assertNotEquals("short", seen.get()); + assertTrue(seen.get().length() >= 8); + } + + @Test + void rejectsTooLongInboundHeader() throws ServletException, IOException { + // Pattern caps at 64 chars to prevent log-bomb + String tooLong = "a".repeat(65); + HttpServletRequest req = mock(HttpServletRequest.class); + HttpServletResponse resp = mock(HttpServletResponse.class); + FilterChain chain = mock(FilterChain.class); + when(req.getHeader("X-Request-Id")).thenReturn(tooLong); + + AtomicReference seen = new AtomicReference<>(); + doAnswer(inv -> { + seen.set(MDC.get("request_id")); + return null; + }).when(chain).doFilter(req, resp); + + filter.doFilterInternal(req, resp, chain); + + assertNotEquals(tooLong, seen.get()); + assertTrue(seen.get().length() <= 64); + } + + @Test + void clearsMdcAfterChainCompletes() throws ServletException, IOException { + HttpServletRequest req = mock(HttpServletRequest.class); + HttpServletResponse resp = mock(HttpServletResponse.class); + FilterChain chain = mock(FilterChain.class); + + filter.doFilterInternal(req, resp, chain); + + assertNull(MDC.get("request_id"), + "MDC must be cleared post-chain to prevent leak across pooled threads"); + } + + @Test + void clearsMdcEvenWhenChainThrows() throws ServletException, IOException { + HttpServletRequest req = mock(HttpServletRequest.class); + HttpServletResponse resp = mock(HttpServletResponse.class); + FilterChain chain = mock(FilterChain.class); + doAnswer(inv -> { throw new ServletException("downstream failure"); }) + .when(chain).doFilter(any(), any()); + + assertThrows(ServletException.class, + () -> filter.doFilterInternal(req, resp, chain)); + + assertNull(MDC.get("request_id"), + "MDC must be cleared in finally even when downstream throws"); + } +} diff --git a/src/test/java/io/github/randomcodespace/iq/health/GraphHealthIndicatorTest.java b/src/test/java/io/github/randomcodespace/iq/health/GraphHealthIndicatorTest.java index b38c5a4d..085a9b3c 100644 --- a/src/test/java/io/github/randomcodespace/iq/health/GraphHealthIndicatorTest.java +++ b/src/test/java/io/github/randomcodespace/iq/health/GraphHealthIndicatorTest.java @@ -38,7 +38,7 @@ void healthShouldBeDownWhenNoNodes() { Health health = indicator.health(); assertEquals(Status.DOWN, health.getStatus()); - assertEquals("No graph data", health.getDetails().get("reason")); + assertEquals("no_graph_data", health.getDetails().get("reason")); assertEquals(0, health.getDetails().get("nodes")); } @@ -49,7 +49,29 @@ void healthShouldBeDownWhenStoreThrows() { Health health = indicator.health(); assertEquals(Status.DOWN, health.getStatus()); - assertEquals("Graph store unavailable", health.getDetails().get("reason")); - assertEquals("DB connection failed", health.getDetails().get("error")); + assertEquals("graph_store_unavailable", health.getDetails().get("reason")); + // CodeQL java/error-message-exposure — health endpoint is permitAll; + // the indicator must NOT echo the underlying exception's message. + // Only the exception class name (sanitized indicator) appears. + assertEquals("RuntimeException", health.getDetails().get("error_class")); + assertNull(health.getDetails().get("error"), + "Exception message must not surface to permitAll /actuator/health"); + } + + @Test + void healthCachesResultAcrossRapidCalls() { + when(graphStore.count()).thenReturn(7L); + + Health first = indicator.health(); + Health second = indicator.health(); + Health third = indicator.health(); + + // Only one underlying count() invocation despite three probes — the + // 30s TTL cache absorbs the flood. Verifies readiness probes at 1Hz + // don't hammer Cypher. + org.mockito.Mockito.verify(graphStore, org.mockito.Mockito.times(1)).count(); + assertEquals(Status.UP, first.getStatus()); + assertEquals(Status.UP, second.getStatus()); + assertEquals(Status.UP, third.getStatus()); } } diff --git a/src/test/java/io/github/randomcodespace/iq/mcp/McpToolsTest.java b/src/test/java/io/github/randomcodespace/iq/mcp/McpToolsTest.java index 869cd101..4c812ddd 100644 --- a/src/test/java/io/github/randomcodespace/iq/mcp/McpToolsTest.java +++ b/src/test/java/io/github/randomcodespace/iq/mcp/McpToolsTest.java @@ -477,8 +477,16 @@ void readFileShouldHandleMissingFile(@TempDir Path tempDir) throws IOException { String result = mcpTools.readFile("nonexistent.txt", null, null); Map parsed = parseJson(result); - assertNotNull(parsed.get("error")); - assertTrue(parsed.get("error").toString().contains("Failed to read file")); + // PR 4 envelope contract: structured `{code, message, request_id, error}`. + // The `code` field is the canonical machine-parseable failure category; + // `error` is preserved for legacy clients but mirrors the JDK exception's + // message (NoSuchFileException text) rather than the old hand-built + // "Failed to read file: ..." prefix. + assertEquals("FILE_READ_FAILED", parsed.get("code")); + assertNotNull(parsed.get("message")); + assertTrue(parsed.containsKey("request_id"), + "Envelope must include request_id for log correlation (null is OK in tests)"); + assertNotNull(parsed.get("error"), "Legacy `error` field preserved for backwards-compat"); } @Test