feat(operator): foundation for an agent that can safely write - #622
Merged
Conversation
A generated POST/PUT/PATCH could not carry a body. buildBodyTemplate emitted
Qute variables — one per schema property, or {requestBody} for the unresolved
$ref that is the common case here — but registered none of them, and
paramDescriptions was only ever filled from path and query parameters.
AgentOrchestrator builds the tool schema from getParameters() alone, so those
variables were invisible to the model; with strict rendering off they render
empty. Every generated write therefore went out structurally valid and
semantically empty, and failed at the far end rather than at the config.
buildBodyTemplate now returns the template together with the variables it
expects, and buildApiCall declares them. Path and query names win a collision:
they are structural, and one Qute variable cannot mean two things.
The whole-body case is also the most reviewable shape, which matters for the
approval gate — what the model wrote is exactly what gets sent, rather than
inputs to a template the approver cannot see.
Three tests, mutation-checked: dropping the registration fails all three.
27 tests pass in McpApiToolBuilderTest.
Approval patterns could only name a tool, and generated tool names come from
operationId or a slug — they say nothing about what the call does and they
drift when the spec changes. Since ToolApprovalGate fails open on an unmatched
name, a renamed or newly generated write arrived ungated and silently.
The method and path were available and discarded one line into registration
(toolSources.put(name, "http")). They now travel alongside, so a pattern may
match a tool three ways: its bare name, source:name, or source.method:path.
"requireApproval": ["http.post:*", "http.put:*", "http.patch:*", "http.delete:*"]
gates every mutation without naming a single tool, so a newly generated
endpoint cannot arrive ungated. And because different POSTs carry different
weight:
"requireApproval": ["http.post:/agentstore/agents"]
addresses exactly one. Both forms speak the same METHOD /path vocabulary as the
endpoint allow-list, so the two can be generated from one source instead of
hand-maintained in two.
Backward compatibility was the risk worth designing around: making the source
itself "http.post" would have stopped "http:*" matching in every existing
config, removing gating silently. The source is unchanged and endpoint identity
travels in a parallel map, behind a classify() overload; the previous signature
delegates with an empty map. A test asserts http:* still gates every http tool
when no endpoint data is present.
Two details that would be easy to break later, both pinned by tests: a path
template's braces are literals only because compile() quotes every non-wildcard
segment, and a typo'd method prefix (http.pots:) is rejected rather than
compiled into a pattern that never matches — which would itself be an ungated
write.
522 tests pass on a clean build.
Review found the new validation opened the failure it was written to close. isMethodQualifiedSource accepted <anyKnownSource>.<method>, but only the http branch of tool registration records an endpoint — mcp and a2a register a source alone. So "mcp.post:*" saved cleanly, matched nothing at runtime, and because the gate allows an unmatched call, every MCP write executed unapproved while the config looked protected. The error message made it worse by suggesting the method-qualified form on every unknown-prefix rejection, teaching the shape that silently does nothing. The qualifier is now http-only, and the hint says so. Widen it again only alongside whatever populates endpoints for another source. Also normalises the stored path. The httpcall config accepts "/a/b", "a/b" and an absolute URL for the same endpoint (ApiCallExecutor applies the same leading-slash rule), so storing it verbatim made a pattern written in the documented shape miss two of the three — and a require-pattern that misses is an ungated write. docs/hitl.md documented two pattern forms; there are now three. The endpoint-qualified form, its http-only restriction, and the path normalisation are described there. Both fixes mutation-checked: restoring the any-source qualifier fails a test, and storing the raw path fails another. 414 tests pass.
Closes the two follow-ups from review together, because they share a cause: a per-property body template. - Every body variable became a REQUIRED tool parameter. AgentOrchestrator builds the tool schema from ApiCall.parameters and marks all of them required, and a Map<String,String> has nowhere to record optionality, so a PATCH of one field forced the model to restate every other — a partial update became a full overwrite. - Values were substituted into the JSON unescaped. The templating engine runs in TEXT mode and escapes nothing, so a model-supplied value containing a quote could break the body or add fields the schema never declared. For an agent that reads untrusted content and now writes through an API, that is an injection boundary rather than a formatting bug. Both disappear when the model writes the body itself: one variable, no substitution boundary, nothing to mark optional. It is also what makes the approval card honest — the HITL surface shows tool arguments, so "the arguments are the request" only holds if the body is one of them. The shape a decomposed template would have implied is preserved in the parameter description, which now names each property with its type and marks which are required — the model's only clue about what to write. Three existing tests asserted the decomposed shape and were rewritten rather than deleted, since the behaviour changed deliberately. The invariant test no longer names specific variables: it extracts every variable the template references and asserts each is declared, so it keeps holding whatever shape the template takes. 547 tests pass on a clean build.
An MCP tool call ran as whatever static credential the config named, so an
agent could not use MCP "on behalf of the user" the way an apicall can. A
${caller:token} there was passed through GlobalVariableResolver and
SecretResolver unchanged — neither handles the caller namespace — and sent as
the literal placeholder, failing silently rather than closed.
The transport supported this all along. customHeaders has three overloads and
EDDI used the Map one, which langchain4j wraps in a constant lambda; the
McpHeadersSupplier overload is applied per request inside createRequest. Using
it makes the credential per-call while the client stays cached.
The split it produces is the one we want, and the library enforces it: the
initialize handshake and tools/list carry a null invocation context, so
discovery cannot run under a user's credential and then be reused for
everyone's calls. Only tools/call carries the caller. A caller-bound config
therefore sends discovery unauthenticated and lets the server decide.
Also fixes a privilege bug this exposed rather than caused. Clients were cached
by URL alone, so two agents naming the same server with DIFFERENT credentials
silently shared whichever client was built first — the second borrowed the
first's authorization. The key now includes a digest of the configured
credential. Digest, not the value, so a literal key never becomes a map key
that could reach a heap dump; taken unresolved, so configs sharing one vault
reference still share a client. This does not multiply clients per user: a
caller-bound config yields one client whose supplier reads the caller per
request. closeClient closes every client for a URL, since a URL can now hold
more than one.
Worth noting what is NOT solved: on an expired MCP session the transport
retries initialize() on an HttpClient callback thread where the caller binding
does not exist. That path now sends the request unauthenticated instead of
silently falling back to a static key under the caller's intent — visible
failure rather than wrong authority.
My first credential-isolation tests were vacuous: they exercised cacheKey()
directly, so reverting the lookup to config.getUrl() left them all green. The
call site is now pinned by seeding the credential-aware key and requiring a
cache hit — mutation-checked, that reversion fails it.
415 tests pass on a clean build.
…oundation # Conflicts: # src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java # src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java
Contributor
|
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:
📝 WalkthroughWalkthroughThe PR changes HTTP request-body modeling to use one whole-body variable, adds endpoint-qualified HITL approval matching, and updates MCP authentication, metrics, caches, and circuit isolation for caller and credential separation. Documentation and tests cover these behaviors. ChangesOperator write foundation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AgentOrchestrator
participant ToolApprovalGate
participant ToolApprovalPatterns
AgentOrchestrator->>ToolApprovalGate: classify tool call with endpoint metadata
ToolApprovalGate->>ToolApprovalPatterns: match normalized method and path
ToolApprovalGate-->>AgentOrchestrator: approval decision
sequenceDiagram
participant McpToolProviderManager
participant CallerIdentityContext
participant McpServer
McpToolProviderManager->>McpServer: discover tools without caller token
McpToolProviderManager->>CallerIdentityContext: resolve caller token
CallerIdentityContext-->>McpToolProviderManager: authorization header
McpToolProviderManager->>McpServer: invoke tool with caller token
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR lays the groundwork for a workspace “operator” agent that can safely perform future deployment mutations by fixing MCP/http tool generation and HITL approval matching so that (a) generated write requests are actually representable to the model and (b) approval patterns can reliably gate mutations by HTTP method+path rather than unstable tool names.
Changes:
- Make generated OpenAPI/MCP httpcall request bodies model-supplied via a single whole-body parameter, and ensure body template variables are declared in the tool schema.
- Add endpoint provenance (
method:path) for httpcall tools and extend HITL approval patterns to matchhttp.<method>:/path(with backward compatibility). - Allow MCP calls to run under the chatting user via per-request header resolution, and isolate cached MCP clients by URL + credential digest.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java | Caller-bound MCP auth, credential-aware client cache keying, and updated close semantics. |
| src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java | Records http tool endpoint provenance and adds endpoint path normalization for approval matching. |
| src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java | Whole-body request template + parameter declaration for model-written request bodies. |
| src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGate.java | Adds endpoint-qualified matching support during tool-call classification. |
| src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalPatterns.java | Validates method-qualified http.<method>: prefixes for approval patterns. |
| src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerAdditionalTest.java | Adds tests for credential-isolated client cache keys. |
| src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java | Adds tests for endpoint path normalization; updates record constructor usage. |
| src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.java | Updates record constructor usage. |
| src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.java | Updates ToolSetup constructor usage. |
| src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java | Updates body template expectations and adds parameter/required-shape assertions. |
| src/test/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGateTest.java | Adds tests covering method/path-qualified patterns and backward compatibility. |
| docs/hitl.md | Documents endpoint-qualified approval patterns and method qualification constraints. |
| docs/changelog.md | Adds a changelog entry describing the operator-write foundation work and rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…dings
The discovery/invocation split was decided by asking whether a caller was bound
to the thread — but discovery runs inside the turn, on a thread that IS bound.
So initialize and tools/list went out with the first caller's token, and since
the client is cached that session was reused by everyone after them, with a tool
list reflecting one user's permissions offered to the next. langchain4j
distinguishes the two (listTools delegates a null InvocationContext,
McpToolExecutor always builds one) but enforces nothing; EDDI now reads
McpCallContext.invocationContext() and acts on it. Static analysis flagged the
parameter as unused, which it was — the fix was to use it, not drop it.
Four more from the same review round:
- A caller-bound key was put in the header raw while the static path prefixed
"Bearer ". apiKey is documented as a key/token, so the natural
apiKey: "${caller:token}" sent a bare token with no scheme and the server
would reject it. Both paths now prefix, and an author who spells the scheme
out is not double-prefixed.
- The tool cache was still keyed by URL alone. CachedTools holds executors bound
to a client, so tools discovered under one credential could be reused under
another — the same cross-credential leak fixed at the client layer, left open
one level up. Both now key on the credential, and closeClient sweeps the tool
cache by prefix rather than dropping a key that no longer exists.
- callerBound was detected with containsReference, which only matches the
documented ${caller:...} form. A bare {caller:token} looked like a static key
and was sent as literal text; rejectUnsupportedReference now turns it into the
clear error it was written for.
- normalizeEndpointPath treated anything starting with "http" as a URL. Narrowed
to a real http(s):// prefix: an opaque URI such as "httpfoo:bar" has a null
path, so the loose test collapsed it to empty and the endpoint provenance —
and the approval pattern's ability to match — vanished.
docs/hitl.md and the validation message both still listed the old allowed
character set; endpoint patterns permit / { } and now say so, in the two places
that had drifted.
The first version of the path test was vacuous: a relative URI keeps its path,
so both implementations agreed on "httpcalls/agents". It now asserts the opaque
case that actually distinguishes them, verified to fail against the old code.
810 tests pass on a clean build.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java:290
- The whole-body variable name ("requestBody") can collide with an explicit query/path parameter of the same name. Because body variables are added with putIfAbsent, that collision would skip declaring the body parameter even though the body template still references it, reintroducing the "empty body" failure mode (the model can’t supply an undeclared parameter and Qute renders it empty). Consider renaming the body variable on collision so it’s always declared.
// built from getParameters() alone (AgentOrchestrator), and with
// strict-rendering off an undeclared variable renders as empty. The
// request would go out structurally valid and semantically empty.
// Path and query names win a collision — those are structural.
body.variables().forEach(paramDescriptions::putIfAbsent);
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java (1)
261-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a top-level import instead of the inline FQN for
Pattern.Line 275 uses
java.util.regex.Pattern.compile(...)inline rather than importingPatternat the top of the file.🧹 Proposed fix
+import java.util.regex.Pattern; @@ - var matcher = java.util.regex.Pattern.compile("\\{([A-Za-z0-9_]+)}").matcher(createPet.getRequest().getBody()); + var matcher = Pattern.compile("\\{([A-Za-z0-9_]+)}").matcher(createPet.getRequest().getBody());As per coding guidelines, "Use simple names with top-level imports instead of inline fully qualified names; only use an inline FQN to disambiguate conflicting types, and remove unused imports."
🤖 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 `@src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java` around lines 261 - 283, Replace the inline java.util.regex.Pattern reference in parseAndBuild_requestBodyVariablesAreDeclaredAsParameters with the imported Pattern simple name, adding the top-level import and removing any now-unused or conflicting import as needed.Source: Coding guidelines
src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java (1)
327-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale javadoc describes removed per-property decomposition behavior.
The comment at Lines 327-333 ("Produces a Qute-templated JSON body where each property is a template variable... Only handles flat schemas...") describes the old decomposition logic that this PR removes, and now sits orphaned above the new
BodyTemplaterecord instead ofbuildBodyTemplate. Similarly, theWHOLE_BODY_VARIABLEjavadoc (Line 350) says it's "used when the schema has no properties," butbuildBodyTemplatenow uses it unconditionally for every non-null schema (Line 376). Both are misleading to future maintainers who might reintroduce per-property decomposition based on outdated docs.📝 Proposed doc fix
- /** - * Build a JSON body template from a schema. Produces a Qute-templated JSON body - * where each property is a template variable. - * <p> - * Note: Only handles flat schemas (direct properties). Nested objects and - * arrays fall back to a single {`@code` {requestBody}} template variable. - */ /** * A request-body template together with the tool parameters the model must * supply to fill it. @@ /** Name of the whole-body variable used when the schema has no properties. */ + /** Name of the single whole-body variable used for every JSON request body. */ static final String WHOLE_BODY_VARIABLE = "requestBody";🤖 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 `@src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java` around lines 327 - 351, Remove the stale schema-decomposition Javadoc above BodyTemplate, or move only any still-accurate documentation to the buildBodyTemplate method. Update WHOLE_BODY_VARIABLE’s Javadoc to state that requestBody is the whole-body template variable used for every non-null schema, matching buildBodyTemplate’s current behavior.src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java (3)
385-424: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo new metrics for the credential-isolation/caller-bound auth feature.
This introduces a security-relevant behavior change (per-credential client isolation, per-request caller-bound headers, discovery-vs-invocation authorization split) with no
MeterRegistryinstrumentation — e.g. a counter for "MCP tool call sent unauthenticated because no caller was bound" (line 617-618) would make silent auth failures in production visible instead of only appearing in DEBUG logs. As per coding guidelines, "Add metrics to new features using Micrometer MeterRegistry, including counters, timers, or gauges as appropriate."🤖 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 `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java` around lines 385 - 424, Instrument the credential-isolation and caller-bound authorization flow in McpToolProviderManager using the existing Micrometer MeterRegistry, adding a counter for MCP tool calls sent without a bound caller at the unauthenticated invocation path around getOrCreateClient and the caller-header handling. Ensure the counter is incremented whenever that silent unauthenticated request occurs, while preserving the existing client caching and authorization behavior.Source: Coding guidelines
70-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale javadoc on
clientCache/toolCacheafter the rekeying change.Both field comments still say "keyed by server URL" / "per server URL", but
getOrCreateClient/discoverToolsnow key oncacheKey(serverConfig)(URL + credential digest). This is exactly the distinction the PR is built around (privilege isolation between different credentials on the same URL), so leaving the doc unchanged risks a future maintainer reasoning about the cache with the pre-fix mental model.📝 Suggested doc update
/** - * Cache of active MCP clients, keyed by server URL. Connections are reused - * across conversation turns to avoid reconnect overhead. + * Cache of active MCP clients, keyed by {`@link` `#cacheKey`(McpServerConfig)} + * (server URL plus a digest of the configured credential). Connections are + * reused across conversation turns to avoid reconnect overhead. */ private final Map<String, McpClient> clientCache = new ConcurrentHashMap<>(); /** - * Cache of discovered tool specs/executors per server URL. Finding F12: without + * Cache of discovered tool specs/executors, keyed the same way as + * {`@link` `#clientCache`}. Finding F12: without * this, every conversation turn issued a live {`@code` tools/list} RPC to every🤖 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 `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java` around lines 70 - 82, Update the Javadoc for clientCache and toolCache to state that entries are keyed by cacheKey(serverConfig), incorporating the server URL and credential digest rather than the URL alone. Keep the existing reuse and discovery-cache behavior descriptions while documenting credential-based isolation consistently with getOrCreateClient and discoverTools.
403-416: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
"|"delimiter incacheKey/closeClientisn't escaped against a URL that itself contains"|".
cacheKeybuildsurl + "|" + digest, andcloseClientmatches onurl + "|"as a prefix. If two configured URLs are such that one is a"|"-prefix of another's cache key (e.g.http://xvs.http://x|anonymous),closeClient("http://x")would also tear down the second server's unrelated cached client, since its key also starts with"http://x|".validateServerUrldoesn't reject|in the URL, so this isn't structurally impossible, just very unlikely in practice.Using a compound key (e.g. a small record/
Map.entry(url, digest)as the cache key type) instead of a delimited string would remove the ambiguity entirely.Also applies to: 630-654
🤖 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 `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java` around lines 403 - 416, The cache key and close-client matching use an ambiguous delimiter when URLs contain “|”. Update cacheKey and closeClient to use a structured key containing the URL and credential identity (for example, a record or Map.Entry) rather than string concatenation and prefix matching, while preserving anonymous and hashed-credential isolation.
🤖 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 `@src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalPatterns.java`:
- Around line 76-80: Update the validation around KNOWN_SOURCES and
isMethodQualifiedSource in ToolApprovalPatterns to reject endpoint-looking
prefixes such as unqualified http or mcp targets unless they use a valid
http.<method> qualification. Require qualified http targets to have a remainder
beginning with / or *, so http.post:agents is rejected while valid path/template
targets remain accepted. Add regression coverage for http:/…, mcp:/…, and
http.post:agents.
In `@src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java`:
- Around line 386-415: Update describeBodySchema to branch its opening
description on schema.getType(): retain the single JSON object wording for
object schemas, and describe array or primitive schemas according to their
actual top-level shape. Ensure property enumeration and its existing
required/description details remain limited to object schemas so the generated
request-body guidance matches the schema structure.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java`:
- Around line 601-622: Update createTransport and its
discoverTools/client-construction flow to validate caller-bound apiKey
configurations before caching the transport. When apiKey contains a
${caller:...} reference while caller-identity is disabled, force the existing
discovery or client validation path to resolve and report the failure as
INVALID_CONFIGURATION instead of deferring CallerIdentityException to
authorizationHeader during a request. Preserve current validation for URL and
transport errors.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java`:
- Around line 327-351: Remove the stale schema-decomposition Javadoc above
BodyTemplate, or move only any still-accurate documentation to the
buildBodyTemplate method. Update WHOLE_BODY_VARIABLE’s Javadoc to state that
requestBody is the whole-body template variable used for every non-null schema,
matching buildBodyTemplate’s current behavior.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java`:
- Around line 385-424: Instrument the credential-isolation and caller-bound
authorization flow in McpToolProviderManager using the existing Micrometer
MeterRegistry, adding a counter for MCP tool calls sent without a bound caller
at the unauthenticated invocation path around getOrCreateClient and the
caller-header handling. Ensure the counter is incremented whenever that silent
unauthenticated request occurs, while preserving the existing client caching and
authorization behavior.
- Around line 70-82: Update the Javadoc for clientCache and toolCache to state
that entries are keyed by cacheKey(serverConfig), incorporating the server URL
and credential digest rather than the URL alone. Keep the existing reuse and
discovery-cache behavior descriptions while documenting credential-based
isolation consistently with getOrCreateClient and discoverTools.
- Around line 403-416: The cache key and close-client matching use an ambiguous
delimiter when URLs contain “|”. Update cacheKey and closeClient to use a
structured key containing the URL and credential identity (for example, a record
or Map.Entry) rather than string concatenation and prefix matching, while
preserving anonymous and hashed-credential isolation.
In `@src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java`:
- Around line 261-283: Replace the inline java.util.regex.Pattern reference in
parseAndBuild_requestBodyVariablesAreDeclaredAsParameters with the imported
Pattern simple name, adding the top-level import and removing any now-unused or
conflicting import as needed.
🪄 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: d5cfe5f8-9b15-4a44-a61c-0ef9f934835f
📒 Files selected for processing (13)
docs/changelog.mddocs/hitl.mdsrc/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGate.javasrc/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalPatterns.javasrc/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.javasrc/test/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGateTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerAdditionalTest.java
…oundation # Conflicts: # docs/changelog.md
CodeRabbit found two more shapes of the fail-open I have now fixed three times.
Running the validator confirmed all of these saved cleanly and matched nothing,
which for a require-rule leaves the call allowed while the config looks like
protection:
http:/agentstore/agents — a path needs a method-qualified prefix
mcp:/x — mcp tools carry no endpoint at all
http.post:agents — an endpoint path always begins with '/'
Rather than name a third special case, validation now enforces the invariant in
both directions: an endpoint-shaped target requires http.<method>, and a
method-qualified prefix requires a target that starts with '/' or '*'. Wildcard
prefixes such as http.*:/x are left alone — those do match.
Also from the same review:
- describeBodySchema opened with "a single JSON object" whatever the schema
declared. For a top-level array the model wraps the payload in braces and the
API rejects a request the config cannot explain; it now names the real
container.
- An MCP apiKey holding ${caller:...} with eddi.caller-identity.enabled=false
threw once per request, forever, for a mistake made once. It is validated
alongside the URL and transport checks and reported as INVALID_CONFIGURATION,
where an operator will see it. CallerIdentityResolver gained isEnabled() for
that.
812 tests pass on a clean build.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java:356
- When the OpenAPI media type declares a requestBody but its schema is null, buildBodyTemplate() returns a fixed "{}" and declares no parameters. That recreates the original failure mode for those specs: the model has no way to provide a body and every write goes out semantically empty. Treat a null schema the same as an undecomposable schema by using the whole-body variable and declaring it as a tool parameter.
private static BodyTemplate buildBodyTemplate(Schema<?> schema) {
if (schema == null) {
return new BodyTemplate("{}", Map.of());
}
src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java:412
- cacheKey() truncates the SHA-256 digest to 8 bytes (64 bits). Because this key is a privilege boundary (it determines whether two different credentials can share a client), a hash collision would reintroduce credential-sharing across agents. Using the full digest (or at least a larger prefix) avoids collision risk without leaking the credential.
try {
var digest = MessageDigest.getInstance("SHA-256").digest(apiKey.getBytes(StandardCharsets.UTF_8));
return config.getUrl() + "|" + HexFormat.of().formatHex(digest, 0, 8);
} catch (NoSuchAlgorithmException e) {
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java (1)
628-635: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed when a caller-bound tool call has no caller context.
A caller-bound credential currently sends the tool request without
Authorizationwhen the context is absent. That can execute against an anonymous MCP role rather than the chatting user’s permissions. ThrowCallerIdentityExceptionhere (or letresolveValue()do so) instead of returning an empty header map.🤖 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 `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java` around lines 628 - 635, Update the no-caller branch in McpToolProviderManager’s caller-bound credential resolution to fail closed by throwing CallerIdentityException, or by delegating to resolveValue() so it throws, instead of returning Map.of(). Preserve the existing behavior for calls with a valid caller context.docs/changelog.md (1)
123-132: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the changelog entry separator.
Line [123] starts the next entry without the
---delimiter used between the preceding entries. Add the separator to preserve consistent changelog structure and rendering.Proposed fix
+--- + ## 🔎 fix(llm): review follow-ups — workflow version parse, log sanitization (2026-07-29)🤖 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/changelog.md` around lines 123 - 132, Add the missing `---` separator immediately before the changelog entry beginning with “fix(llm): review follow-ups,” matching the delimiter structure used between the preceding entries. Leave the entry content unchanged.
🧹 Nitpick comments (1)
src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java (1)
294-294: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a top-level
Patternimport.
Patternis only referenced once and there is no conflicting type here, sojava.util.regex.Pattern.compile(...)should be written with a top-leveljava.util.regex.Patternimport instead of an inline FQN.🤖 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 `@src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java` at line 294, Import java.util.regex.Pattern at the top of McpApiToolBuilderTest and update the matcher creation to use Pattern.compile(...) instead of the fully qualified name.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 `@src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalPatterns.java`:
- Around line 90-93: Update the validation in ToolApprovalPatterns to reject
wildcard source prefixes targeting endpoint paths when they cannot match any
runtime http.<method> key, including mcp*:/x. Preserve valid wildcard patterns
such as http.*:/x and *:/x, while retaining the existing rejection for
non-method-qualified endpoint patterns.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java`:
- Around line 466-471: The validateCallerBoundKey method only handles supported
caller references; add validation for malformed or unsupported caller-reference
syntax before createTransport is reached, including forms such as {caller:token}
and unsupported ${caller:...} tokens. Reuse CallerIdentityResolver’s existing
rejection/validation behavior and translate any resulting exception into
IllegalArgumentException so these cases are classified as invalid configuration
rather than CONNECTION_FAILURE.
---
Outside diff comments:
In `@docs/changelog.md`:
- Around line 123-132: Add the missing `---` separator immediately before the
changelog entry beginning with “fix(llm): review follow-ups,” matching the
delimiter structure used between the preceding entries. Leave the entry content
unchanged.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java`:
- Around line 628-635: Update the no-caller branch in McpToolProviderManager’s
caller-bound credential resolution to fail closed by throwing
CallerIdentityException, or by delegating to resolveValue() so it throws,
instead of returning Map.of(). Preserve the existing behavior for calls with a
valid caller context.
---
Nitpick comments:
In `@src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java`:
- Line 294: Import java.util.regex.Pattern at the top of McpApiToolBuilderTest
and update the matcher creation to use Pattern.compile(...) instead of the fully
qualified name.
🪄 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: 301e856d-2258-4a94-821c-d06b1ff07aec
📒 Files selected for processing (7)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalPatterns.javasrc/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.javasrc/main/java/ai/labs/eddi/engine/security/CallerIdentityResolver.javasrc/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.javasrc/test/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGateTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java
Round two from CodeRabbit and Copilot, four findings.
The wildcard escape hatch in the previous commit was too wide: "mcp*:/x"
contains a '*' so it skipped the path check, yet no endpoint key can ever match
it, so the pattern saved and gated nothing. Rather than reason about glob
shapes, the prefix is now compiled and tried against every http.<method> EDDI
actually emits — the same question the gate asks at runtime. "http.*:/x" and
"*:/x" keep working because they genuinely match.
That is the fifth shape of this one bug. The invariant is simply that a pattern
which cannot match must not save, and it is now checked by construction rather
than by enumerating the ways to get it wrong.
- A declared request body whose media type has no schema returned "{}" and
declared no variable — recreating, for that spec shape, the empty-body bug
this branch exists to fix. It now uses the whole-body variable like any other
undecomposable schema.
- A malformed caller reference ({caller:token}, ${caller:tokn}) reached
createTransport, threw there, and was recorded as CONNECTION_FAILURE — which
blames the server and trips the circuit breaker for a mistake the config made.
It is validated with the URL and transport checks and reported as
INVALID_CONFIGURATION.
- The client cache key truncated its SHA-256 to 64 bits. That key decides
whether two credentials share a client, so a collision would reintroduce the
privilege leak it was added to close; the full digest costs nothing.
813 tests pass on a clean build.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java:291
buildBodyTemplate()always uses therequestBodyvariable, butbuildApiCall()only registers body variables viaputIfAbsent. If an OpenAPI operation already defines a query/path parameter namedrequestBody, the body variable will be skipped, leaving the template variable undeclared again (so the model can’t fill it and the rendered body can still go out empty). Consider renaming the body variable on collision so the template and parameters stay in sync.
// strict-rendering off an undeclared variable renders as empty. The
// request would go out structurally valid and semantically empty.
// Path and query names win a collision — those are structural.
body.variables().forEach(paramDescriptions::putIfAbsent);
}
src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java:599
withBearerPrefix()usesvalue.trim()only for the scheme check but then returns the originalvalue, preserving leading/trailing whitespace. That can produce anAuthorizationheader with unexpected spaces (or even a newline if misconfigured). Trimming once and returning the trimmed value keeps behavior predictable.
if (value == null || value.isBlank()) {
return value;
}
// A scheme is one token followed by a space; anything else is a bare secret.
return value.trim().matches("(?i)(bearer|basic|token|apikey)\\s.+") ? value : "Bearer " + value;
src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java:2519
normalizeEndpointPath()logs the raw configured path on parse failure. Since this value can include templates or other untrusted config text, it’s safer/consistent to pass it throughLogSanitizer.sanitize()to avoid log injection (newlines, control chars).
// Not parseable as a URI — a templated host, most likely. Leave it be:
// matching something odd is better than throwing during discovery.
LOGGER.debugf("Could not normalise endpoint path '%s' for approval matching", rawPath);
return path;
docs/hitl.md:264
- The docs say each call is tested against
source:namefirst, then the bare name, but the implementation now tries the endpoint-qualified form (source.method:path) beforesource:name(when endpoint provenance exists). Updating this sentence avoids readers assuming endpoint patterns are evaluated later.
- **`*` is the only wildcard** — it matches any run of characters (including empty). Every other character is a quoted literal, so compilation is **ReDoS-safe**.
- **Source-qualified or bare.** A pattern may carry a known source prefix (`mcp:read_*`, `http:*`) or match the bare tool name (`delete_account`). Each call is tested against `source:name` **first**, then the bare dispatch name — a tool with an unknown source still matches bare-name patterns (fail-safe).
- **Endpoint-qualified** (`http` tools only). A pattern may also address *what a tool calls* rather than what it is named: `http.post:*` matches every POST, `http.post:/agentstore/agents` matches exactly one endpoint. This is the robust form for tools generated from an OpenAPI spec, whose names come from `operationId` or a slug and change when the spec does — `http.post:*` keeps gating every mutation even when a name changes or a new endpoint appears. The path is matched **as the httpcall config declares it**, normalised to a leading slash (an absolute URL contributes only its path).
src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java:266
- Now that clients/tools are cached per
cacheKey(serverConfig)(URL + credential digest), the circuit breaker still keys failures by the bareurl(seeisCircuitOpen(url)/recordFailure(url)/recordSuccess(url)). If two MCP server entries share a URL but use different credentials, repeated auth failures for one credential can open the circuit for the whole URL and suppress discovery for the other (valid) credential too. Consider keying the circuit breaker state by the same cache key (or URL+credential) to keep isolation consistent.
// F12: serve the tool list from the TTL cache when it is still fresh —
// a live tools/list RPC per server per turn was pure overhead. Nothing
// reaches the cache without having passed validation first, so this is
// checked before the (potentially DNS-resolving) validation below.
CachedTools cached = toolCache.get(cacheKey(serverConfig));
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java:290
- Potential parameter-name collision: the request body is always
{requestBody}, but it is only added toApiCall.parametersviaputIfAbsent. If an OpenAPI operation has a query/path parameter namedrequestBody, the body parameter will be dropped and the model again has no declared way to populate the request body (recreating the original empty-body failure mode for that endpoint).
// built from getParameters() alone (AgentOrchestrator), and with
// strict-rendering off an undeclared variable renders as empty. The
// request would go out structurally valid and semantically empty.
// Path and query names win a collision — those are structural.
body.variables().forEach(paramDescriptions::putIfAbsent);
src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java:599
withBearerPrefixchecksvalue.trim()to detect an explicit scheme, but then returns the original untrimmed value (or prefixes the untrimmed value). Leading/trailing whitespace in a configured or resolved token will be sent on the wire and can cause authentication failures (e.g., pasted secrets with a trailing newline).
if (value == null || value.isBlank()) {
return value;
}
// A scheme is one token followed by a space; anything else is a bare secret.
return value.trim().matches("(?i)(bearer|basic|token|apikey)\\s.+") ? value : "Bearer " + value;
Five findings from the review round on the docs commit. The circuit breaker still keyed failures by bare URL after the client and tool caches moved to a credential-aware key. Two configs pointing at one server with different credentials therefore shared a circuit: one agent's revoked key would trip discovery for the other, still-valid one. Keyed by the same cacheKey now, with an isCircuitOpen(McpServerConfig) overload — the contract callers actually want, since the key is a credential-aware implementation detail and asking by URL gives the wrong answer. Two tests drove real discovery and then asserted by URL; they ask by config now. The rest seed and read the same string and are unaffected. - A spec naming a path or query parameter "requestBody" made putIfAbsent skip the body variable, leaving the template referencing something undeclared — the empty-body bug again, for that one spec shape. The body variable is renamed on collision instead of dropped; the structural parameter keeps its name. - withBearerPrefix trimmed only for its scheme check and returned the original, so leading or trailing whitespace — a newline above all — could reach an Authorization header. It returns the trimmed value. - normalizeEndpointPath logged the raw configured path; sanitized, consistent with main's CWE-117 sweep. - docs/hitl.md claimed a call is tested against source:name first. The endpoint-qualified form is tried before it. Corrected — the doc described the order I had written, not the one the code runs. 815 tests pass on a clean build.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGate.java:36
- The class-level Javadoc still says patterns are matched against
source:namefirst, but the implementation now tries the endpoint-qualified form first (source.method:path). This is an important behavioral detail for config authors, so the Javadoc should be updated to match the new precedence.
public GateResult classify(List<ToolExecutionRequest> batch, Map<String, String> toolSources,
ToolApprovalsConfig cfg, Set<String> clearedCallIds) {
return classify(batch, toolSources, Map.of(), cfg, clearedCallIds);
}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java (1)
347-366: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the
$reftest match its fixture.The fixture defines
#/components/schemas/Thingat Line 357, so the reference is resolvable. This test does not cover the “unresolved$ref” case described by its name and comment; either use a genuinely unresolved reference and assert the parser’s behavior, or rename the test to cover an empty resolved schema.🤖 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 `@src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java` around lines 347 - 366, Align parseAndBuild_wholeBodyVariableIsDeclared with its fixture by either changing the schema reference to an actually unresolved $ref and retaining assertions for the unresolved-reference behavior, or renaming the test and comment to describe the empty resolved Thing schema while preserving the corresponding expected behavior.src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java (1)
407-445: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve nested body schema shape in the parameter description
src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java:407-445only describes top-level properties. Nested objects, array item schemas, andintegervs.numberconstraints are lost, so the model can emit bodies that no longer match the OpenAPI schema. Recursively render nested shapes or reject unsupported schemas, and add nested object/array coverage.🤖 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 `@src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java` around lines 407 - 445, Extend describeBodySchema to recursively preserve nested object properties, array item schemas, and numeric distinctions or constraints when building the parameter description. Reuse the existing schema type and property rendering flow, adding a focused recursive helper for nested schemas and coverage for nested objects and arrays; do not silently flatten unsupported shapes.src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java (1)
594-600: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrim Authorization values before returning them.
value.trim()is used only for matching; the qualified branch returns the original value, and the bare branch prefixes the untrimmed value. Leading or trailing whitespace therefore survives.Proposed fix
if (value == null || value.isBlank()) { return value; } +value = value.trim(); -return value.trim().matches("(?i)(bearer|basic|token|apikey)\\s.+") ? value : "Bearer " + value; +return value.matches("(?i)(bearer|basic|token|apikey)\\s.+") ? value : "Bearer " + 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 `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java` around lines 594 - 600, Update withBearerPrefix to trim the input once and use the trimmed value for both scheme detection and the returned result, ensuring qualified Authorization values and bearer-prefixed secrets contain no leading or trailing whitespace.
🧹 Nitpick comments (3)
src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java (1)
256-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
Patternat the top level.The new test references
java.util.regex.Patterninline at Line 256. ImportPatternand use it directly. As per coding guidelines, reference types and annotations through top-level imports rather than fully qualified names inline.🤖 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 `@src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java` at line 256, Update McpApiToolBuilderTest by adding a top-level import for java.util.regex.Pattern, then change the matcher declaration to reference Pattern directly instead of using its fully qualified name.Source: Coding guidelines
src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java (1)
351-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the body-template Javadoc.
The preceding Javadoc still describes per-property variables and array fallback, but the implementation now always emits one whole-body variable; it is also no longer attached to
buildBodyTemplate. Move and update it so the documented contract matches the code.🤖 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 `@src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java` around lines 351 - 370, Move the Javadoc from the BodyTemplate record to buildBodyTemplate and revise it to document that the method always emits a single whole-body variable, including schemas with properties or arrays. Remove references to per-property variables and array fallback, and describe the resulting template and variable contract accurately.src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java (1)
720-738: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAttach the
circuitKeyJavadoc to the correct overload.The
@param circuitKeyblock is separated from the declarations by another Javadoc block, so it is orphaned whileisCircuitOpen(String)remains undocumented. Merge the blocks or move the parameter documentation directly above the string overload.🤖 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 `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java` around lines 720 - 738, Move the existing circuitKey `@param` documentation so it directly precedes the isCircuitOpen(String circuitKey) overload, merging it with that method’s description as needed. Keep the isCircuitOpen(McpServerConfig config) Javadoc focused on the config overload and ensure both declarations have correctly attached documentation.
🤖 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 `@AGENTS.md`:
- Around line 840-843: Synchronize caller-token restrictions across the
documented sites: retain the canonical complete wording in AGENTS.md lines
840-843; update docs/httpcalls.md lines 88-94 to reject ${caller:token} in query
parameters, request bodies, and paths; and expand the “Headers only” rule in
docs/security.md line 130 to include request bodies and paths.
In `@docs/mcp-server.md`:
- Around line 687-689: Update the documentation sentence near the signed-in user
HTTP call reference to replace “apicall headers” with consistent “API call
headers” terminology, leaving the remaining guarantees and link unchanged.
In `@src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java`:
- Around line 288-307: Update buildApiCall to construct the effective parameter
set from both the surrounding PathItem and operation.getParameters() before
populating paramDescriptions and selecting body variable names. Ensure
path-level parameters participate in the existing collision/rename logic in the
body variable loop, while preserving operation-level precedence and behavior for
non-colliding names; add coverage for a path-level requestBody parameter
collision.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java`:
- Around line 254-255: Add Micrometer instrumentation to the credential-scoped
client/tool cache and circuit-breaker flows in McpToolProviderManager, using
MeterRegistry to track cache hits and misses, discovery outcomes and timing, and
requests skipped while the circuit is open. Keep metric tags low-cardinality and
derive them from non-sensitive identifiers such as server or outcome values;
never expose raw credentials.
- Around line 254-255: Update
McpToolProviderManagerCircuitBreakerTest.circuitOpen_skipsServer so
injectFailures(...) stores failures under the credential-aware key produced by
cacheKey(serverConfig), including the anonymous credential suffix, matching
discoverTools() lookup behavior.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java`:
- Around line 407-445: Extend describeBodySchema to recursively preserve nested
object properties, array item schemas, and numeric distinctions or constraints
when building the parameter description. Reuse the existing schema type and
property rendering flow, adding a focused recursive helper for nested schemas
and coverage for nested objects and arrays; do not silently flatten unsupported
shapes.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java`:
- Around line 594-600: Update withBearerPrefix to trim the input once and use
the trimmed value for both scheme detection and the returned result, ensuring
qualified Authorization values and bearer-prefixed secrets contain no leading or
trailing whitespace.
In `@src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java`:
- Around line 347-366: Align parseAndBuild_wholeBodyVariableIsDeclared with its
fixture by either changing the schema reference to an actually unresolved $ref
and retaining assertions for the unresolved-reference behavior, or renaming the
test and comment to describe the empty resolved Thing schema while preserving
the corresponding expected behavior.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java`:
- Around line 351-370: Move the Javadoc from the BodyTemplate record to
buildBodyTemplate and revise it to document that the method always emits a
single whole-body variable, including schemas with properties or arrays. Remove
references to per-property variables and array fallback, and describe the
resulting template and variable contract accurately.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java`:
- Around line 720-738: Move the existing circuitKey `@param` documentation so it
directly precedes the isCircuitOpen(String circuitKey) overload, merging it with
that method’s description as needed. Keep the isCircuitOpen(McpServerConfig
config) Javadoc focused on the config overload and ensure both declarations have
correctly attached documentation.
In `@src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java`:
- Line 256: Update McpApiToolBuilderTest by adding a top-level import for
java.util.regex.Pattern, then change the matcher declaration to reference
Pattern directly instead of using its fully qualified name.
🪄 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: 51a8ea9a-6343-4e5c-9d7d-bce87d66aebd
📒 Files selected for processing (12)
AGENTS.mddocs/hitl.mddocs/httpcalls.mddocs/mcp-server.mddocs/security.mdsrc/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.javasrc/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerAdditionalTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerCircuitBreakerTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerDiscoveryTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerAdditionalTest.java
- docs/hitl.md
- src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java
Of eight unresolved review threads, four were already fixed and only looked
open — GitHub marks a thread outdated when its hunk moves, not when the finding
is addressed, so "unresolved and not outdated" is not the same as "still true".
Checked each against the code: toolCache already keys on the credential,
rejectUnsupportedReference is already called before callerBound is decided, the
illegal-character message already lists / { }, and describeBodySchema already
branches on the schema type.
The four that were real:
- Adding isCircuitOpen(McpServerConfig) left three stacked Javadoc blocks, so
an @PARAM documented a parameter the following method does not have. Each
overload has its own now.
- The new caches and the credential-scoped circuit breaker had no metrics, which
AGENTS.md requires for a new feature. eddi.mcp.discovery counts cache_hit,
success, failure and circuit_open. A test asserts no tag carries the URL or
the credential — the URL is unbounded cardinality, and the credential is the
thing cacheKey hashes precisely so it never becomes a map key.
- AGENTS.md said a token reference is rejected in query parameters, bodies and
paths while httpcalls.md and security.md still mentioned only query
parameters. The code rejects all three; the two narrower pages now say so.
- "apicall headers" reads as a typo in prose — "API call headers".
816 tests pass on a clean build.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java:2517
- normalizeEndpointPath() collapses an absolute URL with an empty path (e.g. "https://host" or "https://host?x=1") to "". That makes endpoint-qualified approval keys like "post:" which can never be matched by patterns expecting a leading '/'. Treat an empty extracted path as "/" so root endpoints can still be gated by endpoint-qualified patterns.
if (lower.startsWith("http://") || lower.startsWith("https://")) {
try {
String extracted = URI.create(path).getPath();
path = extracted != null ? extracted : "";
src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java:627
- withBearerPrefix() trims the value for matching but returns the original (potentially whitespace-padded) string when it already has a scheme. That can emit an invalid Authorization header like " Bearer …" and is also inconsistent with the prefixed branch. Safer to trim once and use the trimmed value for both branches.
if (value == null || value.isBlank()) {
return value;
}
// A scheme is one token followed by a space; anything else is a bare secret.
return value.trim().matches("(?i)(bearer|basic|token|apikey)\\s.+") ? value : "Bearer " + value;
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java (1)
664-671: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed when a caller-bound tool call has no caller.
Returning
Map.of()still sends the MCP request without authorization. For${caller:...}tool calls, throw before the transport sends the request; discovery can remain unauthenticated only when it is not caller-bound. Also updateunboundToolCallSendsNothingto expect authentication failure instead of empty headers.🤖 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 `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java` around lines 664 - 671, The caller-bound MCP tool path must fail before transport when callerIdentityContext.current() is null instead of returning Map.of() and sending unauthenticated. Update the logic around caller-bound detection and the no-caller branch in McpToolProviderManager to throw the established authentication failure, while preserving unauthenticated discovery for non-caller-bound calls; update unboundToolCallSendsNothing to assert authentication failure rather than empty headers.
🧹 Nitpick comments (1)
src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerAdditionalTest.java (1)
307-316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse imports for Micrometer test types.
Import
SimpleMeterRegistryandTagat the top of the file instead of using fully qualified names inline.As per coding guidelines, “Reference types with top-level imports rather than inline fully qualified names; remove unused imports.”
🤖 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 `@src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerAdditionalTest.java` around lines 307 - 316, Update McpToolProviderManagerAdditionalTest to import SimpleMeterRegistry and Tag at the top, then replace their fully qualified inline references in the registry and tag-value collection code with the imported types; remove any now-unused imports.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
`@src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerAdditionalTest.java`:
- Around line 310-313: Update the test around
McpToolProviderManagerAdditionalTest to override fetchToolsFromServer(...) with
a deterministic exception instead of invoking discovery against the unreachable
URL. Keep the test focused on asserting the failure metric and ensure no live
network call or timeout-dependent behavior remains.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java`:
- Around line 664-671: The caller-bound MCP tool path must fail before transport
when callerIdentityContext.current() is null instead of returning Map.of() and
sending unauthenticated. Update the logic around caller-bound detection and the
no-caller branch in McpToolProviderManager to throw the established
authentication failure, while preserving unauthenticated discovery for
non-caller-bound calls; update unboundToolCallSendsNothing to assert
authentication failure rather than empty headers.
---
Nitpick comments:
In
`@src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerAdditionalTest.java`:
- Around line 307-316: Update McpToolProviderManagerAdditionalTest to import
SimpleMeterRegistry and Tag at the top, then replace their fully qualified
inline references in the registry and tag-value collection code with the
imported types; remove any now-unused imports.
🪄 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: 040f4840-2e20-40be-8056-c35985db3c65
📒 Files selected for processing (5)
docs/httpcalls.mddocs/mcp-server.mddocs/security.mdsrc/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.javasrc/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerAdditionalTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/httpcalls.md
- docs/security.md
- docs/mcp-server.md
… root path The trim fix claimed in ec86304 never landed. The scripted edit did not match, I did not check the result, and the commit message asserted a change the code did not contain — so withBearerPrefix still returned the untrimmed value and a padded config would emit " Bearer …". Applied properly this time and verified in the file. Two more from the same round: - normalizeEndpointPath collapsed an absolute URL with no path ("https://host", "https://host?x=1") to the empty string, making the endpoint key "post:" — which no pattern expecting a leading slash can match, and an unmatched require-pattern is an ungated call. A root endpoint is "/". - The metrics test I added drove real discovery against an unreachable host, so it depended on DNS and waited out the 30-second default timeout. It stubs fetchToolsFromServer on a spy, like the discovery tests already do. 816 tests pass on a clean build.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGate.java:36
- The class-level Javadoc still says patterns are tested against "source:name" first, but classify()/firstMatch() now tests the endpoint-qualified form first (when present). This doc mismatch can mislead future changes to the precedence rules.
public GateResult classify(List<ToolExecutionRequest> batch, Map<String, String> toolSources,
ToolApprovalsConfig cfg, Set<String> clearedCallIds) {
return classify(batch, toolSources, Map.of(), cfg, clearedCallIds);
}
Request.path defaults to "" and means the target server's root — ApiCallExecutor sends the request to targetServerUrl unchanged. normalizeEndpointPath returned that empty string as-is, so the endpoint key was "post:", which no pattern can match, and a require-rule left the call ungated. The previous commit fixed the neighbouring case (an absolute URL carrying no path) and missed this one: an empty path never reaches the URL branch and falls past the leading-slash guard untouched. Mutation-checked: removing the guard fails the test. 816 tests pass on a clean build.
Member
Author
Review thread dispositionEvery unresolved thread checked against current code. GitHub marks a thread outdated only when its diff hunk moves, not when the finding is addressed — so several below read as open while already being fixed. Verified individually rather than by that flag. Fixed in this round
Already addressed in an earlier commit (verified in the current tree, not assumed)
Verification: 816 tests pass on a clean build; checkstyle clean. Each behavioural fix on this branch was mutation-checked by reverting it and confirming a test fails. One caveat for the approver. The central control here — |
aisabella-ai
self-requested a review
July 30, 2026 13:46
aisabella-ai
approved these changes
Jul 30, 2026
ginccc
added a commit
that referenced
this pull request
Jul 30, 2026
main moved 16 commits (#622 operator-write foundation, caller-bound MCP credentials, HITL endpoint patterns), putting the PR into CONFLICTING — which matters beyond tidiness: a conflicting PR has no computable merge ref, so no workflow can run, and the Integration Tests failure GitHub still showed was a stale pre-fix result. One textual conflict (docs/changelog.md, a pure union of entries). Three files auto-merged silently: McpToolProviderManager, AgentOrchestrator and McpToolProviderManagerAdditionalTest. One of those auto-merges was wrong. #622 made the MCP tool cache credential-scoped, not just the client cache: toolCache is keyed on cacheKey(config) (url|sha-256 of apiKey, or url|anonymous). The F12 TTL tests seeded it under the bare URL, so after the merge the seeded entry no longer matched the lookup: - freshEntryIsServedFromCache failed loudly (expected <1> but was <0>) - staleEntryIsNotServed kept PASSING, vacuously — nothing was served because nothing matched the key, not because the entry was stale The helper now derives the key by reflectively calling the production cacheKey, the same idiom #622's own tests use, so a future change to the key shape carries these tests with it instead of hollowing them out. Production was correct throughout; only the test was stale. The fix is included in this merge commit so the commit builds and tests clean, per AGENTS.md rule 6. Verified the other two auto-merges rather than trusting them: AgentOrchestrator was javadoc-only on our side; McpToolProviderManager's guard block interleaved coherently (validateServerUrl + our validateTransport + #622's validateCallerBoundKey, with our one-time deprecated-transport warning after it), and our URL-keyed deprecatedTransportWarned set is correctly unaffected by credential scoping — transport is a property of the server, not the credential.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Groundwork for a workspace operator agent that manages a deployment — creating and updating agents and groups — while acting as the person chatting to it, with that person's permissions.
No new capability is granted here. The operator's endpoint allow-list is untouched and still read-only. What changes is that widening it is now safe, where before it was neither safe nor functional.
Generated writes did not work at all
McpApiToolBuilder.buildBodyTemplateemitted Qute variables for a request body but registered none of them as tool parameters, andAgentOrchestratorbuilds the tool schema fromApiCall.getParameters()alone. The model therefore had no documented way to fill a body; with strict rendering off those variables render empty.Every generated
POST/PUT/PATCHwent out structurally valid and semantically empty. Adding a write endpoint before this fix would have produced garbage requests that fail at the far end rather than at the config.The body is now one model-written variable
A per-property template looked more helpful and was worse three ways:
Map<String,String>has nowhere to record optionality, so aPATCHof one field forced the model to restate all the others, turning a partial update into a full overwrite.TEXTmode), so a value containing a quote could break the body or add fields the schema never declared. For an agent that reads untrusted content and writes through an API, that is an injection boundary.The shape a decomposed template implied now lives in the parameter description, which names each property with its type and marks which are required.
Approval patterns can address the endpoint a tool calls
Tool names come from
operationIdor a slug and drift when a spec changes — andToolApprovalGateallows an unmatched call, so a renamed or newly generated write arrived ungated and silently. Method and path were available and discarded one line into registration.{ "requireApproval": ["http.post:*", "http.put:*", "http.patch:*", "http.delete:*"], "exempt": ["http.get:*"] }gates every mutation without naming a single tool, so a new endpoint cannot arrive ungated.
http.post:/agentstore/agentsaddresses exactly one, because different POSTs carry different weight. Both forms speak the sameMETHOD /pathvocabulary as the endpoint allow-list, so the two can be generated from one source rather than maintained in two.Design rule: enumerate downward, never upward. Whether something is gated stays in
requireApproval/exempt. A missed exemption costs an approval prompt; a missed requirement is an ungated write. The same reasoning restricts the method qualifier tohttp— the only source whose tools record an endpoint — somcp.post:is rejected at save time instead of saved as a pattern nothing could ever match.Backward compatibility was the risk worth designing around: making the source itself
http.postwould have stoppedhttp:*matching in every existing config, removing gating silently. The source is unchanged; endpoint identity travels in a parallel map behind aclassify()overload, and a test assertshttp:*still gates every http tool when no endpoint data is present.MCP tool calls can now run as the chatting user
They previously ran as whatever static credential the config named, and a
${caller:token}there passed through the global-variable and secret resolvers untouched and was sent as the literal placeholder — failing silently rather than closed.The transport supported this all along:
customHeadershas three overloads and EDDI used the constant one. The per-requestMcpHeadersSupplieroverload makes the credential per-call while the client stays cached. The split it produces is the one we want and langchain4j enforces it —initializeandtools/listcarry a null invocation context, so discovery cannot run under one user's credential and then be reused for everyone's calls.Fixed in passing: a privilege bug
MCP clients were cached by URL alone, so two agents naming the same server with different credentials silently shared whichever client was constructed first — the second borrowed the first's authorization. The key now includes a digest of the configured credential: a digest so a literal key never becomes a map key that could reach a heap dump, taken unresolved so configs sharing a vault reference still share a client. No per-user client explosion — a caller-bound config yields one client whose supplier reads the caller per request.
Not solved, deliberately
On an expired MCP session the transport retries
initialize()on an HTTP callback thread where the caller binding does not exist. That path now sends the request unauthenticated rather than falling back to the static key under the caller's intent — a visible failure instead of the wrong authority.Verification
566 tests pass on a clean build across the affected suites; checkstyle clean. Each fix was mutation-checked by reverting it and confirming a test fails.
Worth flagging for reviewers: four tests written during this work were found vacuous by that check and rewritten. Three asserted against a helper or against state seeded through the very wrapper under test; one seeded a thread binding that the fix then restored, so it passed either way. The mutation check caught every one — the tests here are green and verified to fail without their fix.
Review notes
Three earlier claims were corrected during this work, each of which changed a decision:
McpCallsConfiguration.toolsWhitelistdoes not fail closed — filtering is skipped when the list is empty, the same shape as the approval gate.toolApprovalsis not agent-only —LlmConfigurationcarries a per-task override that fully replaces the agent-level block.PUTon a config does not reach a running agent:HistorizedResourceStore.updatecreatesversion + 1while agents pin a version, so self-modification takes a chain of write → re-point → redeploy, each independently gated. The thing to guard is whatever can re-point a version reference, notPUTin general.Next
Per-endpoint approval friction (
timeoutPolicy,approvalTimeoutand the pause message are still single scalars for every gated tool) · agent-readable documentation (EDDI's MCP client never reads MCP resources, soeddi://docs/*is reachable from a desktop client but not from an agent) · widening the allow-list · the Manager scope picker and approval surface.Summary by CodeRabbit
requestBodyparameter.${caller:token}placement rules.