feat: docs for agents on every surface, MCP resource bridge, strict task-level toolApprovals - #668
Conversation
…matches reality docs/mcp-server.md has long documented toolsWhitelist: ["read_docs", "list_docs"] — tools that did not exist. A whitelist that matches nothing exposes nothing, so anyone copying the example got a silently tool-less server. The two tools now exist (McpDocTools), delegating to the same DocsService as REST and the eddi://docs/* resources. Tools ALONGSIDE resources on purpose: agentic MCP clients — EDDI's own McpToolProviderManager included — consume tools/list and never call resources/read, so resources alone reach desktop clients and no agent. Role set mirrors IRestDocs exactly via the new McpToolUtils.requireAnyRole: EDDI has no role hierarchy, so any-of-five must be enumerated or the two surfaces guard the same published pages differently.
… by a path hack One flag in DocsService turns every docs surface off together (REST list/read, MCP resources, MCP tools) — all four delegate here. The previous way to disable docs was pointing eddi.docs.path at a directory that does not exist, which works but reads as a misconfiguration in every log line and diagnostic. A policy deserves a switch, not a hack. Default true, and the field is initialized to true as well so a plain-constructed instance (unit tests construct DocsService directly) matches the CDI default instead of silently disabling docs. Disabled reads resolve exactly like the absent-directory case — no new response shape for callers that never handled one.
…server's resources MCP resources are the half of the protocol tool-consuming agents never see: EDDI's client (like most agentic clients) calls tools/list and never resources/read, even though langchain4j's McpClient has supported listResources/readResource all along. exposeResources: true on an mcpcalls config now synthesizes <name>_list_resources and <name>_read_resource tools bridging that gap for ANY server, EDDI's own included. Design decisions, each deliberate: - Opt-in per config, default false — a config written before this existed must not silently grow two tools. - NOT subject to toolsWhitelist: the whitelist governs names the SERVER advertises; these two are synthesized by EDDI and carry their own opt-in. A pre-existing whitelist must not disable the feature it predates, nor may a server occupy the synthesized names. - Construction is purely local: executors dial lazily through the shared credential-keyed client cache, so an unreachable server costs an error tool RESULT at call time, not a discovery failure. - Text capped at 64K chars with a truncation marker; binary content is described, never base64-dumped into model context. - Same static-config rejections as discoverTools (URL, transport, caller-bound key), surfaced as INVALID_CONFIGURATION failures.
…te (strict by default) A per-task toolApprovals FULLY REPLACED the agent-level gate — the identical ternary in LlmTask and ToolLoopResumer — so requireApproval: [] buried among forty fields of an llmstore document was a complete bypass: reviewed as an ordinary config edit, effective as a security change. This is also what forced EDDI-Manager to refuse llmstore writes for its Platform Operator outright (gate-guard.ts). eddi.hitl.tool.task-approvals.mode now decides, via the shared TaskToolApprovalsResolver both sites call: - strict (default): the task block can only STRENGTHEN the agent gate. requireApproval = union (string-level union is semantically exact for the gate's any-match OR, and neutralizes the empty-list bypass: [] ∪ agent = agent). exempt = the agent's list verbatim, task entries ignored — exempt beats require (ToolApprovalGate P1), so a task-added exemption is precisely the ungating vector; and a string-level intersection would be semantically WRONG (a task exempting a strict subset of the agent's patterns shares no strings with it and would silently gate every read — the trap the critical design pass caught). Task AUTO_APPROVE (scalar or rule) is demoted to WAIT_INDEFINITELY unless the agent itself grants it, generalizing the existing inherited-AUTO_APPROVE demotion. maxAutoApprovalsPerTurn takes the minimum; cosmetics stay task-first. - replace (legacy): the historical wholesale override, for designs that deliberately run one task looser than its agent. Mode via ConfigProvider (precedented: AgentOrchestrator, DeploymentContextCondition) since ToolLoopResumer is not a CDI bean. LlmStore warns at save time about task exempt/AUTO_APPROVE that strict mode will not honour — visibility, not rejection: stored configs never brick and replace mode still honours them. LlmTaskCoverageTest.toolApprovals_taskOverrideUsed deliberately updated: it pinned the replace semantics (assertSame); it now pins the strict merge reaching the orchestrator. Full contract in TaskToolApprovalsResolverTest.
…surface cross-refs Second critical pass over the branch, two findings: 1. maxAutoApprovalsPerTurn: an unset agent value is not 'no cap' — the runtime resolves it to DEFAULT_MAX_AUTO_APPROVALS_PER_TURN (2) — so the naive min-of-non-nulls let a task state 10 against an unset agent value and raise the effective budget. Today the fixed carried >= 2 no-progress threshold happens to bound the damage, but the resolver's 'budget may only shrink' contract must not depend on a distant guard staying fixed. The default constant now lives on ToolApprovalsConfig (single source; ConversationHitlService aliases it) and strict mode clamps a stated task value to min(task, agent ?? default). Pinned by strictBudgetCannotGrowPastAnUnsetAgentValue. 2. DocsService/McpDocResources javadocs still described a world where no doc surface reached an agent — stale the moment McpDocTools landed. Cross-references updated to name all four surfaces and the one switch that governs them.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds strict task-level HITL approval merging, MCP tools for documentation access, an opt-in MCP resource bridge, and the ChangesHITL approval resolution
Documentation surfaces
MCP resource bridge
Supporting records
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
… the deployment honours it warnStrictModeImplications fired unconditionally, so a deployment running eddi.hitl.tool.task-approvals.mode=replace — where a task-level exempt and AUTO_APPROVE ARE honoured verbatim — told authors on every save that their settings were being ignored. A warning that is sometimes flatly wrong is worse than none: authors learn to disregard it, including on the strict deployments where it is the only signal. TaskToolApprovalsResolver.configuredMode() is now public so a caller that merely describes the semantics can read what this deployment will actually do rather than asserting the default. Also bounds McpResourceBridgeTest's unreachable-server case to a 500ms timeout: the default is 30s, and a CI host that black-holes rather than refuses would otherwise stall the suite.
…cuted cold Standalone implementation plan for the follow-up this branch's PR body only sketched. Written so a coding agent with no context on the thread can pick it up: the three concrete gaps (gate cannot classify MCP read vs write, thin approval previews, Manager guards blind to MCP), the facts already verified against the code (quarkus-mcp-server 1.13.1 DOES support tool annotations; langchain4j-mcp 1.18.1 does NOT surface them, which is why Phase 2 needs a first-party registry), and what is explicitly NOT a gap (request pinning holds structurally for MCP — args are frozen in the batch, there is no resolution step to drift). Four phases, each shippable alone, ordered so no phase leaves the operator holding ungated write tools.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/main/java/ai/labs/eddi/configs/llm/mongo/LlmStore.java (1)
73-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider gating the warnings on the configured mode.
warnStrictModeImplicationsalways logs, even when the deployment setseddi.hitl.tool.task-approvals.mode=replace. Inreplacemode the task-levelexemptandAUTO_APPROVEvalues ARE honoured, so the warnings describe behavior that does not occur. Each save of an affectedllmstoredocument then emits misleading WARN lines.The message text names the strict mode explicitly, so this is not incorrect information. It is noise. Read the mode through
TaskToolApprovalsResolver.MODE_PROPERTYand skip the warnings when the mode isREPLACE.♻️ Proposed refactor
private static void warnStrictModeImplications(ToolApprovalsConfig cfg, String fieldPath) { if (cfg == null) { return; } + if (TaskToolApprovalsResolver.Mode.parse(ConfigProvider.getConfig() + .getOptionalValue(TaskToolApprovalsResolver.MODE_PROPERTY, String.class).orElse("strict")) + == TaskToolApprovalsResolver.Mode.REPLACE) { + return; + } if (cfg.getExempt() != null && !cfg.getExempt().isEmpty()) {🤖 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/configs/llm/mongo/LlmStore.java` around lines 73 - 95, Update warnStrictModeImplications to read the configured mode using TaskToolApprovalsResolver.MODE_PROPERTY and return without logging when the mode is REPLACE. Keep the existing warning checks and messages unchanged for strict mode or any other applicable mode.src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskCoverageTest.java (1)
305-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the approval mode explicit in this test.
The assertion at Line 331 only holds when
eddi.hitl.tool.task-approvals.moderesolves tostrict.LlmTask.executeTaskcalls the two-argumentTaskToolApprovalsResolver.resolve, which reads that property throughConfigProvider.getConfig(). This test class is a plain Mockito test, so the value comes from whatever config source the test classpath provides. If a test profile later setsreplace, this test fails withList.of("delete_*")and the failure will not point at the cause.Set the property for the test, or assert the resolved mode first so the failure is self-describing.
♻️ Proposed change
void toolApprovals_taskOverrideUsed() throws Exception { + // Pin the mode this assertion depends on; the resolver reads it from config. + assertEquals(TaskToolApprovalsResolver.Mode.STRICT, + TaskToolApprovalsResolver.Mode.parse(ConfigProvider.getConfig() + .getOptionalValue(TaskToolApprovalsResolver.MODE_PROPERTY, String.class).orElse("strict")), + "this test pins the STRICT merge; set eddi.hitl.tool.task-approvals.mode=strict for it"); llmTask.toolHitlEnabled = true;🤖 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/LlmTaskCoverageTest.java` around lines 305 - 332, Make the approval mode explicit in toolApprovals_taskOverrideUsed by setting eddi.hitl.tool.task-approvals.mode to strict through the test’s existing configuration mechanism before calling llmTask.execute, or assert the resolved mode before the merge assertion so configuration failures are self-describing. Keep the expected merged requireApproval values unchanged.src/main/java/ai/labs/eddi/engine/hitl/tools/TaskToolApprovalsResolver.java (1)
96-100: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider caching the mode lookup.
resolvereadsMODE_PROPERTYthroughConfigProvider.getConfig().getOptionalValue(...)on every call.LlmTask.executeTaskcalls this once per matching task per turn, andToolLoopResumercalls it once per resume. The value is a deployment-level setting that does not change at runtime.Cache the parsed mode in a
static volatileholder, or expose the resolver as an@ApplicationScopedbean with an injected@ConfigProperty. The bean form also aligns with the repository rule to use Quarkus CDI for components; the current static form is documented as a deliberate exception becauseToolLoopResumeris not a CDI bean.♻️ Minimal caching variant
+ private static volatile Mode cachedMode; + public static ToolApprovalsConfig resolve(ToolApprovalsConfig agentLevel, ToolApprovalsConfig taskLevel) { - Mode mode = Mode.parse(ConfigProvider.getConfig() - .getOptionalValue(MODE_PROPERTY, String.class).orElse("strict")); + Mode mode = cachedMode; + if (mode == null) { + mode = Mode.parse(ConfigProvider.getConfig() + .getOptionalValue(MODE_PROPERTY, String.class).orElse("strict")); + cachedMode = mode; + } return resolve(agentLevel, taskLevel, mode); }🤖 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/hitl/tools/TaskToolApprovalsResolver.java` around lines 96 - 100, Cache the parsed MODE_PROPERTY value in the static resolve(ToolApprovalsConfig, ToolApprovalsConfig) path so configuration lookup occurs once and subsequent calls reuse the deployment-level Mode. Use a static volatile holder with safe lazy initialization, preserving the existing "strict" default and delegation to resolve(agentLevel, taskLevel, mode); do not change the overload’s behavior or call sites.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 `@docs/hitl.md`:
- Line 315: Update the Configuration section’s statement about task-level
toolApprovals to describe the mode-dependent behavior: strict is the default and
combines policies as defined by TaskToolApprovalsResolver, while replace
preserves full replacement as legacy behavior. Point readers to the Precedence
section for the merge rules, removing the claim that task blocks always replace
agent-level policy.
In `@docs/mcp-server.md`:
- Line 13: Update the documented whitelist tool count near the later MCP tool
reference from 74 to 76, matching the “Available Tools (76)” heading and
eliminating the conflicting total.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java`:
- Around line 531-624: Update renderResourceList and renderResourceContents to
process every remote resource, template, description, and text field through the
existing directive-filtering content guard used for remote tool descriptions.
Enforce the 65,536-character limit before appending each field, preserving one
aggregate remaining-character budget across the complete rendered output.
Delimit the resulting resource data as untrusted remote content and retain
truncation behavior.
- Around line 495-513: Update the resource tool executors around listExecutor
and readExecutor so blocking getOrCreateClient(...).listResources/readResource
calls run on a dedicated bounded executor with an explicit timeout and
cancellation handling. Return or propagate the completed result asynchronously
without waiting on the conversation or event-loop thread, while preserving the
existing validation and error responses.
In `@src/test/java/ai/labs/eddi/modules/llm/impl/McpResourceBridgeTest.java`:
- Around line 98-107: Add a package-visible client-creation seam in the resource
bridge setup, then update unreachableServerFailsAtCallTime to inject a mocked
McpClient whose listResources() throws instead of connecting to 127.0.0.1:9.
Keep construction successful and preserve the assertion that executing
dead_list_resources returns text beginning with “Error listing resources”.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/configs/llm/mongo/LlmStore.java`:
- Around line 73-95: Update warnStrictModeImplications to read the configured
mode using TaskToolApprovalsResolver.MODE_PROPERTY and return without logging
when the mode is REPLACE. Keep the existing warning checks and messages
unchanged for strict mode or any other applicable mode.
In `@src/main/java/ai/labs/eddi/engine/hitl/tools/TaskToolApprovalsResolver.java`:
- Around line 96-100: Cache the parsed MODE_PROPERTY value in the static
resolve(ToolApprovalsConfig, ToolApprovalsConfig) path so configuration lookup
occurs once and subsequent calls reuse the deployment-level Mode. Use a static
volatile holder with safe lazy initialization, preserving the existing "strict"
default and delegation to resolve(agentLevel, taskLevel, mode); do not change
the overload’s behavior or call sites.
In `@src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskCoverageTest.java`:
- Around line 305-332: Make the approval mode explicit in
toolApprovals_taskOverrideUsed by setting eddi.hitl.tool.task-approvals.mode to
strict through the test’s existing configuration mechanism before calling
llmTask.execute, or assert the resolved mode before the merge assertion so
configuration failures are self-describing. Keep the expected merged
requireApproval values unchanged.
🪄 Autofix
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: 962842c2-3f68-40c1-a899-fd1837ef5213
📒 Files selected for processing (21)
docs/changelog.mddocs/hitl.mddocs/mcp-server.mdsrc/main/java/ai/labs/eddi/configs/hitl/model/ToolApprovalsConfig.javasrc/main/java/ai/labs/eddi/configs/llm/mongo/LlmStore.javasrc/main/java/ai/labs/eddi/configs/mcpcalls/model/McpCallsConfiguration.javasrc/main/java/ai/labs/eddi/engine/docs/DocsService.javasrc/main/java/ai/labs/eddi/engine/hitl/tools/TaskToolApprovalsResolver.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationHitlService.javasrc/main/java/ai/labs/eddi/engine/mcp/McpDocResources.javasrc/main/java/ai/labs/eddi/engine/mcp/McpDocTools.javasrc/main/java/ai/labs/eddi/engine/mcp/McpToolUtils.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.javasrc/main/java/ai/labs/eddi/modules/llm/impl/McpToolsProvider.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ToolLoopResumer.javasrc/main/resources/application.propertiessrc/test/java/ai/labs/eddi/engine/hitl/tools/TaskToolApprovalsResolverTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpDocToolsTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskCoverageTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/McpResourceBridgeTest.java
| ToolExecutor listExecutor = (request, memoryId) -> { | ||
| try { | ||
| return renderResourceList(getOrCreateClient(serverConfig)); | ||
| } catch (Exception e) { | ||
| LOGGER.warnf("MCP list_resources failed for '%s': %s", sanitize(serverName), e.getMessage()); | ||
| return "Error listing resources from MCP server '" + serverName + "': " + e.getMessage(); | ||
| } | ||
| }; | ||
| ToolExecutor readExecutor = (request, memoryId) -> { | ||
| String uri = resourceUriArgument(request); | ||
| if (isNullOrEmpty(uri)) { | ||
| return "Error: the 'uri' argument is required - call " + listName + " for the available uris."; | ||
| } | ||
| try { | ||
| return renderResourceContents(getOrCreateClient(serverConfig).readResource(uri), uri); | ||
| } catch (Exception e) { | ||
| LOGGER.warnf("MCP read_resource failed for '%s' uri '%s': %s", sanitize(serverName), sanitize(uri), e.getMessage()); | ||
| return "Error reading resource '" + uri + "' from MCP server '" + serverName + "': " + e.getMessage(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
For LangChain4j 1.18.1-beta28, what supported non-blocking or asynchronous APIs exist for MCP resource operations such as listResources, listResourceTemplates, and readResource?
💡 Result:
In LangChain4j 1.18.1-beta28, the McpClient interface provides direct, blocking synchronous APIs for MCP resource operations, specifically listResources, listResourceTemplates, and readResource(String) [1][2][3]. There are no explicit non-blocking or asynchronous versions of these specific methods (e.g., returning a CompletableFuture or using a reactive type) exposed in the McpClient interface [3]. While these core MCP client methods are synchronous, you can manage them in a non-blocking way using standard Java concurrency patterns, such as wrapping these calls in a CompletableFuture.supplyAsync block [2][3]. Additionally, it is important to distinguish these resource operations from other parts of the LangChain4j MCP framework: - McpClientAgent: This component provides an @McpClientAgent annotation with an async attribute, but this is specifically for wrapping and invoking MCP tools as agents, not for performing raw resource operations like listResources or readResource [4]. - Asynchronous Transports: The underlying transports (such as Streamable HTTP/SSE or Stdio) handle communication asynchronously at the protocol level, but this abstraction is encapsulated within the DefaultMcpClient implementation, which exposes a synchronous API to the user [3][5][6].
Citations:
- 1: https://docs.langchain4j.dev/apidocs/dev/langchain4j/mcp/client/McpClient.html
- 2: https://docs.langchain4j.dev/apidocs/dev/langchain4j/mcp/client/DefaultMcpClient.html
- 3: https://github.com/langchain4j/langchain4j/blob/main/langchain4j-mcp/src/main/java/dev/langchain4j/mcp/client/McpClient.java
- 4: https://docs.langchain4j.dev/apidocs/dev/langchain4j/agentic/declarative/McpClientAgent.html
- 5: https://github.com/langchain4j/langchain4j/blob/main/docs/docs/tutorials/mcp.md
- 6: https://blog.marcnuri.com/connecting-to-mcp-server-with-langchain4j
🏁 Script executed:
#!/bin/bash
set -e
file="src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java"
printf '%s\n' '--- relevant methods and call sites ---'
rg -n -C 8 'ToolExecutor|listResources|readResource|timeoutMs|RESOURCE_CONTENT_MAX_CHARS|getOrCreateClient|renderResource' "$file"
printf '%s\n' '--- MCP execution abstractions ---'
rg -n -C 5 'MCP|McpClient|execute.*Mcp|CompletableFuture|AsyncResponse|ToolExecutor' src/main/javaRepository: labsai/EDDI
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -e
file="src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java"
printf '%s\n' '--- manager structure ---'
rg -n '^(public|private|protected).*\(|ToolExecutor|listResources|readResource|timeoutMs|RESOURCE_CONTENT_MAX_CHARS|renderResource' "$file"
printf '%s\n' '--- target implementation ---'
sed -n '430,535p' "$file"
printf '%s\n' '--- tool execution service implementation ---'
rg -l 'class ToolExecutionService|executeToolWrapped' src/main/java | head -20Repository: labsai/EDDI
Length of output: 8712
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- exact tool invocation path ---'
rg -n -C 10 'execute\(|ToolExecutor|executeToolWrapped|resourceBridgeTools|McpResourceBridge' \
src/main/java/ai/labs/eddi/modules/llm/impl/ToolLoopRunner.java \
src/main/java/ai/labs/eddi/modules/llm/impl/ToolLoopResumer.java \
src/main/java/ai/labs/eddi/modules/llm/tools/ToolExecutionService.java \
src/main/java/ai/labs/eddi/modules/mcpcalls/impl/McpCallsTask.java \
src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java
printf '%s\n' '--- MCP timeout configuration and client construction ---'
rg -n -C 8 'timeoutMs|timeout|McpClientBuilder|DefaultMcpClient|McpServerConfig|getOrCreateClient|clientCache' \
src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java \
src/main/java/ai/labs/eddi/modulesRepository: labsai/EDDI
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -e
for f in \
src/main/java/ai/labs/eddi/modules/llm/impl/ToolLoopRunner.java \
src/main/java/ai/labs/eddi/modules/llm/impl/ToolLoopResumer.java \
src/main/java/ai/labs/eddi/modules/llm/tools/ToolExecutionService.java \
src/main/java/ai/labs/eddi/modules/mcpcalls/impl/McpCallsTask.java \
src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java; do
echo "--- $f ---"
rg -n 'executeToolWrapped|resourceBridgeTools|McpResourceBridge|executor\.execute|ToolExecutor|timeoutMs|getTimeoutMs|getOrCreateClient|listResources|readResource' "$f" | head -120
doneRepository: labsai/EDDI
Length of output: 4966
🏁 Script executed:
#!/bin/bash
set -e
sed -n '520,580p' src/main/java/ai/labs/eddi/modules/llm/impl/ToolLoopRunner.java
sed -n '80,180p' src/main/java/ai/labs/eddi/modules/llm/tools/ToolExecutionService.java
sed -n '670,715p' src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java
sed -n '210,245p' src/main/java/ai/labs/eddi/modules/llm/impl/ToolLoopResumer.javaRepository: labsai/EDDI
Length of output: 14456
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- tool loop scheduling and timeout boundaries ---'
rg -n 'ExecutorService|ManagedExecutor|Virtual|CompletableFuture|future|supplyAsync|executeSingleToolCallResult|runToolLoop|run\(' \
src/main/java/ai/labs/eddi/modules/llm/impl/ToolLoopRunner.java \
src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java \
src/main/java/ai/labs/eddi/modules/llm/impl/CascadingModelExecutor.java | head -180
printf '%s\n' '--- resource rendering bounds ---'
sed -n '528,625p' src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java
printf '%s\n' '--- dependency version ---'
rg -n -C 2 'langchain4j|1\.18|mcp' pom.xml '**/pom.xml' 2>/dev/null | head -120Repository: labsai/EDDI
Length of output: 13411
Isolate synchronous MCP resource calls from conversation execution.
McpClient.listResources() and McpClient.readResource() are blocking APIs in LangChain4j 1.18.1-beta28. ToolExecutor invokes them synchronously, so a slow MCP server can hold the tool-loop thread for the 30-second default transport timeout. Add an asynchronous tool-execution path, or isolate these calls on a dedicated bounded executor with timeout and cancellation handling. Do not wait on the result from the conversation or event-loop thread.
🤖 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 495 - 513, Update the resource tool executors around listExecutor
and readExecutor so blocking getOrCreateClient(...).listResources/readResource
calls run on a dedicated bounded executor with an explicit timeout and
cancellation handling. Return or propagate the completed result asynchronously
without waiting on the conversation or event-loop thread, while preserving the
existing validation and error responses.
Source: Coding guidelines
A static final array is still element-mutable — a standard static-analysis finding (SpotBugs MS_MUTABLE_ARRAY) and a real hazard for a constant that decides an authorization check. requireAnyRole now takes a Collection.
…te resource content Addresses CodeRabbit review on PR #668, plus the functional bug its tool-count nitpick was a shadow of. THE REAL BUG: McpToolFilter is a name whitelist and list_docs/read_docs were never added to it, so ToolFilter#test returned false and the tools were invisible to every external MCP client. The feature was dead on arrival, and McpDocToolsTest missed it because it calls the methods directly, bypassing the filter entirely. Fixed, and McpToolFilterCoverageTest now pins BOTH directions by reflection — every @tool is whitelisted (the invisible-tool bug) and every whitelisted name is declared (stale entries that read as coverage while protecting nothing). Writing that test immediately exposed a second subtlety: an omitted @tool name defaults to the sentinel Tool.ELEMENT_NAME, not to blank, which McpGroupTools relies on for all 18 of its tools. SECURITY (CodeRabbit, valid): the resource bridge returned remote descriptions and content to the model with no directive filtering, while governDescription has sanitized remote TOOL descriptions since finding F16 — the bridge was the easy way around a guard the tool path already had, over a larger surface. All remote text now goes through the same DIRECTIVE_PATTERN redaction, is delimited as untrusted server data, and is bounded per field: the previous code appended an unbounded description and only checked the running length afterwards, so one oversized field sailed past the aggregate cap. TEST DETERMINISM (CodeRabbit, valid): the unreachable-server case asserted a property of the host, not of this code. getOrCreateClient is now a package-visible seam (same precedent as fetchToolsFromServer) and the test stubs a throwing client. DOCS (CodeRabbit, valid): hitl.md's Configuration section still stated the pre-6.3.0 'full replace' rule, contradicting the new Precedence row — a reader stopping there would author a task-level exempt expecting it to loosen the gate. mcp-server.md's whitelist paragraph still said 74 tools. Also moves the follow-up plan from the skill-branded docs/superpowers/ to planning/, which is where this repo's 25 other plans live, and renames it to that folder's convention.
|
Thanks — this was a high-value review. Four of five addressed in 🐛 The tool-count nitpick found a dead featureChasing the 74-vs-76 inconsistency led to Fixed, plus the systemic guard: 🔒 Remote-content boundary — agreed, and it was the sharper version of your pointYou're right, and the inconsistency was the damning part: All remote text now goes through the same 🧪 Test determinism — agreedThe test asserted a property of the CI host, not of this code. 📄 Both docs findings — agreed
⏭️ Declined: isolating blocking MCP resource calls onto a bounded executorCorrect that If EDDI wants non-blocking tool execution, it belongs at the ℹ️ Already fixed before the review landedThe |
docs/superpowers/ was a Claude-Code-skill-branded folder that leaked in from earlier agent sessions. Two problems with it: this repo already has a planning/ folder holding 26 plans and design docs (flat, kebab-case, no date prefixes), and docs/ is PUBLISHED — GitBook renders it and the Docker image ships from it — so internal planning material does not belong there. Moves the one remaining file (the 2026-07-21 manager-coverage backend design) to planning/manager-coverage-backend-design.md, dropping the date prefix to match the folder's convention; the document carries its own **Date:** line, so nothing is lost. Repointed all three inbound references rather than leaving them dangling: two in docs/changelog.md and one in TenantQuotaService's javadoc, which cites the design's C5 seam. The folder is now gone entirely. (The surviving 'superpowers:' mentions in two planning/ files are skill invocation names for agentic workers, not paths — deliberately untouched.)
…itl-strict # Conflicts: # docs/changelog.md
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/mcp-server.md (1)
110-110: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete the
list_docsdescription.The row ends with “the repository's” and omits the referenced noun. Change it to “the repository's documentation set” so the description is complete.
🤖 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/mcp-server.md` at line 110, Complete the list_docs row description by changing the unfinished phrase “the repository's” to “the repository's documentation set,” leaving the rest of the description unchanged.
🤖 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 `@planning/manager-coverage-backend-design.md`:
- Around line 92-100: Update the C1(b)/C2 status section to reflect that the
includeDeleted fix has shipped: remove the instruction to land it and the
statement that C2 is blocked on C1(b). Mark the prior concern as historical,
then state that C2 is unblocked and retain only its remaining design questions.
- Line 137: Remove the stale “Redesign around ObservableChatModel” directive and
replace it with guidance to implement the six-site ChatResponse recorder design,
covering default, streaming, and conversation-attribution paths. Update the
surrounding design guidance in the documented section without changing the
earlier rationale that rejects ObservableChatModel as the metering seam.
In `@planning/operator-mcp-guardrails-plan.md`:
- Line 108: Update findGateCarryingCalls so the MCP security check does not rely
on a hardcoded three-tool allowlist: use a shared or generated mutating-tool
classification, treating unknown MCP tools as writes. For MCP resource-writing
calls, retain the existing containsToolApprovalsKey traversal, llm-store
resourceType detection, and fail-closed behavior when argsTruncated is true.
- Around line 91-94: Update the EDDI-server detection in McpToolProviderManager
so the McpReadOnlyToolRegistry is applied only after an authenticated trust
decision, such as validated credentials, a trusted origin with authentication,
or certificate verification; do not rely solely on /administration/docs or an
identity tool because foreign servers can spoof them. If authenticated trust
cannot be established, leave toolReadOnly absent.
- Line 80: Update McpToolAnnotationsCoverageTest to validate annotation values,
not just that annotations exist: explicitly assert that mutating or destructive
`@Tool` methods are not marked readOnlyHint = true, or compare against an expected
read-only tool set. Preserve coverage for every annotated MCP tool and ensure
the assertions prevent write tools from entering McpReadOnlyToolRegistry.
- Line 107: Update the self-guard flow around findSelfTargetedCalls to use
authoritative, non-redacted MCP arguments rather than
PendingToolCallView.arguments; if those arguments are unavailable, redacted, or
argsTruncated is true, fail closed by disabling approval. Preserve the existing
known-read handling and uriTargetsAgent matching semantics for complete
arguments.
---
Outside diff comments:
In `@docs/mcp-server.md`:
- Line 110: Complete the list_docs row description by changing the unfinished
phrase “the repository's” to “the repository's documentation set,” leaving the
rest of the description unchanged.
🪄 Autofix
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: aac335a5-0d29-4bc4-ac2b-a39cc4927967
📒 Files selected for processing (14)
docs/changelog.mddocs/hitl.mddocs/mcp-server.mdplanning/manager-coverage-backend-design.mdplanning/operator-mcp-guardrails-plan.mdsrc/main/java/ai/labs/eddi/configs/llm/mongo/LlmStore.javasrc/main/java/ai/labs/eddi/engine/hitl/tools/TaskToolApprovalsResolver.javasrc/main/java/ai/labs/eddi/engine/mcp/McpDocTools.javasrc/main/java/ai/labs/eddi/engine/mcp/McpToolFilter.javasrc/main/java/ai/labs/eddi/engine/mcp/McpToolUtils.javasrc/main/java/ai/labs/eddi/engine/tenancy/TenantQuotaService.javasrc/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.javasrc/test/java/ai/labs/eddi/engine/mcp/McpToolFilterCoverageTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/McpResourceBridgeTest.java
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/ai/labs/eddi/configs/llm/mongo/LlmStore.java
- src/main/java/ai/labs/eddi/engine/mcp/McpDocTools.java
- docs/hitl.md
- src/main/java/ai/labs/eddi/engine/hitl/tools/TaskToolApprovalsResolver.java
- src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java
| 3. **Populate it for EDDI's own server.** In `McpToolProviderManager`, after `tools/list`, resolve read-only per tool: | ||
| - langchain4j does not surface annotations (see §2), so introduce a small first-party source of truth: a `McpReadOnlyToolRegistry` listing EDDI's own read-only tool names, derived from the Phase 1 annotations. **Generate or test-pin it against the annotations so the two cannot drift** — e.g. a test that reflects over the `@Tool` methods and asserts the registry matches exactly. | ||
| - Apply it only when the server is EDDI's own. Detect by probing `GET {baseUrl}/administration/docs` or a dedicated identity tool — **do not** infer from the URL string. | ||
| - For foreign servers, leave the entry absent. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not trust an unauthenticated identity probe.
A foreign MCP server can imitate /administration/docs or a dedicated identity tool and expose a write tool with a read-only name. Use an authenticated trust decision, such as a trusted origin with credentials or a certificate. Otherwise, leave toolReadOnly empty.
🤖 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 `@planning/operator-mcp-guardrails-plan.md` around lines 91 - 94, Update the
EDDI-server detection in McpToolProviderManager so the McpReadOnlyToolRegistry
is applied only after an authenticated trust decision, such as validated
credentials, a trusted origin with authentication, or certificate verification;
do not rely solely on /administration/docs or an identity tool because foreign
servers can spoof them. If authenticated trust cannot be established, leave
toolReadOnly absent.
|
|
||
| **Goal:** the two hard controls stop being blind to MCP. | ||
|
|
||
| 1. **`self-guard.ts`** — `findSelfTargetedCalls` currently returns `[]` for any call without `requestPreview`. Add an MCP branch: when `call.source === "mcp"` and the method is not a known read, parse `call.arguments` (JSON) and refuse if the operator's own `agentId` appears in any string value. Reuse the existing `uriTargetsAgent` case-insensitive/percent-decoded comparison semantics for the id match. **Preserve the module's stated asymmetry**: a false positive costs one refused approval; a false negative costs the gate. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Use authoritative arguments for the self-guard.
PendingToolCallView.arguments is redacted and may be truncated (Line [64]). Parsing this display payload can hide agentId and produce a false negative. Pass non-redacted arguments to the guard, or disable approval when the payload is incomplete or redacted. Apply the same fail-closed rule to argsTruncated.
🤖 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 `@planning/operator-mcp-guardrails-plan.md` at line 107, Update the self-guard
flow around findSelfTargetedCalls to use authoritative, non-redacted MCP
arguments rather than PendingToolCallView.arguments; if those arguments are
unavailable, redacted, or argsTruncated is true, fail closed by disabling
approval. Preserve the existing known-read handling and uriTargetsAgent matching
semantics for complete arguments.
| **Goal:** the two hard controls stop being blind to MCP. | ||
|
|
||
| 1. **`self-guard.ts`** — `findSelfTargetedCalls` currently returns `[]` for any call without `requestPreview`. Add an MCP branch: when `call.source === "mcp"` and the method is not a known read, parse `call.arguments` (JSON) and refuse if the operator's own `agentId` appears in any string value. Reuse the existing `uriTargetsAgent` case-insensitive/percent-decoded comparison semantics for the id match. **Preserve the module's stated asymmetry**: a false positive costs one refused approval; a false negative costs the gate. | ||
| 2. **`gate-guard.ts`** — `findGateCarryingCalls` matches `/llmstore/llms` in the URI. Add: when `source === "mcp"` and the tool is a resource-writing tool (`update_resource`, `create_resource`, `apply_agent_changes`), inspect `arguments` for a `toolApprovals` key at any depth (the existing `containsToolApprovalsKey` walker already does this — reuse it) and for `resourceType` naming the llm store. **`argsTruncated: true` must fail closed**, exactly as `bodyTruncated` does today. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not make the three-tool list the security boundary.
The MCP branch covers only update_resource, create_resource, and apply_agent_changes. A later resource-writing tool can carry toolApprovals or target the LLM store and skip the Manager guard. Use a shared or generated mutating-tool classification, with unknown MCP tools treated as writes.
🤖 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 `@planning/operator-mcp-guardrails-plan.md` at line 108, Update
findGateCarryingCalls so the MCP security check does not rely on a hardcoded
three-tool allowlist: use a shared or generated mutating-tool classification,
treating unknown MCP tools as writes. For MCP resource-writing calls, retain the
existing containsToolApprovalsKey traversal, llm-store resourceType detection,
and fail-closed behavior when argsTruncated is true.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/mcp-server.md (1)
110-110: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete the
list_docsdescription.The row ends with “the repository's” and omits the referenced noun. Change it to “the repository's documentation set” so the description is complete.
🤖 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/mcp-server.md` at line 110, Complete the list_docs row description by changing the unfinished phrase “the repository's” to “the repository's documentation set,” leaving the rest of the description unchanged.
🤖 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 `@planning/manager-coverage-backend-design.md`:
- Around line 92-100: Update the C1(b)/C2 status section to reflect that the
includeDeleted fix has shipped: remove the instruction to land it and the
statement that C2 is blocked on C1(b). Mark the prior concern as historical,
then state that C2 is unblocked and retain only its remaining design questions.
- Line 137: Remove the stale “Redesign around ObservableChatModel” directive and
replace it with guidance to implement the six-site ChatResponse recorder design,
covering default, streaming, and conversation-attribution paths. Update the
surrounding design guidance in the documented section without changing the
earlier rationale that rejects ObservableChatModel as the metering seam.
In `@planning/operator-mcp-guardrails-plan.md`:
- Line 108: Update findGateCarryingCalls so the MCP security check does not rely
on a hardcoded three-tool allowlist: use a shared or generated mutating-tool
classification, treating unknown MCP tools as writes. For MCP resource-writing
calls, retain the existing containsToolApprovalsKey traversal, llm-store
resourceType detection, and fail-closed behavior when argsTruncated is true.
- Around line 91-94: Update the EDDI-server detection in McpToolProviderManager
so the McpReadOnlyToolRegistry is applied only after an authenticated trust
decision, such as validated credentials, a trusted origin with authentication,
or certificate verification; do not rely solely on /administration/docs or an
identity tool because foreign servers can spoof them. If authenticated trust
cannot be established, leave toolReadOnly absent.
- Line 80: Update McpToolAnnotationsCoverageTest to validate annotation values,
not just that annotations exist: explicitly assert that mutating or destructive
`@Tool` methods are not marked readOnlyHint = true, or compare against an expected
read-only tool set. Preserve coverage for every annotated MCP tool and ensure
the assertions prevent write tools from entering McpReadOnlyToolRegistry.
- Line 107: Update the self-guard flow around findSelfTargetedCalls to use
authoritative, non-redacted MCP arguments rather than
PendingToolCallView.arguments; if those arguments are unavailable, redacted, or
argsTruncated is true, fail closed by disabling approval. Preserve the existing
known-read handling and uriTargetsAgent matching semantics for complete
arguments.
---
Outside diff comments:
In `@docs/mcp-server.md`:
- Line 110: Complete the list_docs row description by changing the unfinished
phrase “the repository's” to “the repository's documentation set,” leaving the
rest of the description unchanged.
🪄 Autofix
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: aac335a5-0d29-4bc4-ac2b-a39cc4927967
📒 Files selected for processing (14)
docs/changelog.mddocs/hitl.mddocs/mcp-server.mdplanning/manager-coverage-backend-design.mdplanning/operator-mcp-guardrails-plan.mdsrc/main/java/ai/labs/eddi/configs/llm/mongo/LlmStore.javasrc/main/java/ai/labs/eddi/engine/hitl/tools/TaskToolApprovalsResolver.javasrc/main/java/ai/labs/eddi/engine/mcp/McpDocTools.javasrc/main/java/ai/labs/eddi/engine/mcp/McpToolFilter.javasrc/main/java/ai/labs/eddi/engine/mcp/McpToolUtils.javasrc/main/java/ai/labs/eddi/engine/tenancy/TenantQuotaService.javasrc/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.javasrc/test/java/ai/labs/eddi/engine/mcp/McpToolFilterCoverageTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/McpResourceBridgeTest.java
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/ai/labs/eddi/configs/llm/mongo/LlmStore.java
- src/main/java/ai/labs/eddi/engine/mcp/McpDocTools.java
- docs/hitl.md
- src/main/java/ai/labs/eddi/engine/hitl/tools/TaskToolApprovalsResolver.java
- src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java
🛑 Comments failed to post (2)
planning/manager-coverage-backend-design.md (2)
92-100: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the C1(b) and C2 status.
Line [55] already records the
includeDeletedfix as shipped, but Line [96] still instructs the reader to land it. Line [100] still says selective purge is blocked on C1(b). Mark this text as historical, or state that C2 is now unblocked and list only its remaining design questions. Otherwise, stale status can block implementation or cause duplicate work.🤖 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 `@planning/manager-coverage-backend-design.md` around lines 92 - 100, Update the C1(b)/C2 status section to reflect that the includeDeleted fix has shipped: remove the instruction to land it and the statement that C2 is blocked on C1(b). Mark the prior concern as historical, then state that C2 is unblocked and retain only its remaining design questions.
137-137: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the stale
ObservableChatModeldirective.Lines [120]-[124] reject
ObservableChatModelas the metering seam and identify sixChatResponsecall sites. Line [137] still directs implementation aroundObservableChatModel. Replace it with the six-site recorder design. Otherwise, future metering work can miss default, streaming, and conversation-attribution paths.Proposed wording
-Redesign around ObservableChatModel before implementing. +Implement the recorder at the six verified ChatResponse call sites; do not use ObservableChatModel as the primary seam.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Implement the recorder at the six verified ChatResponse call sites; do not use ObservableChatModel as the primary seam.🤖 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 `@planning/manager-coverage-backend-design.md` at line 137, Remove the stale “Redesign around ObservableChatModel” directive and replace it with guidance to implement the six-site ChatResponse recorder design, covering default, streaming, and conversation-attribution paths. Update the surrounding design guidance in the documented section without changing the earlier rationale that rejects ObservableChatModel as the metering seam.
…rdrails plan All four were design flaws in the plan, caught before anyone implemented it — which is what a plan review is for. 1. Phase 1's coverage test asserted annotation PRESENCE only. Phase 2 derives the read-only registry from those annotations, so a write tool mistakenly carrying readOnlyHint=true would pass the test and then be exempted by mcp.readonly:* — a mislabelled annotation becomes an ungated write. The test now pins an explicit expected read-only set (exact match, both directions) plus a mutating-prefix assertion, so adding a tool forces the 'is this really read-only?' review moment. 2. Phase 2 proposed identifying EDDI's own MCP server by probing /administration/docs. Unsound: a hostile server can just answer the probe, then expose a write tool under a read-only name and inherit the exemption. An unauthenticated probe is a liveness check, never a trust decision. Trust must be configuration-side and authenticated; anything unproven stays absent, which the gate already treats as 'gate it'. 3. Phase 3's self-guard was to search PendingToolCallView.arguments for the agent id — but that payload is redacted and size-capped, so finding nothing there is a FALSE NEGATIVE, i.e. a silently unguarded write. Now fails closed on argsTruncated and on unparseable payloads, matching the bodyTruncated rule gate-guard.ts already applies. 4. Phase 3's gate-guard scoped its MCP branch to three named tools. That fails open for the next resource-writing tool added — the same fail-open-enumeration bug that shipped list_docs/read_docs invisible through McpToolFilter, and the reason tool-scopes.ts is an allow-list. Now classifies positively: an MCP tool is a write unless positively known read-only, reusing Phase 2's classification. The three generalisable lessons are added to the plan's guardrails section so the next reader inherits them rather than rediscovering them.
|
All four accepted and fixed in 1. Annotation values, not presence. Right, and this was the sharpest one. Phase 2 derives the read-only registry from those annotations, so a write tool mistakenly carrying 2. Unauthenticated identity probe. Agreed, and the plan was plainly wrong here. Probing 3. Redacted/truncated arguments. Agreed, and it's inconsistent that I didn't apply my own rule. 4. Three-tool list as the boundary. Agreed — and this is the same bug class that already bit this PR: I also promoted the three generalisable lessons into the plan's guardrails section, so the next reader inherits them instead of rediscovering them:
Thanks — this materially improved the plan. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@planning/operator-mcp-guardrails-plan.md`:
- Around line 112-116: Update findSelfTargetedCalls and findGateCarryingCalls to
inspect authoritative, non-redacted MCP arguments rather than
PendingToolCallView.arguments; if only the display payload is available, add and
honor a trusted indicator for redaction or incompleteness and refuse approval
whenever it is set, alongside argsTruncated or parse failure. Preserve the
existing known-read classification and fail-closed behavior for untrusted
payloads.
- Around line 112-116: Define a trusted Manager-side read-only classification
contract for MCP calls used by findSelfTargetedCalls and findGateCarryingCalls,
keyed by source and dispatch tool name. Propagate a validated toolReadOnly bit
or reuse a trusted shared/generated mapping, and treat missing, stale, or
unmapped classifications as writes so unknown tools remain guarded.
🪄 Autofix
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: bf81088e-dd5d-4010-bfac-1dbb6a098cbb
📒 Files selected for processing (1)
planning/operator-mcp-guardrails-plan.md
| 1. **`self-guard.ts`** — `findSelfTargetedCalls` currently returns `[]` for any call without `requestPreview`. Add an MCP branch: when `call.source === "mcp"` and the tool is not a known read, parse `call.arguments` (JSON) and refuse if the operator's own `agentId` appears in any string value. Reuse the existing `uriTargetsAgent` case-insensitive/percent-decoded comparison semantics for the id match. | ||
| **`arguments` is a display payload, so absence of evidence is not evidence of absence.** It is redacted and size-capped (`PendingToolCallView.arguments` / `argsTruncated`), so the id can be missing from what the Manager holds while being present in what would execute — searching it and finding nothing yields a **false negative**, i.e. a silently unguarded write. Therefore: **refuse when `argsTruncated` is true, and refuse when the payload does not parse** — the same fail-closed rule `gate-guard.ts` already applies to `bodyTruncated`. Only a complete, parseable payload that demonstrably lacks the id may pass. | ||
| **Preserve the module's stated asymmetry**: a false positive costs one refused approval; a false negative costs the gate. | ||
| 2. **`gate-guard.ts`** — `findGateCarryingCalls` matches `/llmstore/llms` in the URI. Add an MCP branch that inspects `arguments` for a `toolApprovals` key at any depth (reuse the existing `containsToolApprovalsKey` walker) and for a `resourceType` naming the llm store. **`argsTruncated: true`, and any unparseable payload, must fail closed** — exactly as `bodyTruncated` does today, and for the reason spelled out in step 1. | ||
| **Do not make an enumerated tool list the security boundary.** An earlier draft scoped this branch to `update_resource`, `create_resource`, `apply_agent_changes`; that fails open the moment a resource-writing tool is added — the same fail-open-enumeration bug that shipped `list_docs`/`read_docs` invisible through `McpToolFilter`, and the reason `tool-scopes.ts` is an allow-list rather than a deny-list. Classify instead: **an MCP tool is a write unless it is positively known to be read-only**, reusing the Phase 2 read-only classification rather than a second hand-maintained list. Then the guard inspects every MCP call that is not a known read, and an unrecognised future tool is covered by default. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
Fail closed on redacted arguments, not only truncated arguments.
PendingToolCallView.arguments is a redacted display payload. Valid JSON can omit agentId, toolApprovals, or resourceType after redaction. argsTruncated == false and successful parsing do not prove that the inspected fields were absent. Pass authoritative non-redacted arguments to both guards, or add a trusted redaction/incomplete indicator and refuse approval when it is set.
Also applies to: 139-139
🧰 Tools
🪛 LanguageTool
[grammar] ~116-~116: Use a hyphen to join words.
Context: ...only classification rather than a second hand-maintained list. Then the guard ins...
(QB_NEW_EN_HYPHEN)
🤖 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 `@planning/operator-mcp-guardrails-plan.md` around lines 112 - 116, Update
findSelfTargetedCalls and findGateCarryingCalls to inspect authoritative,
non-redacted MCP arguments rather than PendingToolCallView.arguments; if only
the display payload is available, add and honor a trusted indicator for
redaction or incompleteness and refuse approval whenever it is set, alongside
argsTruncated or parse failure. Preserve the existing known-read classification
and fail-closed behavior for untrusted payloads.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'PendingToolCallView|toolReadOnly|findSelfTargetedCalls|findGateCarryingCalls|argsTruncated|toolName|source' \
--glob '*.{java,ts,tsx}' .Repository: labsai/EDDI
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- planning document ---'
sed -n '80,155p' planning/operator-mcp-guardrails-plan.md
printf '%s\n' '--- relevant tracked paths ---'
git ls-files | rg '(^|/)(self-guard|gate-guard|tool-scopes|PendingToolCall|.*Mcp.*|.*mcp.*|.*guard.*)\.(ts|tsx|java|md)$' | head -200
printf '%s\n' '--- exact symbols in source and planning files ---'
rg -n -C 4 \
'PendingToolCallView|toolReadOnly|ToolContribution|ToolSetup|ToolApprovalGate|containsToolApprovalsKey|argsTruncated|findSelfTargetedCalls|findGateCarryingCalls' \
planning src --glob '*.md' --glob '*.ts' --glob '*.tsx' --glob '*.java' \
| head -1200Repository: labsai/EDDI
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Manager and PendingToolCallView files ---'
git ls-files | rg '(^|/)(EDDI-Manager|src/lib/api/hitl\.ts|.*PendingToolCallView.*|.*self-guard.*|.*gate-guard.*)$' || true
printf '%s\n' '--- all read-only classification references ---'
rg -n -C 3 'toolReadOnly|read-only classification|McpReadOnlyToolRegistry|mcp\.readonly' \
planning docs src --glob '*.md' --glob '*.java' --glob '*.ts' --glob '*.tsx' \
| head -800
printf '%s\n' '--- Manager-facing contract definitions in planning documents ---'
rg -n -C 8 'PendingToolCallView|arguments \(redacted|argsTruncated|requestPreview' \
planning --glob '*.md' | head -500Repository: labsai/EDDI
Length of output: 47441
Define the trusted Manager-side source for read-only classification.
Phase 3 reuses Phase 2's toolReadOnly classification, but it defines no transport or Manager-side contract for it. PendingToolCallView contains only source, toolName, redacted arguments, argsTruncated, and requestPreview. Propagate a validated bit keyed by source and dispatch name, or provide a trusted shared/generated mapping. Treat missing, stale, or unmapped entries as writes.
🧰 Tools
🪛 LanguageTool
[grammar] ~116-~116: Use a hyphen to join words.
Context: ...only classification rather than a second hand-maintained list. Then the guard ins...
(QB_NEW_EN_HYPHEN)
🤖 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 `@planning/operator-mcp-guardrails-plan.md` around lines 112 - 116, Define a
trusted Manager-side read-only classification contract for MCP calls used by
findSelfTargetedCalls and findGateCarryingCalls, keyed by source and dispatch
tool name. Propagate a validated toolReadOnly bit or reuse a trusted
shared/generated mapping, and treat missing, stale, or unmapped classifications
as writes so unknown tools remain guarded.
…f-target check Follow-on to the previous fix. Refusing on argsTruncated closes the easy half; the harder half is that PendingToolCallView.arguments is REDACTED, so a complete, valid-JSON payload can still have had the agent id scrubbed. A guard that scans it and treats a miss as a pass reports success precisely when it cannot see. The plan no longer pretends this has a client-side answer. It names the two acceptable resolutions — a backend-computed authoritative self-target determination (preferred, since the server holds the real arguments), or refusing every MCP write that could name an agent — and states plainly which approach is not acceptable.
main moved again before this branch was pushed (#664, #665, #667, #668). Conflicts, both in docs: - docs/changelog.md — both sides prepended entries again; kept both, nothing dropped. - docs/secrets-vault.md — #667 documented vault agent grants independently, and #664 changed the default to enforce, which made this branch's "warn (default)" row wrong. Resolved in main's favour: main's section is kept whole and this branch's duplicate dropped, rather than interleaved. Follow-on fixes: the group docs' cross-reference now points at main's anchor and says the thing main's section does not — that a sub-agent inheriting a parent's vault reference must itself be granted the secret, or under the new default it will not deploy. Re-verified after the merge: the "80+ MCP tools" claim in README/docs/AGENTS still holds (84 @tool methods, matching McpToolFilter's whitelist exactly).
2 participants
Four items making EDDI's documentation and MCP surface genuinely usable by agents, plus the security fix that stops a task-level
toolApprovalsfrom bypassing the agent gate. Driven by the EDDI-Manager Platform Operator work (write-by-default; llmstore writes behind the Manager's gate-guard); each item survived two critical design passes, and the second pass changed the design twice — details below and indocs/changelog.md.1. Real
list_docs/read_docsMCP tools (McpDocTools)docs/mcp-server.mdhas long documentedtoolsWhitelist: ["read_docs", "list_docs"]— tools that did not exist. A whitelist that matches nothing exposes nothing, so anyone copying the example got a silently tool-less server. The tools now exist, delegating to the sameDocsServiceas REST and theeddi://docs/*resources.Tools alongside resources on purpose: agentic MCP clients — EDDI's own
McpToolProviderManagerincluded — consumetools/listand never callresources/read, so resources alone reach desktop clients and no agent. Role check mirrorsIRestDocs' five-role enumeration via a newMcpToolUtils.requireAnyRole(EDDI has no role hierarchy; a single-role check ofeddi-viewerwould refuse aneddi-admin).2.
eddi.docs.enabled(defaulttrue)One switch in
DocsServicedisables every docs surface together (REST list/read, MCP resources, MCP tools). Previously the only "off" was pointingeddi.docs.pathat a nonexistent directory — a hack that reads as misconfiguration in every diagnostic. Honest scope note: low value, ~15 lines, kept because the cost is near-zero and a policy deserves a switch.3. MCP resource bridge (
exposeResourcesonmcpcallsconfigs)Opt-in per config: synthesizes
<name>_list_resources/<name>_read_resourcetools so an agent can reach any MCP server's resources — the protocol half tool-consuming agents otherwise never see (langchain4j's client has supportedlistResources/readResourceall along; nothing called them).Deliberate choices: off by default (a pre-existing config must not silently grow tools); not subject to
toolsWhitelist(that filter governs server-advertised names; this feature carries its own opt-in, and a server must not be able to occupy the synthesized names); construction is purely local, executors dial lazily through the shared credential-keyed client cache (unreachable server = error tool result, not a discovery failure); text capped at 64K chars, binary described rather than base64-dumped into context; same static-config rejections asdiscoverTools.4. Strict task-level
toolApprovals(eddi.hitl.tool.task-approvals.mode, defaultstrict)The load-bearing one. A per-task
toolApprovalsfully replaced the agent-level gate (the identical ternary inLlmTaskandToolLoopResumer), sorequireApproval: []buried among forty fields of an llmstore document was a complete bypass — reviewed as an ordinary config edit, effective as a security change. It is also what forced EDDI-Manager to hard-refuse llmstore writes for its Platform Operator.Under
strict(via the sharedTaskToolApprovalsResolver), a task block can only strengthen the agent gate:requireApproval= union — semantically exact for the gate's any-match OR; neutralizes the[]bypass ([] ∪ agent = agent)exempt= agent's list verbatim, task entries ignored — exempt beats require (ToolApprovalGateP1), so a task-added exemption is precisely the ungating vector. The critical pass killed a string-intersection design here: a task exempting a strict subset of the agent's patterns shares no strings with it and would have silently gated every readAUTO_APPROVE(scalar or rule) demoted toWAIT_INDEFINITELYunless the agent itself grants it — generalizing the existing inherited-AUTO_APPROVEdemotionmin(task, agent ?? default). The default constant moved toToolApprovalsConfigso the runtime and the resolver read one sourcereplacekeeps the pre-6.3.0 wholesale override for designs that deliberately loosen one taskLlmStorewarns at save time about taskexempt/AUTO_APPROVEthat strict mode will not honour — visibility, not rejection: stored configs never brick,replacemode still honours them.LlmTaskCoverageTest.toolApprovals_taskOverrideUseddeliberately flipped from pinning replace semantics to pinning the strict merge; the full contract lives inTaskToolApprovalsResolverTest(17 tests).Tests
150 green across the touched areas (17 resolver, 7
McpDocToolsTest, 5McpResourceBridgeTest, 5RestDocsTest, 43LlmTaskCoverageTest, gate/rules/provider suites unchanged). TheA2AToolProviderManager*/Embedding*loopback errors in the wider package are the documented sandbox socket limitation (AGENTS.md §Build & Test) — CI is the source of truth there.Follow-up trail (not in this PR): operator-over-MCP with full guardrails
Verified facts for the next step, so the path is concrete:
@Tool(annotations = @Tool.Annotations(readOnlyHint = true, ...))— EDDI's 76 tools can declare read/write semantics now.annotationsfromtools/list(no annotation type exists in the client jar), so the gate cannot classify foreign MCP tools by hint without an upstream contribution — or, for EDDI's own server, a first-party name→readOnly map threaded intoToolApprovalGate.classifythe same waytoolEndpointsalready is (givingmcptools the same fail-safe "exempt reads, gate everything else" invarianthttp.get:*provides).self-guard.ts/gate-guard.tsmatchrequestPreview.uriand are blind to MCP calls; they needsource == "mcp"matching on tool name + parsed arguments.setup-api'smcpServerUrlscreates mcpcalls configs with no whitelist — a curated MCP tool subset (e.g. onlyapply_agent_changes,list_agent_resources) is not yet provisionable in one call.The single highest-value composite verb to expose either way is
apply_agent_changes, which collapses the config→workflow→agent→deploy chain the operator currently walks in four gated steps.Summary by CodeRabbit
New Features
Improvements
Follow-up plan
The operator-over-MCP trail above is written up in full as
planning/operator-mcp-guardrails-plan.md— four independently shippable phases, ordered so no phase leaves the operator holding ungated write tools, and written to be executed by someone with no context on this PR.