feat(operator): per-endpoint approval friction, gate provisioning, and agent-readable docs - #625
Merged
Merged
Conversation
timeoutPolicy, approvalTimeout, pauseReason and pendingMessage were single scalars for every gated tool, so "deploy an agent" and "create an agent" could not differ in how long a reviewer had or what the approval card said. Adds an optional `rules` list addressed by the same pattern language as requireApproval (bare name, source:name, or source.method:path). A rule tunes friction only -- whether a call is gated stays entirely in requireApproval/exempt, because the gate allows an unmatched call and a rule able to ungate would let a config grant capability by adding an entry. Resolution: most specific wins per call (fewest wildcards, then longest pattern, so list order cannot change the outcome); strictest wins per batch (WAIT_INDEFINITELY > ABORT > AUTO_REJECT > AUTO_APPROVE > none), so bundling a lenient call into a batch can never soften a stricter rule. Fields fall back to the scalars individually. The governing rule is resolved at gate time and persisted on PendingToolCallBatch: the batch keeps names and sources but no endpoints, so an endpoint-addressed rule could not be re-matched by the post-pause resolvers. ToolApprovalGate.addressesOf is extracted and shared so the gate and the rule resolver can never drift. Adds the eddi.hitl.rule.matched counter, tagged by the configured pattern and deduplicated per pause.
CreateApiAgentRequest had no HITL field and createApiAgent built a bare AgentConfiguration, so every agent the wizard has ever created had hitlConfig == null and an inert gate -- nothing could provision a gated agent through setup-api at all. hitlConfig is now a request field, set at step 7 on v1 of the agent document. Creating it with the agent rather than PUTing it afterwards matters: an update writes version + 1 and leaves the ungated v1 reachable by a redeploy. Validated before the first resource exists. AgentStore.create validates it too, but that runs at step 7, so a bad pattern previously surfaced only after the apicalls, parser, behaviour, LLM and workflow had been created and left all five orphaned. Deliberately not exposed on the MCP create_api_agent tool, which already takes a caller-chosen endpoint filter -- letting the caller choose the gate too would make it a complete escape from any allow-list. Also adds mcpServerUrls to the API-agent path so one agent can hold both the tools generated from its OpenAPI spec and an MCP server's; the per-URL creation loop is shared with setupAgent rather than duplicated.
An MCP resource is only usable by a client that asks for it, and EDDI own MCP
client never calls resources/read -- it consumes tools. So eddi://docs/* made
EDDI documentation readable by a desktop MCP client and not by an agent running
on EDDI, which is backwards for an agent whose job is to explain the platform.
Extracts DocsService from McpDocResources (filesystem access plus the
path-traversal guard) and adds GET /administration/docs and
/administration/docs/{name}, both eddi-viewer. McpDocResources becomes a thin
delegate; its pre-existing tests are kept assertion-for-assertion as evidence
that no MCP client sees a different response.
Mutation-checking the name shape-check (/, \, ..) killed nothing -- readDoc also
verifies the resolved path stays under the docs directory, which subsumes it.
The check is kept because it is what lets MCP answer "invalid name" rather than
"not found", but McpDocResources had restated the predicate to pick that
message. It is now one shared DocsService.isValidDocName. REST returns a bare
404 for both cases so a traversal string is never echoed back.
Documents that the runtime doc set is smaller than the repository: the image
ships only top-level docs/*.md and removes four of them.
…pin the gate-preservation property
PUT /{id}/updateResourceUri on the agent and workflow stores is the only
re-point path available to an approval-gated agent, because the full
PUT /agentstore/agents/{id} is permanently unbound (the gate lives in that
document). It is safe to bind only if it provably cannot drop the gate, so this
pins the property rather than assuming it: the caller cannot supply a
hitlConfig (the body is a text/plain URI) and the implementation preserves the
stored one by round-tripping the whole document. Asserted by capturing the
written AgentConfiguration -- gate intact, only the URI list changed.
Both variants also computed substring(0, lastIndexOf("?")), which throws
StringIndexOutOfBoundsException on a URI with no ?version= and turned malformed
caller input into a 500. On this path that failure mode is one an LLM walking
the re-point cascade will hit and has to act on, so it is now a 400.
GET /administration/docs would have refused an admin. It was written as @RolesAllowed("eddi-viewer") -- the role the MCP surface uses -- but EDDI has no role hierarchy: both JAX-RS @RolesAllowed and McpToolUtils.requireRole are literal hasRole checks, and eddi-viewer appears in no other REST endpoint. An eddi-admin principal, which is what an operator agent runs as, would have been 403d by the one endpoint built for it. The read tier is now enumerated like every other REST resource here. recordRuleMatches had been inserted directly above recordPauseCapGuard, leaving two consecutive javadoc blocks -- the original doc detached from its method and recordPauseCapGuard ended up undocumented. Method moved. Refuses a rules[].match string-identical to an exempt pattern: an exempt call is never gated, so no rule is ever resolved for it, making the rule dead config that reads as if it did something. Only exact equality is refused -- a broader rule may legitimately overlap an exemption while still covering gated calls. Asserts that PendingToolCallBatch.effectiveRule round-trips through the snapshot serializer. If it did not, a paused conversation would render the rule pending message and the resume would recompute the scalar one, stranding the placeholder in the resolved turn.
Contributor
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (10)
📝 WalkthroughWalkthroughThis change adds per-tool HITL approval rules with validation, matching, precedence, persistence, and runtime handling. It extends API-agent setup with HITL and MCP configuration. It adds REST documentation access and validates malformed resource URIs. ChangesPer-tool HITL approval rules
API-agent HITL and MCP setup
Documentation access and URI validation
Estimated code review effort: 4 (Complex) | ~45 minutes 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 |
There was a problem hiding this comment.
Pull request overview
This PR extends EDDI’s HITL tool-approval system with per-endpoint “friction” rules (messages + timeout behavior), makes the setup-api able to provision a HITL gate on the initial v1 agent document (plus optional MCP server URLs), and exposes EDDI’s markdown documentation to EDDI-hosted agents via a REST surface backed by a shared DocsService.
Changes:
- Add
toolApprovals.rulesresolution (most-specific per call; strictest per batch) and persist the governing rule ontoPendingToolCallBatchfor deterministic post-pause behavior. - Enable setup-api (
CreateApiAgentRequest) to accepthitlConfigand provision it on agent creation v1; also addmcpServerUrlssupport and share MCP-calls creation logic. - Extract docs filesystem logic into
DocsServiceand expose it viaGET /administration/docs(+{name}), while keeping MCP resource behavior intact; hardenupdateResourceUripaths to return 400 on URIs missing?version=.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java | Resolves and records per-call rule matches, selects governing rule, persists it in pending batches, and uses it for pause reason rendering + metrics. |
| src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGate.java | Extracts and centralizes tool-call addressing (addressesOf) used by both gating and rule matching. |
| src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalRules.java | New: matches toolApprovals.rules per call (most-specific) and reduces to a governing rule per batch (strictest). |
| src/main/java/ai/labs/eddi/engine/memory/model/PendingToolCallBatch.java | Persists governing rule (effectiveRule) and per-call matched rule id (matchedRule). |
| src/main/java/ai/labs/eddi/engine/internal/ConversationService.java | Applies effective timeout policy using the persisted governing rule, while preserving legacy behavior for older batches. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java | Renders end-user pending message using governing rule override, falling back to effective tool-approval config scalars. |
| src/main/java/ai/labs/eddi/configs/hitl/model/ToolApprovalsConfig.java | Adds rules and ApprovalRule model for per-tool friction overrides. |
| src/main/java/ai/labs/eddi/configs/hitl/HitlConfigValidation.java | Adds save-time validation for toolApprovals.rules (dead/unmatchable patterns, duplicates, finite-policy-without-duration, etc.). |
| src/main/java/ai/labs/eddi/engine/setup/CreateApiAgentRequest.java | Adds hitlConfig and mcpServerUrls fields to setup-api request payload. |
| src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java | Validates hitlConfig up-front; provisions HITL gate on agent v1; adds MCP server URLs support for API-agent wizard using shared helper. |
| src/main/java/ai/labs/eddi/engine/mcp/McpSetupTools.java | Extends MCP create_api_agent tool signature to accept mcpServerUrls (still deliberately not exposing hitlConfig). |
| src/main/java/ai/labs/eddi/engine/docs/DocsService.java | New: shared filesystem-backed doc listing/reading with traversal protections. |
| src/main/java/ai/labs/eddi/engine/api/IRestDocs.java | New: REST API contract for listing/reading docs, with explicit @RolesAllowed tier. |
| src/main/java/ai/labs/eddi/engine/rest/RestDocs.java | New: REST implementation delegating to DocsService, returning 404 without reflecting attacker input. |
| src/main/java/ai/labs/eddi/engine/mcp/McpDocResources.java | Refactors MCP resources surface to delegate to DocsService while preserving existing response shapes. |
| src/main/java/ai/labs/eddi/configs/agents/rest/RestAgentStore.java | Returns 400 (not 500) when updateResourceUri receives a URI lacking a query (?version=). |
| src/main/java/ai/labs/eddi/configs/workflows/rest/RestWorkflowStore.java | Same 400-guard for workflow updateResourceUri on malformed URIs. |
| docs/hitl.md | Documents per-tool friction rules semantics, reduction, persistence, and validation constraints. |
| docs/mcp-server.md | Documents REST docs surface and setup-api HITL provisioning behavior. |
| docs/changelog.md | Adds changelog entry describing the new HITL friction rules, setup-api gate provisioning, docs REST surface, and updateResourceUri hardening. |
| src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.java | Adds coverage tests for pause reason precedence and persisted governing rule/matchedRule behavior in pending batches. |
| src/test/java/ai/labs/eddi/engine/internal/ConversationServiceToolTimeoutTest.java | Adds tests asserting governing-rule timeout policy/duration precedence and inheritance behavior. |
| src/test/java/ai/labs/eddi/engine/runtime/internal/ConversationHitlTest.java | Adds tests ensuring governing rule pendingMessage overrides scalar levels and falls back correctly. |
| src/test/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalRulesTest.java | New: unit tests for matching specificity, strictest-governs reduction, and the “rules never gate/ungate” invariant. |
| src/test/java/ai/labs/eddi/configs/hitl/ToolApprovalRulesValidationTest.java | New: validation tests for toolApprovals.rules save-time constraints. |
| src/test/java/ai/labs/eddi/engine/memory/model/PendingToolCallBatchSnapshotTest.java | Ensures effectiveRule + per-call matchedRule round-trip through snapshot serialization. |
| src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceTest.java | Validates that invalid HITL configs are rejected before creating any resources; valid configs proceed past the guard. |
| src/test/java/ai/labs/eddi/engine/mcp/McpSetupToolsTest.java | Verifies setup-api provisions HITL gate on v1 agent doc and that MCP URLs add an MCP step alongside httpcalls. |
| src/test/java/ai/labs/eddi/engine/mcp/McpDocResourcesTest.java | Refactors tests to validate MCP doc responses unchanged after extracting DocsService. |
| src/test/java/ai/labs/eddi/engine/rest/RestDocsTest.java | New: tests REST docs list/read behavior and traversal handling (404 without reflection), and missing-dir behavior. |
| src/test/java/ai/labs/eddi/configs/agents/rest/RestAgentStoreExpandedTest.java | Asserts updateResourceUri preserves hitlConfig and rejects URIs without ?version=. |
| src/test/java/ai/labs/eddi/configs/workflows/rest/RestWorkflowStoreCrudTest.java | Adds regression test: URI without ?version returns 400 and does not update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+175
to
+178
| | Endpoint | Role | Returns | | ||
| | -------- | ---- | ------- | | ||
| | `GET /administration/docs` | `eddi-viewer` | JSON array of page names, without the `.md` suffix | | ||
| | `GET /administration/docs/{name}` | `eddi-viewer` | The page's markdown source as `text/plain`; `404` if absent | |
|
|
||
| **An MCP resource does not reach an EDDI agent.** A resource is only usable by a client that asks for it, and EDDI's own MCP client never calls `resources/read` — it consumes *tools*. So `eddi://docs/*` made EDDI's documentation readable by a desktop MCP client and not by an agent running on EDDI, which is exactly backwards for an agent whose job is to explain the platform. | ||
|
|
||
| `DocsService` is extracted from `McpDocResources` (filesystem access plus the path-traversal guard) and served over REST at `GET /administration/docs` and `GET /administration/docs/{name}`, both `eddi-viewer` — the docs are published documentation, so anyone who may look at the deployment may read them. `McpDocResources` becomes a thin delegate, and its pre-existing test class is kept assertion-for-assertion as the evidence that no MCP client sees a different response than before. |
Copilot review on #625: both docs said the endpoints were eddi-viewer, which was true of the first draft but not of the code after the review-pass fix. EDDI has no role hierarchy, so eddi-viewer alone would refuse an eddi-admin; the annotation enumerates the widest read tier and the docs now say so.
…apability # Conflicts: # docs/changelog.md
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/main/java/ai/labs/eddi/configs/workflows/rest/RestWorkflowStore.java:102
- Same as the agent-store variant: this only checks for a
?, but not that the query containsversion=. That can accept and persist non-versioned resource URIs (e.g....?foo=bar), which later breaks version-pinned workflow references.
String resourceURIString = resourceURI.toString();
int queryStart = resourceURIString.lastIndexOf('?');
if (queryStart < 0) {
// substring(0, -1) would throw and turn a caller's malformed input into a
// 500 — same guard as the agent-store variant, which shares this shape.
src/main/java/ai/labs/eddi/configs/agents/rest/RestAgentStore.java:149
- The new guard only checks that the URI contains a
?, but the endpoint contract (and error message) requires?version=. As written, a caller can send...?foo=barand still get a 200 update, leaving the agent pointing at a non-versioned resource URI.
int queryStart = resourceURIString.lastIndexOf('?');
if (queryStart < 0) {
// substring(0, -1) would throw and turn a caller's malformed input into a
// 500. This endpoint sits on the re-point cascade that an approval-gated
// agent has to walk to finish an edit, so its failure mode is one an LLM
src/main/java/ai/labs/eddi/engine/docs/DocsService.java:112
nameis attacker-controlled (REST path param / MCP arg) but is logged verbatim viaLOGGER.warnf(...), which enables log forging (CWE-117) via encoded newlines/control chars. Either sanitize before logging (preferred) or avoid logging the raw value.
if (!isValidDocName(name)) {
LOGGER.warnf("Rejected doc name: %s", name);
return null;
src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java:406
createMcpCallsResourcesreturnsnullonly when the raw string is blank/null; if it contains only commas/whitespace (e.g. ", ,"), the loop produces no locations and the method returns an empty list. That contradicts the method contract and forces callers to treat empty/non-empty lists equivalently.
if (mcpServerUrls == null || mcpServerUrls.isBlank()) {
return null;
}
var locations = new ArrayList<String>();
for (String url : mcpServerUrls.split(",")) {
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![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: 6
🤖 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/mcp-server.md`:
- Around line 175-178: Update the REST documentation endpoint table to enumerate
the complete permitted read roles, adding eddi-admin alongside the existing
eddi-viewer entry for both GET /administration/docs and GET
/administration/docs/{name}.
In `@src/main/java/ai/labs/eddi/configs/agents/rest/RestAgentStore.java`:
- Around line 145-155: Require a parsed, valid version parameter in both
resource update paths instead of treating any query delimiter as sufficient:
update RestAgentStore around resourceURIWithoutVersion and RestWorkflowStore’s
equivalent logic to reject missing or invalid version values with HTTP 400
before performing updates. Add ?other=2 regression cases in
RestAgentStoreExpandedTest and RestWorkflowStoreCrudTest, asserting HTTP 400 and
that no update occurs.
In `@src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalRules.java`:
- Around line 150-164: Update ToolApprovalRules.compile to catch failures from
ToolApprovalPatterns.compile for each nonblank rule, skip invalid entries, and
log the unusable match so one bad pattern does not abort the live turn. Preserve
valid-rule compilation and the existing specificity ordering, including the 1024
multiplier invariant relative to ToolApprovalPatterns.MAX_LENGTH.
In `@src/main/java/ai/labs/eddi/engine/internal/ConversationService.java`:
- Around line 2435-2474: Update applyEffectiveToolTimeoutPolicy to compute
effectiveTimeout once before the policy-selection branches, preserving the
precedence ruleTimeout, toolApprovals approvalTimeout, then hitlConfig
approvalTimeout. Replace the approvalTimeout presence checks for both rule and
toolApprovals with blank-aware validation (non-null and non-blank), so
whitespace-only values fall back to the next duration.
In `@src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java`:
- Around line 759-762: Update the pending-message condition in Conversation’s
rule-selection logic to use the blank-aware validation used by
HitlConfigValidation.isNullOrBlank instead of isNullOrEmpty, so whitespace-only
rule.getPendingMessage() values fall through to the configuration or default
message.
In `@src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java`:
- Line 169: Validate and parse every non-blank URL from request.mcpServerUrls()
before the setup flow writes any resources, including before the existing
resource creation paths around createdResources and createMcpCallsResources.
Reuse McpCallsConfiguration.validate() for each URL and abort setup on any
invalid value so no resources are persisted. Add coverage for both an invalid
first URL and an invalid later URL.
🪄 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: ddf525b0-ed73-4b7d-b9ed-2efe6cd31202
📒 Files selected for processing (32)
docs/changelog.mddocs/hitl.mddocs/mcp-server.mdsrc/main/java/ai/labs/eddi/configs/agents/rest/RestAgentStore.javasrc/main/java/ai/labs/eddi/configs/hitl/HitlConfigValidation.javasrc/main/java/ai/labs/eddi/configs/hitl/model/ToolApprovalsConfig.javasrc/main/java/ai/labs/eddi/configs/workflows/rest/RestWorkflowStore.javasrc/main/java/ai/labs/eddi/engine/api/IRestDocs.javasrc/main/java/ai/labs/eddi/engine/docs/DocsService.javasrc/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGate.javasrc/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalRules.javasrc/main/java/ai/labs/eddi/engine/internal/ConversationService.javasrc/main/java/ai/labs/eddi/engine/mcp/McpDocResources.javasrc/main/java/ai/labs/eddi/engine/mcp/McpSetupTools.javasrc/main/java/ai/labs/eddi/engine/memory/model/PendingToolCallBatch.javasrc/main/java/ai/labs/eddi/engine/rest/RestDocs.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.javasrc/main/java/ai/labs/eddi/engine/setup/AgentSetupService.javasrc/main/java/ai/labs/eddi/engine/setup/CreateApiAgentRequest.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/test/java/ai/labs/eddi/configs/agents/rest/RestAgentStoreExpandedTest.javasrc/test/java/ai/labs/eddi/configs/hitl/ToolApprovalRulesValidationTest.javasrc/test/java/ai/labs/eddi/configs/workflows/rest/RestWorkflowStoreCrudTest.javasrc/test/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalRulesTest.javasrc/test/java/ai/labs/eddi/engine/internal/ConversationServiceToolTimeoutTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpDocResourcesTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpSetupToolsTest.javasrc/test/java/ai/labs/eddi/engine/memory/model/PendingToolCallBatchSnapshotTest.javasrc/test/java/ai/labs/eddi/engine/rest/RestDocsTest.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/ConversationHitlTest.javasrc/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.java
Comment on lines
+150
to
+164
| private static List<CompiledRule> compile(ToolApprovalsConfig cfg) { | ||
| if (cfg == null || cfg.getRules() == null || cfg.getRules().isEmpty()) { | ||
| return List.of(); | ||
| } | ||
| List<ApprovalRule> sorted = new ArrayList<>(); | ||
| for (ApprovalRule rule : cfg.getRules()) { | ||
| // A blank match is refused at save time (HitlConfigValidation); skipping it | ||
| // here keeps a hand-edited or legacy document from failing a live turn. | ||
| if (rule != null && rule.getMatch() != null && !rule.getMatch().isBlank()) { | ||
| sorted.add(rule); | ||
| } | ||
| } | ||
| sorted.sort(Comparator.comparingInt(ToolApprovalRules::specificity).reversed()); | ||
| return sorted.stream().map(r -> new CompiledRule(r, ToolApprovalPatterns.compile(r.getMatch()))).toList(); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check ToolApprovalPatterns.compile failure mode and the pattern length cap.
set -euo pipefail
fd -t f 'ToolApprovalPatterns.java' | xargs -r -I{} ast-grep outline {} --items all
# The compile method body: does it throw on an invalid pattern?
fd -t f 'ToolApprovalPatterns.java' | while IFS= read -r f; do
ast-grep run --lang java --pattern 'static Pattern compile($$$) { $$$ }' "$f" || true
rg -n -C4 'throw|IllegalArgument|256|MAX_LEN|maxLength' "$f"
done
# Does any caller already guard compile() against an exception?
rg -nP --type=java -C3 'ToolApprovalPatterns\.compile\s*\('Repository: labsai/EDDI
Length of output: 2949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ToolApprovalPatterns.java =="
sed -n '1,120p' src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalPatterns.java
echo
echo "== ToolApprovalRules.java relevant section =="
sed -n '130,185p' src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalRules.java
echo
echo "== ToolApprovalRules.java call sites and logging =="
rg -n 'matchByCallId|compile(|LOGGER|Logging\.getLogger|Logger' src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalRules.javaRepository: labsai/EDDI
Length of output: 8179
Drop individually invalid rule patterns so the live turn can still pause.
compile(r.getMatch()) can throw from invalid regex syntax after compile() has already filtered only null and blank matches. This violates the stated behavior that invalid entries are dropped and can make a turn error out instead of pausing behind a bad rule. Wrap the per-rule compile, drop the entry, and log the unusable match.
Also preserve the wildcard-vs-length ranking invariant: the 1024 score multiplier only avoids trading off wildcard count and length while ToolApprovalPatterns.MAX_LENGTH stays below 1024.
🤖 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/ToolApprovalRules.java` around
lines 150 - 164, Update ToolApprovalRules.compile to catch failures from
ToolApprovalPatterns.compile for each nonblank rule, skip invalid entries, and
log the unusable match so one bad pattern does not abort the live turn. Preserve
valid-rule compilation and the existing specificity ordering, including the 1024
multiplier invariant relative to ToolApprovalPatterns.MAX_LENGTH.
Comment on lines
+759
to
+762
| var rule = batch != null ? batch.getEffectiveRule() : null; | ||
| if (rule != null && !isNullOrEmpty(rule.getPendingMessage())) { | ||
| template = rule.getPendingMessage(); | ||
| } else if (cfg != null && !isNullOrEmpty(cfg.getPendingMessage())) { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a blank-aware check for rule.getPendingMessage().
isNullOrEmpty only checks String.isEmpty(), not .isBlank(). A whitespace-only rule.getPendingMessage() is treated as "set" here and used verbatim instead of falling back to cfg.getPendingMessage() or DEFAULT_PENDING_MESSAGE. This mirrors the same not-blank-aware pattern in ConversationService.applyEffectiveToolTimeoutPolicy, where HitlConfigValidation.isNullOrBlank (which does check .isBlank()) is the semantic the save-time validator already relies on.
🛠️ Proposed fix
var rule = batch != null ? batch.getEffectiveRule() : null;
- if (rule != null && !isNullOrEmpty(rule.getPendingMessage())) {
+ if (rule != null && rule.getPendingMessage() != null && !rule.getPendingMessage().isBlank()) {
template = rule.getPendingMessage();
- } else if (cfg != null && !isNullOrEmpty(cfg.getPendingMessage())) {
+ } else if (cfg != null && cfg.getPendingMessage() != null && !cfg.getPendingMessage().isBlank()) {
template = cfg.getPendingMessage();
}📝 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.
Suggested change
| var rule = batch != null ? batch.getEffectiveRule() : null; | |
| if (rule != null && !isNullOrEmpty(rule.getPendingMessage())) { | |
| template = rule.getPendingMessage(); | |
| } else if (cfg != null && !isNullOrEmpty(cfg.getPendingMessage())) { | |
| var rule = batch != null ? batch.getEffectiveRule() : null; | |
| if (rule != null && rule.getPendingMessage() != null && !rule.getPendingMessage().isBlank()) { | |
| template = rule.getPendingMessage(); | |
| } else if (cfg != null && cfg.getPendingMessage() != null && !cfg.getPendingMessage().isBlank()) { | |
| template = cfg.getPendingMessage(); | |
| } |
🤖 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/runtime/internal/Conversation.java` around
lines 759 - 762, Update the pending-message condition in Conversation’s
rule-selection logic to use the blank-aware validation used by
HitlConfigValidation.isNullOrBlank instead of isNullOrEmpty, so whitespace-only
rule.getPendingMessage() values fall through to the configuration or default
message.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/main/java/ai/labs/eddi/engine/docs/DocsService.java:119
- DocsService.readDoc() falls back to reading the raw
namepath when<name>.mdis absent. That means a caller can fetch any regular file in the configured docs directory by passing its exact filename (e.g.not-markdown.txt), even though this surface is meant to serve only markdown docs. With the REST endpoint exposed to broad read roles, this becomes an unintended file disclosure risk if non-doc files ever land in that directory.
Consider only allowing the fallback path when the caller already supplied a .md suffix (to support requests like architecture.md), and otherwise returning 404.
if (!Files.isRegularFile(docFile)) {
// The name may already carry the .md suffix.
docFile = docsDir.resolve(name).normalize();
if (!docFile.startsWith(docsDir) || !Files.isRegularFile(docFile)) {
return null;
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/changelog.md (1)
35-35: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winDo not claim bounded metric cardinality from pause deduplication.
eddi.hitl.rule.matchedusesmatchas a label value, and each distinct pattern creates a distinct time series. Old series are not evicted by the per-pause deduplication, and the Prometheus retention value is deployment-configurable, so this should not state cardinality as bounded by the currentruleslist.🤖 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` at line 35, Update the `eddi.hitl.rule.matched` documentation in the Metric entry to remove the claim that label cardinality is bounded by the configured `rules` list. Retain the accurate descriptions of per-pause deduplication and the `match` value’s source and sensitivity.
🤖 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.
Outside diff comments:
In `@docs/changelog.md`:
- Line 35: Update the `eddi.hitl.rule.matched` documentation in the Metric entry
to remove the claim that label cardinality is bounded by the configured `rules`
list. Retain the accurate descriptions of per-pause deduplication and the
`match` value’s source and sensitivity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f28cb479-1935-4965-9d47-979d8c01ad68
📒 Files selected for processing (5)
docs/changelog.mddocs/mcp-server.mdsrc/main/java/ai/labs/eddi/engine/internal/ConversationService.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java
- src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java
- docs/mcp-server.md
- src/main/java/ai/labs/eddi/engine/internal/ConversationService.java
…wo silent degradations PR review on #625. updateResourceUri could UNPIN a reference. The versionless-URI guard tested only for the presence of a "?", so ".../workflows/{id}?other=2" satisfied it, matched the stored "?version=1" reference by path prefix, and replaced it with a versionless one -- silently unpinning the workflow an agent resolves at runtime. Both stores now go through RestUtilities.pathWithoutVersionQuery, which parses the query and requires a non-negative integer version. A whitespace-only approvalTimeout degraded a finite policy silently. RuntimeUtilities.isNullOrEmpty is isEmpty-only, but HitlConfigValidation uses isBlank when deciding whether a finite rule may inherit the enclosing duration -- so a blank value saved as absent and then resolved as present, won the chain, threw inside Duration.parse, armed no schedule, and left the bookmark reporting a finite policy that could never fire. Both resolution sites are blank-aware now. The three identical duration ternaries in applyEffectiveToolTimeoutPolicy are computed once: the timeout resolves down its own chain regardless of which branch picks the policy, and three copies would drift. MCP server URLs are swept before the first write, like hitlConfig already was -- McpCallsConfiguration.validate rejects a non-http(s) URL, so a bad second URL aborted with the first one persisted, and on the API-agent path with the apicalls, parser, behaviour and LLM resources persisted too.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java:401
- There are two consecutive Javadoc blocks here; the first one (about creating McpCalls resources) is not attached to any declaration and will trip Javadoc/Checkstyle rules in many builds. Remove the stray block (or move it to the method it documents).
/**
* Creates one McpCalls resource per comma-separated server URL, recording each
* location in {@code createdResources}. Returns null when no URLs were given,
* which is what {@code createWorkflowConfig} expects for "no MCP step".
* <p>
src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java:828
- The rule-level pendingMessage is treated as blank-aware, but the scalar toolApprovals.pendingMessage still uses isNullOrEmpty (isEmpty-only). A whitespace-only scalar pendingMessage would be used verbatim and render as an empty user-facing bubble instead of falling back to the default.
if (rule != null && rule.getPendingMessage() != null && !rule.getPendingMessage().isBlank()) {
template = rule.getPendingMessage();
} else if (cfg != null && !isNullOrEmpty(cfg.getPendingMessage())) {
template = cfg.getPendingMessage();
aisabella-ai
approved these changes
Aug 1, 2026
This was referenced Aug 11, 2026
Merged
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.
Follows #622, which granted no capability but made granting it possible. This is the second half of that: the backend can now install an approval gate, differentiate friction per endpoint, and let an agent read EDDI's own documentation. No write capability is granted here — the Manager's endpoint allow-list is untouched and still read-only.
Two repos, two PRs. This is the backend; the EDDI-Manager half (scope picker, approval surface, system prompt, the write grant itself) depends on this merging and deploying first.
Iteration 1 — per-endpoint approval friction
timeoutPolicy,approvalTimeout,pauseReasonandpendingMessagewere single scalars covering every gated tool, so "deploy an agent" and "create an agent" could not differ in how long a reviewer had or what the approval card said.A rule tunes friction; it never gates or ungates. That stays entirely in
requireApproval/exempt. The gate allows an unmatched call, so it only survives by gating broadly and exempting narrowly — a rule able to ungate would let a config grant capability by adding an entry, which is the enumerate-upward failure the design exists to avoid. Asserted against the gate itself: a config whose rules name an exempt GET and give a required POSTAUTO_APPROVEchanges neither classification.Two resolution decisions worth reviewing closely:
http.post:/agentstore/agentsbeatshttp.post:*regardless of JSON array order. Order-dependence would mean losing intended friction to a list edit.WAIT_INDEFINITELY>ABORT>AUTO_REJECT>AUTO_APPROVE> none). Taking the first match instead would have let a model turn "delete waits for a human" into "delete auto-rejects in five minutes" by pairing a delete with a deploy.The governing rule is resolved once at gate time and persisted on the batch:
PendingToolCallBatchkeeps each gated call's name and source but no endpoint, so an endpoint-addressed rule could not be re-matched by the post-pause resolvers — and a rule resolving differently on the two sides of a pause is exactly the bug that field removes.ToolApprovalGate.addressesOfis extracted and shared so the gate and the rule resolver can never drift.New metric
eddi.hitl.rule.matched{match="<configured pattern>"}— deduplicated per pause, tagged with the config pattern only, never a URL, argument or user id.Iteration 2 — provision the gate through setup-api (the blocker)
CreateApiAgentRequesthad no HITL field andcreateApiAgentbuilt a bareAgentConfiguration, so every agent the wizard has ever created hashitlConfig == nulland an inert gate. Nothing could provision a gated agent through setup-api at all.hitlConfigis now a request field, set at step 7 on v1 of the agent document — creating it with the agent rather thanPUT-ing it afterwards matters, because an update writesversion + 1and leaves the ungated v1 reachable by a redeploy.Validated before the first resource exists.
AgentStore.createvalidates it too, but at step 7 — so a bad pattern previously surfaced only after the apicalls, parser, behaviour, LLM and workflow had been created, orphaning all five.Deliberately not exposed on the MCP
create_api_agenttool, which already takes a caller-chosen endpoint filter; letting the caller choose the gate too would make it a complete escape from any allow-list.Also adds
mcpServerUrlsto the API-agent path so one agent can hold both spec-generated tools and an MCP server's — previously unreachable through the wizard.Iteration 3 — EDDI's docs readable by an EDDI agent
An MCP resource is only usable by a client that asks for it, and EDDI's own MCP client never calls
resources/read— it consumes tools. Soeddi://docs/*made EDDI's documentation readable by a desktop MCP client and not by an agent running on EDDI, which is backwards for an agent whose job is to explain the platform.DocsServiceis extracted fromMcpDocResourcesand served atGET /administration/docsand/administration/docs/{name}.McpDocResourcesbecomes a thin delegate; its pre-existing test class is kept assertion-for-assertion as evidence no MCP client sees a different response.Documents that the runtime doc set is smaller than the repo's: the image ships only top-level
docs/*.mdand removes four of them, so callers must read the index rather than assume a page exists.updateResourceUriverified as the gate-immune re-point pathPUT /agentstore/agents/{id}andPUT /llmstore/llms/{id}must stay permanently unbound for a gated operator — the gate lives in those documents. That leaves editing an agent apparently impossible, since the re-point cascade needs document writes.PUT /{id}/updateResourceUriis the escape hatch, so it was checked rather than assumed: the caller cannot supply ahitlConfig(the body is atext/plainURI) and the implementation preserves the stored one by round-tripping the whole document. Now pinned by a test that captures the written config.Both variants also computed
substring(0, lastIndexOf("?")), which throws on a URI with no?version=— turning malformed input into a 500 on the exact path an LLM has to walk to finish an edit. Now a 400.Verification
mvnw validateclean; clean rebuild after the rebase ontomain.Review pass found three real things
GET /administration/docswould have 403'd an admin. Written as@RolesAllowed("eddi-viewer")— but EDDI has no role hierarchy (@RolesAllowedandMcpToolUtils.requireRoleare both literalhasRolechecks) andeddi-viewerappears in no other REST endpoint. The principal an operator runs as would have been refused by the one endpoint built for it.recordPauseCapGuardundocumented.rules[].matchstring-identical to anexemptpattern is provably dead config and is now refused — exact equality only, since a broader rule may legitimately overlap an exemption.Open decisions for the owner
PUTs permanently unbound (theupdateResourceUricascade covers re-pointing), or add a deployment-levelrequireApprovalthat merges with and cannot be weakened by the agent document. The latter closes the class properly and is what would eventually make those twoPUTs bindable.eddi-editorservice persona.Docs:
docs/hitl.md(rules),docs/mcp-server.md(REST docs surface, gate-not-on-MCP),docs/changelog.md.Summary by CodeRabbit
New Features
Bug Fixes
Documentation