diff --git a/.gitleaksignore b/.gitleaksignore index d8ec14da5a..0212b2a617 100644 --- a/.gitleaksignore +++ b/.gitleaksignore @@ -58,3 +58,13 @@ # introduced the line stays in history and still needs an entry. Not a real key: # it never authenticated against anything. eebfe898b34858918d5cbbe11e60336d1b6a916e:src/test/java/ai/labs/eddi/secrets/sanitize/SecretScrubberTest.java:generic-api-key:238 + +# ResolvedRequestTest: the same mistake as the SecretScrubberTest entry above, +# made again. Tests proving that a credential in a request BODY is redacted need +# a literal carrying SecretRedactionFilter's `sk-` + 20-char shape, and the first +# version used a realistic-looking one. Replaced in a follow-up commit with a +# zero-entropy value (repeated characters β€” same shape, nothing for the scanner +# to flag), but gitleaks scans a PR's whole commit range, so the commit that +# introduced it stays in history and still needs an entry. Never a real key: it +# never authenticated against anything. +96df3c83fa449e02250c08c1aa420a11617f2ebb:src/test/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequestTest.java:generic-api-key:193 diff --git a/docs/changelog.md b/docs/changelog.md index 4e858bf173..5f6fc2d04a 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -27,6 +27,113 @@ Retiring it also removes a build-time network dependency and a layer from the ru --- +## 🧭 feat(operator): a context-aware side-chat drawer, reachable from Manager and Workforce (2026-08-04) + +**Repo:** EDDI-Manager (`feat/operator-write-scope`) + +The operator existed only as a dedicated page at `/manage/operator` β€” Manager-only, full-page-only, no idea what screen the admin was actually looking at when they opened it. Added a floating-launcher drawer (`operator-drawer.tsx`) mounted once in `AppLayout` and once in each of `WorkforceLayout`'s three viewport branches (mobile/tablet/desktop) β€” a self-positioned `fixed` panel, since those four layouts share no common chrome slot the way the existing `ChatDrawer` shares `AppLayout`'s one. + +**Shared conversation, not a second one.** The drawer reuses `useOperatorChat`/`useOperatorConfig` directly rather than standing up a parallel chat β€” same react-query cache, same conversation. That required promoting `use-operator-chat.ts`'s state off local `useState` onto a Zustand store (`useOperatorChatStore`): today, even the full page silently drops its visible transcript on remount (the backend conversation survives via the `sessionStorage`-remembered id, but `messages` restarts empty), because nothing shared it. The wrapper hook keeps the exact same public API, so `operator.tsx`'s call sites are unchanged. + +That refactor was stress-tested by a dedicated Plan-agent pass before writing it, which caught four things a naive `useState`β†’Zustand translation would have gotten wrong: `set()` merges rather than replaces (so `reset()` must explicitly null the three promoted-from-`useRef` fields, not just the public ones); the eslint-disables in `operator.tsx` don't disappear on their own (the rule flags the *shape* of `chat.reset()`, unrelated to the state container); a second existing test file (`operator.test.tsx`, not just the hook's own test) mounts the real hook and needed the same reset; and `context` (see below) has to be a call-time argument to `send()`, never a store field, or two mounted surfaces would race to overwrite each other's screen context. Mutation-tested the one real bug risk (the merge trap): reverting the internal-field nulling in `reset()` let an orphaned turn β€” one whose conversation was reset mid-stream β€” graft its trace onto the fresh state; a new test (`use-operator-chat.test.tsx`) drives exactly that interleaving and fails without the fix. + +**Pause handling doesn't duplicate `ApprovalBanner`.** That component is security-reviewed for one full-width surface (redacted previews, self-guard, blocked-calls) β€” a docked drawer has no room to review a gated write responsibly, and forking a second smaller copy is exactly the "two systems drift apart" trap this whole feature has spent most of its review cycles closing. `operator-chat.tsx` gained one prop, `pauseSurface?: "banner" | "compact"` (default `"banner"`, zero diff for the full page); compact renders the pause reason plus a link to `/manage/operator`, where the real banner picks up the identical pause β€” same conversation, no re-ask. + +**Context flows through a transport that already existed and was unused.** `sendMessageStreaming`'s `InputData` has had an optional `context?: Record` field since well before this β€” it flows into the backend's per-turn `{context.x}` Qute variable, the documented mechanism for exactly this. Nothing populated it. Added `useCurrentScreenContext()` (route β†’ `{screen, agentId, workflowId, groupId, boardId}`, matched via `matchPath` against an ordered table β€” the drawer lives above the routed ``, so `useParams()` can't see it there, and `matchPath` has no cross-pattern ranking, so literal routes have to precede the param routes they'd otherwise collide with) and thread its output into `send(input, context)` from the drawer only (the full page's own location is always just "the operator page" β€” not informative). A new unconditional section of the system prompt (`BODY_APP_CONTEXT`, Qute-conditional so it degrades to nothing when no context was sent) reads it back as `{context.screen}` etc. Zero backend changes. Existing operators pick this up on their next reconfigure, same as every other prompt-body change in this feature. + +**Caught live, not by the test suite:** the mobile Workforce viewport has a `fixed bottom-0 h-16` tab bar (`WorkforceBottomTabs`) that jsdom can't lay out, so nothing in the automated suite could have caught the drawer's default `bottom-6` sitting ~40px inside it. Found by actually resizing a running dev-server browser to the mobile breakpoint and reading `getBoundingClientRect()`; fixed with a `clearsBottomTabBar` prop (mirrors the same layout's own `
`, used only on mobile), verified the fix live, then added a regression test asserting the class difference (`operator-drawer.test.tsx`) since geometry itself isn't observable in jsdom. + +i18n: `operator.chat.pauseCompact{Fallback,Review}`, `operator.drawer.{title,notActivated,activate}` β€” all 11 locales. + +**Verification:** typecheck and lint clean; full suite 309 files / 4642 tests green (+4 files / +26 tests over baseline); production build succeeds; manual pass in a live dev server (MSW mock backend) across Manager and all three Workforce viewport branches, including the mobile fix above. + +--- + +## πŸ”’ fix(hitl): a resume verdict that resolved to null was one comparison away from executing as approved (2026-08-03) + +**Repo:** EDDI (`feat/operator-request-fingerprint`) + +Found by an automated review comment on [#627](https://github.com/labsai/EDDI/pull/627) (Copilot), on `AgentOrchestrator.resumeToolLoop`'s per-call verdict resolution: `HitlVerdict verdict = cd != null && cd.getVerdict() != null ? cd.getVerdict() : topVerdict` falls back to `topVerdict` with no null check, and the only gate downstream is `if (verdict == REJECTED) { ...skip... }` β€” a null verdict is not `== REJECTED`, so it silently fell through to the execute branch. The metric emitted alongside it made this worse, not just neutral: `recordWriteApprovalDecision`'s `verdict == APPROVED ? "approved" : "rejected"` would have tagged the very same call `"rejected"` while it executed β€” the telemetry that should have caught the bug in production would have shown the opposite of what happened. + +Traced every caller of the shared choke point (`ConversationService.resumeConversation`) before concluding this was live: `RestAgentEngine` (`decision.getVerdict() == null` β†’ 400), `SlackInteractivityHandler` (`verdictFor` checked before `ParsedAction` exists), `McpHitlTools` (`parseVerdictOrNull` checked before the tool call proceeds), `HitlTimeoutHandler` (verdict is a hardcoded `APPROVED`/`REJECTED` ternary), `GroupConversationService`'s member-tool-pause auto-resolution (hardcoded `REJECTED`) β€” all five independently guarantee a non-null top-level verdict today. Not exploitable as the code stands, but fragile: the invariant was enforced four separate times, never once at the method every one of them funnels through, so a sixth caller (or a refactor of any of the five) could silently reintroduce the gap with nothing to catch it. + +Fixed at both ends rather than patching the symptom: +- **`ConversationService.resumeConversation`** now rejects `decision == null || decision.getVerdict() == null` up front with `IllegalArgumentException`, mirroring `RestAgentEngine`'s existing message β€” enforced once, for every current and future caller, instead of assumed five times over. +- **`AgentOrchestrator`**, per Copilot's specific suggestion: normalizes an (now theoretically unreachable, but no longer trusted blindly) unresolved verdict to `REJECTED` before either the metric emit or the execution check, so the two can never disagree with each other again. + +Mutation-verified both independently: reverting the `ConversationService` guard makes the new null-decision/null-verdict tests fail with `ResourceNotFoundException` instead of `IllegalArgumentException` (proving the check, not something else, produces the 400); reverting the `AgentOrchestrator` normalization makes `unresolvedVerdictFailsClosed` fail on `journalStore.tryClaim` actually being invoked β€” i.e. with the fix removed, the call really does execute. Both restored and re-verified green (`ConversationServiceHitlCoverage2Test` 14/14, `AgentOrchestratorResumeToolLoopTest` 12/12, `ConversationServiceResumeTest` 18/18). + +Also landed on this branch: reattached `auditOutcomeUnknown`'s Javadoc, separated from its method by the request-pinning commit's insertion point (also a review finding, cosmetic β€” see the commit itself). + +--- + +## πŸ”“ feat(setup): let the standard agent-setup path install a HITL gate too (2026-08-03) + +**Repo:** EDDI (`feat/operator-request-fingerprint`) + +`CreateApiAgentRequest` (the OpenAPI-spec agent path) has carried a `hitlConfig` field since the setup-api gate provisioning work referenced above β€” `SetupAgentRequest` (the "standard" agent path: behavior rules + LLM + output, no OpenAPI spec) never got the same field, so every agent it created had `hitlConfig == null` and no gate. Added the field, mirroring `CreateApiAgentRequest`'s reasoning exactly: validated up front (`HitlConfigValidation.validate`, same as `createApiAgent`), wired onto `AgentConfiguration` at creation time β€” before `createAgent()` is called, never via a later PUT, for the same "v1 must ship gated or a redeploy reaches an ungated version" reason documented on `createApiAgent`. Deliberately absent from the MCP `setup_agent` tool's arguments (stays `null`, same as `create_api_agent`) β€” that tool already lets the caller choose the created agent's tool surface, so also letting it choose the approval gate would be a caller-controlled way to produce an ungated agent. Provisioning a gated agent goes through `POST /administration/agents/setup` directly, which the JAX-RS layer deserializes with no such restriction. + +Closes a real, previously undocumented gap: this was the one remaining agent-creation path with no `hitlConfig` support at all β€” a prerequisite for letting the operator provision *any* type of agent (not just OpenAPI-spec ones) with an approval gate installed from v1. + +New coverage: `HitlConfigWiringTests` (`AgentSetupServiceTest`) asserts β€” via `ArgumentCaptor` β€” that the exact `hitlConfig` object reaches `createAgent()`, and that an absent one leaves the agent ungated rather than inventing a default. This assertion didn't previously exist for either `setupAgent` or `createApiAgent`; adding it for the new path closed the gap for both. Mutation-tested: removing the `setHitlConfig` call fails `hitlConfigReachesTheCreatedAgentConfiguration` (asserted `null` where the real object was expected); restored and re-verified 95/95 green. + +**Verification.** Full `mvnw test` run checked against the documented environmental baseline (~288 no-network loopback errors in `Web*ToolTest`, 8 pre-existing failures in `EmbeddingModelFactoryBranchTest`); this run: 313 errors / 8 failures, none in a touched class. + +--- + +## πŸ”’ feat(hitl): approval binds to the resolved request, not the tool name (2026-08-03) + +**Repo:** EDDI (`feat/operator-request-fingerprint`, branched from `main` after PR #625 merged β€” the per-endpoint-friction entry below plus setup-api gate provisioning, docs-over-REST, and `mcpServerUrls`; builds on the foundation laid in [#622](#-featoperator-the-foundation-for-an-agent-that-can-safely-write-2026-07-29)) + +Closes the gap the operator write-scope plan (`planning/operator-write-scope-plan.md` Β§3) flagged as the reason `WRITE_ENDPOINTS` had to stay empty: an approver of a gated `http` call saw the tool's name and the model's raw arguments, never the actual request. Method, path, query and body are only produced inside `ApiCallExecutor#execute`, **after** approval β€” so what an approver signed off on and what ran could, in principle, differ. + +**Four commits, one seam apiece:** + +1. `IApiCallExecutor#resolve` β€” builds the request `execute` would send, without sending it. Deliberately weaker than `execute`: it skips `preRequest.propertyInstructions` because those write to conversation memory and previewing a call must never do that, so a call that has them comes back with no fingerprint rather than one that doesn't match what execution will actually build. Shares one redaction definition (`RequestRedactor`, extracted from `ApiCallExecutor`'s private scrub) between the conversation-memory debug record and the approval preview, so the two paths cannot drift apart on what counts as a credential. +2. Gate time: each gated httpcall tool is resolved, and a redacted preview plus a SHA-256 fingerprint are persisted on the pause (`PendingToolCall.requestPreview` / `.requestFingerprint`). The fingerprint deliberately hashes the **redacted** request, not the live one β€” `ApiCallExecutor` resolves `${caller:token}` into `Authorization`, the approver is routinely a different person than whoever's turn raised the pause, and fingerprinting the live header would mismatch on every cross-user approval (the normal case), which would just get the check disabled. Canonicalization is length-prefixed rather than delimiter-joined, so a body containing a crafted newline cannot impersonate an extra header field and collide. +3. Resume time: an approved, pinned call is re-resolved and refused β€” synthetic `NOT_EXECUTED`, audited as `hitl.tool.request_changed` (tool + callId + reason, never the request) β€” if the fingerprint moved. This is the actual enforcement; everything before it was groundwork. Three situations fail *closed* rather than being waved through: the tool vanished from the workflow across the pause, re-resolution throws, or the call's config gained `preRequest.propertyInstructions` mid-pause. "Cannot verify" is a different answer than "unchanged" β€” treating it as the latter would make reconfiguring an agent while a human decides the way around the guard. +4. `eddi.operator.write.approval{decision=approved|rejected|timeout}` β€” the rubber-stamping signal the plan's metrics table calls for, emitted the instant a gated call's verdict is resolved regardless of what happens to it afterwards. `timeout` is its own bucket (`decidedBy == "system:timeout"`, from `HitlTimeoutHandler`) rather than folded into `approved`/`rejected` β€” an unattended auto-approval inflating "approved" would defeat the point of the metric. + +**Two metrics the backend cannot honestly emit itself.** `eddi.operator.canary` (+`.duration`) and `eddi.operator.gate.verified` describe facts the Manager establishes client-side β€” the write canary is a synthetic conversation it drives in the browser, gate verification is it re-reading every version of the operator agent document β€” and this codebase has no first-class notion of "the operator" to hang a server-side event on. `POST /administration/operator/{canary-result,gate-status}` (`eddi-admin`) exists purely to relay those already-established facts onto `/q/metrics`, so on-call doesn't need a Manager tab open. **Not a verification endpoint** β€” a report is trusted at face value, which is why it sits behind the same tier that can provision the operator at all. The gauge defaults to 0 before any report arrives, which is indistinguishable from "activated, and broken"; that ambiguity is real and this signal alone doesn't resolve it. + +**Verification.** Full `mvnw validate` + `mvnw test` run checked against the documented environmental baseline (no-network loopback failures in `Web*ToolTest`); none of the touched classes appear in the failure list. The fingerprint discrimination properties (method/URI/query/body/header changes each move the hash; header casing, ordering, and redacted-credential values do not) and the enforcement decision (pinned+changed β†’ refused; unpinned, amended, or matching β†’ proceeds; unresolvable β†’ fails closed) are both covered with dedicated unit tests. Four mutations applied against the enforcement path, each confirmed to kill exactly the tests guarding that branch; one applied against the timeout-tagging logic, confirmed to kill only the two timeout tests and leave approved/rejected untouched. + +Documented in [`docs/hitl.md`](hitl.md) (new Β§"Request pinning β€” approval binds to a request, not a tool name"; Operations metrics list extended). + +### Two more commits on the same branch: the preview reaches the wire, and everything above gets a real metric (2026-08-03) + +The pinning above persisted `requestPreview`/`requestFingerprint` on the pause record, but nothing external ever read them back β€” `RestAgentEngine.buildToolCallPauseDetails` builds its response as an explicit field-by-field map, so a new model field is invisible to a caller until something puts it there. `GET .../approval-status` now surfaces `requestPinned` and, when pinned, the redacted `requestPreview` (`method`/`uri`/`queryParams`/`headers`/`body`/`bodyTruncated`) per call β€” this is what an approver actually reads, replacing what the Manager previously had to guess by reconstructing an `operationId` against a separately-fetched spec. The raw fingerprint stays internal; it means nothing to a human. `namesOnlyPendingToolCalls` β€” the security-motivated projection for the generic (non-approver) read surfaces β€” needed no code change, since it's an explicit allow-list and a field it was never told to copy is absent by construction; only its doc comment needed the two new field names added. + +Also lands `eddi.operator.write.approval{decision=approved|rejected|timeout}` (a real backend-native metric β€” the orchestrator observes every gated-call verdict directly) and the relay endpoints `POST /administration/operator/{canary-result,gate-status}` for the two metrics the backend cannot honestly emit itself. + +**Verification note worth recording**: this repo's `@Nested`-only JUnit classes report `Tests run: 0` in the plain-text surefire report even when every test inside passed β€” already documented in memory from a prior session, and it still cost real time to rediscover mid-session before the XML `` attribute was checked. Both touched test classes' real results: `RestAgentEngineToolPauseDetailsTest` 11/11, `ConversationMemoryUtilitiesHitlTest` 8/8. + +### The preview leaked the body it was supposed to protect (2026-08-03) + +Found while scoping the operator's authoring UI, and the reason that scope changed: `RequestRedactor` only ever touched `headers`. Both consumers of a resolved request β€” the debug record persisted to the conversation document and the approval preview shown to a human β€” passed the **body** through verbatim. A config write carries its credential in the body, not a header, so a `POST` creating an agent with a provider key would have shown that key in plaintext to whoever approved the pause β€” routinely a different admin than the one whose turn raised it. + +Fixed by giving `RequestRedactor` a `redactBody` (delegating to `SecretRedactionFilter`, the same value-shape scan already behind `argumentsRedacted` β€” one filter for one class of data, rather than a second scheme that would drift), wired into both `redactRequestMap` and `ResolvedRequest#of`. + +The ordering matters more than the redaction. Headers stay fingerprinted **redacted** for the cross-user-approval reason documented above; the body is fingerprinted **raw** and only the stored copy is redacted, because a body has no equivalent legitimate variance (`${caller:token}` is header-only; `${vault:…}` resolves identically both times). Redacting first would hash two *different* credentials to one marker and so to one fingerprint β€” a swapped secret would pass the pre-execution re-check as an unchanged request. `ResolvedRequest#of` does the redaction itself so no call site can get that order wrong; a test asserts two distinct keys produce distinct fingerprints, and it fails if the redaction is hoisted above the hash. The fingerprint is never exposed to a client, so hashing raw reveals nothing. + +The limitation is stated rather than papered over: value-shape matching catches `sk-…`, `sk-ant-…`, bearer tokens and vault refs, not a hand-rolled secret in a generically named field. That is the same limitation `argumentsRedacted` already carries. + +### Review findings on the PR β€” pinning was silently not applying (2026-08-03) + +Three defects found by automated review on [#627](https://github.com/labsai/EDDI/pull/627), all in the pinning path, all fixed with tests that fail without the fix. + +**Query parameters broke pinning entirely.** `IRequest#toMap` returns them as `Map>` β€” `HttpClientWrapper` accumulates repeats β€” but `resolve` cast that to `Map`. The cast erases cleanly and then throws `ClassCastException` inside the fingerprint canonicaliser, which `pinResolvedRequest` catches and downgrades to "approved unpinned". So **every gated endpoint carrying a query parameter was silently unpinned**, `POST .../deploy/{agentId}?version=N` β€” a granted write β€” among them. The headline guarantee of this PR did not apply where it mattered most, and nothing failed loudly. Fixed by normalising both shapes, canonicalising one length-prefixed field per value (so `?tag=a&tag=b` cannot be forged by a single value containing the display separator), and correcting the `KEY_QUERY_PARAMS` javadoc that asserted the wrong type. + +**Query parameters were not redacted.** Same class as the body leak above and missed for the same reason β€” `?api_key=…` is a conventional credential channel, and the query string is shown to the approver. Redacted for display, hashed raw, exactly as the body is. + +**A dropped tool kept its resolver.** `mergeExternalTools` resolves a name collision by dropping the incoming tool, but the resolver was registered before that verdict was known. A builtin that won a collision against an http tool of the same name would then be pinned against the *dropped* tool's request β€” the approver shown a preview of a call that never runs, and the pre-execution check comparing against that same fabricated request and passing. Resolvers are now pruned to names a surviving http tool actually owns. + +Also: the tool name in the resolve-failure WARN now goes through `sanitize` (it is model-chosen and could forge log records), and the docs no longer claim `requestPinned: false` implies `requestPreview: null` β€” a call with `preRequest.propertyInstructions` is previewed best-effort *and* left unpinnable, so both are true at once. + +**`WRITE_ENDPOINTS` is now populated**, on the Manager side β€” see that repo's own changelog for the write canary, the curated endpoints (four operational verbs plus group create), real `read_write` scope selection, and the approval banner rendering this backend's `requestPreview` in place of the client-side `operationId` reconstruction it was always labelled as a stand-in for. Nothing further is required on this side. + +--- + ## 🎚️ feat(hitl): per-endpoint approval friction (2026-08-01) **Repo:** EDDI (`feat/operator-write-capability`) diff --git a/docs/hitl.md b/docs/hitl.md index 09cee2e0e8..45a923287b 100644 --- a/docs/hitl.md +++ b/docs/hitl.md @@ -366,6 +366,41 @@ A **REJECTED** call is not executed; instead the LLM receives a structured rejec `GET /agents/{id}/approval-status` returns a `TOOL_CALL` `pauseDetails` object (computed at read time; see the [`pauseDetails` shape](#pausedetails-shape) reference above for the full JSON). Its `calls[].arguments` is **always** the redacted, size-capped value (`argumentsRedacted`) β€” the raw arguments never appear. `executedUngatedCalls` names ungated calls in the same batch that already ran (see decision 4). `outcomeUnknown` lists callIds with an `EXECUTING` journal entry β€” a prior approval that crashed mid-execution β€” and is empty in the common case. +### Request pinning β€” approval binds to a request, not a tool name + +For an `http`-sourced call, the tool name alone tells an approver little: it comes from the endpoint's `operationId` (or a generated slug) and says nothing about which resource is targeted or with what body. So at gate time each gated httpcall tool is **resolved** β€” `IApiCallExecutor.resolve` builds the method, URL, query, headers and body it would send, without sending it β€” and both a **redacted preview** and a **SHA-256 fingerprint** of that resolved request are persisted on the pause (`PendingToolCall.requestPreview`, `.requestFingerprint`). The preview is what an approver should actually look at, not the raw tool arguments. + +`GET .../approval-status` surfaces this: each entry in `pauseDetails.calls[]` carries `requestPinned` (boolean) and, when the call could be resolved at all, `requestPreview` β€” `{method, uri, queryParams, headers, body, bodyTruncated}`, all already redacted. The raw fingerprint itself is never exposed; it is an internal comparison value with no meaning to a human. + +**The two fields are independent, and a client must not infer one from the other.** `requestPinned` says whether a fingerprint will be *enforced* before execution β€” not whether a preview exists. An `http` call carrying `preRequest.propertyInstructions` is previewed best-effort but deliberately left unpinnable, so it arrives with `requestPinned: false` and a non-null `requestPreview`: show it, but do not present it as guaranteed to be what runs. `requestPreview: null` means there was nothing to resolve (every non-`http` tool), which is not a resolution failure a caller should treat as an error. + +On resume, an **approved** call is re-resolved and its fingerprint re-compared immediately before execution. A mismatch refuses the call β€” a synthetic `{"status":"NOT_EXECUTED","reason":"the request changed after it was approved"}` result, an audit line (`hitl.tool.request_changed`, tool + callId + reason only β€” never the request itself), and the rest of the batch proceeds normally. This is what makes the approval bind to *the request that runs*, not to the name of the tool that was called. + +**Headers are fingerprinted redacted; the body is fingerprinted raw.** For **headers** the fingerprint deliberately covers the redacted form: `ApiCallExecutor` resolves `${caller:token}` into `Authorization` at execution time, and the approver is routinely a different person than whoever's turn raised the pause β€” fingerprinting the live header would mismatch on every cross-user approval (the normal case) and the check would have to be disabled. Redacting first makes the fingerprint answer what approval is actually about β€” *what the request does* β€” and leaves *whose credentials carry it* to authentication, which the fingerprint does not participate in. + +The **body** has no such legitimate variance (`${caller:token}` is rejected outside headers, and a `${vault:…}` reference resolves identically at gate time and at execution), so it is hashed as resolved and only the *stored* copy is redacted. Redacting before hashing would collapse two different credentials to one marker and therefore to one fingerprint, letting a swapped secret pass the pre-execution re-check unnoticed. + +The fingerprint is never returned through the client API β€” but it is a SHA-256 digest, not encryption, and it *is* persisted on the pause record. Treat the stored value as sensitive internal data: for a predictable body it supports offline guessing, and equal digests reveal that two requests were identical. It is excluded from every client-facing projection for that reason, not merely because it is meaningless to a human. + +Query parameters get the same treatment as the body β€” hashed as resolved, redacted for display β€” because `?api_key=…` is a conventional way to pass a credential and the query string is shown to the approver too. A repeated parameter (`?tag=a&tag=b`) is canonicalised as one length-prefixed field *per value*, so a single value containing the display separator cannot impersonate two. + +Body and query redaction are by **value shape**, not field name β€” a body is caller-defined JSON (or another format entirely) with no fixed key vocabulary to match on the way headers have. `SecretRedactionFilter` (the same filter behind `argumentsRedacted`) removes OpenAI/Anthropic-style keys, bearer tokens and vault references wherever they appear. A hand-rolled secret in a generically named field, matching none of those shapes, is not caught β€” the same limitation the redacted tool arguments already carry, and the reason a config write that must carry a credential belongs behind a vault reference rather than a literal. + +**A call can be unpinned**, and that is a deliberate degrade, not a bug. A call is left unpinnable whenever `execute()` could legitimately build a request that `resolve()` did not β€” the guard is "never pin what cannot be honoured", so the set is defined by that property rather than by a list of features: + +| Unpinnable when | Why `execute()` can diverge from `resolve()` | +| --- | --- | +| The tool is not `http` (builtin/mcp/a2a) | There is no HTTP request on this side of the boundary to pin. | +| `preRequest.propertyInstructions` is set | Those write to conversation memory, so resolving them ahead of execution would apply them twice. | +| `fireAndForget` **and** `preRequest.batchRequests` | The batch expands at execution time into N requests, none of them the single one that was previewed. | +| `postResponse.retryApiCallInstruction` with `maxRetries >= 1` | `buildRequest` sits inside the retry loop, and each attempt re-renders templates against a memory that the previous attempt wrote to (`{…Error}`, `{…HttpCode}`, `{responseObjectName}`). Attempt 2 is a request nobody previewed. | + +The retry row is the easy one to trip over: `RetryApiCallInstruction.maxRetries` **defaults to 3**, so `"postResponse": {"retryApiCallInstruction": {}}` is by itself enough to unpin an otherwise-pinnable write β€” and a retry can fire on a **2xx** response when `responseValuePathMatchers` matches, not only on `retryOnHttpCodes`. Read `requestPinned` per call; do not infer it from the endpoint. + +An unpinned call (`requestFingerprint == null`) is approved on name and arguments alone, exactly as before pinning existed β€” nothing is ever refused on a comparison that was never sound. An **amended** call (`amendedArguments` set) is likewise never fingerprint-checked: the approver rewrote the request themselves, so the pin describes the request they replaced. + +Three situations fail **closed** instead β€” refused, not silently allowed β€” because "cannot verify" is a different answer than "unchanged": the tool disappeared from the workflow between pause and resume, re-resolution throws, or the call's config gained any of the unpinnable properties above mid-pause (a pinned call becoming unpinnable β€” adding `propertyInstructions`, or a `retryApiCallInstruction`, while a human is deciding). Treating any of these as "unchanged" would make reconfiguring an agent while a human is deciding the way around the guard. + ### The execution journal (at-most-once) Approved tool executions are protected by a write-ahead journal (`IHitlToolJournalStore`) so a human approval is executed **at most once**, across pod crashes and re-approvals: @@ -433,7 +468,8 @@ Config: `eddi.hitl.crash-recovery.enabled` (default `true`), `eddi.hitl.crash-re ## Operations -- **Metrics** (`/q/metrics`): `eddi_hitl_pause_count`, `eddi_hitl_resume_count`, `eddi_hitl_timeout_count`, each tagged `surface=regular|group`; `eddi_group_member_pause_skipped_count` for auto-cancelled member pauses inside groups. +- **Metrics** (`/q/metrics`): `eddi_hitl_pause_count`, `eddi_hitl_resume_count`, `eddi_hitl_timeout_count`, each tagged `surface=regular|group`; `eddi_group_member_pause_skipped_count` for auto-cancelled member pauses inside groups; `eddi.operator.write.approval{decision=approved|rejected|timeout}`, one per gated call the instant its verdict is resolved (`timeout` is a distinct bucket from `approved`/`rejected` β€” see [Request pinning](#request-pinning--approval-binds-to-a-request-not-a-tool-name) above; not operator-specific despite the name, it fires for any gated call regardless of which agent). +- **Operator canary/gate metrics** (client-reported): the write canary and gate-installed check both run entirely client-side (there is no server-side notion of "the operator", just an agent with a particular `hitlConfig`), so the Manager reports outcomes via `POST /administration/operator/{canary-result,gate-status}` (`eddi-admin` only) purely so they show up on `/q/metrics` without a Manager tab open. This is a relay, not a verification β€” a report is trusted at face value. Produces `eddi.operator.canary{outcome=pass|fail|unknown}`, `eddi.operator.canary.duration`, and the gauge `eddi.operator.gate.verified` (1 only while every provisioned version last read back with a sound gate; defaults to 0, including on a deployment that has never activated an operator β€” that ambiguity is real and unresolved by this signal alone). - **Undeploy**: paused conversations do **not** count as active β€” an agent version with pending approvals can be undeployed. Resuming afterwards returns `409 agent not deployed` and the pause is restored (redeploy, then retry). The idle-conversation cleanup sweep likewise **spares** `AWAITING_HUMAN` conversations β€” a pending approval is never silently force-ended by maintenance. - **Cancel semantics (regular)**: cancels a paused conversation, or signals a turn executing on the same pod to stop at the next task boundary. `CANCEL_IMMEDIATE` currently degrades to graceful on the regular surface. Cancelling an idle conversation returns `409` (use `endConversation`). - **Timeout schedules are not manually operable**: HITL timeout schedules live in the schedule store but the schedule REST surface refuses to fire them manually (`409`, use `/resume` or `/cancel` β€” manual firing would bypass the approval authz), restricts update/delete/enable/disable to `eddi-admin` (`403` otherwise, so an editor cannot disarm an ABORT/AUTO_REJECT safety timeout), and redacts them from non-admin listings. diff --git a/planning/operator-write-scope-plan.md b/planning/operator-write-scope-plan.md index 081d020781..926ce50ab5 100644 --- a/planning/operator-write-scope-plan.md +++ b/planning/operator-write-scope-plan.md @@ -1,5 +1,19 @@ # Implementation Plan β€” Approval-Gated Write Capability for the Platform Operator +> **Superseded, kept as historical record.** This plan scoped `WRITE_ENDPOINTS` to +> four narrow operational verbs and explicitly excluded any agent-authoring +> endpoint (Β§5) β€” correct reasoning for what existed at the time (no `hitlConfig` +> support on the standard agent-setup path, no escalation-flag mechanism, no +> request-pinning). Both landed later, and the operator now has real +> create-and-modify capability over agents, agent groups, and every +> workflow-extension store, each still individually approval-gated. The +> authoritative reference for what is actually granted today is the doc comment +> on `WRITE_ENDPOINTS` in `EDDI-Manager/src/lib/operator/tool-scopes.ts` β€” treat +> this document as explaining the ORIGINAL reasoning, not the current state; Β§5's +> stale exclusion is struck through below rather than silently deleted, since the +> reasoning it once carried still explains why the later change was carefully +> scoped rather than done casually. + ## 0. Premise The HITL gate itself is complete and fires before execution (`AgentOrchestrator.java:1169-1253`; gated requests never reach `executeSingleToolCall`). Nothing in the gate needs changing. The blocker is **provisioning** plus **verification**: `setup-api` cannot install a gate, and `tool-scopes.ts` is a provisioning-time constant, not a runtime boundary β€” so "writes are gated" must be an *asserted, read-back fact*, not an assumption. @@ -91,7 +105,7 @@ Because `requireApproval` is `["http:*"]`, **anything later added to `WRITE_ENDP ## 3. Pause UX -**Where.** Inline in the operator chat (`components/operator/operator-chat.tsx`), after the last message β€” the precedent is `discussion-transcript.tsx:579-599`, which already renders `ApprovalBanner` inside a live transcript. Not the approvals page: `pages/approvals.tsx:332-344` deliberately refuses to decide `TOOL_CALL` pauses and links out instead. That page remains the correct *someone else's queue* fallback and needs no change. +**Where.** Inline in the operator chat (`components/operator/operator-chat.tsx`), after the last message β€” the precedent is `discussion-transcript.tsx:579-599`, which already renders `ApprovalBanner` inside a live transcript. `pages/approvals.tsx` deliberately refused to decide `TOOL_CALL` pauses at the time this was written, for the same reason `WRITE_ENDPOINTS` stayed empty: the approver had nothing but a client-side `operationId` guess to review. That reason no longer holds once request pinning ships (Β§3 note below, and EDDI#627) β€” the inbox now expands a `TOOL_CALL` row in place into the same `ApprovalBanner`/`RequestPreview` the operator chat uses, so any `eddi-admin`, not only whoever is at the operator screen, can decide a gated write. **Detecting the pause.** There is no SSE pause event on the 1:1 surface (`RestAgentEngineStreaming.java:66-138`). Two paths, both already proven in `use-chat.ts`: 1. `use-operator-chat.ts:218` currently does `if (event.type === "done") break;` and discards the payload. Parse it: `conversationState === "AWAITING_HUMAN"`, plus `hitlPauseType` and the names-only `hitlPendingToolCalls` (`ConversationMemoryUtilities.java:268-307`) which ride on the snapshot for free. @@ -173,12 +187,12 @@ The gauge is the one worth alerting on: it is the machine-readable form of "writ ## 5. What I would NOT do -- **Not populate `WRITE_ENDPOINTS` beyond the four.** Specifically never bind, regardless of approval: `setup-api`/`setup` (one call provisions a *new* agent with an arbitrary `endpoints` filter and no gate β€” complete escape from the allow-list); `POST /agents/{id}/resume` (self-approval β€” `HitlAccessGuard` has no "not the requester" check); `PATCH /agents/{id}/state` and `/cancel` (clears `AWAITING_HUMAN` under the gate); `PUT /variablestore/variables/...` (the operator's own config blob lives at key `platform.operator`, `operator.ts:74`); all `/secretstore` writes; `/backup/import*`; `apicallstore`/`mcpcallsstore`/`channelstore` writes; `/ragstore/.../ingest`; `usermemorystore` writes; `AgentTriggerStore` writes; `/administration/quotas`; `DELETE /administration/orphans`. +- ~~Not populate `WRITE_ENDPOINTS` beyond the four. Specifically never bind, regardless of approval: `setup-api`/`setup` (one call provisions a *new* agent with an arbitrary `endpoints` filter and no gate β€” complete escape from the allow-list)~~ β€” superseded (see the banner at the top). `SetupAgentRequest` gained the same `hitlConfig` field `CreateApiAgentRequest` already had, both are now bound, and `escalation-flags.ts`'s `agentCreatedWithoutGate`/`agentCreatedWithBroadEndpoints` checks surface exactly the two risks named here (no gate; unbounded `endpoints`) to the approver above the raw JSON. `apicallstore`/`mcpcallsstore` writes are bound too, for the same "modify an existing agent's tool wiring" reason the other workflow-extension stores are β€” narrower than blanket "never," and still gated like everything else. `POST /agents/{id}/resume` (self-approval), `PATCH /agents/{id}/state` and `/cancel`, `PUT /variablestore/variables/...` (the operator's own config), all `/secretstore` writes, `/backup/import*`, `channelstore` writes, `/ragstore/.../ingest`, `usermemorystore` writes, `AgentTriggerStore` writes, `/administration/quotas`, and `DELETE /administration/orphans` remain excluded β€” this correction is scoped to exactly the two items it names, not a blanket reopening. - **Not upgrade an existing read-only operator in place.** Changing scope **re-provisions** a new agent (fresh `setup-api`, gate on v1) and resets the old one via `resetOperator`. An in-place `PUT` would leave an older, ungated version of the same agent that a bound `deployAgent` call could roll back to. This is why "every version carries the gate" is the read-back invariant rather than "the current version does". - **Not change backend `AUTO_APPROVE` semantics.** Explicit `toolApprovals.timeoutPolicy: AUTO_APPROVE` is honored (`ConversationService.java:2242-2247`) and existing agents may rely on it. Refuse it Manager-side for the operator only. - **Not enable Slack approvals for operator writes.** `SlackHitlSupport.java:69,75` truncates to 5 calls and 300 chars of arguments while keeping the same buttons β€” the realistic rubber-stamping surface. - **Not add `POST /agents/{id}/resume/stream`.** Real gap (`ConversationService.resumeConversation` already accepts a handler; only the REST adapter passes `null`), but a separate backend PR. A post-decision re-read is adequate. -- **Not touch `pages/approvals.tsx`.** Its refusal to decide TOOL_CALL pauses is correct. +- ~~Not touch `pages/approvals.tsx`.~~ Superseded once request pinning shipped β€” see Β§3. - **Not build "approve all".** - **Not treat `tool-scopes.ts` as a security boundary.** It is applied at provisioning time only. Say so in the file comment. diff --git a/src/main/java/ai/labs/eddi/engine/api/IConversationService.java b/src/main/java/ai/labs/eddi/engine/api/IConversationService.java index f128a1c514..5f56e3adf6 100644 --- a/src/main/java/ai/labs/eddi/engine/api/IConversationService.java +++ b/src/main/java/ai/labs/eddi/engine/api/IConversationService.java @@ -308,6 +308,16 @@ CancelOutcome cancelConversation(String conversationId, * the human approval/rejection decision * @param responseHandler * optional callback β€” may be null for fire-and-forget + * @throws IllegalArgumentException + * {@code decision} is null, carries no top-level {@code verdict}, + * or its {@code toolDecisions} fail validation β€” maps to HTTP 400; + * checked before the AWAITING_HUMAN->IN_PROGRESS CAS, so the + * pause is never consumed by a malformed request. Every current + * caller (REST, Slack, MCP, timeout auto-resolution) already + * guarantees a non-null verdict before calling this method; this is + * the one place that guarantee is enforced rather than assumed, so + * a future caller that forgets fails loudly here instead of + * silently reaching the tool-execution gate with nothing to check. * @throws IllegalStateException * wrong-state conflict (not AWAITING_HUMAN, or agent not deployed) * β€” maps to HTTP 409; the pause is preserved/restored diff --git a/src/main/java/ai/labs/eddi/engine/api/IRestOperatorMetrics.java b/src/main/java/ai/labs/eddi/engine/api/IRestOperatorMetrics.java new file mode 100644 index 0000000000..192f181731 --- /dev/null +++ b/src/main/java/ai/labs/eddi/engine/api/IRestOperatorMetrics.java @@ -0,0 +1,62 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.engine.api; + +import ai.labs.eddi.engine.api.model.OperatorCanaryReport; +import ai.labs.eddi.engine.api.model.OperatorGateStatusReport; +import jakarta.annotation.security.RolesAllowed; +import jakarta.ws.rs.Consumes; +import jakarta.ws.rs.POST; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.Response; +import org.eclipse.microprofile.openapi.annotations.Operation; +import org.eclipse.microprofile.openapi.annotations.responses.APIResponse; +import org.eclipse.microprofile.openapi.annotations.tags.Tag; + +/** + * Lets the Manager report the outcome of a client-driven operator check onto + * this deployment's {@code /q/metrics}. + *

+ * The write canary and the gate-installed check both run entirely in the + * Manager: one drives a synthetic conversation and inspects its pause, the + * other re-reads every version of the operator agent document. Neither has a + * server-side equivalent β€” this deployment has no first-class notion of "the + * operator", just an agent like any other β€” so what this endpoint provides is + * purely visibility: an on-call engineer watching Grafana should not have to + * have a Manager tab open to see whether the write gate is currently sound. + *

+ * This is not a verification endpoint. A report is trusted at face + * value, which is exactly why it sits behind {@code eddi-admin} β€” the same tier + * that can provision the operator in the first place. Anyone who could + * misreport through this endpoint could just as easily reconfigure the operator + * directly. + * + * @since 6.2.0 + */ +@Path("/administration/operator") +@Tag(name = "Operations / Operator Metrics", description = "Client-reported operator canary and gate-verification outcomes") +@RolesAllowed("eddi-admin") +public interface IRestOperatorMetrics { + + @POST + @Path("/canary-result") + @Consumes(MediaType.APPLICATION_JSON) + @Operation(summary = "Report a write canary outcome", + description = "Records the result of a client-run write canary (a synthetic conversation that provokes and then rejects a real " + + "gated write) as eddi.operator.canary{outcome} and eddi.operator.canary.duration.") + @APIResponse(responseCode = "204", description = "Recorded.") + @APIResponse(responseCode = "400", description = "outcome was missing or not one of pass/fail/unknown.") + Response reportCanaryResult(OperatorCanaryReport report); + + @POST + @Path("/gate-status") + @Consumes(MediaType.APPLICATION_JSON) + @Operation(summary = "Report a gate-verification outcome", + description = "Sets the eddi.operator.gate.verified gauge to 1 when every provisioned version of the operator agent read back " + + "with a sound approval gate, 0 otherwise. This is the meter worth alerting on.") + @APIResponse(responseCode = "204", description = "Recorded.") + Response reportGateStatus(OperatorGateStatusReport report); +} diff --git a/src/main/java/ai/labs/eddi/engine/api/OperatorMetricsService.java b/src/main/java/ai/labs/eddi/engine/api/OperatorMetricsService.java new file mode 100644 index 0000000000..c3ba1218e4 --- /dev/null +++ b/src/main/java/ai/labs/eddi/engine/api/OperatorMetricsService.java @@ -0,0 +1,96 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.engine.api; + +import ai.labs.eddi.engine.api.model.OperatorCanaryReport; +import io.micrometer.core.instrument.Counter; +import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.Timer; +import jakarta.annotation.PostConstruct; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; + +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +/** + * Backs the three write-canary and gate-verification meters the Manager cannot + * emit itself. + *

+ * Every meter here describes a fact the Manager establishes client-side β€” the + * canary is a synthetic conversation it drives, the gate check is a set of + * agent-document reads it performs β€” and reports over + * {@link ai.labs.eddi.engine.rest.RestOperatorMetrics} purely so the fact + * becomes visible on {@code /q/metrics} rather than only in a browser tab. This + * service does not, and cannot, verify any of it independently: it trusts the + * report the same way any metrics endpoint trusts its caller, which is exactly + * why {@link ai.labs.eddi.engine.api.IRestOperatorMetrics} is restricted to + * {@code eddi-admin} β€” the same tier that can provision the operator at all. + */ +@ApplicationScoped +public class OperatorMetricsService { + + private static final List VALID_OUTCOMES = List.of(OperatorCanaryReport.OUTCOME_PASS, OperatorCanaryReport.OUTCOME_FAIL, + OperatorCanaryReport.OUTCOME_UNKNOWN); + + private final MeterRegistry meterRegistry; + + /** + * Backing store for {@code eddi.operator.gate.verified}. 1 while every + * provisioned version last read back with a sound gate, 0 otherwise β€” including + * before any report has ever arrived. A fresh deployment that has never + * activated an operator therefore also reads 0: "not yet proven true" is the + * correct default for anything this metric guards, even though it cannot be + * distinguished from "activated, and broken" by this signal alone. + */ + private final AtomicInteger gateVerified = new AtomicInteger(0); + + @Inject + public OperatorMetricsService(MeterRegistry meterRegistry) { + this.meterRegistry = meterRegistry; + } + + /** + * Public rather than package-private: under CDI this runs automatically via + * {@code @PostConstruct}, but a test constructing this service directly (no + * container) has to be able to call it too, or the gauge is never registered + * and every read-back is silently absent. + */ + @PostConstruct + public void registerGateGauge() { + // Registered once, here, rather than on every report: a Micrometer gauge is + // a live read of a supplier, not a value you push β€” calling gauge(...) + // again on each report would keep re-registering the same meter id, which + // most registries tolerate but is not the contract. + meterRegistry.gauge("eddi.operator.gate.verified", gateVerified, AtomicInteger::get); + } + + /** + * Whether a canary/gate report's outcome string is one this service accepts. + * {@code List.of(...).contains(null)} throws NPE rather than returning false, + * so null is checked explicitly ahead of it. + */ + public static boolean isValidOutcome(String outcome) { + return outcome != null && VALID_OUTCOMES.contains(outcome); + } + + /** + * @param outcome + * must be one of {@link #isValidOutcome} β€” validated by the REST + * layer before this is called, so an invalid value here is a + * programming error, not a client mistake to degrade gracefully for. + */ + public void recordCanaryResult(String outcome, Long durationMs) { + Counter.builder("eddi.operator.canary").tag("outcome", outcome).register(meterRegistry).increment(); + if (durationMs != null && durationMs >= 0) { + Timer.builder("eddi.operator.canary.duration").register(meterRegistry).record(durationMs, TimeUnit.MILLISECONDS); + } + } + + public void recordGateStatus(boolean verified) { + gateVerified.set(verified ? 1 : 0); + } +} diff --git a/src/main/java/ai/labs/eddi/engine/api/model/OperatorCanaryReport.java b/src/main/java/ai/labs/eddi/engine/api/model/OperatorCanaryReport.java new file mode 100644 index 0000000000..2b8dbf975b --- /dev/null +++ b/src/main/java/ai/labs/eddi/engine/api/model/OperatorCanaryReport.java @@ -0,0 +1,31 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.engine.api.model; + +/** + * Outcome of one client-run write canary, reported for {@code /q/metrics} + * visibility. + *

+ * The canary itself runs in the Manager: it starts a synthetic conversation, + * provokes a real gated write, asserts the turn paused with the expected tool + * pending, then rejects it so nothing executes. The backend has no way to + * observe that sequence on its own β€” a conversation looks like any other from + * this side β€” so the Manager reports the result after the fact. + * + * @param outcome + * {@code pass}, {@code fail}, or {@code unknown}. Fixed vocabulary, + * validated server-side β€” never free text, so the metric's + * cardinality cannot grow from client input. + * @param durationMs + * wall-clock time of the probe; negative or absent values are simply + * not recorded as a timer sample rather than rejected, since a + * malformed duration says nothing about whether the gate held. + */ +public record OperatorCanaryReport(String outcome, Long durationMs) { + + public static final String OUTCOME_PASS = "pass"; + public static final String OUTCOME_FAIL = "fail"; + public static final String OUTCOME_UNKNOWN = "unknown"; +} diff --git a/src/main/java/ai/labs/eddi/engine/api/model/OperatorGateStatusReport.java b/src/main/java/ai/labs/eddi/engine/api/model/OperatorGateStatusReport.java new file mode 100644 index 0000000000..4e4a1c4c3d --- /dev/null +++ b/src/main/java/ai/labs/eddi/engine/api/model/OperatorGateStatusReport.java @@ -0,0 +1,21 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.engine.api.model; + +/** + * Result of one client-run gate verification, reported for {@code /q/metrics} + * visibility. + *

+ * {@code verifyGateInstalled} (Manager-side) reads every provisioned version of + * the operator agent back and checks the approval gate is installed and sane on + * each. That fact has no backend-side equivalent to observe directly β€” the + * operator is not a distinct concept in this codebase, just an agent document + * like any other β€” so the Manager reports the outcome after checking it. + * + * @param verified + * true only when every version read back with a sound gate. + */ +public record OperatorGateStatusReport(boolean verified) { +} diff --git a/src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java b/src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java index f03a5f5665..73364266f6 100644 --- a/src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java +++ b/src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java @@ -35,6 +35,46 @@ public interface IRequest { IResponse send() throws HttpRequestException; + /** Key of the fully-resolved target URI in {@link #toMap()}. */ + String KEY_URI = "uri"; + /** Key of the HTTP method name in {@link #toMap()}. */ + String KEY_METHOD = "method"; + /** + * Key of the headers map in {@link #toMap()}. + *

+ * Read the values as {@code Object}, not {@code String}: the default + * implementation happens to store strings, but this interface has other + * implementations and nothing enforces it β€” which is why + * {@code RequestRedactor#redactHeaders} takes {@code Map} and + * coerces. The neighbouring {@link #KEY_QUERY_PARAMS} documented a narrower + * type than it delivered and that produced a real defect; this one is + * deliberately stated loosely rather than optimistically. + */ + String KEY_HEADERS = "headers"; + /** + * Key of the query parameters in {@link #toMap()}. + *

+ * The value is a {@code Map>}, not a + * {@code Map}: a parameter may legitimately repeat + * ({@code ?tag=a&tag=b}) and the default implementation accumulates repeats + * into a list. Reading it back through a single-valued cast compiles and erases + * cleanly, then throws a {@link ClassCastException} at first use β€” see + * {@code ApiCallExecutor#normalizeQueryParams}, which tolerates both shapes + * rather than trusting either. + */ + String KEY_QUERY_PARAMS = "queryParams"; + /** Key of the request body in {@link #toMap()}; absent when there is none. */ + String KEY_BODY = "body"; + /** Key of the User-Agent header in {@link #toMap()}; absent when unset. */ + String KEY_USER_AGENT = "userAgent"; + + /** + * The request as a plain map, keyed by the {@code KEY_*} constants above. + *

+ * Header values are live β€” resolved secrets and bearer tokens included. + * Anything that persists or displays this must redact it first + * ({@code RequestRedactor}). + */ Map toMap(); void send(ICompleteListener completeListener) throws HttpRequestException; diff --git a/src/main/java/ai/labs/eddi/engine/httpclient/impl/HttpClientWrapper.java b/src/main/java/ai/labs/eddi/engine/httpclient/impl/HttpClientWrapper.java index 86f2ab4961..c4d74c3822 100644 --- a/src/main/java/ai/labs/eddi/engine/httpclient/impl/HttpClientWrapper.java +++ b/src/main/java/ai/labs/eddi/engine/httpclient/impl/HttpClientWrapper.java @@ -28,14 +28,17 @@ @ApplicationScoped public class HttpClientWrapper implements IHttpClient { - private static final String KEY_URI = "uri"; - private static final String KEY_METHOD = "method"; - private static final String KEY_HEADERS = "headers"; + // The toMap() key names live on IRequest: they are part of that method's + // contract, and readers of the map (RequestRedactor, ApiCallExecutor#resolve) + // must key off the same constants rather than re-spelling the strings. + private static final String KEY_URI = IRequest.KEY_URI; + private static final String KEY_METHOD = IRequest.KEY_METHOD; + private static final String KEY_HEADERS = IRequest.KEY_HEADERS; + private static final String KEY_QUERY_PARAMS = IRequest.KEY_QUERY_PARAMS; + private static final String KEY_BODY = IRequest.KEY_BODY; + private static final String KEY_USER_AGENT = IRequest.KEY_USER_AGENT; private static final String KEY_LOGICAL_AND = "&"; private static final String KEY_EQUALS = "="; - private static final String KEY_QUERY_PARAMS = "queryParams"; - private static final String KEY_BODY = "body"; - private static final String KEY_USER_AGENT = "userAgent"; private static final String KEY_MAX_LENGTH = "maxLength"; private static final int TEXT_LIMIT = 150; private final WebClientSession webClient; diff --git a/src/main/java/ai/labs/eddi/engine/internal/ConversationService.java b/src/main/java/ai/labs/eddi/engine/internal/ConversationService.java index 1be22ac354..a604af9c70 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/ConversationService.java +++ b/src/main/java/ai/labs/eddi/engine/internal/ConversationService.java @@ -1550,6 +1550,12 @@ public void resumeConversation(String conversationId, ai.labs.eddi.engine.lifecycle.model.HitlDecision decision, ConversationResponseHandler handler) throws ResourceStoreException, ResourceNotFoundException { + // See resumeConversation's @throws IllegalArgumentException javadoc + // (IConversationService) for why this is checked here rather than trusted + // from each caller. + if (decision == null || decision.getVerdict() == null) { + throw new IllegalArgumentException("decision.verdict is required (APPROVED or REJECTED)"); + } // B3: a resume enqueues a FULL turn through the same coordinator the shutdown // drain is waiting on, so admitting one during the drain both extends the // drain and risks the turn being SIGKILLed halfway. Rejected here, BEFORE the diff --git a/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java b/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java index 9bfafa5028..7946a9e361 100644 --- a/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java +++ b/src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java @@ -16,6 +16,7 @@ import ai.labs.eddi.engine.lifecycle.model.HitlDecision; import ai.labs.eddi.engine.memory.IConversationMemoryStore; import ai.labs.eddi.engine.model.PendingApprovalSummary; +import ai.labs.eddi.engine.memory.ConversationMemoryUtilities; import ai.labs.eddi.engine.memory.model.ConversationMemorySnapshot; import ai.labs.eddi.engine.memory.model.ConversationMemorySnapshot.ConversationStepSnapshot; import ai.labs.eddi.engine.memory.model.ConversationMemorySnapshot.WorkflowRunSnapshot; @@ -434,7 +435,10 @@ public Response getApprovalStatus(String conversationId, String detail) { + "is awaiting approval β€” use the summary view") .build(); } - return Response.ok(snapshot).build(); + // The fingerprint is internal: it digests the RAW body and query + // values, which is exactly what the preview beside it redacts. + // See ConversationMemoryUtilities#stripRequestFingerprintsForRead. + return Response.ok(ConversationMemoryUtilities.stripRequestFingerprintsForRead(snapshot)).build(); } // Bookmark fields describe the pause β€” suppress them once the // conversation left AWAITING_HUMAN so stale fields (e.g. after a @@ -500,6 +504,15 @@ private Map buildToolCallPauseDetails(String conversationId, Con callView.put("arguments", call.getArgumentsRedacted()); callView.put("argsTruncated", call.isArgsTruncated()); callView.put("gateReason", call.getGateReason()); + // The approver's honest replacement for guessing a method/path from a + // tool name: what this call actually resolves to, already redacted at + // gate time. requestPinned tells the caller whether that preview is + // backed by a fingerprint that will be re-checked immediately before + // execution (see IApiCallExecutor#resolve) β€” false for every non-http + // tool, so a client must not read its absence as "this call is + // somehow less real", only "there is nothing to preview here". + callView.put("requestPinned", call.isRequestPinned()); + callView.put("requestPreview", toRequestPreviewView(call.getRequestPreview())); calls.add(callView); if (pauseEpoch != null && call.getCallId() != null) { @@ -517,6 +530,33 @@ private Map buildToolCallPauseDetails(String conversationId, Con return details; } + /** + * View of a {@link PendingToolCallBatch.ResolvedRequestPreview}, or + * {@code null} when the call could not be resolved ahead of execution (every + * non-http tool, and an http call whose config could not be previewed without + * side effects β€” see {@code IApiCallExecutor#resolve}). + *

+ * Explicit field-by-field like the rest of this method rather than handing back + * the POJO for Jackson to serialize: this keeps the exposed shape under the + * same review as {@code arguments} above, and the redaction already happened + * before this object was ever persisted β€” nothing here is sensitive to begin + * with, but the pattern of "build the view explicitly" stays uniform across + * every field in {@code callView}. + */ + private Map toRequestPreviewView(PendingToolCallBatch.ResolvedRequestPreview preview) { + if (preview == null) { + return null; + } + var view = new LinkedHashMap(); + view.put("method", preview.getMethod()); + view.put("uri", preview.getUri()); + view.put("queryParams", preview.getQueryParams() != null ? preview.getQueryParams() : Map.of()); + view.put("headers", preview.getHeaders() != null ? preview.getHeaders() : Map.of()); + view.put("body", preview.getBody()); + view.put("bodyTruncated", preview.isBodyTruncated()); + return view; + } + private Map buildRulePauseDetails(ConversationMemorySnapshot snapshot) { var details = new LinkedHashMap(); details.put("type", "RULE"); diff --git a/src/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.java b/src/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.java index 2c926263e7..dbd4d23d0e 100644 --- a/src/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.java +++ b/src/main/java/ai/labs/eddi/engine/mcp/McpHitlTools.java @@ -17,6 +17,7 @@ import ai.labs.eddi.engine.lifecycle.model.ControlSignal; import ai.labs.eddi.engine.lifecycle.model.HitlDecision; import ai.labs.eddi.engine.lifecycle.model.HitlDecision.HitlVerdict; +import ai.labs.eddi.engine.memory.ConversationMemoryUtilities; import ai.labs.eddi.engine.memory.model.ConversationMemorySnapshot; import ai.labs.eddi.engine.memory.model.ConversationState; import ai.labs.eddi.engine.security.OwnershipValidator; @@ -169,7 +170,10 @@ public String getApprovalStatus( return errorJson("Full approval status is available to approvers only while awaiting approval β€” " + "use the summary view", "FORBIDDEN", null); } - return jsonSerialization.serialize(snapshot); + // Same internal-fingerprint strip as the REST surface β€” this + // serializes the identical snapshot object, so leaving it out + // here would just move the leak to the other door. + return jsonSerialization.serialize(ConversationMemoryUtilities.stripRequestFingerprintsForRead(snapshot)); } Map summary = new LinkedHashMap<>(); summary.put("conversationId", conversationId); diff --git a/src/main/java/ai/labs/eddi/engine/mcp/McpSetupTools.java b/src/main/java/ai/labs/eddi/engine/mcp/McpSetupTools.java index ac8dab3d11..8125dbd6d7 100644 --- a/src/main/java/ai/labs/eddi/engine/mcp/McpSetupTools.java +++ b/src/main/java/ai/labs/eddi/engine/mcp/McpSetupTools.java @@ -82,8 +82,14 @@ public String setupAgent(@ToolArg(description = "Agent name (required)") String @ToolArg(description = "Environment: 'production' (default) or 'test'") String environment) { requireRole(identity, authEnabled, "eddi-editor"); try { + // hitlConfig is deliberately null and has no @ToolArg: this tool already + // lets the caller choose the created agent's own tool surface + // (enableBuiltInTools, builtInToolsWhitelist, mcpServerUrls), so also + // letting it choose that agent's gate would let a caller build an + // ungated agent at will. Provisioning a gated agent goes through the + // REST setup endpoint. var request = new SetupAgentRequest(agentName, systemPrompt, provider, model, apiKey, baseUrl, introMessage, enableBuiltInTools, - builtInToolsWhitelist, enableQuickReplies, enableSentimentAnalysis, mcpServerUrls, deploy, environment); + builtInToolsWhitelist, enableQuickReplies, enableSentimentAnalysis, mcpServerUrls, deploy, environment, null); var result = agentSetupService.setupAgent(request); return jsonSerialization.serialize(result); } catch (AgentSetupException e) { diff --git a/src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.java b/src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.java index e97800fdcc..7718105910 100644 --- a/src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.java +++ b/src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.java @@ -281,11 +281,15 @@ private static SimpleConversationMemorySnapshot getSimpleMemorySnapshot(Conversa *

* This copy therefore carries ONLY per-call {@code callId}/{@code toolName}/ * {@code source}/{@code gateReason}/{@code argsTruncated} β€” never - * {@code argumentsRaw} or {@code argumentsRedacted} β€” and leaves - * {@code chatTranscriptJson}, {@code traceSoFar}, and {@code fingerprint} null. - * Consumers that read tool NAMES (delegated/group/MCP parity via - * {@code batch.getCalls().getToolName()}) keep working unchanged. Returns - * {@code null} when there is no batch. + * {@code argumentsRaw}, {@code argumentsRedacted}, {@code requestFingerprint}, + * or {@code requestPreview} β€” and leaves {@code chatTranscriptJson}, + * {@code traceSoFar}, and {@code fingerprint} null. {@code requestPreview} is + * excluded for the same reason as {@code argumentsRedacted}: both are already + * redacted at persistence time, so the exclusion is not about a fresh secret + * leak β€” it is that this view's whole contract is "names only", and a request + * preview is materially more detail than a name. Consumers that read tool NAMES + * (delegated/group/MCP parity via {@code batch.getCalls().getToolName()}) keep + * working unchanged. Returns {@code null} when there is no batch. */ private static PendingToolCallBatch namesOnlyPendingToolCalls(PendingToolCallBatch source) { if (source == null) { @@ -348,6 +352,61 @@ public static ConversationMemorySnapshot redactRawPendingToolCallsForRead(Conver return snapshot; } + /** + * Stands in for a stripped fingerprint. A constant, so it carries none of the + * digest β€” but non-null, so {@code PendingToolCall#isRequestPinned()} (which + * derives from the field) keeps reporting the truth. + */ + static final String REDACTED_FINGERPRINT = ""; + + /** + * Strips the request fingerprints from a snapshot about to be returned in FULL + * to an approver. + *

+ * {@code approval-status?detail=full} deliberately returns the whole snapshot β€” + * an approver needs the arguments and the request preview β€” so + * {@link #namesOnlyPendingToolCalls} is far too aggressive here. But + * {@code requestFingerprint} must not ride along: it is a SHA-256 over a + * canonical string that includes the RAW body and RAW query values, i.e. + * precisely the credential material {@code RequestRedactor} stripped out of the + * preview beside it. Handing an approver both the digest and everything that + * went into it except the secret is an offline guessing exercise, which is why + * {@code PendingToolCallBatch}, {@code ResolvedRequest} and + * {@code docs/hitl.md} all state it is never exposed. This is what makes that + * true on this path. + *

+ * A read-time projection rather than {@code @JsonIgnore} on the getter: + * {@code SerializationCustomizer.configureObjectMapper} is shared with + * {@code PersistenceMapperProducer}, so ignoring the field would also drop it + * from the PERSISTED document β€” silently disabling pinning everywhere, since + * the fingerprint would no longer survive the pause it exists to guard. + *

+ * Mutates the passed snapshot, matching + * {@link #redactRawPendingToolCallsForRead}: both operate on a snapshot freshly + * loaded for one request, never on shared state. + */ + public static ConversationMemorySnapshot stripRequestFingerprintsForRead(ConversationMemorySnapshot snapshot) { + if (snapshot == null || snapshot.getHitlPendingToolCalls() == null + || snapshot.getHitlPendingToolCalls().getCalls() == null) { + return snapshot; + } + for (var call : snapshot.getHitlPendingToolCalls().getCalls()) { + if (call != null && call.getRequestFingerprint() != null) { + // A marker, NOT null. `isRequestPinned()` is derived from this + // field, and it is a documented contract field the approver's UI + // renders as "verified" vs "preview only". Nulling the digest + // therefore silently flipped every pinned call to + // requestPinned:false on this surface β€” telling the approver the + // request is NOT re-checked before execution when it is, and + // disagreeing with detail=summary about the same conversation. + // Replacing rather than clearing keeps the boolean honest while + // revealing nothing: the marker is a constant, not a digest. + call.setRequestFingerprint(REDACTED_FINGERPRINT); + } + } + return snapshot; + } + public static SimpleConversationMemorySnapshot convertSimpleConversationMemorySnapshot(IConversationMemory returnConversationMemory, Boolean returnDetailed, Boolean returnCurrentStepOnly, List returningFields) { diff --git a/src/main/java/ai/labs/eddi/engine/memory/model/PendingToolCallBatch.java b/src/main/java/ai/labs/eddi/engine/memory/model/PendingToolCallBatch.java index e881554308..dbf7646ba8 100644 --- a/src/main/java/ai/labs/eddi/engine/memory/model/PendingToolCallBatch.java +++ b/src/main/java/ai/labs/eddi/engine/memory/model/PendingToolCallBatch.java @@ -26,6 +26,15 @@ public class PendingToolCallBatch { public static final int ARGS_REDACTED_MAX_BYTES = 32_768; public static final int AMENDED_ARGS_MAX_BYTES = 32_768; public static final int TRACE_ENTRY_MAX_BYTES = 65_536; + /** + * Cap for the request body kept in the approval preview. + *

+ * Display-only, and deliberately smaller than {@link #ARGS_RAW_MAX_BYTES}: an + * approver cannot meaningfully read more than this, and the pause is persisted + * as part of the conversation document. Truncation here never affects the + * fingerprint, which is computed over the full body before any capping. + */ + public static final int PREVIEW_BODY_MAX_BYTES = 8_192; /** A single gated tool call awaiting a human verdict. */ public static class PendingToolCall { @@ -38,6 +47,40 @@ public static class PendingToolCall { private String gateReason; // the matched pattern, e.g. "mcp:*" private String matchedRule; // toolApprovals.rules[].match that tuned this call, or null + /** + * SHA-256 of the HTTP request this call resolved to at gate time, re-derived + * and compared immediately before execution. + *

+ * Headers participate in their redacted form and the query and body as + * resolved β€” see {@code ResolvedRequest} for why the two differ (a + * caller token legitimately varies between requester and approver; a query + * value or body does not, and collapsing two credentials to one marker before + * hashing would let a swapped one pass this check). Never exposed through any + * client-facing projection: it is a digest, not encryption, and for a + * predictable body it would support offline guessing. + *

+ * Distinct from the batch-level {@code fingerprint} above, which hashes tool + * names and arguments to detect a wedged no-progress loop. This one answers a + * different question β€” is the request about to run the one that was + * approved β€” and is what makes approval bind to a request rather than to a + * tool name. + *

+ * Null for anything not resolvable ahead of execution: every non-http tool, and + * an http call whose pre-request property instructions would have to run first + * (see {@code IApiCallExecutor#resolve}). Null means unenforced, not failed β€” a + * call is never rejected on a comparison that was never sound. + */ + private String requestFingerprint; + + /** + * The redacted request, for display to the approver β€” {@code METHOD uri}, query + * and body, credentials already removed. + *

+ * This is the honest replacement for reconstructing an endpoint client-side + * from an {@code operationId}, which is a guess from a spec that can drift. + */ + private ResolvedRequestPreview requestPreview; + public String getCallId() { return callId; } @@ -101,6 +144,103 @@ public String getMatchedRule() { public void setMatchedRule(String matchedRule) { this.matchedRule = matchedRule; } + + public String getRequestFingerprint() { + return requestFingerprint; + } + + public void setRequestFingerprint(String requestFingerprint) { + this.requestFingerprint = requestFingerprint; + } + + public ResolvedRequestPreview getRequestPreview() { + return requestPreview; + } + + public void setRequestPreview(ResolvedRequestPreview requestPreview) { + this.requestPreview = requestPreview; + } + + /** Whether this call was pinned to a request at gate time. */ + public boolean isRequestPinned() { + return requestFingerprint != null && !requestFingerprint.isBlank(); + } + } + + /** + * The redacted HTTP request a gated call resolved to, as persisted on the pause + * and shown to the approver. + *

+ * A plain POJO rather than the {@code ResolvedRequest} record it is built from: + * this is written to the conversation document and read back by Jackson, and + * the persisted shape must not be coupled to a type in the apicalls module. + * Credentials are already redacted before anything reaches here β€” nothing on + * this object is ever sensitive. + */ + public static class ResolvedRequestPreview { + private String method; + private String uri; + private Map queryParams; + /** + * Redacted headers. + *

+ * Shown even though they are mostly uninteresting, because the fingerprint + * covers them: a header the approver never saw could otherwise be the thing + * that later fails the pre-execution check, and "approve what you are shown" + * has to mean the whole of what is checked. + */ + private Map headers; + private String body; + /** True when the body was cut to {@link #PREVIEW_BODY_MAX_BYTES}. */ + private boolean bodyTruncated; + + public String getMethod() { + return method; + } + + public void setMethod(String method) { + this.method = method; + } + + public String getUri() { + return uri; + } + + public void setUri(String uri) { + this.uri = uri; + } + + public Map getQueryParams() { + return queryParams; + } + + public void setQueryParams(Map queryParams) { + this.queryParams = queryParams; + } + + public Map getHeaders() { + return headers; + } + + public void setHeaders(Map headers) { + this.headers = headers; + } + + public String getBody() { + return body; + } + + public void setBody(String body) { + this.body = body; + } + + public boolean isBodyTruncated() { + return bodyTruncated; + } + + public void setBodyTruncated(boolean bodyTruncated) { + this.bodyTruncated = bodyTruncated; + } } private String pauseEpoch; // UUID per pause β€” journal key component diff --git a/src/main/java/ai/labs/eddi/engine/rest/RestOperatorMetrics.java b/src/main/java/ai/labs/eddi/engine/rest/RestOperatorMetrics.java new file mode 100644 index 0000000000..cba61f643d --- /dev/null +++ b/src/main/java/ai/labs/eddi/engine/rest/RestOperatorMetrics.java @@ -0,0 +1,52 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.engine.rest; + +import ai.labs.eddi.engine.api.IRestOperatorMetrics; +import ai.labs.eddi.engine.api.OperatorMetricsService; +import ai.labs.eddi.engine.api.model.OperatorCanaryReport; +import ai.labs.eddi.engine.api.model.OperatorGateStatusReport; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; +import jakarta.ws.rs.BadRequestException; +import jakarta.ws.rs.core.Response; + +/** + * REST implementation of {@link IRestOperatorMetrics}. Validates, then + * delegates to {@link OperatorMetricsService}. + */ +@ApplicationScoped +public class RestOperatorMetrics implements IRestOperatorMetrics { + + private final OperatorMetricsService operatorMetricsService; + + @Inject + public RestOperatorMetrics(OperatorMetricsService operatorMetricsService) { + this.operatorMetricsService = operatorMetricsService; + } + + @Override + public Response reportCanaryResult(OperatorCanaryReport report) { + // Distinguished, not collapsed: telling a caller who sent no body that its + // "outcome" is wrong sends them looking at a field they never sent. + if (report == null) { + throw new BadRequestException("request body is required"); + } + if (!OperatorMetricsService.isValidOutcome(report.outcome())) { + throw new BadRequestException("outcome must be one of: pass, fail, unknown"); + } + operatorMetricsService.recordCanaryResult(report.outcome(), report.durationMs()); + return Response.noContent().build(); + } + + @Override + public Response reportGateStatus(OperatorGateStatusReport report) { + if (report == null) { + throw new BadRequestException("request body is required"); + } + operatorMetricsService.recordGateStatus(report.verified()); + return Response.noContent().build(); + } +} diff --git a/src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java b/src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java index a864b8737e..d4a3f5e096 100644 --- a/src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java +++ b/src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java @@ -124,6 +124,16 @@ public SetupResult setupAgent(SetupAgentRequest request) throws AgentSetupExcept if (!isLocalLLM && (request.apiKey() == null || request.apiKey().isBlank())) { throw new AgentSetupException("API key is required for cloud LLM providers (anthropic, openai, gemini)"); } + // Validate the HITL config HERE, before a single resource exists β€” same + // reasoning as createApiAgent: AgentStore.create validates it too, but only + // at step 7, and a bad pattern would otherwise surface after the parser, + // behaviour, LLM and workflow had all been created, leaving every one of + // them orphaned. + try { + HitlConfigValidation.validate(request.hitlConfig()); + } catch (IllegalArgumentException e) { + throw new AgentSetupException("Invalid hitlConfig: " + e.getMessage(), e); + } validateMcpServerUrls(request.mcpServerUrls()); var params = resolveParamsValidated(request.provider(), request.model(), request.deploy(), request.environment()); @@ -193,6 +203,11 @@ public SetupResult setupAgent(SetupAgentRequest request) throws AgentSetupExcept // --- Step 7: Create Agent --- var agentConfig = new AgentConfiguration(); agentConfig.setWorkflows(List.of(URI.create(workflowLocation))); + // The gate is installed on v1 of the agent document. It has to be created + // WITH the agent rather than PUT afterwards: an update writes version + 1 and + // leaves the ungated v1 reachable by a redeploy, so a two-step provision would + // ship an agent that can be returned to an ungated state. + agentConfig.setHitlConfig(request.hitlConfig()); Response agentResponse = getRestStore(IRestAgentStore.class).createAgent(agentConfig); String agentLocation = agentResponse.getHeaderString("Location"); String agentId = extractIdFromLocation(agentLocation); @@ -378,16 +393,6 @@ public SetupResult createApiAgent(CreateApiAgentRequest request) throws AgentSet } } - /** - * 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". - *

- * Shared by {@code setupAgent} and {@code createApiAgent} so an API agent can - * hold both the tools generated from its OpenAPI spec and an MCP server's β€” - * previously only the former, which made "REST plus MCP" unreachable through - * the wizard. - */ /** * Validates every MCP server URL before any of them is written. *

@@ -415,6 +420,17 @@ private void validateMcpServerUrls(String mcpServerUrls) throws AgentSetupExcept } } + /** + * 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". + *

+ * Shared by {@code setupAgent} and {@code createApiAgent} so an API agent can + * hold both the tools generated from its OpenAPI spec and an MCP server's β€” + * previously only the former, which made "REST plus MCP" unreachable through + * the wizard. + */ + private List createMcpCallsResources(String mcpServerUrls, String agentName, Map createdResources) throws Exception { if (mcpServerUrls == null || mcpServerUrls.isBlank()) { diff --git a/src/main/java/ai/labs/eddi/engine/setup/SetupAgentRequest.java b/src/main/java/ai/labs/eddi/engine/setup/SetupAgentRequest.java index ccd711a60c..e6b190cb6c 100644 --- a/src/main/java/ai/labs/eddi/engine/setup/SetupAgentRequest.java +++ b/src/main/java/ai/labs/eddi/engine/setup/SetupAgentRequest.java @@ -4,6 +4,7 @@ */ package ai.labs.eddi.engine.setup; +import ai.labs.eddi.configs.agents.model.AgentConfiguration; import com.fasterxml.jackson.annotation.JsonAlias; import com.fasterxml.jackson.annotation.JsonProperty; @@ -17,5 +18,23 @@ public record SetupAgentRequest(@JsonProperty(required = true) @JsonAlias("name") String agentName, @JsonProperty(required = true) String systemPrompt, String provider, String model, String apiKey, String baseUrl, String introMessage, Boolean enableBuiltInTools, String builtInToolsWhitelist, - Boolean enableQuickReplies, Boolean enableSentimentAnalysis, String mcpServerUrls, Boolean deploy, String environment) { + Boolean enableQuickReplies, Boolean enableSentimentAnalysis, String mcpServerUrls, Boolean deploy, String environment, + /* + * HITL configuration for the created agent β€” the exact counterpart of + * CreateApiAgentRequest.hitlConfig, added for the identical reason: without it + * this path could only ever build a bare AgentConfiguration, so every standard + * agent it created had hitlConfig == null and an inert gate. + * + * Deliberately NOT exposed on the MCP setup_agent tool, for the same reason + * create_api_agent's is not: this path already lets the caller choose the + * created agent's own tool surface (enableBuiltInTools, builtInToolsWhitelist, + * mcpServerUrls), so also letting it choose that agent's gate would let a + * caller build an ungated agent at will. McpSetupTools and CreateSubAgentTool + * both pass null. + * + * Appended last, matching CreateApiAgentRequest's own convention β€” every + * positional-constructor call site adds new fields at the end so existing + * argument positions never shift. + */ + AgentConfiguration.HitlConfig hitlConfig) { } diff --git a/src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java b/src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java index 15d9818cdc..241d64244d 100644 --- a/src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java +++ b/src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java @@ -18,6 +18,7 @@ import ai.labs.eddi.engine.runtime.IRuntime; import ai.labs.eddi.modules.llm.tools.UrlValidationUtils; import ai.labs.eddi.modules.templating.ITemplatingEngine; +import ai.labs.eddi.utils.LogSanitizer; import ai.labs.eddi.secrets.SecretResolver; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; @@ -27,6 +28,7 @@ import java.net.URI; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -82,6 +84,7 @@ public class ApiCallExecutor implements IApiCallExecutor { private final SecretResolver secretResolver; private final CallerIdentityResolver callerIdentityResolver; private final CallerIdentityContext callerIdentityContext; + private final RequestRedactor requestRedactor; private final boolean ssrfProtectionEnabled; private final long defaultTimeoutInMillis; private final int defaultMaxResponseSizeInBytes; @@ -89,7 +92,7 @@ public class ApiCallExecutor implements IApiCallExecutor { @Inject public ApiCallExecutor(IHttpClient httpClient, IJsonSerialization jsonSerialization, IRuntime runtime, PrePostUtils prePostUtils, GlobalVariableResolver globalVariableResolver, SecretResolver secretResolver, CallerIdentityResolver callerIdentityResolver, - CallerIdentityContext callerIdentityContext, + CallerIdentityContext callerIdentityContext, RequestRedactor requestRedactor, @ConfigProperty(name = "eddi.security.ssrf-protection.enabled", defaultValue = "false") boolean ssrfProtectionEnabled, @ConfigProperty(name = "eddi.httpcalls.default-timeout-millis", defaultValue = "30000") long defaultTimeoutInMillis, @ConfigProperty(name = "eddi.httpcalls.default-max-response-size-bytes", defaultValue = "2000000") int defaultMaxResponseSizeInBytes) { @@ -101,6 +104,7 @@ public ApiCallExecutor(IHttpClient httpClient, IJsonSerialization jsonSerializat this.secretResolver = secretResolver; this.callerIdentityResolver = callerIdentityResolver; this.callerIdentityContext = callerIdentityContext; + this.requestRedactor = requestRedactor; this.ssrfProtectionEnabled = ssrfProtectionEnabled; this.defaultTimeoutInMillis = defaultTimeoutInMillis; this.defaultMaxResponseSizeInBytes = defaultMaxResponseSizeInBytes; @@ -142,11 +146,13 @@ public Map execute(ApiCall call, IConversationMemory memory, Map request = buildRequest(targetServerUrl, call, templateDataObjects); var objectName = call.getName() + "Request"; var requestMap = request.toMap(); - // Scrub resolved secrets from request map before persisting to conversation - // memory. - // The actual request (with secrets) was already built β€” this only affects the - // debug record. - scrubSensitiveHeaders(requestMap); + // Scrub resolved secrets β€” headers, query parameters and body β€” from + // the request map before it is persisted to conversation memory. The + // actual request was already built and still carries them; each entry + // here is REPLACED with a redacted copy, so this only affects the debug + // record. Shares RequestRedactor with the approval preview so the two + // cannot disagree about what counts as a credential. + requestRedactor.redactRequestMap(requestMap); prePostUtils.createMemoryEntry(currentStep, requestMap, objectName, KEY_HTTP_CALLS); response = executeAndMeasureRequest(call, request, retryCall, amountOfExecutions); @@ -247,6 +253,149 @@ public Map execute(ApiCall call, IConversationMemory memory, Map } } + @Override + @SuppressWarnings("unchecked") + public ResolvedRequest resolve(ApiCall call, IConversationMemory memory, Map templateDataObjects, String targetServerUrl) + throws LifecycleException { + if (call == null) { + throw new IllegalArgumentException("call cannot be null"); + } + if (memory == null) { + throw new IllegalArgumentException("memory cannot be null"); + } + if (templateDataObjects == null) { + throw new IllegalArgumentException("templateDataObjects cannot be null"); + } + if (targetServerUrl == null || targetServerUrl.trim().isEmpty()) { + throw new IllegalArgumentException("targetServerUrl cannot be null or empty"); + } + + try { + // Note the absence of executePreRequestPropertyInstructions: it writes + // to conversation memory, and previewing a call must not change the + // conversation. See IApiCallExecutor#resolve for what that costs. + var requestMap = buildRequest(targetServerUrl, call, templateDataObjects).toMap(); + var headers = requestMap.get(IRequest.KEY_HEADERS) instanceof Map h ? (Map) h : Map.of(); + var queryParams = normalizeQueryParams(requestMap.get(IRequest.KEY_QUERY_PARAMS)); + Object body = requestMap.get(IRequest.KEY_BODY); + + // The RAW body goes in: ResolvedRequest redacts it for display itself, + // while fingerprinting what was actually resolved. Redacting here + // instead would fingerprint the redacted form and make two different + // credentials hash identically β€” see ResolvedRequest#of. + return ResolvedRequest.of( + String.valueOf(requestMap.get(IRequest.KEY_METHOD)), + String.valueOf(requestMap.get(IRequest.KEY_URI)), + queryParams, + requestRedactor.redactHeaders(headers), + body == null ? null : body.toString(), + !canExecuteDivergeFromResolve(call)); + } catch (Exception e) { + // Deliberately NOT logged here β€” throw only. Unlike execute(), the sole + // caller of resolve() is the gate-time/pre-execution pinning path, which + // catches this and logs it with the severity the situation actually has: + // a WARN saying the call will be approved unpinned (a documented, benign + // degrade) or that execution is refused. An ERROR here would double every + // one of those lines and label a normal degrade as breakage. + // + // The message is generic and the cause is attached rather than unwrapped: + // a failure here comes out of template rendering or request building, + // whose messages quote the material being rendered β€” Jackson appends the + // offending source verbatim β€” so putting it in a log or an exception + // message would leak the credential from the very operation whose job is + // to show the approver a REDACTED request. + throw new LifecycleException( + "could not resolve the request for ApiCall '" + LogSanitizer.sanitize(call.getName()) + "'", e); + } + } + + /** + * Read the query parameters out of {@link IRequest#toMap()} without assuming + * their shape. + *

+ * {@code HttpClientWrapper} accumulates repeats, so the values are lists β€” + * casting the map to {@code Map} compiles, erases cleanly, and + * then throws a {@link ClassCastException} deep in the fingerprint + * canonicaliser. The gate-time caller catches that and approves the call + * unpinned, so the failure is silent and pinning simply stops applying + * to every endpoint that carries a query parameter. A single-valued map is + * still accepted, because this interface has other implementations and the + * contract has been ambiguous. + */ + private static Map> normalizeQueryParams(Object rawQueryParams) { + if (!(rawQueryParams instanceof Map params)) { + return Map.of(); + } + var normalized = new LinkedHashMap>(); + for (var entry : params.entrySet()) { + String name = String.valueOf(entry.getKey()); + Object value = entry.getValue(); + if (value instanceof List values) { + normalized.put(name, values.stream().map(v -> v == null ? "" : v.toString()).toList()); + } else { + normalized.put(name, List.of(value == null ? "" : value.toString())); + } + } + return normalized; + } + + /** + * Whether {@link #execute} can build a request this method's caller did not + * resolve β€” the question a fingerprint's soundness actually turns on. + *

+ * It used to ask something narrower ("does this call have pre-request property + * instructions"), and the gap between the two questions was the fail-open in + * the pinning design. Each miss below produces a call that is PINNED, whose + * gate-time and pre-execution resolutions agree with each other β€” because both + * skip the divergence β€” while {@code execute} sends something else entirely. + * The guard then passes on a comparison that was never sound: + *

    + *
  • An empty-but-present {@code propertyInstructions} list. + * {@code isNullOrEmpty} treated it as absent, but + * {@code PrePostUtils#executePreRequestPropertyInstructions} guards on + * {@code != null} β€” so it still re-runs {@code memoryItemConverter.convert}, + * discarding the model arguments merged in for this call. Every {@code {arg}} + * then renders empty at execution and non-empty in the preview. Hence + * {@code != null}, matching the code that actually runs.
  • + *
  • {@code fireAndForget} with {@code preRequest.batchRequests}. + * {@code execute} routes to {@code executeFireAndForgetCalls}, which calls + * {@code buildRequest} once PER iteration object β€” N distinct requests, none of + * them the single one {@code resolve} builds (the iteration variable renders + * empty there). {@code batchRequests} is a different field from + * {@code propertyInstructions}, so this was pinned, and an approver shown one + * request authorised N unreviewed ones.
  • + *
+ * Returning true here means unpinnable, not refused: the call still needs its + * approval, it is previewed best-effort, and only the fingerprint enforcement + * is skipped β€” which is the honest state for a request we genuinely cannot pin, + * rather than a pin we cannot honour. + */ + private static boolean canExecuteDivergeFromResolve(ApiCall call) { + var preRequest = call.getPreRequest(); + if (preRequest != null && preRequest.getPropertyInstructions() != null) { + return true; + } + // One resolved request cannot stand for N. Guarded on fireAndForget too + // because that is what selects the batching branch in execute(). + if (Boolean.TRUE.equals(call.getFireAndForget()) && preRequest != null && preRequest.getBatchRequests() != null) { + return true; + } + // Same argument, different loop: buildRequest sits INSIDE execute()'s + // retry do-while, and between attempts the shared templateDataObjects + // map gains {responseObjectName}, …Error, …HttpCode and the response + // headers. A call whose path, body or headers template any of those + // sends attempts 2..N as requests that were never resolved, never + // previewed and never fingerprinted β€” while the approver saw only + // attempt 1. Keyed on the instruction being present and actually able + // to fire (maxRetries >= 1), which is exactly what retryCall() tests. + var postResponse = call.getPostResponse(); + if (postResponse instanceof ai.labs.eddi.configs.apicalls.model.HttpPostResponse httpPostResponse) { + var retry = httpPostResponse.getRetryApiCallInstruction(); + return retry != null && retry.getMaxRetries() >= 1; + } + return false; + } + private IResponse executeAndMeasureRequest(ApiCall call, IRequest request, boolean retryCall, int amountOfExecutions) throws IRequest.HttpRequestException, ExecutionException, InterruptedException { @@ -532,39 +681,4 @@ private IRequest buildRequest(String targetServerUrl, ApiCall call, Map - * Header-name matching only catches conventional names, so a resolved caller - * token is additionally matched by value β€” otherwise placing it in an - * arbitrarily named header would defeat the redaction. - */ - @SuppressWarnings("unchecked") - private void scrubSensitiveHeaders(Map requestMap) { - Object headersObj = requestMap.get("headers"); - if (headersObj instanceof Map) { - var headers = (Map) headersObj; - var scrubbed = new HashMap<>(headers); - for (var entry : scrubbed.entrySet()) { - // Locale.ROOT, not the default locale: under a Turkish locale - // "Authorization" lowercases to "authorΔ±zation" (dotless i), every - // name test below misses, and the header is persisted unredacted. - String headerName = entry.getKey().toLowerCase(Locale.ROOT); - if (headerName.contains("authorization") || headerName.contains("api-key") || headerName.contains("api_key") - || headerName.contains("apikey") || headerName.contains("x-api-key") || headerName.contains("token") - || headerName.contains("secret") || headerName.contains("credential")) { - entry.setValue(""); - } else if (entry.getValue() instanceof String val && (val.contains("${vault:") || val.contains("${eddivault:"))) { - entry.setValue(""); - } else if (entry.getValue() instanceof String val) { - // Catches a caller token placed in an unconventionally named - // header, which the name patterns above would miss. - entry.setValue(callerIdentityResolver.redactCallerToken(val, "")); - } - } - requestMap.put("headers", scrubbed); - } - } } diff --git a/src/main/java/ai/labs/eddi/modules/apicalls/impl/IApiCallExecutor.java b/src/main/java/ai/labs/eddi/modules/apicalls/impl/IApiCallExecutor.java index 58f915b516..841b966fe0 100644 --- a/src/main/java/ai/labs/eddi/modules/apicalls/impl/IApiCallExecutor.java +++ b/src/main/java/ai/labs/eddi/modules/apicalls/impl/IApiCallExecutor.java @@ -33,4 +33,29 @@ public interface IApiCallExecutor { */ Map execute(ApiCall httpCall, IConversationMemory memory, Map templateDataObjects, String targetServerUrl) throws LifecycleException; + + /** + * Resolves what this call would send, without sending it. + *

+ * Exists so a human approving a gated tool call can be shown the actual request + * β€” method, target, query, body β€” rather than the tool's name and the model's + * raw arguments, and so the approved request can be pinned to a fingerprint + * that is re-checked immediately before execution. + *

+ * Side-effect free, and deliberately weaker than {@link #execute} because of + * it. {@code execute} first runs the call's pre-request property + * instructions, which write to conversation memory; running those here would + * apply them twice β€” once to preview a call and again to make it. So they are + * skipped, and a call that has them cannot be resolved to the same request + * {@code execute} will build. Such a call comes back with a null + * {@link ResolvedRequest#fingerprint()}: the preview is still useful, but + * nothing is pinned and the pre-execution check has nothing to compare. Tools + * generated from an OpenAPI spec never carry pre-request instructions, so they + * are always pinned. + * + * @return the resolved request with every credential redacted β€” never the live + * header values + */ + ResolvedRequest resolve(ApiCall httpCall, IConversationMemory memory, Map templateDataObjects, String targetServerUrl) + throws LifecycleException; } diff --git a/src/main/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactor.java b/src/main/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactor.java new file mode 100644 index 0000000000..576a4630c5 --- /dev/null +++ b/src/main/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactor.java @@ -0,0 +1,345 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.modules.apicalls.impl; + +import ai.labs.eddi.engine.httpclient.IRequest; +import ai.labs.eddi.engine.security.CallerIdentityResolver; +import ai.labs.eddi.secrets.sanitize.SecretRedactionFilter; +import jakarta.enterprise.context.ApplicationScoped; +import jakarta.inject.Inject; + +import java.util.HashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; + +/** + * Removes credential material from a resolved request β€” headers, query + * parameters and body alike. + *

+ * One definition, two consumers: the debug record written to conversation + * memory and the approval preview shown to a human. They must not drift β€” a + * part redacted in one and not the other is a credential leak through whichever + * path was forgotten. Each of the three has been that leak at some point, which + * is why they are all defined here rather than at the call sites. + */ +@ApplicationScoped +public class RequestRedactor { + + /** What a redacted value is replaced with. */ + public static final String REDACTED = ""; + + private final CallerIdentityResolver callerIdentityResolver; + + @Inject + public RequestRedactor(CallerIdentityResolver callerIdentityResolver) { + this.callerIdentityResolver = callerIdentityResolver; + } + + /** + * Whether a header carries credential material, judged by its name. + *

+ * {@code Locale.ROOT}, not the default locale: under a Turkish locale + * "Authorization" lowercases to "authorΔ±zation" (dotless i), every test below + * misses, and the header is persisted unredacted. + */ + public static boolean isSensitiveHeaderName(String headerName) { + if (headerName == null) { + return false; + } + String name = headerName.toLowerCase(Locale.ROOT); + return name.contains("authorization") || name.contains("api-key") || name.contains("api_key") || name.contains("apikey") + || name.contains("x-api-key") || name.contains("token") || name.contains("secret") || name.contains("credential") + || name.contains("password"); + } + + /** + * Redact one header value. + *

+ * Name matching only catches conventional names, so an unresolved vault + * reference and a resolved caller token are additionally matched by value β€” + * otherwise placing either in an arbitrarily named header would defeat the + * redaction entirely. + *

+ * The value-shape scan is the last step, and it is the one that closes the + * asymmetry this class kept for a while: a query parameter and a body both ran + * through {@link SecretRedactionFilter}, and a header did not. So + * {@code X-Client-Auth: Bearer eyJhbGciOi…} β€” a name matching none of the + * conventional patterns, a value that is not a vault reference and not the + * current caller's token β€” was stored and shown to an approver in full, while + * the identical string one field away in the body was caught. The shape is + * recognisable and the filter already existed; only the wiring was missing. + *

+ * Note this makes header redaction slightly more aggressive, and headers are + * deliberately fingerprinted in their REDACTED form (see + * {@link ResolvedRequest}). Two different secret-shaped values under the same + * header therefore hash alike β€” but that is the pre-existing, documented + * trade-off for headers, not a new one: a caller token legitimately differs + * between requester and approver, so header values were already excluded from + * change detection. Gate time and resume time run this same code, so they + * continue to agree. + */ + public String redactHeaderValue(String headerName, Object headerValue) { + if (isSensitiveHeaderName(headerName)) { + return REDACTED; + } + if (headerValue instanceof String value) { + if (value.contains("${vault:") || value.contains("${eddivault:")) { + return REDACTED; + } + return redactBody(callerIdentityResolver.redactCallerToken(value, REDACTED)); + } + return headerValue == null ? null : headerValue.toString(); + } + + /** Redact every header in a name-to-value map. */ + public Map redactHeaders(Map headers) { + var redacted = new HashMap(); + if (headers == null) { + return redacted; + } + for (var entry : headers.entrySet()) { + redacted.put(entry.getKey(), redactHeaderValue(entry.getKey(), entry.getValue())); + } + return redacted; + } + + /** + * Redact secret-shaped values out of a request body. + *

+ * A body has no fixed key vocabulary to check by name the way headers do β€” it + * is caller-defined JSON, or another format entirely β€” so this scans by VALUE + * SHAPE via {@link SecretRedactionFilter} instead: an OpenAI/Anthropic style + * key, a bearer token, or a vault reference is redacted wherever it appears, + * independent of which field it sits under. A hand-rolled secret in a + * generically named field with none of those shapes is not caught β€” the same + * limitation this filter already accepts for LLM tool-call arguments + * ({@code PendingToolCallBatch.PendingToolCall#argumentsRedacted}); reusing it + * here keeps the two consistent rather than inventing a second, differently + * effective scheme for the same class of data. + *

+ * Static, unlike the header methods, because it needs no injected state β€” and + * so that {@link ResolvedRequest#of} can reach it without an executor. That + * matters for the class invariant above: this stays the one definition + * of "redacted body" across both consumers. + */ + public static String redactBody(String body) { + return SecretRedactionFilter.redact(body); + } + + /** + * Redact a query parameter's value. + *

+ * Judged by name like a header, and for the same reason: {@code ?api_key=…} or + * {@code ?access_token=…} is a conventional way to pass a credential, and this + * value is shown to an approver who is routinely not the person whose turn + * raised the pause. Value-shape matching backs the name check up so a + * credential under an unconventional name is still caught. + */ + public static String redactQueryParamValue(String name, String value) { + if (isSensitiveHeaderName(name)) { + return REDACTED; + } + if (value == null) { + return ""; + } + if (value.contains("${vault:") || value.contains("${eddivault:")) { + return REDACTED; + } + // No caller-token check, unlike a header: CallerIdentityResolver rejects + // ${caller:token} outside headers outright, so a live caller token cannot + // legitimately reach a query parameter. That is what lets this stay static + // β€” and static is what lets ResolvedRequest#of apply it itself, keeping + // "fingerprint the raw, store the redacted" in one place. + return redactBody(value); + } + + /** + * Shape-scan the part of a URI before any query string, WITHOUT letting the + * scan eat the authority. + *

+ * {@code SecretRedactionFilter}'s generic rule matches + * {@code (api_key|token|secret|password|authorization)[=:]<8+ chars>}, and its + * trailing character class does not exclude {@code /}. Run over a whole URI + * that scan consumes to the end of the string the moment the host itself ends + * in one of those words followed by a port β€” plausible for an in-cluster + * service name β€” so {@code https://vault-secret:8200/v1/agents/a1} collapsed to + * {@code https://vault-secret=}. That is worse than the leak it + * guards: the approver loses the method's target entirely, and what is left is + * not even a URI. Over-redaction hides what is being written to; a human who + * cannot see the target cannot approve it. + *

+ * So the scheme and authority are held aside and the scan is applied only to + * the path, where a templated credential can actually land. + */ + private static String redactUpToQuery(String beforeQuery) { + int schemeEnd = beforeQuery.indexOf("://"); + if (schemeEnd < 0) { + // Relative or scheme-less: it is all path. + return redactBody(beforeQuery); + } + int authorityStart = schemeEnd + 3; + int pathStart = beforeQuery.indexOf('/', authorityStart); + String authority = pathStart < 0 ? beforeQuery.substring(authorityStart) : beforeQuery.substring(authorityStart, pathStart); + String path = pathStart < 0 ? "" : beforeQuery.substring(pathStart); + + // The authority is kept verbatim EXCEPT its userinfo: `user:sk-…@host` + // really does carry a credential, and it is bounded by '@', so scanning + // it cannot run away into the host and path the way scanning the whole + // authority did. Everything from '@' onward (host, port) stays legible. + int at = authority.lastIndexOf('@'); + String safeAuthority = at < 0 ? authority : redactBody(authority.substring(0, at)) + authority.substring(at); + + return beforeQuery.substring(0, authorityStart) + safeAuthority + redactBody(path); + } + + /** + * Redact a request URI. + *

+ * The URI was the one field of a resolved request that carried no redaction of + * any kind, which made it the leak the rest of this class exists to prevent: a + * credential templated into the path β€” + * {@code "/v1/invoices?api_key=${vault:k}"} β€” is resolved to its live value by + * {@code ApiCallExecutor#buildRequest} before the URI is ever built, and the + * same value then appeared REDACTED in {@code queryParams} and PLAINTEXT in + * {@code uri}, adjacent fields of one JSON object shown to an approver who is + * routinely not the person whose turn raised the pause. + *

+ * Two passes, because a URI has two places to hide one: + *

    + *
  • the query string is split and each value run through + * {@link #redactQueryParamValue} β€” the SAME function the {@code queryParams} + * map uses, so the two views of one credential cannot disagree;
  • + *
  • whatever remains (scheme, userinfo, host, path) goes through + * {@link #redactBody}'s value-shape scan, which catches + * {@code https://user:sk-…@host} and a key segment inside a path.
  • + *
+ *

+ * Static and null-tolerant for the same reason as {@link #redactBody}: + * {@link ResolvedRequest#of} applies it without an executor, keeping + * "fingerprint the raw, store the redacted" resolved in exactly one place. + */ + + /** + * Redact one query value from a URI, judging it in its DECODED form. + *

+ * The scan has to see what the value actually is. {@code HttpClientWrapper} + * decodes into {@code queryParamsMap}, but {@code toMap()} hands back the raw + * {@code uri.toString()} β€” so the same credential arrives here still encoded, + * and percent-encoding defeats the shape rules outright: a bearer token becomes + * {@code Bearer%20aaaa…}, which the {@code Bearer\s+…} rule no longer matches, + * and a {@code ${vault:…}} reference survives as {@code $%7Bvault%3A…}. The + * result was a credential redacted in {@code queryParams} and plaintext one + * field away in {@code uri} β€” the exact pair of adjacent contradictory fields + * this method was added to stop. + *

+ * The ORIGINAL value is emitted when nothing matched, so the preview keeps + * showing what is genuinely on the wire; only a value the scan actually hit is + * replaced. A malformed escape falls back to scanning the raw form rather than + * skipping the check. + */ + private static String redactQueryValueDecoded(String name, String rawValue) { + String decoded = rawValue; + try { + decoded = java.net.URLDecoder.decode(rawValue, java.nio.charset.StandardCharsets.UTF_8); + } catch (IllegalArgumentException malformedEscape) { + // Keep the raw form β€” an unparseable escape is no reason to skip the scan. + } + String redacted = redactQueryParamValue(name, decoded); + return redacted.equals(decoded) ? rawValue : redacted; + } + + public static String redactUri(String uri) { + if (uri == null) { + return null; + } + int queryStart = uri.indexOf('?'); + if (queryStart < 0) { + return redactUpToQuery(uri); + } + String beforeQuery = redactUpToQuery(uri.substring(0, queryStart)); + String query = uri.substring(queryStart + 1); + // Preserve the fragment: it is not a query parameter and splitting on '&' + // would otherwise fold it into the last value. + String fragment = ""; + int fragmentStart = query.indexOf('#'); + if (fragmentStart >= 0) { + fragment = redactBody(query.substring(fragmentStart)); + query = query.substring(0, fragmentStart); + } + var redactedQuery = new StringBuilder(); + for (String pair : query.split("&", -1)) { + if (!redactedQuery.isEmpty()) { + redactedQuery.append('&'); + } + int eq = pair.indexOf('='); + if (eq < 0) { + // A valueless flag carries no credential to redact, but could still + // BE one (?sk-live-…), so it is shape-scanned like anything else. + redactedQuery.append(redactBody(pair)); + continue; + } + String name = pair.substring(0, eq); + redactedQuery.append(name).append('=').append(redactQueryValueDecoded(name, pair.substring(eq + 1))); + } + return beforeQuery + "?" + redactedQuery + fragment; + } + + /** + * Redact the {@link IRequest#KEY_URI}, {@link IRequest#KEY_HEADERS}, + * {@link IRequest#KEY_QUERY_PARAMS} and {@link IRequest#KEY_BODY} entries of a + * request map, as produced by {@link IRequest#toMap()}. + *

+ * Each entry is REPLACED with a redacted copy rather than rewritten in place. + * That distinction is load-bearing for the query parameters: + * {@code HttpClientWrapper.RequestWrapper#toMap} hands back its live + * {@code queryParamsMap} rather than a copy, so mutating the nested map would + * corrupt the request that is about to be sent β€” while swapping the entry in + * this (freshly built) outer map cannot. + */ + @SuppressWarnings("unchecked") + public void redactRequestMap(Map requestMap) { + if (requestMap == null) { + return; + } + // The KEY_* constants, not string literals: this map's shape is + // IRequest#toMap's contract, and a redactor that spells the keys itself is + // one rename away from silently redacting nothing. + if (requestMap.get(IRequest.KEY_URI) instanceof String uri) { + requestMap.put(IRequest.KEY_URI, redactUri(uri)); + } + if (requestMap.get(IRequest.KEY_HEADERS) instanceof Map headers) { + requestMap.put(IRequest.KEY_HEADERS, redactHeaders((Map) headers)); + } + if (requestMap.get(IRequest.KEY_QUERY_PARAMS) instanceof Map queryParams) { + requestMap.put(IRequest.KEY_QUERY_PARAMS, redactQueryParams((Map) queryParams)); + } + if (requestMap.get(IRequest.KEY_BODY) instanceof String body) { + requestMap.put(IRequest.KEY_BODY, redactBody(body)); + } + } + + /** + * Redact a query-parameter map, preserving its multi-valued shape. + *

+ * Values arrive as {@code List} from the default implementation but a + * bare value is tolerated, for the same reason + * {@code ApiCallExecutor#normalizeQueryParams} tolerates both. + */ + public static Map redactQueryParams(Map queryParams) { + var redacted = new HashMap(); + if (queryParams == null) { + return redacted; + } + queryParams.forEach((name, value) -> { + if (value instanceof List values) { + redacted.put(name, values.stream().map(v -> redactQueryParamValue(name, v == null ? null : v.toString())).toList()); + } else { + redacted.put(name, redactQueryParamValue(name, value == null ? null : value.toString())); + } + }); + return redacted; + } +} diff --git a/src/main/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequest.java b/src/main/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequest.java new file mode 100644 index 0000000000..05f25a6eae --- /dev/null +++ b/src/main/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequest.java @@ -0,0 +1,197 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.modules.apicalls.impl; + +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.TreeMap; +import java.util.stream.Collectors; + +/** + * The HTTP request an {@code ApiCall} resolves to, with every credential + * already redacted, plus a fingerprint re-checked before execution. + *

+ * This is what a human approves. Approving a tool name is close to + * meaningless for a generated API client: the name comes from an + * {@code operationId} and says nothing about which resource is being written or + * with what body. The approver sees this, and {@link #fingerprint()} is + * re-derived immediately before execution so what runs is what was approved. + * + *

Headers: fingerprinted redacted. Body: fingerprinted raw.

+ * + * The asymmetry is deliberate, and it is not a compromise on either side. + *

+ * Headers are fingerprinted in their redacted form because + * {@code ApiCallExecutor} resolves {@code ${caller:token}} into the + * {@code Authorization} header, and on a resumed turn the caller is whoever + * approved the pause, who is routinely not the person whose turn + * raised it. Fingerprinting the live header would mismatch on every cross-user + * approval β€” the normal, desirable case β€” and the guard would fire constantly + * on correct behaviour until someone disabled it. Whose credentials + * carry a request is governed by authentication, not approval, and deliberately + * does not participate. + *

+ * Bodies have no such legitimate variance: {@code ${caller:token}} is + * rejected outside headers, and a {@code ${vault:...}} reference resolves to + * the same value at gate time and at execution. So the body is hashed as + * resolved and only the stored copy is redacted β€” {@link #of} does that itself + * so no call site can get the order wrong. Redacting first would collapse two + * different credentials to one marker and so to one fingerprint, + * letting a swapped secret pass the pre-execution check unnoticed. The + * fingerprint is never exposed to any client, so hashing the raw body reveals + * nothing. + */ +public record ResolvedRequest( + String method, + String uri, + Map queryParams, + Map headers, + String body, + String fingerprint) { + + /** + * Build a resolved request: fingerprint the raw body, store a redacted one. + * + * @param redactedHeaders + * already redacted by the caller, which owns the injected + * {@code CallerIdentityResolver} needed to match a live caller token + * by value. + * @param rawBody + * the body as resolved. Redacted here rather than by the + * caller so that {@link #body()} is always safe to display and the + * fingerprint always covers what will actually be sent β€” see the + * class javadoc for why those must be the two different forms. + * @param fingerprintable + * false when this call cannot be resolved ahead of execution without + * side effects β€” see {@link IApiCallExecutor#resolve}. The preview + * is still produced; {@link #fingerprint()} is null, and enforcement + * is skipped rather than failing a call it cannot honestly pin. + */ + public static ResolvedRequest of(String method, String uri, Map> queryParams, Map redactedHeaders, + String rawBody, boolean fingerprintable) { + + var sortedQuery = sorted(queryParams); + var sortedHeaders = sortedByLowercasedName(redactedHeaders); + // Fingerprint the RAW uri, store the REDACTED one β€” the same order, and for + // the same reason, as the body below: two different credentials in the same + // query position must not collapse to one marker before hashing, or swapping + // one for the other would pass the pre-execution re-check as "unchanged". + String fingerprint = fingerprintable ? fingerprintOf(method, uri, sortedQuery, sortedHeaders, rawBody) : null; + return new ResolvedRequest(method, RequestRedactor.redactUri(uri), displayQuery(sortedQuery), sortedHeaders, + RequestRedactor.redactBody(rawBody), fingerprint); + } + + /** Whether this request was pinned to a fingerprint at gate time. */ + public boolean isPinned() { + return fingerprint != null; + } + + /** + * SHA-256 over a length-prefixed canonical encoding. + *

+ * Length prefixes rather than plain delimiters because a JSON body can contain + * any separator we might pick: without them, moving a newline from a body into + * a header value could produce two different requests with one fingerprint. + * Header names are lowercased and both maps sorted, so ordering and casing β€” + * neither of which changes what the request does β€” cannot change the hash. + */ + private static String fingerprintOf(String method, String uri, Map> queryParams, Map headers, + String body) { + + var canonical = new StringBuilder(); + appendField(canonical, "method", method == null ? "" : method.toUpperCase(Locale.ROOT)); + appendField(canonical, "uri", uri); + for (var entry : queryParams.entrySet()) { + // One field per value, indexed: a query parameter may legitimately + // repeat (?tag=a&tag=b), and joining the values into one string would + // let a single value containing the separator impersonate two β€” the + // same field-boundary forgery the length prefixes exist to stop. + var values = entry.getValue(); + for (int i = 0; i < values.size(); i++) { + appendField(canonical, "query." + entry.getKey() + "[" + i + "]", values.get(i)); + } + } + for (var entry : headers.entrySet()) { + appendField(canonical, "header." + entry.getKey(), entry.getValue()); + } + appendField(canonical, "body", body); + + try { + var digest = MessageDigest.getInstance("SHA-256"); + byte[] hash = digest.digest(canonical.toString().getBytes(StandardCharsets.UTF_8)); + var hex = new StringBuilder(hash.length * 2); + for (byte b : hash) { + hex.append(Character.forDigit((b >> 4) & 0xF, 16)).append(Character.forDigit(b & 0xF, 16)); + } + return hex.toString(); + } catch (NoSuchAlgorithmException e) { + // SHA-256 is mandated by the JLS for every conforming JVM. + throw new IllegalStateException("SHA-256 is unavailable", e); + } + } + + private static void appendField(StringBuilder canonical, String name, String value) { + String safe = value == null ? "" : value; + canonical.append(name).append(':').append(safe.length()).append(':').append(safe).append('\n'); + } + + /** + * Sort by name and normalise each value list. + *

+ * Takes the multi-valued shape {@code IRequest#toMap} actually produces + * ({@code HttpClientWrapper} accumulates repeats into a list), rather than the + * single-valued one it is tempting to assume: an unchecked cast to + * {@code Map} erases cleanly and then throws a + * {@link ClassCastException} in here, which the gate-time caller catches and + * downgrades to "approved unpinned" β€” silently disabling pinning for every + * endpoint carrying a query parameter. + */ + private static Map> sorted(Map> values) { + var result = new TreeMap>(); + if (values == null) { + return result; + } + values.forEach((key, value) -> { + if (value == null || value.isEmpty()) { + // A present-but-valueless parameter (?flag) is not the same request + // as one that is absent, so it is kept as a single empty value. + result.put(key, List.of("")); + } else { + result.put(key, value.stream().map(v -> v == null ? "" : v).toList()); + } + }); + return result; + } + + /** + * The display form of the query parameters β€” one redacted string per name, + * repeats joined. + *

+ * Redacted here and hashed raw above, for the same reason the body is: a + * credential does show up in a query string ({@code ?api_key=…}), the approver + * must not be shown it, and collapsing two different credentials to one marker + * before hashing would let a swapped one pass the pre-execution re-check. The + * join is display-only and cannot weaken the fingerprint, which uses the + * per-value structured form. + */ + private static Map displayQuery(Map> queryParams) { + var result = new TreeMap(); + queryParams.forEach((key, values) -> result.put(key, + values.stream().map(value -> RequestRedactor.redactQueryParamValue(key, value)).collect(Collectors.joining(", ")))); + return result; + } + + private static Map sortedByLowercasedName(Map values) { + var result = new TreeMap(); + if (values != null) { + values.forEach((key, value) -> result.put(key == null ? "" : key.toLowerCase(Locale.ROOT), value == null ? "" : value)); + } + return result; + } +} diff --git a/src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java b/src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java index e08b860d8b..f2a98a7539 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java +++ b/src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java @@ -43,6 +43,7 @@ import ai.labs.eddi.engine.setup.AgentSetupService; import com.fasterxml.jackson.core.io.JsonStringEncoder; import ai.labs.eddi.modules.apicalls.impl.IApiCallExecutor; +import ai.labs.eddi.modules.apicalls.impl.ResolvedRequest; import ai.labs.eddi.modules.llm.capability.JsonResponseFormatPolicy; import ai.labs.eddi.modules.llm.model.LlmConfiguration; import ai.labs.eddi.modules.llm.model.LlmConfiguration.A2AAgentConfig; @@ -555,9 +556,18 @@ ExecutionResult resumeToolLoop(ChatModel chatModel, LlmConfiguration.Task task, for (PendingToolCallBatch.PendingToolCall c : batch.getCalls()) { ToolCallDecision cd = perCall.get(c.getCallId()); - HitlDecision.HitlVerdict verdict = cd != null && cd.getVerdict() != null ? cd.getVerdict() : topVerdict; + HitlDecision.HitlVerdict resolvedVerdict = cd != null && cd.getVerdict() != null ? cd.getVerdict() : topVerdict; + // Every current caller of resumeConversation validates a non-null verdict + // before this point, so resolvedVerdict should never actually be null β€” + // but the check that guarantees it lives in each caller, not here. Fail + // closed rather than trust that invariant silently: the REJECTED check + // below is the only thing standing between an unresolved verdict and + // executing a gated call, and "not REJECTED" is a dangerous way to spell + // "approved". + HitlDecision.HitlVerdict verdict = resolvedVerdict != null ? resolvedVerdict : HitlDecision.HitlVerdict.REJECTED; String note = cd != null ? cd.getNote() : decision.getNote(); String amended = cd != null ? cd.getAmendedArguments() : null; + recordWriteApprovalDecision(verdict, decision.getDecidedBy()); if (verdict == HitlDecision.HitlVerdict.REJECTED) { currentMessages.add(ToolExecutionResultMessage.from(rebuiltRequest(c), rejectionEnvelope(c.getToolName(), note))); @@ -577,6 +587,19 @@ ExecutionResult resumeToolLoop(ChatModel chatModel, LlmConfiguration.Task task, continue; } + // Approval binds to a REQUEST, not to a tool name: re-resolve now and + // refuse if what is about to be sent is not what was approved. Checked + // before the journal claim so a refusal consumes nothing and stays + // replayable. + String changed = requestChangedSinceApproval(c, amended, setup.toolRequestResolvers()); + if (changed != null) { + auditRequestChanged(memory, c, changed); + currentMessages.add(ToolExecutionResultMessage.from(rebuiltRequest(c), + "{\"status\":\"NOT_EXECUTED\",\"reason\":\"the request changed after it was approved\"}")); + trace.add(Map.of("type", "hitl_request_changed", "tool", c.getToolName(), "callId", c.getCallId(), "detail", changed)); + continue; + } + // Journal protocol β€” at-most-once across crashes/re-approvals. if (journalStore.tryClaim(conversationId, pauseEpoch, c.getCallId(), c.getToolName(), decision.getDecidedBy())) { String args = amended != null ? amended : c.getArgumentsRaw(); @@ -656,6 +679,14 @@ ExecutionResult resumeToolLoop(ChatModel chatModel, LlmConfiguration.Task task, /** Journal-stored result cap (bytes) β€” matches the journal store's own cap. */ private static final int JOURNAL_RESULT_MAX_BYTES = 32_768; + /** + * Cap for tool arguments echoed into a log line. Deliberately small: a log is + * for identifying WHICH call failed to parse, not for reproducing its payload, + * and an unbounded model-supplied string is a log-flooding vector on top of the + * redaction concern. + */ + private static final int ARGS_LOG_MAX_BYTES = 512; + /** * Restores the active-spec surface on resume. For EAGER, every registered spec. * For LAZY, exactly the specs that were active at pause time (by name), falling @@ -820,6 +851,66 @@ private static String toJson(Object value) { } } + /** + * Whether the request this approved call would now send differs from the one + * that was approved β€” the check that makes an approval bind to a request. + * + * @return null when the call may proceed, otherwise a short reason for the + * audit trail and trace + */ + String requestChangedSinceApproval(PendingToolCallBatch.PendingToolCall c, String amendedArguments, + Map resolvers) { + + if (!c.isRequestPinned()) { + // Never pinned, so there is nothing to compare: every non-http tool, and + // any call that could not be resolved ahead of execution. Enforcing here + // would reject calls on a comparison that was never sound. + return null; + } + if (amendedArguments != null) { + // The approver rewrote the arguments themselves. The pinned fingerprint + // describes the request they replaced, so comparing against it would + // refuse every amendment. An amendment is already a deliberate, audited + // act by the same human whose approval the pin exists to honour. + return null; + } + + var resolver = resolvers.get(c.getToolName()); + if (resolver == null) { + // Pinned at gate time and unresolvable now: the tool is gone from the + // workflow, or the agent was reconfigured across the pause. We cannot + // show that what runs is what was approved, so it does not run. + return "the tool is no longer available to re-check the approved request"; + } + try { + ResolvedRequest current = resolver.resolve(rebuiltRequest(c)); + if (current.fingerprint() == null) { + return "the request could no longer be resolved for comparison"; + } + if (!current.fingerprint().equals(c.getRequestFingerprint())) { + return "the resolved request no longer matches the approved fingerprint"; + } + return null; + } catch (Exception e) { + // Fail closed: a pinned call whose request cannot be re-derived is + // exactly the case this check exists for. Type only, no throwable β€” + // see errorType. + LOGGER.warnf("Could not re-resolve the request for approved tool '%s' (%s); refusing to execute it.", sanitize(c.getToolName()), + errorType(e)); + return "the request could not be re-resolved before execution"; + } + } + + /** + * Records that an approved call was refused because its request no longer + * matched. Deliberately logs no argument, body or header β€” only the tool, the + * call id and the fixed reason. + */ + void auditRequestChanged(IConversationMemory memory, PendingToolCallBatch.PendingToolCall c, String reason) { + LOGGER.warnf("hitl.tool.request_changed: approved tool '%s' (callId '%s') for conversation '%s' was NOT executed β€” %s.", + sanitize(c.getToolName()), sanitize(c.getCallId()), sanitize(memory.getConversationId()), sanitize(reason)); + } + /** * Records an at-most-once outcome-unknown event. No lightweight * {@code hitl.tool.*} audit collector is reachable from this task (the @@ -859,7 +950,8 @@ void auditOutcomeUnknown(IConversationMemory memory, PendingToolCallBatch.Pendin */ record ToolSetup(List toolSpecs, Map toolExecutors, Map toolSources, List builtInSpecs, - Map toolCanonicalNames, Map toolEndpoints) { + Map toolCanonicalNames, Map toolEndpoints, + Map toolRequestResolvers) { } /** @@ -928,12 +1020,18 @@ ToolSetup buildToolSetup(LlmConfiguration.Task task, IConversationMemory memory) // Copy built-in specs before merging external ones β€” LAZY activation needs it. List builtInSpecs = new ArrayList<>(toolSpecs); + Map toolRequestResolvers = new HashMap<>(); + // Merge httpcall tools discovered from workflow (if any) if (httpCallTools != null) { mergeExternalTools(httpCallTools.toolSpecs(), httpCallTools.executors(), "http", toolSpecs, toolExecutors, toolSources); // Endpoint provenance travels beside the source so an approval pattern can // address what a tool calls, not just what it is named. toolEndpoints.putAll(httpCallTools.endpoints()); + // Only httpcall tools resolve to an HTTP request, so only they can be + // pinned. Pruned below once every source has merged β€” a name whose http + // tool LOST a collision must not keep its resolver. + toolRequestResolvers.putAll(httpCallTools.resolvers()); } // Merge mcpcalls tools discovered from workflow (if any) @@ -946,7 +1044,28 @@ ToolSetup buildToolSetup(LlmConfiguration.Task task, IConversationMemory memory) mergeExternalTools(a2aTools.toolSpecs(), a2aTools.executors(), "a2a", toolSpecs, toolExecutors, toolSources); } - return new ToolSetup(toolSpecs, toolExecutors, toolSources, builtInSpecs, Map.copyOf(toolCanonicalNames), Map.copyOf(toolEndpoints)); + pruneResolversToSurvivingHttpTools(toolRequestResolvers, toolSources); + + return new ToolSetup(toolSpecs, toolExecutors, toolSources, builtInSpecs, Map.copyOf(toolCanonicalNames), Map.copyOf(toolEndpoints), + Map.copyOf(toolRequestResolvers)); + } + + /** + * Drop every request resolver whose name is not owned by a surviving http tool. + *

+ * {@link #mergeExternalTools} resolves a name collision by DROPPING the + * incoming tool and leaving the incumbent in place, but the dropped tool's + * resolver was registered before that verdict was known. Left in, a builtin (or + * mcp/a2a) tool that won a collision would be pinned against the losing http + * tool's request: the approver would be shown a preview of a request that is + * not the one about to run, and the pre-execution re-check would compare + * against it too β€” a fabricated request passing as a verified one. + *

+ * Run after every source has merged, so {@code toolSources} already records the + * final owner of each name. + */ + static void pruneResolversToSurvivingHttpTools(Map resolvers, Map toolSources) { + resolvers.keySet().removeIf(name -> !"http".equals(toolSources.get(name))); } /** @@ -1291,7 +1410,8 @@ private String runToolCallLoop(ChatModel chatModel, List initialMes // 3) snapshot + persist the pending batch, then abort the loop PendingToolCallBatch batch = buildPendingBatch(currentMessages, gateResult, task, memory, i, activatedToolNames(isLazy, activeSpecs), trace, pausesSoFar + 1, llmTaskIndex, - toolSources, effectiveToolApprovals, transcriptMaxBytes, ruleByCallId, governingRule); + toolSources, effectiveToolApprovals, transcriptMaxBytes, ruleByCallId, governingRule, + setup.toolRequestResolvers()); memory.setHitlPendingToolCalls(batch); incrementToolPauseCount(memory, pausesSoFar); throw new ToolApprovalRequiredException( @@ -1742,6 +1862,22 @@ static Double resolveOverride(Map toolPricing, String dispatchNa // ─── Tool-approval gate helpers ─── + /** + * The only part of a failure from the request-resolution path that is safe to + * log: its type. + *

+ * Not the throwable and not its message. These failures come out of template + * rendering and request building, so the message routinely quotes the material + * being rendered β€” Jackson in particular appends a snippet of the offending + * source ({@code at [Source: (String)"{\"apiKey\":\"sk-…"]}), which puts the + * credential straight back into the log line that carefully redacted it. The + * type alone is what an operator triages on; the payload is already available, + * redacted, in the same message. + */ + private static String errorType(Throwable e) { + return e == null ? "unknown" : e.getClass().getSimpleName(); + } + /** Maps a built-in tool instance to its gate source tag. */ private static String sourceForBuiltInTool(Object tool) { Class c = tool.getClass(); @@ -1786,6 +1922,38 @@ private static int maxPausesPerTurn(ToolApprovalsConfig cfg) { return Math.max(1, Math.min(10, cfg.getMaxPausesPerTurn())); } + /** + * {@code eddi.operator.write.approval} β€” one per gated call the moment its + * verdict is resolved, regardless of what happens to it afterwards (truncated + * args, a changed-request refusal, and a successful execution are all still an + * instance of a human's β€” or the timeout policy's β€” decision). + *

+ * "write" describes the mechanism, not the source: any call reaching this loop + * was gated by {@code toolApprovals.requireApproval}, whether it dispatches + * over http, mcp, or a2a. Restricting the tag to http-sourced calls would + * silently exclude a gated MCP tool that writes to an external system, which is + * exactly the rubber-stamping risk this counter exists to surface. + *

+ * {@code decidedBy} distinguishes a real decision from one the timeout policy + * made ({@link HitlTimeoutHandler}, {@code decidedBy = "system:timeout"}) β€” + * folding those into {@code approved}/{@code rejected} would count an operator + * walking away from their desk as an approval, which is the opposite of what + * "approvals ≫ rejections is a rubber-stamping signal" is trying to detect. + *

+ * Tagged only with the decision outcome β€” never a tool name, argument, or + * conversation id. + */ + void recordWriteApprovalDecision(HitlDecision.HitlVerdict verdict, String decidedBy) { + try { + String decisionTag = "system:timeout".equals(decidedBy) + ? "timeout" + : verdict == HitlDecision.HitlVerdict.APPROVED ? "approved" : "rejected"; + Metrics.globalRegistry.counter("eddi.operator.write.approval", "decision", decisionTag).increment(); + } catch (Exception e) { + LOGGER.debugf("write.approval metric emit failed: %s", e.getMessage()); + } + } + /** * Counts which friction rules actually fire, tagged by the CONFIGURED pattern β€” * never a URL, credential, tool argument or user id, so cardinality is bounded @@ -1869,7 +2037,7 @@ PendingToolCallBatch buildPendingBatch(List currentMessages, ToolAp Map toolSources, ToolApprovalsConfig effectiveToolApprovals, int transcriptMaxBytes) { return buildPendingBatch(currentMessages, gateResult, task, memory, iterationIndex, activatedToolNames, trace, - pauseCountThisTurn, llmTaskIndex, toolSources, effectiveToolApprovals, transcriptMaxBytes, Map.of(), null); + pauseCountThisTurn, llmTaskIndex, toolSources, effectiveToolApprovals, transcriptMaxBytes, Map.of(), null, Map.of()); } /** @@ -1889,6 +2057,11 @@ PendingToolCallBatch buildPendingBatch(List currentMessages, ToolAp * the single rule governing this pause (strictest of the above), or * null; persisted so the post-pause resolvers read the same answer * this gate computed + * @param resolvers + * per httpcall tool name, how to resolve what it would send β€” + * {@code ToolSetup#toolRequestResolvers}. Absent entries (every + * non-http tool) leave the call unpinned, which is the pre-pinning + * behaviour. */ PendingToolCallBatch buildPendingBatch(List currentMessages, ToolApprovalGate.GateResult gateResult, LlmConfiguration.Task task, IConversationMemory memory, int iterationIndex, @@ -1897,7 +2070,8 @@ PendingToolCallBatch buildPendingBatch(List currentMessages, ToolAp Map toolSources, ToolApprovalsConfig effectiveToolApprovals, int transcriptMaxBytes, Map ruleByCallId, - ToolApprovalsConfig.ApprovalRule governingRule) { + ToolApprovalsConfig.ApprovalRule governingRule, + Map resolvers) { PendingToolCallBatch batch = new PendingToolCallBatch(); batch.setPauseEpoch(UUID.randomUUID().toString()); batch.setLlmTaskId(task.getId()); @@ -1952,6 +2126,7 @@ PendingToolCallBatch buildPendingBatch(List currentMessages, ToolAp // on a batch should be able to tell which call brought it. var callRule = req.id() != null ? ruleByCallId.get(req.id()) : null; call.setMatchedRule(callRule != null ? callRule.getMatch() : null); + pinResolvedRequest(call, req, resolvers); calls.add(call); } batch.setCalls(calls); @@ -1968,6 +2143,65 @@ PendingToolCallBatch buildPendingBatch(List currentMessages, ToolAp return batch; } + /** + * Resolve what this gated call would send, and pin it to the pause. + * + *

+ * Records both the redacted preview (so the approver sees the actual request + * rather than a tool name) and its fingerprint (so the request can be + * re-checked immediately before execution). + * + *

+ * Never fails the pause. A tool with no resolver β€” every non-http source + * β€” and a resolve that throws both leave the call simply unpinned, which is + * exactly the behaviour that existed before pinning: approval on name and + * arguments. Letting a template error here abort the batch would turn a display + * feature into a way to kill a turn, and the honest failure mode for "we could + * not determine the request" is to say so, not to guess. + */ + private void pinResolvedRequest(PendingToolCallBatch.PendingToolCall call, ToolExecutionRequest req, + Map resolvers) { + + var resolver = resolvers.get(req.name()); + if (resolver == null) { + return; + } + try { + ResolvedRequest resolved = resolver.resolve(req); + call.setRequestFingerprint(resolved.fingerprint()); + call.setRequestPreview(toPreview(resolved)); + } catch (Exception e) { + // sanitize: the tool name is model-chosen, so it can carry newlines or + // control characters and forge log records β€” same treatment as every + // other name-bearing log statement in this class. The throwable is + // omitted for the reason given on errorType: this failure comes out of + // request building, whose message quotes the request being built. + LOGGER.warnf("Could not resolve the request for gated tool '%s' (%s); it will be approved unpinned.", sanitize(req.name()), + errorType(e)); + } + } + + /** The persisted, display-shaped view of a resolved request. */ + private static PendingToolCallBatch.ResolvedRequestPreview toPreview(ResolvedRequest resolved) { + var preview = new PendingToolCallBatch.ResolvedRequestPreview(); + preview.setMethod(resolved.method()); + preview.setUri(resolved.uri()); + preview.setQueryParams(resolved.queryParams()); + preview.setHeaders(resolved.headers()); + + String body = resolved.body(); + if (body != null && body.getBytes(StandardCharsets.UTF_8).length > PendingToolCallBatch.PREVIEW_BODY_MAX_BYTES) { + // Capped for display only β€” the fingerprint above was computed over the + // whole body, so truncating here cannot weaken the pre-execution check. + preview.setBody(capUtf8(body, PendingToolCallBatch.PREVIEW_BODY_MAX_BYTES)); + preview.setBodyTruncated(true); + } else { + preview.setBody(body); + preview.setBodyTruncated(false); + } + return preview; + } + /** Caps a string to at most maxBytes UTF-8 bytes without splitting a char. */ private static String capUtf8(String s, int maxBytes) { if (s == null) { @@ -2568,7 +2802,20 @@ private DynamicAgentConfig createDefaultDynamicConfig() { * method and path are what the agent designer actually wrote in the * endpoint allow-list. */ - record HttpCallToolsResult(List toolSpecs, Map executors, Map endpoints) { + record HttpCallToolsResult(List toolSpecs, Map executors, Map endpoints, + Map resolvers) { + } + + /** + * Resolves what an httpcall tool would send, without sending it. + *

+ * Only httpcall tools have one. A builtin, MCP or A2A tool is not an HTTP + * request this side of the boundary, so there is nothing to pin β€” those calls + * pause and are approved on their name and arguments alone, exactly as before. + */ + @FunctionalInterface + interface ToolRequestResolver { + ResolvedRequest resolve(ToolExecutionRequest toolRequest) throws LifecycleException; } /** @@ -2618,6 +2865,48 @@ static String normalizeEndpointPath(String rawPath) { return path; } + /** + * Template data for one httpcall tool invocation: conversation memory plus the + * model's arguments merged over it. + *

+ * Shared by the executor and the resolver on purpose. The gate-time fingerprint + * only means anything if it was computed from the same inputs execution will + * use β€” two copies of this merge would eventually disagree, and the guard would + * then reject correct calls (or, worse, pass altered ones). + */ + private Map templateDataFor(IConversationMemory memory, ToolExecutionRequest toolRequest) { + Map templateData = memoryItemConverter.convert(memory); + if (toolRequest.arguments() != null && !toolRequest.arguments().isBlank()) { + try { + @SuppressWarnings("unchecked") + Map args = jsonSerialization.deserialize(toolRequest.arguments(), Map.class); + safeTemplateMerge(templateData, args); + } catch (IOException e) { + // Redacted and capped, never raw: these are model-supplied arguments + // that routinely carry credentials β€” the pause record keeps only a + // SecretRedactionFilter'd copy for exactly this reason, and a log + // line is no safer a place for the plaintext than that record was. + // + // The throwable is deliberately NOT passed: a Jackson parse error + // quotes the offending source in its own message, which would undo + // the redaction on the line right next to it. See errorType. + // Order is load-bearing. Redact FIRST, on the full string: capping + // first would cut a credential mid-token, and the fragment left + // behind no longer matches the shape rules β€” a partial secret in + // the log instead of a marker. Sanitize LAST: these arguments are + // model-chosen and therefore prompt-injectable, and + // SecretRedactionFilter only substitutes secret-shaped VALUES β€” it + // leaves \r and \n untouched, so a model could forge whole log + // records in the HITL audit stream. The tool name beside it was + // already sanitized for exactly this reason; the argument string + // is the more attacker-controllable of the two. + LOGGER.warnf("Failed to parse arguments for tool '%s' (%s): %s", sanitize(toolRequest.name()), errorType(e), + sanitize(capUtf8(SecretRedactionFilter.redact(toolRequest.arguments()), ARGS_LOG_MAX_BYTES))); + } + } + return templateData; + } + /** * Discovers httpcall configurations from the workflow and creates * ToolSpecification + ToolExecutor for each ApiCall. @@ -2626,10 +2915,12 @@ static String normalizeEndpointPath(String rawPath) { * WorkflowConfiguration β†’ filter httpcall steps β†’ load ApiCallsConfiguration β†’ * create tools from each ApiCall. */ + HttpCallToolsResult discoverHttpCallTools(IConversationMemory memory) { List toolSpecs = new ArrayList<>(); Map executors = new HashMap<>(); Map endpoints = new HashMap<>(); + Map resolvers = new HashMap<>(); try { LOGGER.infof("Discovering httpcall tools for agent: %s v%s", memory.getAgentId(), memory.getAgentVersion()); @@ -2668,19 +2959,15 @@ HttpCallToolsResult discoverHttpCallTools(IConversationMemory memory) { apiRequest.getMethod().toLowerCase(Locale.ROOT) + ":" + normalizeEndpointPath(apiRequest.getPath())); } + // Resolving and executing MUST build their template data the same + // way: the fingerprint pinned at gate time is only meaningful if + // it describes the request execution will actually construct. + resolvers.put(apiCall.getName(), + toolRequest -> apiCallExecutor.resolve(apiCall, memory, templateDataFor(memory, toolRequest), targetServerUrl)); + executors.put(apiCall.getName(), (toolRequest, memoryId) -> { try { - Map templateData = memoryItemConverter.convert(memory); - - if (toolRequest.arguments() != null && !toolRequest.arguments().isBlank()) { - try { - @SuppressWarnings("unchecked") - Map args = jsonSerialization.deserialize(toolRequest.arguments(), Map.class); - safeTemplateMerge(templateData, args); - } catch (IOException e) { - LOGGER.warn("Failed to parse tool arguments: " + toolRequest.arguments(), e); - } - } + Map templateData = templateDataFor(memory, toolRequest); Map result = apiCallExecutor.execute(apiCall, memory, templateData, targetServerUrl); @@ -2702,7 +2989,7 @@ HttpCallToolsResult discoverHttpCallTools(IConversationMemory memory) { LOGGER.warn("Failed to discover httpcall tools from workflow", e); } - return new HttpCallToolsResult(toolSpecs, executors, endpoints); + return new HttpCallToolsResult(toolSpecs, executors, endpoints, resolvers); } // --- McpCalls auto-discovery from workflow --- diff --git a/src/main/java/ai/labs/eddi/modules/llm/tools/CreateSubAgentTool.java b/src/main/java/ai/labs/eddi/modules/llm/tools/CreateSubAgentTool.java index 2c638d2d7d..fc751c883b 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/tools/CreateSubAgentTool.java +++ b/src/main/java/ai/labs/eddi/modules/llm/tools/CreateSubAgentTool.java @@ -158,7 +158,10 @@ public String createSubAgent( null, // enableSentimentAnalysis null, // mcpServerUrls true, // deploy - null // environment + null, // environment + null // hitlConfig β€” dynamic sub-agents are not gated; see the + // dynamicAgents.allowCreation escalation flag on the group + // that provisioned this one ); SetupResult result = agentSetupService.setupAgent(request); diff --git a/src/test/java/ai/labs/eddi/engine/api/OperatorMetricsServiceTest.java b/src/test/java/ai/labs/eddi/engine/api/OperatorMetricsServiceTest.java new file mode 100644 index 0000000000..66fbfde39a --- /dev/null +++ b/src/test/java/ai/labs/eddi/engine/api/OperatorMetricsServiceTest.java @@ -0,0 +1,101 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.engine.api; + +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +@DisplayName("OperatorMetricsService") +class OperatorMetricsServiceTest { + + private SimpleMeterRegistry registry; + private OperatorMetricsService service; + + @BeforeEach + void setUp() { + registry = new SimpleMeterRegistry(); + service = new OperatorMetricsService(registry); + service.registerGateGauge(); + } + + @Test + @DisplayName("isValidOutcome accepts exactly pass/fail/unknown") + void isValidOutcomeAcceptsTheFixedVocabulary() { + assertTrue(OperatorMetricsService.isValidOutcome("pass")); + assertTrue(OperatorMetricsService.isValidOutcome("fail")); + assertTrue(OperatorMetricsService.isValidOutcome("unknown")); + assertFalse(OperatorMetricsService.isValidOutcome("PASS")); + assertFalse(OperatorMetricsService.isValidOutcome("passed")); + assertFalse(OperatorMetricsService.isValidOutcome("")); + assertFalse(OperatorMetricsService.isValidOutcome(null)); + } + + @Test + @DisplayName("recordCanaryResult increments the outcome-tagged counter") + void recordCanaryResultIncrementsTheOutcomeCounter() { + service.recordCanaryResult("pass", 120L); + service.recordCanaryResult("pass", 80L); + service.recordCanaryResult("fail", 50L); + + assertEquals(2.0, registry.counter("eddi.operator.canary", "outcome", "pass").count()); + assertEquals(1.0, registry.counter("eddi.operator.canary", "outcome", "fail").count()); + assertEquals(0.0, registry.counter("eddi.operator.canary", "outcome", "unknown").count()); + } + + @Test + @DisplayName("recordCanaryResult records the duration as a timer sample") + void recordCanaryResultRecordsDuration() { + service.recordCanaryResult("pass", 250L); + + var timer = registry.find("eddi.operator.canary.duration").timer(); + assertEquals(1, timer.count()); + assertEquals(250.0, timer.totalTime(java.util.concurrent.TimeUnit.MILLISECONDS), 0.001); + } + + @Test + @DisplayName("a null duration is a valid report β€” no timer sample, no exception") + void nullDurationRecordsNoSample() { + service.recordCanaryResult("unknown", null); + + assertEquals(1.0, registry.counter("eddi.operator.canary", "outcome", "unknown").count()); + assertNull(registry.find("eddi.operator.canary.duration").timer()); + } + + @Test + @DisplayName("a negative duration is silently not recorded, not rejected") + void negativeDurationIsIgnored() { + // A malformed duration says nothing about whether the gate held, so the + // outcome must still count even though the timer sample does not. + service.recordCanaryResult("pass", -5L); + + assertEquals(1.0, registry.counter("eddi.operator.canary", "outcome", "pass").count()); + assertNull(registry.find("eddi.operator.canary.duration").timer()); + } + + @Test + @DisplayName("the gate gauge defaults to 0 before any report ever arrives") + void gateGaugeDefaultsToUnverified() { + assertEquals(0.0, registry.find("eddi.operator.gate.verified").gauge().value()); + } + + @Test + @DisplayName("recordGateStatus moves the gauge to 1, and back to 0 on a later failure") + void recordGateStatusMovesTheGauge() { + service.recordGateStatus(true); + assertEquals(1.0, registry.find("eddi.operator.gate.verified").gauge().value()); + + // The alertable case: a gate that WAS sound stops being sound. The gauge must + // actually move, not just have moved once and stuck. + service.recordGateStatus(false); + assertEquals(0.0, registry.find("eddi.operator.gate.verified").gauge().value()); + } +} diff --git a/src/test/java/ai/labs/eddi/engine/internal/ConversationServiceHitlCoverage2Test.java b/src/test/java/ai/labs/eddi/engine/internal/ConversationServiceHitlCoverage2Test.java index 0e5dea5329..e5fe6366f6 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/ConversationServiceHitlCoverage2Test.java +++ b/src/test/java/ai/labs/eddi/engine/internal/ConversationServiceHitlCoverage2Test.java @@ -180,6 +180,38 @@ void unknownConversation_notFound() throws Exception { verify(conversationMemoryStore, never()).loadConversationMemorySnapshot(CONVERSATION_ID); } + @Test + @DisplayName("null decision β†’ IllegalArgumentException before even the 404 check") + void nullDecision_illegalArgument() throws Exception { + assertThrows(IllegalArgumentException.class, + () -> conversationService.resumeConversation(CONVERSATION_ID, null, null)); + + // Every caller of resumeConversation already guarantees a real verdict + // (RestAgentEngine, Slack, MCP, timeout auto-resolution) β€” this guards the + // ONE shared choke point they all funnel through, so a future caller that + // forgets fails loudly here rather than reaching AgentOrchestrator with a + // verdict that is neither APPROVED nor REJECTED. Checked first: not even + // the conversation-existence lookup runs on a malformed request. + verify(conversationMemoryStore, never()).getConversationState(any()); + verify(conversationMemoryStore, never()).compareAndSetState(any(), any(), any()); + } + + @Test + @DisplayName("decision with no top-level verdict β†’ IllegalArgumentException before even the 404 check") + void nullVerdict_illegalArgument() throws Exception { + HitlDecision decision = new HitlDecision(); + // verdict left unset (null) β€” e.g. a hand-built or future caller that + // forgot to set it, or a per-call-only decision with no top-level default. + decision.setToolDecisions(Map.of("call_abc", toolDecision(HitlVerdict.APPROVED))); + + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, + () -> conversationService.resumeConversation(CONVERSATION_ID, decision, null)); + assertTrue(e.getMessage().contains("verdict"), "message should name the missing field: " + e.getMessage()); + + verify(conversationMemoryStore, never()).getConversationState(any()); + verify(conversationMemoryStore, never()).compareAndSetState(any(), any(), any()); + } + @Test @DisplayName("toolDecisions present but pre-CAS snapshot null β†’ validation skipped, CAS still runs") void toolDecisionsButNullPreCasSnapshot_skipsValidation() throws Exception { diff --git a/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.java b/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.java index d1f8d82b0a..a07763fd58 100644 --- a/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.java +++ b/src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.java @@ -108,6 +108,17 @@ private Map summaryOf(Response response) { return (Map) response.getEntity(); } + private PendingToolCallBatch.ResolvedRequestPreview preview(String method, String uri, String body) { + var preview = new PendingToolCallBatch.ResolvedRequestPreview(); + preview.setMethod(method); + preview.setUri(uri); + preview.setQueryParams(Map.of("version", "1")); + preview.setHeaders(Map.of("Authorization", "")); + preview.setBody(body); + preview.setBodyTruncated(false); + return preview; + } + @Nested @DisplayName("pauseDetails β€” TOOL_CALL") class ToolCallPauseDetails { @@ -153,6 +164,128 @@ void redactedArgsOnlyNeverRawValue() throws Exception { assertEquals(List.of("getCurrentDateTime"), pauseDetails.get("executedUngatedCalls")); } + @Test + @DisplayName("a pinned http call exposes its resolved-request preview, so an approver sees the real request") + void pinnedCallExposesRequestPreview() throws Exception { + var snapshot = snapshotInState(ConversationState.AWAITING_HUMAN); + snapshot.setHitlPauseType("TOOL_CALL"); + + var call = toolCall("call-1", "deployAgent", "http", RAW_SECRET, "{}", false, "http.post:*"); + call.setRequestFingerprint("deadbeef"); + call.setRequestPreview(preview("POST", "https://eddi.example/administration/production/deploy/a1", "{}")); + + var batch = new PendingToolCallBatch(); + batch.setPauseEpoch("epoch-1"); + batch.setCalls(List.of(call)); + snapshot.setHitlPendingToolCalls(batch); + doReturn(Optional.empty()).when(hitlToolJournalStore).find(anyString(), anyString(), anyString()); + + Response response = restAgentEngine.getApprovalStatus(CONVERSATION_ID, "summary"); + + var pauseDetails = (Map) summaryOf(response).get("pauseDetails"); + var callView = ((List>) pauseDetails.get("calls")).get(0); + + assertEquals(true, callView.get("requestPinned")); + var previewView = (Map) callView.get("requestPreview"); + assertNotNull(previewView, "a pinned call must expose its preview"); + assertEquals("POST", previewView.get("method")); + assertEquals("https://eddi.example/administration/production/deploy/a1", previewView.get("uri")); + assertEquals(Map.of("version", "1"), previewView.get("queryParams")); + assertEquals(Map.of("Authorization", ""), previewView.get("headers")); + assertEquals("{}", previewView.get("body")); + assertEquals(false, previewView.get("bodyTruncated")); + } + + @Test + @DisplayName("an unpinned call (every non-http tool) has no preview, honestly, rather than a fabricated one") + void unpinnedCallHasNoPreview() throws Exception { + var snapshot = snapshotInState(ConversationState.AWAITING_HUMAN); + snapshot.setHitlPauseType("TOOL_CALL"); + + var call = toolCall("call-1", "sendEmail", "mcp", RAW_SECRET, "{\"to\":\"[REDACTED]\"}", false, "mcp:*"); + // requestFingerprint / requestPreview left unset, exactly as the gate + // leaves them for a non-http tool. + + var batch = new PendingToolCallBatch(); + batch.setPauseEpoch("epoch-1"); + batch.setCalls(List.of(call)); + snapshot.setHitlPendingToolCalls(batch); + doReturn(Optional.empty()).when(hitlToolJournalStore).find(anyString(), anyString(), anyString()); + + Response response = restAgentEngine.getApprovalStatus(CONVERSATION_ID, "summary"); + + var pauseDetails = (Map) summaryOf(response).get("pauseDetails"); + var callView = ((List>) pauseDetails.get("calls")).get(0); + + assertEquals(false, callView.get("requestPinned")); + assertNull(callView.get("requestPreview")); + } + + @Test + @DisplayName("the fingerprint itself never appears in the response β€” it is an internal comparison value, not approver-facing") + void fingerprintNeverAppearsInResponse() throws Exception { + var snapshot = snapshotInState(ConversationState.AWAITING_HUMAN); + snapshot.setHitlPauseType("TOOL_CALL"); + + var call = toolCall("call-1", "deployAgent", "http", RAW_SECRET, "{}", false, "http.post:*"); + String secretFingerprint = "fingerprint-must-not-leak-abc123"; + call.setRequestFingerprint(secretFingerprint); + call.setRequestPreview(preview("POST", "https://eddi.example/deploy/a1", "{}")); + + var batch = new PendingToolCallBatch(); + batch.setPauseEpoch("epoch-1"); + batch.setCalls(List.of(call)); + snapshot.setHitlPendingToolCalls(batch); + doReturn(Optional.empty()).when(hitlToolJournalStore).find(anyString(), anyString(), anyString()); + + Response response = restAgentEngine.getApprovalStatus(CONVERSATION_ID, "summary"); + + assertFalse(summaryOf(response).toString().contains(secretFingerprint), + "the raw fingerprint value must never appear in the approval-status response"); + } + + @Test + @DisplayName("detail=full strips the fingerprint too β€” it digests the RAW body the preview redacts") + void fingerprintNeverAppearsInTheFullSnapshot() throws Exception { + // The gap the summary test above did NOT cover: detail=full returns the + // whole snapshot object, and the getter carries no @JsonIgnore (it cannot + // β€” the persistence mapper shares the same configuration, so ignoring it + // would drop the field from the stored document and disable pinning). + // Without a read-time strip, an approver received a SHA-256 over a + // canonical string containing the raw body and raw query values β€” i.e. + // exactly the credential material RequestRedactor removed from the + // preview sitting beside it. + var snapshot = snapshotInState(ConversationState.AWAITING_HUMAN); + snapshot.setHitlPauseType("TOOL_CALL"); + + var call = toolCall("call-1", "deployAgent", "http", RAW_SECRET, "{}", false, "http.post:*"); + String secretFingerprint = "fingerprint-must-not-leak-abc123"; + call.setRequestFingerprint(secretFingerprint); + call.setRequestPreview(preview("POST", "https://eddi.example/deploy/a1", "{}")); + + var batch = new PendingToolCallBatch(); + batch.setPauseEpoch("epoch-1"); + batch.setCalls(List.of(call)); + snapshot.setHitlPendingToolCalls(batch); + + Response response = restAgentEngine.getApprovalStatus(CONVERSATION_ID, "full"); + + var returned = (ConversationMemorySnapshot) response.getEntity(); + var returnedCall = returned.getHitlPendingToolCalls().getCalls().getFirst(); + assertNotEquals(secretFingerprint, returnedCall.getRequestFingerprint(), + "detail=full must not carry the real request fingerprint"); + // The approver still gets everything they need to decide β€” stripping the + // digest must not cost them the preview it was derived from. + assertNotNull(returnedCall.getRequestPreview(), "the redacted request preview must survive the strip"); + // And must not cost them the PINNED signal either. isRequestPinned() is + // derived from the fingerprint field, so clearing it outright silently + // reported every pinned call as unpinned β€” telling the approver the + // request is not re-checked before execution when it is, and + // contradicting what detail=summary says about the same conversation. + assertTrue(returnedCall.isRequestPinned(), + "stripping the digest must not flip the documented requestPinned contract field"); + } + @Test @DisplayName("no journal entries β†’ outcomeUnknown is empty") void noJournalEntriesMeansEmptyOutcomeUnknown() throws Exception { diff --git a/src/test/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilitiesHitlTest.java b/src/test/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilitiesHitlTest.java index 2e6bc5124a..fa9b877b8c 100644 --- a/src/test/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilitiesHitlTest.java +++ b/src/test/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilitiesHitlTest.java @@ -199,6 +199,8 @@ class SimpleSnapshotProjectionSecurity { private static final String CANARY_SECRET = "sk-live-SECRET-9999"; private static final String CANARY_ARGS = "{\"amount\":250,\"apiKey\":\"" + CANARY_SECRET + "\"}"; private static final String CANARY_TRANSCRIPT = "[{\"type\":\"AI\",\"text\":\"" + CANARY_SECRET + "\"}]"; + private static final String CANARY_FINGERPRINT = "sha256-request-fingerprint-CANARY"; + private static final String CANARY_PREVIEW_URI = "https://eddi.internal/CANARY-should-not-leak/{id}"; private ConversationMemorySnapshot toolPausedSnapshot() { var snapshot = buildMinimalSnapshot(); @@ -214,6 +216,12 @@ private ConversationMemorySnapshot toolPausedSnapshot() { call.setArgumentsRedacted(CANARY_ARGS); call.setArgsTruncated(false); call.setGateReason("http:transfer_*"); + call.setRequestFingerprint(CANARY_FINGERPRINT); + var preview = new PendingToolCallBatch.ResolvedRequestPreview(); + preview.setMethod("POST"); + preview.setUri(CANARY_PREVIEW_URI); + preview.setBody(CANARY_ARGS); + call.setRequestPreview(preview); var batch = new PendingToolCallBatch(); batch.setPauseEpoch("epoch-1"); @@ -261,6 +269,18 @@ void rawArgsAndTranscriptNotSerialized() throws Exception { "argumentsRaw value leaked into generic simple-snapshot JSON"); assertFalse(json.contains("\"argumentsRedacted\":\""), "argumentsRedacted value leaked into generic simple-snapshot JSON"); + // The resolved-request preview (approver-facing detail β€” see + // RestAgentEngine#buildToolCallPauseDetails) and its fingerprint are + // materially more detail than "names only" and must not leak either, + // even though both are already-redacted, not raw secrets. + assertFalse(json.contains(CANARY_FINGERPRINT), + "requestFingerprint value leaked into generic simple-snapshot JSON"); + assertFalse(json.contains(CANARY_PREVIEW_URI), + "requestPreview leaked into generic simple-snapshot JSON"); + assertTrue(json.contains("\"requestFingerprint\":null"), + "requestFingerprint must be projected to null in generic simple-snapshot JSON"); + assertTrue(json.contains("\"requestPreview\":null"), + "requestPreview must be projected to null in generic simple-snapshot JSON"); // But the safe metadata the delegated/group/MCP consumers rely on MUST appear. assertTrue(json.contains("TOOL_CALL"), "pauseType must be present"); diff --git a/src/test/java/ai/labs/eddi/engine/rest/RestOperatorMetricsTest.java b/src/test/java/ai/labs/eddi/engine/rest/RestOperatorMetricsTest.java new file mode 100644 index 0000000000..da406fef9f --- /dev/null +++ b/src/test/java/ai/labs/eddi/engine/rest/RestOperatorMetricsTest.java @@ -0,0 +1,80 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.engine.rest; + +import ai.labs.eddi.engine.api.OperatorMetricsService; +import ai.labs.eddi.engine.api.model.OperatorCanaryReport; +import ai.labs.eddi.engine.api.model.OperatorGateStatusReport; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; +import jakarta.ws.rs.BadRequestException; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Validates at the REST boundary, then delegates β€” the service tests own the + * metric assertions. + */ +@DisplayName("RestOperatorMetrics") +class RestOperatorMetricsTest { + + private SimpleMeterRegistry registry; + private RestOperatorMetrics rest; + + @BeforeEach + void setUp() { + registry = new SimpleMeterRegistry(); + var service = new OperatorMetricsService(registry); + service.registerGateGauge(); + rest = new RestOperatorMetrics(service); + } + + @Test + @DisplayName("a valid canary report is 204 and reaches the meter") + void validCanaryReportIs204() { + var response = rest.reportCanaryResult(new OperatorCanaryReport("pass", 100L)); + assertEquals(204, response.getStatus()); + assertEquals(1.0, registry.counter("eddi.operator.canary", "outcome", "pass").count()); + } + + @Test + @DisplayName("a null report body is rejected, and says so rather than blaming the outcome field") + void nullCanaryReportIsRejected() { + // A caller who sent no body should not be sent looking at a field they + // never supplied β€” the gate-status endpoint already words this correctly. + var e = assertThrows(BadRequestException.class, () -> rest.reportCanaryResult(null)); + assertTrue(e.getMessage().contains("body"), e.getMessage()); + } + + @Test + @DisplayName("an outcome outside the fixed vocabulary is rejected before it reaches the meter") + void invalidOutcomeIsRejected() { + // The vocabulary is enforced HERE, not trusted from the client β€” a free-text + // outcome would let cardinality grow unbounded on a metric label. + assertThrows(BadRequestException.class, () -> rest.reportCanaryResult(new OperatorCanaryReport("PASS", 100L))); + assertThrows(BadRequestException.class, () -> rest.reportCanaryResult(new OperatorCanaryReport("", 100L))); + assertThrows(BadRequestException.class, () -> rest.reportCanaryResult(new OperatorCanaryReport(null, 100L))); + assertEquals(0.0, registry.find("eddi.operator.canary").counters().stream().mapToDouble(io.micrometer.core.instrument.Counter::count) + .sum()); + } + + @Test + @DisplayName("a valid gate-status report is 204 and moves the gauge") + void validGateStatusReportIs204() { + var response = rest.reportGateStatus(new OperatorGateStatusReport(true)); + assertEquals(204, response.getStatus()); + assertEquals(1.0, registry.find("eddi.operator.gate.verified").gauge().value()); + } + + @Test + @DisplayName("a null gate-status body is rejected") + void nullGateStatusReportIsRejected() { + assertThrows(BadRequestException.class, () -> rest.reportGateStatus(null)); + } +} diff --git a/src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceBranchCoverageTest.java b/src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceBranchCoverageTest.java index 9cc29aa54b..6958684674 100644 --- a/src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceBranchCoverageTest.java +++ b/src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceBranchCoverageTest.java @@ -86,7 +86,7 @@ void invalidEnv() { @DisplayName("setupAgent with an unknown environment creates nothing") void setupAgentRejectsUnknownEnvironment() { var request = new SetupAgentRequest("MyAgent", "You are helpful.", "anthropic", "claude-sonnet-4-6", "sk-test", null, null, false, null, - false, false, null, true, "staging"); + false, false, null, true, "staging", null); var exception = assertThrows(AgentSetupService.AgentSetupException.class, () -> service.setupAgent(request)); @@ -326,7 +326,7 @@ class SetupAgentValidation { @DisplayName("null agent name throws") void nullAgentName() { var req = new SetupAgentRequest(null, "prompt", "anthropic", "model", - "key", null, null, null, null, null, null, null, null, null); + "key", null, null, null, null, null, null, null, null, null, null); assertThrows(AgentSetupService.AgentSetupException.class, () -> service.setupAgent(req)); } @@ -334,7 +334,7 @@ void nullAgentName() { @DisplayName("blank agent name throws") void blankAgentName() { var req = new SetupAgentRequest(" ", "prompt", "anthropic", "model", - "key", null, null, null, null, null, null, null, null, null); + "key", null, null, null, null, null, null, null, null, null, null); assertThrows(AgentSetupService.AgentSetupException.class, () -> service.setupAgent(req)); } @@ -342,7 +342,7 @@ void blankAgentName() { @DisplayName("null system prompt throws") void nullSystemPrompt() { var req = new SetupAgentRequest("Agent", null, "anthropic", "model", - "key", null, null, null, null, null, null, null, null, null); + "key", null, null, null, null, null, null, null, null, null, null); assertThrows(AgentSetupService.AgentSetupException.class, () -> service.setupAgent(req)); } @@ -350,7 +350,7 @@ void nullSystemPrompt() { @DisplayName("blank system prompt throws") void blankSystemPrompt() { var req = new SetupAgentRequest("Agent", " ", "anthropic", "model", - "key", null, null, null, null, null, null, null, null, null); + "key", null, null, null, null, null, null, null, null, null, null); assertThrows(AgentSetupService.AgentSetupException.class, () -> service.setupAgent(req)); } @@ -358,7 +358,7 @@ void blankSystemPrompt() { @DisplayName("cloud provider without API key throws") void cloudProviderNoApiKey() { var req = new SetupAgentRequest("Agent", "prompt", "openai", "gpt-4", - null, null, null, null, null, null, null, null, null, null); + null, null, null, null, null, null, null, null, null, null, null); assertThrows(AgentSetupService.AgentSetupException.class, () -> service.setupAgent(req)); } @@ -366,7 +366,7 @@ void cloudProviderNoApiKey() { @DisplayName("cloud provider with blank API key throws") void cloudProviderBlankApiKey() { var req = new SetupAgentRequest("Agent", "prompt", "anthropic", "model", - " ", null, null, null, null, null, null, null, null, null); + " ", null, null, null, null, null, null, null, null, null, null); assertThrows(AgentSetupService.AgentSetupException.class, () -> service.setupAgent(req)); } @@ -374,7 +374,7 @@ void cloudProviderBlankApiKey() { @DisplayName("local provider (ollama) without API key does NOT throw for validation") void localProviderNoApiKeyOk() throws Exception { var req = new SetupAgentRequest("Agent", "prompt", "ollama", "llama3", - null, null, null, null, null, null, null, null, false, null); + null, null, null, null, null, null, null, null, false, null, null); // Will fail at REST call, but validation should pass when(restInterfaceFactory.get(any())).thenThrow(new RestInterfaceFactory.RestInterfaceFactoryException("mock", new RuntimeException())); assertThrows(AgentSetupService.AgentSetupException.class, () -> service.setupAgent(req)); diff --git a/src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceTest.java b/src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceTest.java index 5324a723ff..2f047ed7ad 100644 --- a/src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceTest.java +++ b/src/test/java/ai/labs/eddi/engine/setup/AgentSetupServiceTest.java @@ -569,7 +569,7 @@ class ValidationTests { @DisplayName("throws when agent name is null") void nullAgentName() { var request = new SetupAgentRequest(null, "prompt", "openai", "gpt-4", - "key", null, null, null, null, null, null, null, null, null); + "key", null, null, null, null, null, null, null, null, null, null); assertThrows(AgentSetupService.AgentSetupException.class, () -> service.setupAgent(request)); } @@ -578,7 +578,7 @@ void nullAgentName() { @DisplayName("throws when system prompt is blank") void blankPrompt() { var request = new SetupAgentRequest("Test Agent", "", "openai", "gpt-4", - "key", null, null, null, null, null, null, null, null, null); + "key", null, null, null, null, null, null, null, null, null, null); assertThrows(AgentSetupService.AgentSetupException.class, () -> service.setupAgent(request)); } @@ -587,7 +587,7 @@ void blankPrompt() { @DisplayName("throws when cloud provider has no API key") void cloudProviderNoApiKey() { var request = new SetupAgentRequest("Test Agent", "prompt", "openai", "gpt-4", - null, null, null, null, null, null, null, null, null, null); + null, null, null, null, null, null, null, null, null, null, null); assertThrows(AgentSetupService.AgentSetupException.class, () -> service.setupAgent(request)); } @@ -598,13 +598,174 @@ void localProviderNoApiKey() { // ollama doesn't need an API key, but will fail at REST store call // β€” the validation itself should pass var request = new SetupAgentRequest("Test Agent", "prompt", "ollama", "llama3", - null, null, null, null, null, null, null, null, null, null); + null, null, null, null, null, null, null, null, null, null, null); // Will throw AgentSetupException at the REST call level, not validation var ex = assertThrows(AgentSetupService.AgentSetupException.class, () -> service.setupAgent(request)); // Should NOT be "API key is required" assertFalse(ex.getMessage().contains("API key is required")); } + + @Test + @DisplayName("an unusable approval pattern is refused BEFORE any resource is created") + void invalidHitlConfigRefusedBeforeAnyResourceIsCreated() throws Exception { + // Mirrors createApiAgent's own guard, added in the same place for the same + // reason: without this, a bad pattern would surface only after the parser, + // behaviour, LLM and workflow had all been created, leaving every one + // orphaned. Asserted by proving no store was even asked for. + var restInterfaceFactory = mock(IRestInterfaceFactory.class); + var guardedService = new AgentSetupService(restInterfaceFactory, + mock(IRestAgentAdministration.class), mock(ISecretProvider.class), "http://localhost:11434"); + + var hitl = new ai.labs.eddi.configs.agents.model.AgentConfiguration.HitlConfig(); + var toolApprovals = new ai.labs.eddi.configs.hitl.model.ToolApprovalsConfig(); + toolApprovals.setRequireApproval(List.of("mcp:/agentstore/agents")); + hitl.setToolApprovals(toolApprovals); + + var request = new SetupAgentRequest("Test Agent", "prompt", "openai", "gpt-4", + "key", null, null, null, null, null, null, null, null, null, hitl); + + var ex = assertThrows(AgentSetupService.AgentSetupException.class, + () -> guardedService.setupAgent(request)); + assertTrue(ex.getMessage().startsWith("Invalid hitlConfig:"), ex.getMessage()); + org.mockito.Mockito.verify(restInterfaceFactory, org.mockito.Mockito.never()).get(any()); + } + + @Test + @DisplayName("a valid hitlConfig passes the up-front check and reaches resource creation") + void validHitlConfigPassesTheUpFrontCheck() throws Exception { + // Mutation guard for the test above: the guard must reject only what is + // actually invalid. + var restInterfaceFactory = mock(IRestInterfaceFactory.class); + var guardedService = new AgentSetupService(restInterfaceFactory, + mock(IRestAgentAdministration.class), mock(ISecretProvider.class), "http://localhost:11434"); + + var hitl = new ai.labs.eddi.configs.agents.model.AgentConfiguration.HitlConfig(); + var toolApprovals = new ai.labs.eddi.configs.hitl.model.ToolApprovalsConfig(); + toolApprovals.setRequireApproval(List.of("http.post:*", "http.put:*", "http.delete:*")); + toolApprovals.setExempt(List.of("http.get:*")); + hitl.setToolApprovals(toolApprovals); + + var request = new SetupAgentRequest("Test Agent", "prompt", "openai", "gpt-4", + "key", null, null, null, null, null, null, null, null, null, hitl); + + var ex = assertThrows(AgentSetupService.AgentSetupException.class, + () -> guardedService.setupAgent(request)); + assertFalse(ex.getMessage().startsWith("Invalid hitlConfig:"), + "a valid gate must not be refused by the up-front check; got: " + ex.getMessage()); + } + } + + // ==================== hitlConfig actually reaches the created agent + // ==================== + + /** + * The up-front validation tests above prove a bad gate is refused before any + * resource exists. Neither they, nor any other test in this file, prove the + * other half: that a GOOD gate actually ends up on the + * {@code AgentConfiguration} passed to {@code IRestAgentStore.createAgent} β€” + * the one line (`agentConfig.setHitlConfig(request.hitlConfig())`) that is the + * entire point of this field existing. Mocks every REST store on the minimal + * path (no MCP servers, no intro message) so step 7 is actually reached. + */ + @Nested + @DisplayName("hitlConfig reaches the created agent") + class HitlConfigWiringTests { + + private Response located(String location) { + var response = mock(Response.class); + when(response.getHeaderString("Location")).thenReturn(location); + return response; + } + + private IRestInterfaceFactory wireMinimalHappyPath( + org.mockito.ArgumentCaptor agentCaptor) + throws Exception { + var factory = mock(IRestInterfaceFactory.class); + + // Each Response built as its own statement, never as a nested + // when(...)-inside-when(...) argument expression: Mockito's stubbing + // is recorded through a single ongoing-stub slot, and a nested + // when()/thenReturn() pair started before the outer one completes + // leaves BOTH unfinished (UnfinishedStubbingException) β€” not a + // compile error, only a test-time one, so this is worth spelling out. + var parserResponse = located("/parserstore/parsers/000000000000000000000001?version=1"); + var parserStore = mock(ai.labs.eddi.configs.parser.IRestParserStore.class); + when(parserStore.createParser(any())).thenReturn(parserResponse); + when(factory.get(ai.labs.eddi.configs.parser.IRestParserStore.class)).thenReturn(parserStore); + + var ruleSetResponse = located("/rulestore/rulesets/000000000000000000000002?version=1"); + var ruleSetStore = mock(ai.labs.eddi.configs.rules.IRestRuleSetStore.class); + when(ruleSetStore.createRuleSet(any())).thenReturn(ruleSetResponse); + when(factory.get(ai.labs.eddi.configs.rules.IRestRuleSetStore.class)).thenReturn(ruleSetStore); + + var llmResponse = located("/llmstore/llms/000000000000000000000003?version=1"); + var llmStore = mock(ai.labs.eddi.configs.llm.IRestLlmStore.class); + when(llmStore.createLlm(any())).thenReturn(llmResponse); + when(factory.get(ai.labs.eddi.configs.llm.IRestLlmStore.class)).thenReturn(llmStore); + + var workflowResponse = located("/workflowstore/workflows/000000000000000000000004?version=1"); + var workflowStore = mock(ai.labs.eddi.configs.workflows.IRestWorkflowStore.class); + when(workflowStore.createWorkflow(any())).thenReturn(workflowResponse); + when(factory.get(ai.labs.eddi.configs.workflows.IRestWorkflowStore.class)).thenReturn(workflowStore); + + var agentResponse = located("/agentstore/agents/000000000000000000000005?version=1"); + var agentStore = mock(ai.labs.eddi.configs.agents.IRestAgentStore.class); + when(agentStore.createAgent(agentCaptor.capture())).thenReturn(agentResponse); + when(factory.get(ai.labs.eddi.configs.agents.IRestAgentStore.class)).thenReturn(agentStore); + + // patchDescriptor fires after every creation; an unstubbed mock returning + // null for it is fine, but factory.get(...) still has to resolve the class. + when(factory.get(ai.labs.eddi.configs.descriptors.IRestDocumentDescriptorStore.class)) + .thenReturn(mock(ai.labs.eddi.configs.descriptors.IRestDocumentDescriptorStore.class)); + + return factory; + } + + @Test + @DisplayName("a configured hitlConfig is set on the AgentConfiguration handed to createAgent") + void hitlConfigReachesTheCreatedAgentConfiguration() throws Exception { + var agentCaptor = org.mockito.ArgumentCaptor.forClass(ai.labs.eddi.configs.agents.model.AgentConfiguration.class); + var factory = wireMinimalHappyPath(agentCaptor); + var wiredService = new AgentSetupService(factory, mock(IRestAgentAdministration.class), mock(ISecretProvider.class), + "http://localhost:11434"); + + var hitl = new ai.labs.eddi.configs.agents.model.AgentConfiguration.HitlConfig(); + var toolApprovals = new ai.labs.eddi.configs.hitl.model.ToolApprovalsConfig(); + toolApprovals.setRequireApproval(List.of("http.post:*", "http.put:*", "http.delete:*")); + toolApprovals.setExempt(List.of("http.get:*")); + hitl.setToolApprovals(toolApprovals); + + // deploy=false: this test is about the AgentConfiguration handed to + // createAgent, not about deployment, which would need an + // IRestAgentAdministration mock too. + var request = new SetupAgentRequest("Billing Agent", "You are helpful.", "anthropic", "claude-sonnet-4-6", + "sk-test", null, null, null, null, null, null, null, false, null, hitl); + + wiredService.setupAgent(request); + + assertSame(hitl, agentCaptor.getValue().getHitlConfig(), + "the exact hitlConfig from the request must reach the created agent, not a copy or null"); + } + + @Test + @DisplayName("an absent hitlConfig leaves the created agent ungated β€” the pre-existing default") + void absentHitlConfigLeavesTheAgentUngated() throws Exception { + // The mirror of the test above: this field is opt-in. A caller that + // supplies none must not have one silently invented for them β€” that + // would be a correctness bug in the other direction. + var agentCaptor = org.mockito.ArgumentCaptor.forClass(ai.labs.eddi.configs.agents.model.AgentConfiguration.class); + var factory = wireMinimalHappyPath(agentCaptor); + var wiredService = new AgentSetupService(factory, mock(IRestAgentAdministration.class), mock(ISecretProvider.class), + "http://localhost:11434"); + + var request = new SetupAgentRequest("Billing Agent", "You are helpful.", "anthropic", "claude-sonnet-4-6", + "sk-test", null, null, null, null, null, null, null, false, null, null); + + wiredService.setupAgent(request); + + assertNull(agentCaptor.getValue().getHitlConfig()); + } } // ==================== createApiAgent validation ==================== diff --git a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorBranchCoverageTest.java b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorBranchCoverageTest.java index 1d630b2257..005058f2ec 100644 --- a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorBranchCoverageTest.java +++ b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorBranchCoverageTest.java @@ -39,7 +39,7 @@ * retryOnHttpCodes match - retryCall with null postResponse - retryCall with * null retryApiCallInstruction - path building: no slash, http:// prefix, body * present with custom content type - request delay > 0 (scheduled executor) - - * scrubSensitiveHeaders with various header names - empty targetServerUrl + * request-map redaction with various header names - empty targetServerUrl */ @DisplayName("ApiCallExecutor β€” Branch Coverage v2") class ApiCallExecutorBranchCoverageTest { @@ -78,7 +78,8 @@ void setUp() throws Exception { lenient().when(callerIdentityResolver.resolveValue(anyString(), any())).thenAnswer(inv -> inv.getArgument(0)); lenient().when(callerIdentityResolver.redactCallerToken(anyString(), anyString())).thenAnswer(inv -> inv.getArgument(0)); executor = new ApiCallExecutor(httpClient, jsonSerialization, runtime, - prePostUtils, globalVariableResolver, secretResolver, callerIdentityResolver, callerIdentityContext, false, 30_000L, 2_000_000); + prePostUtils, globalVariableResolver, secretResolver, callerIdentityResolver, callerIdentityContext, + new RequestRedactor(callerIdentityResolver), false, 30_000L, 2_000_000); when(memory.getCurrentStep()).thenReturn(currentStep); when(mockRequest.toMap()).thenReturn(new HashMap<>()); @@ -303,11 +304,11 @@ void headersAndQueryParams() throws Exception { } // ═══════════════════════════════════════════════════════════════ - // scrubSensitiveHeaders β€” comprehensive header name checks + // request-map redaction β€” comprehensive header name checks // ═══════════════════════════════════════════════════════════════ @Nested - @DisplayName("scrubSensitiveHeaders β€” all header name patterns") + @DisplayName("request-map redaction β€” all header name patterns") class ScrubHeaders { @Test @@ -578,11 +579,11 @@ void nullTargetServer() { } // ═══════════════════════════════════════════════════════════════ - // scrubSensitiveHeaders β€” additional patterns + // request-map redaction β€” additional patterns // ═══════════════════════════════════════════════════════════════ @Nested - @DisplayName("scrubSensitiveHeaders β€” additional header patterns") + @DisplayName("request-map redaction β€” additional header patterns") class ScrubHeadersAdditional { @Test diff --git a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorExtendedTest.java b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorExtendedTest.java index a0204f9110..eb8ec1ff21 100644 --- a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorExtendedTest.java +++ b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorExtendedTest.java @@ -61,7 +61,7 @@ void setUp() throws Exception { when(globalVariableResolver.resolveValue(anyString())).thenAnswer(inv -> inv.getArgument(0)); executor = new ApiCallExecutor(httpClient, jsonSerialization, runtime, prePostUtils, globalVariableResolver, secretResolver, - callerIdentityResolver, callerIdentityContext, false, 30_000L, 2_000_000); + callerIdentityResolver, callerIdentityContext, new RequestRedactor(callerIdentityResolver), false, 30_000L, 2_000_000); memory = mock(IConversationMemory.class); currentStep = mock(IWritableConversationStep.class); diff --git a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.java b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.java index 867919f24b..ad74a70aa7 100644 --- a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.java +++ b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.java @@ -72,7 +72,8 @@ void setUp() throws Exception { when(globalVariableResolver.resolveValue(anyString())).thenAnswer(inv -> inv.getArgument(0)); executor = new ApiCallExecutor(httpClient, jsonSerialization, runtime, prePostUtils, globalVariableResolver, secretResolver, - callerIdentityResolver, callerIdentityContext, false, DEFAULT_TIMEOUT_MILLIS, DEFAULT_MAX_RESPONSE_SIZE); + callerIdentityResolver, callerIdentityContext, new RequestRedactor(callerIdentityResolver), false, DEFAULT_TIMEOUT_MILLIS, + DEFAULT_MAX_RESPONSE_SIZE); memory = mock(IConversationMemory.class); currentStep = mock(IWritableConversationStep.class); @@ -319,7 +320,8 @@ void execute_callerTokenInUnconventionalHeader_isRedacted() throws Exception { realContext.bind(new CallerIdentity("caller-jwt-value", "alice", "https://eddi.example:443")); var realResolver = new CallerIdentityResolver(realContext, true); var executorWithRealResolver = new ApiCallExecutor(httpClient, jsonSerialization, runtime, prePostUtils, globalVariableResolver, - secretResolver, realResolver, realContext, false, DEFAULT_TIMEOUT_MILLIS, DEFAULT_MAX_RESPONSE_SIZE); + secretResolver, realResolver, realContext, new RequestRedactor(realResolver), false, DEFAULT_TIMEOUT_MILLIS, + DEFAULT_MAX_RESPONSE_SIZE); try { ApiCall call = createSimpleApiCall("redact-call", false); @@ -348,6 +350,244 @@ void execute_callerTokenInUnconventionalHeader_isRedacted() throws Exception { } } + @Test + @DisplayName("a call carrying query parameters still pins β€” they arrive as List values, not Strings") + void resolve_withQueryParameters_stillProducesAFingerprint() throws Exception { + // HttpClientWrapper stores query params as Map> (a + // param may legitimately repeat). Reading them back as Map + // erases cleanly at the cast and then throws deep inside the fingerprint + // canonicaliser β€” which pinResolvedRequest catches and downgrades to + // "approved unpinned". The whole pinning guarantee would silently not + // apply to any endpoint with a query param, deploy?version=N included. + ApiCall call = createSimpleApiCall("query-call", false); + + Map requestMap = new HashMap<>(); + requestMap.put("uri", "http://example.com/administration/production/deploy/agent-1"); + requestMap.put("method", "POST"); + requestMap.put("headers", new LinkedHashMap()); + Map> queryParams = new LinkedHashMap<>(); + queryParams.put("version", List.of("3")); + requestMap.put("queryParams", queryParams); + when(mockRequest.toMap()).thenReturn(requestMap); + + ResolvedRequest resolved = executor.resolve(call, memory, new HashMap<>(), "http://example.com"); + + assertNotNull(resolved.fingerprint(), "a call with a query parameter must still be pinnable"); + assertTrue(resolved.isPinned()); + assertEquals("3", resolved.queryParams().get("version")); + } + + @Test + @DisplayName("a repeated query parameter keeps both values distinguishable in the fingerprint") + void resolve_withRepeatedQueryParameter_doesNotCollapseValues() throws Exception { + ApiCall call = createSimpleApiCall("multi-query-call", false); + + ResolvedRequest twoValues = resolveWithQuery(call, Map.of("tag", List.of("a", "b"))); + ResolvedRequest oneValue = resolveWithQuery(call, Map.of("tag", List.of("a"))); + + assertNotEquals(twoValues.fingerprint(), oneValue.fingerprint(), + "dropping a repeated value changes what the request does and must change the hash"); + } + + private ResolvedRequest resolveWithQuery(ApiCall call, Map> queryParams) throws Exception { + Map requestMap = new HashMap<>(); + requestMap.put("uri", "http://example.com/x"); + requestMap.put("method", "GET"); + requestMap.put("headers", new LinkedHashMap()); + requestMap.put("queryParams", new LinkedHashMap<>(queryParams)); + when(mockRequest.toMap()).thenReturn(requestMap); + return executor.resolve(call, memory, new HashMap<>(), "http://example.com"); + } + + /** A request map shaped like the one HttpClientWrapper hands back. */ + private void stubRequestMap() { + Map requestMap = new HashMap<>(); + requestMap.put("uri", "http://example.com/api/test"); + requestMap.put("method", "POST"); + requestMap.put("headers", new LinkedHashMap()); + requestMap.put("queryParams", new LinkedHashMap>()); + when(mockRequest.toMap()).thenReturn(requestMap); + } + + @Test + @DisplayName("an EMPTY preRequest.propertyInstructions list still makes the call unpinnable") + void resolve_withEmptyPropertyInstructions_isNotPinned() throws Exception { + // The fail-open this closes. The old predicate used isNullOrEmpty, so an + // empty list read as "absent" and the call was PINNED β€” while + // PrePostUtils guards on != null and therefore still re-runs + // memoryItemConverter.convert, discarding the model arguments merged in + // for this call. Gate time and resume time both skip that (both go + // through resolve), so they agreed with each other and the guard passed + // while execute() sent a request with every {arg} rendered empty. + ApiCall call = createSimpleApiCall("empty-instructions-call", false); + var preRequest = new HttpPreRequest(); + preRequest.setPropertyInstructions(new java.util.ArrayList<>()); + call.setPreRequest(preRequest); + stubRequestMap(); + + ResolvedRequest resolved = executor.resolve(call, memory, new HashMap<>(), "http://example.com"); + + assertNull(resolved.fingerprint(), "an empty-but-present instruction list must not be treated as absent"); + assertFalse(resolved.isPinned()); + // Unpinnable is not unreviewable: the approver still gets a preview. + assertNotNull(resolved.uri()); + } + + @Test + @DisplayName("fireAndForget with batchRequests is unpinnable β€” one resolved request cannot stand for N") + void resolve_withFireAndForgetBatch_isNotPinned() throws Exception { + // execute() routes these to executeFireAndForgetCalls, which calls + // buildRequest once PER iteration object. resolve() builds exactly one, + // with the iteration variable empty. batchRequests is a different field + // from propertyInstructions, so this used to be pinned: the approver saw + // one request, the re-check compared that same never-sent request, and N + // unapproved requests went out on a background thread. + ApiCall call = createSimpleApiCall("batch-call", false); + call.setFireAndForget(true); + var preRequest = new HttpPreRequest(); + preRequest.setBatchRequests(new BatchRequestBuildingInstruction()); + call.setPreRequest(preRequest); + stubRequestMap(); + + ResolvedRequest resolved = executor.resolve(call, memory, new HashMap<>(), "http://example.com"); + + assertNull(resolved.fingerprint(), "a batched fire-and-forget call must not claim a fingerprint"); + assertFalse(resolved.isPinned()); + } + + @Test + @DisplayName("a retryable call is unpinnable β€” attempts 2..N are rebuilt from mutated template data") + void resolve_withRetryInstruction_isNotPinned() throws Exception { + // buildRequest sits INSIDE execute()'s retry do-while, and between + // attempts the shared templateDataObjects map gains the response object, + // its error, its httpCode and the response headers. A call templating any + // of those sends attempts 2..N as requests nobody resolved, previewed or + // fingerprinted β€” while the approver saw only attempt 1. Same "one + // resolved request cannot stand for N" argument as the batched + // fire-and-forget case. + ApiCall call = createSimpleApiCall("retry-call", false); + var postResponse = new HttpPostResponse(); + var retry = new RetryApiCallInstruction(); + retry.setMaxRetries(2); + postResponse.setRetryApiCallInstruction(retry); + call.setPostResponse(postResponse); + stubRequestMap(); + + ResolvedRequest resolved = executor.resolve(call, memory, new HashMap<>(), "http://example.com"); + + assertNull(resolved.fingerprint(), "a call that can retry must not claim a fingerprint"); + assertFalse(resolved.isPinned()); + } + + @Test + @DisplayName("a retry instruction that cannot fire (maxRetries 0) stays pinned") + void resolve_withDisabledRetryInstruction_remainsPinned() throws Exception { + // Mirrors retryCall()'s own test (maxRetries >= 1), so a present-but-inert + // instruction does not needlessly unpin an otherwise verifiable call. + ApiCall call = createSimpleApiCall("no-retry-call", false); + var postResponse = new HttpPostResponse(); + var retry = new RetryApiCallInstruction(); + retry.setMaxRetries(0); + postResponse.setRetryApiCallInstruction(retry); + call.setPostResponse(postResponse); + stubRequestMap(); + + ResolvedRequest resolved = executor.resolve(call, memory, new HashMap<>(), "http://example.com"); + + assertNotNull(resolved.fingerprint(), "an inert retry instruction must not unpin the call"); + } + + @Test + @DisplayName("an ordinary call is still pinned β€” the divergence check is not a blanket opt-out") + void resolve_withOrdinaryCall_remainsPinned() throws Exception { + // The mirror direction. Widening the predicate must not quietly unpin + // everything, which would disable enforcement while every test above + // still passed. + ApiCall call = createSimpleApiCall("ordinary-call", false); + stubRequestMap(); + + ResolvedRequest resolved = executor.resolve(call, memory, new HashMap<>(), "http://example.com"); + + assertNotNull(resolved.fingerprint(), "an ordinary call must still be pinnable"); + assertTrue(resolved.isPinned()); + } + + @Test + @DisplayName("fireAndForget WITHOUT batchRequests stays pinned β€” it sends exactly one request") + void resolve_withPlainFireAndForget_remainsPinned() throws Exception { + ApiCall call = createSimpleApiCall("plain-fnf-call", false); + call.setFireAndForget(true); + stubRequestMap(); + + ResolvedRequest resolved = executor.resolve(call, memory, new HashMap<>(), "http://example.com"); + + assertNotNull(resolved.fingerprint(), "a single fire-and-forget request is still one request"); + } + + @Test + @DisplayName("a secret in the request BODY is scrubbed before persistence, not just headers") + void execute_secretInBody_isRedacted() throws Exception { + // Header-name matching cannot see into a body. A config write (create an + // agent, set a provider key) carries its credential there, and this map is + // persisted to the conversation document. + ApiCall call = createSimpleApiCall("body-secret-call", false); + + Map requestMap = new HashMap<>(); + requestMap.put("headers", new LinkedHashMap()); + // Zero-entropy on purpose β€” see ResolvedRequestTest.BodyRedaction: the + // `sk-` shape is what the filter matches, the randomness is what trips + // the repo's secret scanner. + requestMap.put("body", "{\"apiKey\":\"sk-aaaaaaaaaaaaaaaaaaaaaaaaaa\",\"name\":\"billing\"}"); + when(mockRequest.toMap()).thenReturn(requestMap); + setupSuccessResponse(200, "ok", "text/plain"); + + executor.execute(call, memory, new HashMap<>(), "http://example.com"); + + var captor = ArgumentCaptor.forClass(Object.class); + verify(prePostUtils, atLeastOnce()).createMemoryEntry( + eq(currentStep), captor.capture(), contains("Request"), eq("httpCalls")); + @SuppressWarnings("unchecked") + var capturedMap = (Map) captor.getValue(); + String persistedBody = String.valueOf(capturedMap.get("body")); + assertFalse(persistedBody.contains("sk-aaaaaaaaaaaaaaaaaaaaaaaaaa"), persistedBody); + assertTrue(persistedBody.contains("REDACTED"), persistedBody); + // Over-redaction would make the debug record useless β€” the rest survives. + assertTrue(persistedBody.contains("billing"), persistedBody); + } + + @Test + @DisplayName("a credential in a QUERY parameter is scrubbed before persistence, and the live request is untouched") + void execute_secretInQueryParam_isRedactedWithoutCorruptingTheRequest() throws Exception { + ApiCall call = createSimpleApiCall("query-secret-call", false); + + Map requestMap = new HashMap<>(); + requestMap.put("headers", new LinkedHashMap()); + // The live map RequestWrapper#toMap hands back by reference, not a copy. + Map> liveQueryParams = new LinkedHashMap<>(); + liveQueryParams.put("api_key", new ArrayList<>(List.of("super-secret-value"))); + liveQueryParams.put("version", new ArrayList<>(List.of("3"))); + requestMap.put("queryParams", liveQueryParams); + when(mockRequest.toMap()).thenReturn(requestMap); + setupSuccessResponse(200, "ok", "text/plain"); + + executor.execute(call, memory, new HashMap<>(), "http://example.com"); + + var captor = ArgumentCaptor.forClass(Object.class); + verify(prePostUtils, atLeastOnce()).createMemoryEntry( + eq(currentStep), captor.capture(), contains("Request"), eq("httpCalls")); + @SuppressWarnings("unchecked") + var capturedMap = (Map) captor.getValue(); + String persisted = String.valueOf(capturedMap.get("queryParams")); + assertFalse(persisted.contains("super-secret-value"), persisted); + assertTrue(persisted.contains(""), persisted); + assertTrue(persisted.contains("3"), "an ordinary parameter must survive: " + persisted); + + // The entry is REPLACED, never rewritten in place β€” the request that was + // already sent still carries its real credential. + assertEquals(List.of("super-secret-value"), liveQueryParams.get("api_key"), + "redacting the debug record must not mutate the live request"); + } + @Test void execute_sensitiveHeaders_areScrubbed() throws Exception { ApiCall call = createSimpleApiCall("scrub-call", false); @@ -390,7 +630,8 @@ void execute_callerReferenceInPath_isRejectedClearly() { realContext.bind(new CallerIdentity("caller-jwt-value", "alice", "https://eddi.example:443")); var realResolver = new CallerIdentityResolver(realContext, true); var executorWithRealResolver = new ApiCallExecutor(httpClient, jsonSerialization, runtime, prePostUtils, globalVariableResolver, - secretResolver, realResolver, realContext, false, DEFAULT_TIMEOUT_MILLIS, DEFAULT_MAX_RESPONSE_SIZE); + secretResolver, realResolver, realContext, new RequestRedactor(realResolver), false, DEFAULT_TIMEOUT_MILLIS, + DEFAULT_MAX_RESPONSE_SIZE); try { ApiCall call = createSimpleApiCall("path-ref-call", false); call.getRequest().setPath("/users/${caller:userId}/profile"); @@ -589,7 +830,8 @@ void execute_successfulSave_resultContainsHttpCode() throws Exception { @Test void execute_ssrfProtectionEnabled_blocksInternalUrl() { ApiCallExecutor protectedExecutor = new ApiCallExecutor(httpClient, jsonSerialization, runtime, prePostUtils, globalVariableResolver, - secretResolver, callerIdentityResolver, callerIdentityContext, true, DEFAULT_TIMEOUT_MILLIS, DEFAULT_MAX_RESPONSE_SIZE); + secretResolver, callerIdentityResolver, callerIdentityContext, new RequestRedactor(callerIdentityResolver), true, + DEFAULT_TIMEOUT_MILLIS, DEFAULT_MAX_RESPONSE_SIZE); ApiCall call = createSimpleApiCall("ssrf-call", false); // 169.254.169.254 is a literal IP (no DNS) blocked by UrlValidationUtils. assertThrows(LifecycleException.class, () -> protectedExecutor.execute(call, memory, new HashMap<>(), "http://169.254.169.254")); @@ -598,7 +840,8 @@ void execute_ssrfProtectionEnabled_blocksInternalUrl() { @Test void execute_ssrfProtectionEnabled_disablesRedirectsOnPublicUrl() throws Exception { ApiCallExecutor protectedExecutor = new ApiCallExecutor(httpClient, jsonSerialization, runtime, prePostUtils, globalVariableResolver, - secretResolver, callerIdentityResolver, callerIdentityContext, true, DEFAULT_TIMEOUT_MILLIS, DEFAULT_MAX_RESPONSE_SIZE); + secretResolver, callerIdentityResolver, callerIdentityContext, new RequestRedactor(callerIdentityResolver), true, + DEFAULT_TIMEOUT_MILLIS, DEFAULT_MAX_RESPONSE_SIZE); ApiCall call = createSimpleApiCall("redir-call", false); setupSuccessResponse(200, "ok", "text/plain"); // 1.1.1.1 is a public literal IP β€” passes validation without a DNS lookup. diff --git a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorValidationErrorTest.java b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorValidationErrorTest.java index 5aad1ab73e..db7174912b 100644 --- a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorValidationErrorTest.java +++ b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorValidationErrorTest.java @@ -77,9 +77,10 @@ void setUp() throws Exception { GlobalVariableResolver globalVariableResolver = mock(GlobalVariableResolver.class); when(globalVariableResolver.resolveValue(anyString())).thenAnswer(inv -> inv.getArgument(0)); + CallerIdentityResolver callerIdentityResolver = mock(CallerIdentityResolver.class); executor = new ApiCallExecutor(httpClient, jsonSerialization, runtime, prePostUtils, globalVariableResolver, secretResolver, - mock(CallerIdentityResolver.class), new CallerIdentityContext(null, null), false, DEFAULT_TIMEOUT_MILLIS, - DEFAULT_MAX_RESPONSE_SIZE); + callerIdentityResolver, new CallerIdentityContext(null, null), new RequestRedactor(callerIdentityResolver), false, + DEFAULT_TIMEOUT_MILLIS, DEFAULT_MAX_RESPONSE_SIZE); memory = mock(IConversationMemory.class); currentStep = mock(IWritableConversationStep.class); diff --git a/src/test/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactorTest.java b/src/test/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactorTest.java new file mode 100644 index 0000000000..528087488c --- /dev/null +++ b/src/test/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactorTest.java @@ -0,0 +1,222 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.modules.apicalls.impl; + +import ai.labs.eddi.engine.httpclient.IRequest; +import ai.labs.eddi.engine.security.CallerIdentityResolver; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.*; + +/** + * Direct tests for the one definition of "redacted request". + *

+ * The class had no dedicated test: header coverage came incidentally through + * {@code ApiCallExecutor}'s execute-path tests, and the resolve path β€” the one + * that feeds the approver's preview and the fingerprint β€” had none at all. Its + * own javadoc says the two consumers drifting apart IS the credential leak, so + * the properties below are asserted on the redactor itself rather than through + * whichever caller happened to exercise it. + */ +class RequestRedactorTest { + + // Zero-entropy but shape-correct: SecretRedactionFilter matches on shape, and + // a realistic-looking literal additionally trips the repo's gitleaks scan on a + // value that never authenticated against anything. Do not "improve" these. + private static final String KEY = "sk-aaaaaaaaaaaaaaaaaaaaaaaaaa"; + private static final String BEARER = "Bearer aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + + private RequestRedactor redactor; + + @BeforeEach + void setUp() { + var callerIdentityResolver = mock(CallerIdentityResolver.class); + // Pass the value through untouched unless a test says otherwise β€” this + // resolver only ever redacts the CURRENT caller's live token. + when(callerIdentityResolver.redactCallerToken(anyString(), anyString())) + .thenAnswer(invocation -> invocation.getArgument(0)); + redactor = new RequestRedactor(callerIdentityResolver); + } + + @Nested + @DisplayName("header values are judged by shape, not only by name") + class HeaderShape { + + @Test + void aConventionallyNamedHeaderIsRedacted() { + assertEquals(RequestRedactor.REDACTED, redactor.redactHeaderValue("Authorization", BEARER)); + assertEquals(RequestRedactor.REDACTED, redactor.redactHeaderValue("X-Api-Key", KEY)); + } + + @Test + void aSecretUnderAnUnconventionalHeaderNameIsStillRedacted() { + // The gap this closes. "x-client-auth" contains none of the sensitive + // name fragments, the value is not a vault reference, and it is not the + // current caller's token β€” so before the value-shape scan was wired in, + // it reached the approver and the conversation document verbatim, while + // the identical string in the body or a query parameter was caught. + String redacted = redactor.redactHeaderValue("X-Client-Auth", BEARER); + assertFalse(redacted.contains("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), redacted); + } + + @Test + void anApiKeyShapeUnderAnyHeaderNameIsRedacted() { + String redacted = redactor.redactHeaderValue("X-Custom", KEY); + assertFalse(redacted.contains(KEY), redacted); + } + + @Test + void aHeaderNamedPasswordIsRedactedByNameAlone() { + // "hunter2" alone matches no value shape β€” SecretRedactionFilter's + // generic rule needs the credential name INSIDE the value (e.g. + // "password=hunter2"), not sitting in a separate header name. Only + // isSensitiveHeaderName can catch this, and until now it didn't + // recognize "password" despite this class's own javadoc listing it + // as a recognized credential name (see the generic-rule reference + // above redactUri). + assertEquals(RequestRedactor.REDACTED, redactor.redactHeaderValue("X-Password", "hunter2")); + } + + @Test + void anOrdinaryHeaderIsLeftIntact() { + // Over-redaction is its own failure: an approver who cannot read the + // request cannot meaningfully approve it. + assertEquals("application/json", redactor.redactHeaderValue("Content-Type", "application/json")); + } + + @Test + void anUnresolvedVaultReferenceIsRedacted() { + assertEquals(RequestRedactor.REDACTED, redactor.redactHeaderValue("X-Custom", "${vault:billing-key}")); + } + + @Test + void aNullOrNonStringValueDoesNotThrow() { + assertNull(redactor.redactHeaderValue("X-Custom", null)); + assertEquals("42", redactor.redactHeaderValue("X-Custom", 42)); + } + } + + @Nested + @DisplayName("redactRequestMap covers every channel a credential can ride") + class RequestMap { + + @Test + void uriHeadersQueryAndBodyAreAllRedacted() { + // The class invariant: one definition, and no channel left out. Each of + // these four has been the leak at some point. + var map = new HashMap(); + map.put(IRequest.KEY_URI, "https://x/y?api_key=" + KEY); + map.put(IRequest.KEY_HEADERS, Map.of("X-Client-Auth", BEARER)); + map.put(IRequest.KEY_QUERY_PARAMS, Map.of("api_key", List.of(KEY))); + map.put(IRequest.KEY_BODY, "{\"apiKey\":\"" + KEY + "\"}"); + + redactor.redactRequestMap(map); + + assertFalse(map.get(IRequest.KEY_URI).toString().contains(KEY), "uri: " + map.get(IRequest.KEY_URI)); + assertFalse(map.get(IRequest.KEY_HEADERS).toString().contains("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), + "headers: " + map.get(IRequest.KEY_HEADERS)); + assertFalse(map.get(IRequest.KEY_QUERY_PARAMS).toString().contains(KEY), + "query: " + map.get(IRequest.KEY_QUERY_PARAMS)); + assertFalse(map.get(IRequest.KEY_BODY).toString().contains(KEY), "body: " + map.get(IRequest.KEY_BODY)); + } + + @Test + void aHostThatLooksLikeASecretNameKeepsItsUriIntact() { + // SecretRedactionFilter's generic rule matches name[=:]<8+ chars> and + // its trailing class does not exclude '/', so scanning a whole URI + // consumed everything after a host ending in one of those words plus a + // port. The approver was then shown "https://vault-secret=" + // β€” no host, no path, not a URI. Losing the target of a write is worse + // than the leak the scan defends against. + for (String host : List.of("vault-secret", "token", "authorization", "my-password")) { + String uri = "https://" + host + ":8200/v1/agentstore/agents/a1"; + var map = new HashMap(); + map.put(IRequest.KEY_URI, uri); + redactor.redactRequestMap(map); + assertEquals(uri, map.get(IRequest.KEY_URI), "host '" + host + "' must stay legible"); + } + } + + @Test + void aSecretInThePathIsStillRedactedDespiteTheAuthorityCarveOut() { + // The carve-out must not become a bypass: the path is where a + // templated credential actually lands, and it is still scanned. + var map = new HashMap(); + map.put(IRequest.KEY_URI, "https://token:8200/v1/keys/" + KEY + "/rotate"); + redactor.redactRequestMap(map); + String redacted = map.get(IRequest.KEY_URI).toString(); + assertFalse(redacted.contains(KEY), redacted); + assertTrue(redacted.startsWith("https://token:8200/"), redacted); + } + + @Test + void aQueryParamNamedPasswordIsRedactedByNameAlone() { + // redactQueryParamValue shares isSensitiveHeaderName with header + // redaction β€” this is the same name-check gap, on the other channel + // it feeds. + var map = new HashMap(); + map.put(IRequest.KEY_URI, "https://x/y?password=hunter2"); + redactor.redactRequestMap(map); + assertFalse(map.get(IRequest.KEY_URI).toString().contains("hunter2"), map.get(IRequest.KEY_URI).toString()); + } + + @Test + void aPercentEncodedCredentialInTheUriIsStillRedacted() { + // The scan must see what the value IS. HttpClientWrapper decodes into + // queryParamsMap but toMap() hands back the raw uri, so the same + // credential arrives here encoded β€” and encoding defeats the shape + // rules: "Bearer aaa…" becomes "Bearer%20aaa…", which no longer + // matches Bearer\s+. That produced a value redacted in queryParams and + // plaintext one field away in uri. + var map = new HashMap(); + map.put(IRequest.KEY_URI, "https://x/y?auth=" + java.net.URLEncoder.encode(BEARER, java.nio.charset.StandardCharsets.UTF_8)); + redactor.redactRequestMap(map); + String redacted = map.get(IRequest.KEY_URI).toString(); + assertFalse(redacted.contains("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"), redacted); + } + + @Test + void anEncodedVaultReferenceIsStillRedacted() { + var map = new HashMap(); + map.put(IRequest.KEY_URI, "https://x/y?k=" + java.net.URLEncoder.encode("${vault:billing}", java.nio.charset.StandardCharsets.UTF_8)); + redactor.redactRequestMap(map); + assertFalse(map.get(IRequest.KEY_URI).toString().contains("billing"), map.get(IRequest.KEY_URI).toString()); + } + + @Test + void anOrdinaryEncodedValueKeepsItsONTHEWIREForm() { + // Only a value the scan actually hit is replaced. Everything else is + // emitted as-is, so the preview keeps showing what is genuinely sent + // rather than a decoded approximation of it. + var map = new HashMap(); + map.put(IRequest.KEY_URI, "https://x/y?q=hello%20world&n=1"); + redactor.redactRequestMap(map); + assertEquals("https://x/y?q=hello%20world&n=1", map.get(IRequest.KEY_URI)); + } + + @Test + void aNullMapDoesNotThrow() { + assertDoesNotThrow(() -> redactor.redactRequestMap(null)); + } + + @Test + void absentEntriesAreSimplyNotTouched() { + var map = new HashMap(); + map.put(IRequest.KEY_URI, "https://x/y"); + redactor.redactRequestMap(map); + assertEquals("https://x/y", map.get(IRequest.KEY_URI)); + assertFalse(map.containsKey(IRequest.KEY_BODY)); + } + } +} diff --git a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequestTest.java b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequestTest.java new file mode 100644 index 0000000000..d8c7dab65d --- /dev/null +++ b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequestTest.java @@ -0,0 +1,371 @@ +/* + * Copyright EDDI contributors + * SPDX-License-Identifier: Apache-2.0 + */ +package ai.labs.eddi.modules.apicalls.impl; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * The fingerprint is what makes approval bind to a request rather than a tool + * name, so these tests are about one question: can two requests that would do + * different things share a fingerprint? + */ +class ResolvedRequestTest { + + /** + * Single-valued query params, spelled the way callers usually think of them. + */ + private static ResolvedRequest request(String method, String uri, Map query, Map headers, String body) { + var multi = new LinkedHashMap>(); + query.forEach((key, value) -> multi.put(key, List.of(value))); + return ResolvedRequest.of(method, uri, multi, headers, body, true); + } + + private static ResolvedRequest baseline() { + return request("POST", "https://eddi.example/agentstore/agents", Map.of("version", "1"), Map.of("Content-Type", "application/json"), + "{\"name\":\"ops\"}"); + } + + @Nested + @DisplayName("what must NOT change the fingerprint") + class Stable { + + @Test + void identicalRequestsAgree() { + assertEquals(baseline().fingerprint(), baseline().fingerprint()); + } + + @Test + void headerOrderDoesNotMatter() { + var first = new LinkedHashMap(); + first.put("Accept", "application/json"); + first.put("Content-Type", "application/json"); + var second = new LinkedHashMap(); + second.put("Content-Type", "application/json"); + second.put("Accept", "application/json"); + + assertEquals(request("GET", "https://x/y", Map.of(), first, null).fingerprint(), + request("GET", "https://x/y", Map.of(), second, null).fingerprint()); + } + + @Test + void headerNameCasingDoesNotMatter() { + // HTTP header names are case-insensitive, so casing cannot change what + // the request does and must not change the hash. + assertEquals(request("GET", "https://x/y", Map.of(), Map.of("Content-Type", "application/json"), null).fingerprint(), + request("GET", "https://x/y", Map.of(), Map.of("content-type", "application/json"), null).fingerprint()); + } + + @Test + void methodCasingDoesNotMatter() { + assertEquals(request("post", "https://x/y", Map.of(), Map.of(), null).fingerprint(), + request("POST", "https://x/y", Map.of(), Map.of(), null).fingerprint()); + } + + @Test + void credentialValuesCannotParticipateBecauseTheyAreAlreadyRedacted() { + // The property the whole design rests on: an approver is routinely not + // the requester, so the resolved Authorization header differs between + // gate time and execution time. Both arrive here redacted to the same + // marker, so the fingerprint is stable across approvers β€” and a guard + // that fired on every cross-user approval would simply be switched off. + var atGateTime = Map.of("Authorization", RequestRedactor.REDACTED); + var atExecutionTime = Map.of("Authorization", RequestRedactor.REDACTED); + assertEquals(request("POST", "https://x/y", Map.of(), atGateTime, "{}").fingerprint(), + request("POST", "https://x/y", Map.of(), atExecutionTime, "{}").fingerprint()); + } + } + + @Nested + @DisplayName("what MUST change the fingerprint") + class Discriminating { + + @Test + void method() { + assertNotEquals(baseline().fingerprint(), + request("DELETE", "https://eddi.example/agentstore/agents", Map.of("version", "1"), + Map.of("Content-Type", "application/json"), "{\"name\":\"ops\"}").fingerprint()); + } + + @Test + void targetUri() { + assertNotEquals(baseline().fingerprint(), + request("POST", "https://eddi.example/agentstore/agents/OTHER", Map.of("version", "1"), + Map.of("Content-Type", "application/json"), "{\"name\":\"ops\"}").fingerprint()); + } + + @Test + void queryParameterValue() { + assertNotEquals(baseline().fingerprint(), + request("POST", "https://eddi.example/agentstore/agents", Map.of("version", "99"), + Map.of("Content-Type", "application/json"), "{\"name\":\"ops\"}").fingerprint()); + } + + @Test + void anAddedQueryParameter() { + assertNotEquals(baseline().fingerprint(), + request("POST", "https://eddi.example/agentstore/agents", Map.of("version", "1", "force", "true"), + Map.of("Content-Type", "application/json"), "{\"name\":\"ops\"}").fingerprint()); + } + + @Test + void body() { + assertNotEquals(baseline().fingerprint(), + request("POST", "https://eddi.example/agentstore/agents", Map.of("version", "1"), + Map.of("Content-Type", "application/json"), "{\"name\":\"attacker\"}").fingerprint()); + } + + @Test + void aNonCredentialHeader() { + // Headers are not excluded wholesale β€” only credential VALUES are + // redacted. A changed X-Forwarded-Host still changes the request. + assertNotEquals(request("POST", "https://x/y", Map.of(), Map.of("X-Tenant", "acme"), "{}").fingerprint(), + request("POST", "https://x/y", Map.of(), Map.of("X-Tenant", "evil"), "{}").fingerprint()); + } + + @Test + void anAddedHeader() { + assertNotEquals(request("POST", "https://x/y", Map.of(), Map.of("X-Tenant", "acme"), "{}").fingerprint(), + request("POST", "https://x/y", Map.of(), Map.of("X-Tenant", "acme", "X-Override", "1"), "{}").fingerprint()); + } + } + + @Nested + @DisplayName("field boundaries cannot be forged") + class Injection { + + @Test + void bodyContentCannotImpersonateAHeaderField() { + // Without length prefixes, a canonical form of "name:value\n" lets a body + // containing a newline plus "header.x:..." produce the same byte stream + // as a genuine extra header β€” two different requests, one fingerprint. + var withHeader = request("POST", "https://x/y", Map.of(), Map.of("x", "1"), ""); + var withBodyPretendingToBeAHeader = request("POST", "https://x/y", Map.of(), Map.of(), "\nheader.x:1:1\n"); + assertNotEquals(withHeader.fingerprint(), withBodyPretendingToBeAHeader.fingerprint()); + } + + @Test + void movingContentBetweenAdjacentFieldsChangesIt() { + assertNotEquals(request("POST", "https://x/ab", Map.of(), Map.of(), "").fingerprint(), + request("POST", "https://x/a", Map.of(), Map.of(), "b").fingerprint()); + } + + @Test + void aRepeatedQueryParameterCannotBeForgedByOneValueContainingTheSeparator() { + // The display form joins repeats with ", ". If the fingerprint were + // computed over THAT, then ?tag=a&tag=b and a single tag whose value is + // literally "a, b" would hash identically β€” two different requests, one + // fingerprint. The canonical form emits one length-prefixed field per + // value instead, so the two stay distinct. + var repeated = new LinkedHashMap>(); + repeated.put("tag", List.of("a", "b")); + var singleJoined = new LinkedHashMap>(); + singleJoined.put("tag", List.of("a, b")); + + assertNotEquals(ResolvedRequest.of("GET", "https://x/y", repeated, Map.of(), null, true).fingerprint(), + ResolvedRequest.of("GET", "https://x/y", singleJoined, Map.of(), null, true).fingerprint()); + } + + @Test + void reorderingRepeatedValuesChangesIt() { + // ?tag=a&tag=b and ?tag=b&tag=a are different requests to any server + // that reads the first value, so order within a name is preserved. + var forward = new LinkedHashMap>(); + forward.put("tag", List.of("a", "b")); + var reversed = new LinkedHashMap>(); + reversed.put("tag", List.of("b", "a")); + + assertNotEquals(ResolvedRequest.of("GET", "https://x/y", forward, Map.of(), null, true).fingerprint(), + ResolvedRequest.of("GET", "https://x/y", reversed, Map.of(), null, true).fingerprint()); + } + + @Test + void anEmptyValueIsDistinctFromAnAbsentOne() { + assertNotEquals(request("POST", "https://x/y", Map.of("a", ""), Map.of(), null).fingerprint(), + request("POST", "https://x/y", Map.of(), Map.of(), null).fingerprint()); + } + } + + @Nested + @DisplayName("unpinnable calls") + class Unpinned { + + @Test + void produceNoFingerprintButStillPreview() { + var resolved = ResolvedRequest.of("POST", "https://x/y", Map.of(), Map.of("Accept", "*/*"), "{}", false); + assertNull(resolved.fingerprint()); + assertFalse(resolved.isPinned()); + // The preview is the point of resolving at all β€” it survives. + assertEquals("https://x/y", resolved.uri()); + assertEquals("{}", resolved.body()); + assertEquals(Map.of("accept", "*/*"), resolved.headers()); + } + + @Test + void pinnedOnesReportSo() { + assertTrue(baseline().isPinned()); + } + } + + @Nested + @DisplayName("the stored body is redacted, the fingerprinted one is not") + class BodyRedaction { + + // Deliberately zero-entropy. These have to carry the `sk-` + 20 chars + // shape, because that shape is exactly what SecretRedactionFilter's + // OpenAI rule matches and what these tests assert on β€” but a realistic + // random-looking value additionally trips the repo's gitleaks scan, which + // then fails CI on a literal that never authenticated against anything. + // Repeated characters keep the shape and drop the entropy. Do not + // "improve" these into realistic keys. + private static final String KEY = "sk-aaaaaaaaaaaaaaaaaaaaaaaaaa"; + private static final String OTHER_KEY = "sk-bbbbbbbbbbbbbbbbbbbbbbbbbb"; + + @Test + void aSecretInTheBodyNeverReachesTheStoredCopy() { + // The approver is routinely not the requester, so anything kept here is + // shown to someone who was never entrusted with it. + var resolved = request("POST", "https://x/y", Map.of(), Map.of(), "{\"apiKey\":\"" + KEY + "\"}"); + assertFalse(resolved.body().contains(KEY), resolved.body()); + assertTrue(resolved.body().contains("REDACTED"), resolved.body()); + } + + @Test + void twoDifferentSecretsDoNotShareAFingerprint() { + // The reason the body is hashed RAW. Redacting first collapses both of + // these to "sk-", so a swapped credential would sail through + // the pre-execution re-check as an unchanged request. + assertNotEquals(request("POST", "https://x/y", Map.of(), Map.of(), "{\"apiKey\":\"" + KEY + "\"}").fingerprint(), + request("POST", "https://x/y", Map.of(), Map.of(), "{\"apiKey\":\"" + OTHER_KEY + "\"}").fingerprint()); + } + + @Test + void theSameSecretStillAgreesWithItself() { + // Redaction must not make the fingerprint unstable either β€” the whole + // guard is useless if an unchanged request fails its own re-check. + assertEquals(request("POST", "https://x/y", Map.of(), Map.of(), "{\"apiKey\":\"" + KEY + "\"}").fingerprint(), + request("POST", "https://x/y", Map.of(), Map.of(), "{\"apiKey\":\"" + KEY + "\"}").fingerprint()); + } + + @Test + void nonSecretBodyContentIsLeftAlone() { + // Over-redaction is its own failure: an approver who cannot read the + // request cannot meaningfully approve it. + var resolved = request("POST", "https://x/y", Map.of(), Map.of(), "{\"name\":\"billing-agent\",\"maxTurns\":5}"); + assertEquals("{\"name\":\"billing-agent\",\"maxTurns\":5}", resolved.body()); + } + + @Test + void aVaultReferenceIsRedactedToo() { + var resolved = request("POST", "https://x/y", Map.of(), Map.of(), "{\"apiKey\":\"${vault:openai-key}\"}"); + assertFalse(resolved.body().contains("openai-key"), resolved.body()); + } + + @Test + void aCredentialInAQueryParameterIsRedactedToo() { + // ?api_key=… is a conventional way to pass a credential, and the query + // string is shown to the approver exactly like the body is. + var query = new LinkedHashMap>(); + query.put("api_key", List.of(KEY)); + var resolved = ResolvedRequest.of("GET", "https://x/y", query, Map.of(), null, true); + + assertFalse(resolved.queryParams().get("api_key").contains(KEY), resolved.queryParams().toString()); + assertEquals(RequestRedactor.REDACTED, resolved.queryParams().get("api_key")); + } + + @Test + void twoDifferentQueryCredentialsDoNotShareAFingerprint() { + // Same reason the body is hashed raw: redacting first would collapse + // both to one marker and let a swapped key pass the re-check. + var first = new LinkedHashMap>(); + first.put("api_key", List.of(KEY)); + var second = new LinkedHashMap>(); + second.put("api_key", List.of(OTHER_KEY)); + + assertNotEquals(ResolvedRequest.of("GET", "https://x/y", first, Map.of(), null, true).fingerprint(), + ResolvedRequest.of("GET", "https://x/y", second, Map.of(), null, true).fingerprint()); + } + + @Test + void aCredentialInTheURIItselfIsRedacted() { + // The leak this case exists for. A credential templated into an + // httpcall's path is resolved to its live value BEFORE the URI is + // built, so it arrives here as plaintext. It was previously redacted + // in queryParams and shown verbatim in uri β€” the same secret, two + // adjacent fields of one JSON object handed to an approver. + var resolved = ResolvedRequest.of("GET", "https://x/y?api_key=" + KEY, Map.of(), Map.of(), null, true); + + assertFalse(resolved.uri().contains(KEY), resolved.uri()); + assertTrue(resolved.uri().contains("REDACTED"), resolved.uri()); + // The rest of the URI must survive β€” an approver who cannot see which + // host and path is being called cannot evaluate the request at all. + assertTrue(resolved.uri().startsWith("https://x/y?api_key="), resolved.uri()); + } + + @Test + void aSecretShapedValueAnywhereInTheURIIsRedacted() { + // Not only the query string: userinfo and path segments carry them too. + var inUserInfo = ResolvedRequest.of("GET", "https://user:" + KEY + "@x/y", Map.of(), Map.of(), null, true); + assertFalse(inUserInfo.uri().contains(KEY), inUserInfo.uri()); + + var inPath = ResolvedRequest.of("GET", "https://x/keys/" + KEY + "/rotate", Map.of(), Map.of(), null, true); + assertFalse(inPath.uri().contains(KEY), inPath.uri()); + } + + @Test + void twoDifferentURICredentialsDoNotShareAFingerprint() { + // The uri is hashed RAW and stored REDACTED, exactly like the body and + // query β€” so swapping one credential for another still moves the hash + // and is refused by the pre-execution re-check. + assertNotEquals(ResolvedRequest.of("GET", "https://x/y?api_key=" + KEY, Map.of(), Map.of(), null, true).fingerprint(), + ResolvedRequest.of("GET", "https://x/y?api_key=" + OTHER_KEY, Map.of(), Map.of(), null, true).fingerprint()); + } + + @Test + void anOrdinaryURIIsLeftIntact() { + // Over-redaction is its own failure mode: the method and path are the + // first thing an approver reads. + var resolved = ResolvedRequest.of("PATCH", "https://eddi.example/descriptorstore/descriptors/abc?version=3", + Map.of(), Map.of(), null, true); + assertEquals("https://eddi.example/descriptorstore/descriptors/abc?version=3", resolved.uri()); + } + + @Test + void anOrdinaryQueryParameterSurvivesUnredacted() { + var query = new LinkedHashMap>(); + query.put("version", List.of("3")); + assertEquals("3", ResolvedRequest.of("GET", "https://x/y", query, Map.of(), null, true).queryParams().get("version")); + } + + @Test + void anUnpinnableCallStillGetsARedactedBody() { + // No fingerprint to protect here, but the preview is still shown to a + // human β€” redaction is not conditional on pinning. + var resolved = ResolvedRequest.of("POST", "https://x/y", Map.of(), Map.of(), "{\"apiKey\":\"" + KEY + "\"}", false); + assertNull(resolved.fingerprint()); + assertFalse(resolved.body().contains(KEY), resolved.body()); + } + } + + @Test + void nullsAreToleratedRatherThanThrowing() { + // A call with no body, no query and no headers is ordinary, not an error. + var resolved = ResolvedRequest.of("GET", "https://x/y", null, null, null, true); + assertTrue(resolved.isPinned()); + assertEquals(Map.of(), resolved.queryParams()); + assertEquals(Map.of(), resolved.headers()); + } +} diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.java index 481bb7dbcd..6731fb5454 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.java @@ -15,6 +15,9 @@ import ai.labs.eddi.engine.hitl.tools.ToolApprovalGate; import ai.labs.eddi.engine.hitl.tools.ToolApprovalRequiredException; import ai.labs.eddi.engine.lifecycle.model.HitlDecision; +import io.micrometer.core.instrument.Metrics; +import io.micrometer.core.instrument.simple.SimpleMeterRegistry; +import org.junit.jupiter.api.BeforeAll; import ai.labs.eddi.engine.lifecycle.model.ToolCallDecision; import ai.labs.eddi.engine.memory.IConversationMemory; import ai.labs.eddi.engine.memory.IMemoryItemConverter; @@ -23,7 +26,10 @@ import ai.labs.eddi.engine.runtime.client.configuration.IResourceClientLibrary; import ai.labs.eddi.engine.tenancy.TenantQuotaService; import ai.labs.eddi.engine.tenancy.model.QuotaCheckResult; +import ai.labs.eddi.engine.lifecycle.exceptions.LifecycleException; import ai.labs.eddi.modules.apicalls.impl.IApiCallExecutor; +import ai.labs.eddi.modules.apicalls.impl.RequestRedactor; +import ai.labs.eddi.modules.apicalls.impl.ResolvedRequest; import ai.labs.eddi.modules.llm.model.LlmConfiguration; import ai.labs.eddi.modules.llm.tools.ToolExecutionService; import ai.labs.eddi.modules.llm.tools.ToolInvocation; @@ -79,6 +85,23 @@ */ class AgentOrchestratorCoverageTest { + /** + * {@code recordWriteApprovalDecision} writes to the process-wide + * {@code Metrics.globalRegistry}. Outside a running Quarkus app that registry + * is a bare {@code CompositeMeterRegistry} with no backing registry attached β€” + * meters register and {@code increment()} without throwing, but nothing + * actually stores a count, so every read-back is a silent 0. A real backing + * registry has to be attached before these tests can observe anything at all. + * Guarded so repeat attachment across test classes in the same JVM fork is a + * no-op rather than an error. + */ + @BeforeAll + static void attachMeterRegistryBackingStore() { + if (Metrics.globalRegistry.getRegistries().isEmpty()) { + Metrics.addRegistry(new SimpleMeterRegistry()); + } + } + @Mock private CalculatorTool calculatorTool; @Mock @@ -890,7 +913,7 @@ void tenantQuotaServiceNull_skipsQuotaCheck() throws Exception { // ═══════════════════════════════════════════════════════════════════ private AgentOrchestrator.ToolSetup setupWith(List all, List builtIn) { - return new AgentOrchestrator.ToolSetup(all, Map.of(), Map.of(), builtIn, Map.of(), Map.of()); + return new AgentOrchestrator.ToolSetup(all, Map.of(), Map.of(), builtIn, Map.of(), Map.of(), Map.of()); } private ToolSpecification spec(String name) { @@ -1171,6 +1194,202 @@ void buildPendingBatch_overloadDelegates_usesDefaultCap() { assertFalse(batch.isTranscriptOmitted()); } + /** One gated http call, with whatever resolver the test wants to supply. */ + private PendingToolCallBatch batchWithResolver(AgentOrchestrator.ToolRequestResolver resolver) { + var deploy = ToolExecutionRequest.builder().id("c1").name("deployAgent").arguments("{\"id\":\"a1\"}").build(); + var gr = new ToolApprovalGate.GateResult(List.of(deploy), List.of(), Map.of("c1", "http.post:*")); + List msgs = List.of(UserMessage.from("deploy it"), AiMessage.from(List.of(deploy))); + return orchestrator.buildPendingBatch(msgs, gr, twoToolTask(), memory, 0, + List.of(), new ArrayList<>(), 1, 0, Map.of("deployAgent", "http"), + gateCalculate(), PendingToolCallBatch.TRANSCRIPT_MAX_BYTES_DEFAULT, + Map.of(), null, resolver == null ? Map.of() : Map.of("deployAgent", resolver)); + } + + @Test + void buildPendingBatch_pinsTheResolvedRequestSoApprovalBindsToItNotTheToolName() { + var resolved = ResolvedRequest.of("POST", "https://eddi.example/administration/production/deploy/a1", + Map.of("force", List.of("false")), Map.of("Authorization", RequestRedactor.REDACTED), "{\"id\":\"a1\"}", true); + + var call = batchWithResolver(req -> resolved).getCalls().get(0); + + assertTrue(call.isRequestPinned()); + assertEquals(resolved.fingerprint(), call.getRequestFingerprint()); + // The approver sees the real request, not an operationId. + assertEquals("POST", call.getRequestPreview().getMethod()); + assertEquals("https://eddi.example/administration/production/deploy/a1", call.getRequestPreview().getUri()); + assertEquals("{\"id\":\"a1\"}", call.getRequestPreview().getBody()); + assertFalse(call.getRequestPreview().isBodyTruncated()); + // Headers travel too, because the fingerprint covers them β€” approving what + // you were shown has to mean the whole of what is later checked. + assertEquals(RequestRedactor.REDACTED, call.getRequestPreview().getHeaders().get("authorization")); + } + + @Test + void buildPendingBatch_leavesACallUnpinnedWhenNothingCanResolveIt() { + // Every non-http tool: there is no HTTP request on this side of the + // boundary to pin, so the call is approved on name and arguments exactly + // as it was before pinning existed. + var call = batchWithResolver(null).getCalls().get(0); + + assertFalse(call.isRequestPinned()); + assertNull(call.getRequestFingerprint()); + assertNull(call.getRequestPreview()); + } + + @Test + void buildPendingBatch_survivesAResolverThatThrows() { + // A template error while previewing must not kill the turn. The pause is + // still built; the call is merely unpinned, which is the honest outcome + // for "we could not determine the request". + var batch = batchWithResolver(req -> { + throw new LifecycleException("template blew up", new RuntimeException()); + }); + + assertEquals(1, batch.getCalls().size()); + assertFalse(batch.getCalls().get(0).isRequestPinned()); + assertNotNull(batch.getPauseEpoch()); + } + + @Test + void buildPendingBatch_truncatesAnOversizeBodyForDisplayWithoutWeakeningTheFingerprint() { + String hugeBody = "x".repeat(PendingToolCallBatch.PREVIEW_BODY_MAX_BYTES + 500); + var resolved = ResolvedRequest.of("POST", "https://eddi.example/x", Map.of(), Map.of(), hugeBody, true); + + var call = batchWithResolver(req -> resolved).getCalls().get(0); + + assertTrue(call.getRequestPreview().isBodyTruncated()); + assertTrue(call.getRequestPreview().getBody().length() <= PendingToolCallBatch.PREVIEW_BODY_MAX_BYTES); + // The fingerprint was computed over the WHOLE body before capping, so a + // caller cannot hide a payload change past the display cut-off. + assertEquals(resolved.fingerprint(), call.getRequestFingerprint()); + assertNotEquals(ResolvedRequest.of("POST", "https://eddi.example/x", Map.of(), Map.of(), hugeBody + "y", true).fingerprint(), + call.getRequestFingerprint()); + } + + /** A pinned call as it would have been persisted at gate time. */ + private static PendingToolCallBatch.PendingToolCall pinnedCall(String fingerprint) { + var call = new PendingToolCallBatch.PendingToolCall(); + call.setCallId("c1"); + call.setToolName("deployAgent"); + call.setSource("http"); + call.setArgumentsRaw("{\"id\":\"a1\"}"); + call.setRequestFingerprint(fingerprint); + return call; + } + + private static ResolvedRequest approvedRequest() { + return ResolvedRequest.of("POST", "https://eddi.example/deploy/a1", Map.of(), Map.of(), "{\"id\":\"a1\"}", true); + } + + @Test + void requestChangedSinceApproval_allowsACallWhoseRequestStillMatches() { + var approved = approvedRequest(); + var result = orchestrator.requestChangedSinceApproval(pinnedCall(approved.fingerprint()), null, + Map.of("deployAgent", req -> approved)); + assertNull(result); + } + + @Test + void requestChangedSinceApproval_refusesACallWhoseRequestNoLongerMatches() { + // The whole point: the approver said yes to /deploy/a1, and something now + // resolves to a different target. It does not run. + var tampered = ResolvedRequest.of("POST", "https://eddi.example/deploy/PRODUCTION", Map.of(), Map.of(), "{\"id\":\"a1\"}", true); + var result = orchestrator.requestChangedSinceApproval(pinnedCall(approvedRequest().fingerprint()), null, + Map.of("deployAgent", req -> tampered)); + assertNotNull(result); + assertTrue(result.contains("no longer matches")); + } + + @Test + void requestChangedSinceApproval_ignoresACallThatWasNeverPinned() { + // Every non-http tool, and anything unresolvable at gate time. Enforcing + // here would refuse calls on a comparison that never existed. + var unpinned = pinnedCall(null); + assertNull(orchestrator.requestChangedSinceApproval(unpinned, null, Map.of())); + } + + @Test + void requestChangedSinceApproval_allowsAnAmendedCall() { + // The approver rewrote the arguments themselves, so the pin describes the + // request they replaced. Comparing against it would refuse every amendment. + var result = orchestrator.requestChangedSinceApproval(pinnedCall(approvedRequest().fingerprint()), "{\"id\":\"a2\"}", + Map.of("deployAgent", req -> ResolvedRequest.of("POST", "https://eddi.example/deploy/a2", Map.of(), Map.of(), "{}", true))); + assertNull(result); + } + + @Test + void requestChangedSinceApproval_failsClosedWhenTheToolVanishedAcrossThePause() { + // Pinned at gate time, unresolvable now β€” the agent was reconfigured while + // a human was deciding. We cannot show that what runs is what was + // approved, so it does not run. + var result = orchestrator.requestChangedSinceApproval(pinnedCall(approvedRequest().fingerprint()), null, Map.of()); + assertNotNull(result); + assertTrue(result.contains("no longer available")); + } + + @Test + void requestChangedSinceApproval_failsClosedWhenReResolutionThrows() { + var result = orchestrator.requestChangedSinceApproval(pinnedCall(approvedRequest().fingerprint()), null, + Map.of("deployAgent", req -> { + throw new LifecycleException("template blew up", new RuntimeException()); + })); + assertNotNull(result); + assertTrue(result.contains("could not be re-resolved")); + } + + @Test + void requestChangedSinceApproval_failsClosedWhenTheCallCanNoLongerBePinned() { + // Config gained a pre-request property instruction across the pause, so the + // request is no longer resolvable ahead of execution. Unverifiable is not + // the same as unchanged. + var unpinnable = ResolvedRequest.of("POST", "https://eddi.example/deploy/a1", Map.of(), Map.of(), "{\"id\":\"a1\"}", false); + var result = orchestrator.requestChangedSinceApproval(pinnedCall(approvedRequest().fingerprint()), null, + Map.of("deployAgent", req -> unpinnable)); + assertNotNull(result); + assertTrue(result.contains("could no longer be resolved")); + } + + /** Delta of the named decision-tagged counter across whatever `action` does. */ + private static double approvalCountDelta(String decision, Runnable action) { + double before = Metrics.globalRegistry.find("eddi.operator.write.approval").tag("decision", decision).counters().stream() + .mapToDouble(io.micrometer.core.instrument.Counter::count).sum(); + action.run(); + double after = Metrics.globalRegistry.find("eddi.operator.write.approval").tag("decision", decision).counters().stream() + .mapToDouble(io.micrometer.core.instrument.Counter::count).sum(); + return after - before; + } + + @Test + void recordWriteApprovalDecision_tagsAHumanApprovalAsApproved() { + assertEquals(1.0, approvalCountDelta("approved", + () -> orchestrator.recordWriteApprovalDecision(HitlDecision.HitlVerdict.APPROVED, "user:alice"))); + } + + @Test + void recordWriteApprovalDecision_tagsAHumanRejectionAsRejected() { + assertEquals(1.0, approvalCountDelta("rejected", + () -> orchestrator.recordWriteApprovalDecision(HitlDecision.HitlVerdict.REJECTED, "user:alice"))); + } + + @Test + void recordWriteApprovalDecision_tagsATimeoutAutoApproveAsTimeoutNotApproved() { + // The rubber-stamping signal this counter exists for ("approvals >> + // rejections") is meaningless if an unattended timeout auto-approval + // silently inflates "approved". It must land in its own bucket. + assertEquals(0.0, approvalCountDelta("approved", + () -> orchestrator.recordWriteApprovalDecision(HitlDecision.HitlVerdict.APPROVED, "system:timeout"))); + assertEquals(1.0, approvalCountDelta("timeout", + () -> orchestrator.recordWriteApprovalDecision(HitlDecision.HitlVerdict.APPROVED, "system:timeout"))); + } + + @Test + void recordWriteApprovalDecision_tagsATimeoutAutoRejectAsTimeoutNotRejected() { + assertEquals(0.0, approvalCountDelta("rejected", + () -> orchestrator.recordWriteApprovalDecision(HitlDecision.HitlVerdict.REJECTED, "system:timeout"))); + assertEquals(1.0, approvalCountDelta("timeout", + () -> orchestrator.recordWriteApprovalDecision(HitlDecision.HitlVerdict.REJECTED, "system:timeout"))); + } + @Test void buildPendingBatch_persistsTheGoverningRuleAndThePerCallMatch() { // The rule is resolved at gate time and must SURVIVE the pause: the persisted @@ -1191,7 +1410,7 @@ void buildPendingBatch_persistsTheGoverningRuleAndThePerCallMatch() { var batch = orchestrator.buildPendingBatch(msgs, gr, twoToolTask(), memory, 0, List.of(), new ArrayList<>(), 1, 0, Map.of("deployAgent", "http", "deleteAgent", "http"), gateCalculate(), PendingToolCallBatch.TRANSCRIPT_MAX_BYTES_DEFAULT, - Map.of("c1", deployRule, "c2", deleteRule), deleteRule); + Map.of("c1", deployRule, "c2", deleteRule), deleteRule, Map.of()); assertNotNull(batch.getEffectiveRule()); assertEquals("http.delete:*", batch.getEffectiveRule().getMatch()); diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.java index c6e7bce493..8729777f6d 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.java @@ -639,7 +639,7 @@ class HttpCallToolsResultTests { void testRecordFields() { var spec = ToolSpecification.builder().name("test").description("test").build(); var result = new AgentOrchestrator.HttpCallToolsResult( - List.of(spec), Map.of(), Map.of()); + List.of(spec), Map.of(), Map.of(), Map.of()); assertEquals(1, result.toolSpecs().size()); assertEquals("test", result.toolSpecs().get(0).name()); diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorResumeToolLoopTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorResumeToolLoopTest.java index 1ec9aa60ad..7a3b4438a9 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorResumeToolLoopTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorResumeToolLoopTest.java @@ -338,6 +338,112 @@ void rejectAll() throws Exception { assertTrue(rejectionMsg.contains("policy forbids this"), "note must be embedded in the envelope"); } + @Test + @DisplayName("a pinned call whose request MOVED is refused at the call site β€” not executed, not claimed") + void pinnedCallWithChangedRequestIsRefusedInTheLoop() throws Exception { + // requestChangedSinceApproval has good unit tests, but nothing exercised + // the branch that CALLS it: every test here builds a task with + // enableHttpCallTools=false, so toolRequestResolvers is always empty and + // the refusal is unreachable. The whole block could be deleted and the + // suite stayed green β€” i.e. pinning was proven to compute correctly and + // not proven to apply. This drives the real loop with a resolver present. + var task = twoToolTask(); + var r1 = ToolExecutionRequest.builder().id("c1").name("calculate").arguments("{\"expression\":\"6*7\"}").build(); + var gated = gatedCall("c1", "calculate", "{\"expression\":\"6*7\"}"); + gated.setRequestFingerprint("fingerprint-recorded-at-gate-time"); + var batch = batchWith(0, List.of(gated), List.of(r1)); + + // Re-resolution now yields a DIFFERENT fingerprint β€” the tamper case. + AgentOrchestrator.ToolRequestResolver movedResolver = req -> ai.labs.eddi.modules.apicalls.impl.ResolvedRequest.of("POST", + "https://eddi.example/agentstore/agents/attacker-choice", Map.of(), Map.of(), "{}", true); + var spied = spy(orchestrator); + doAnswer(invocation -> { + var real = (AgentOrchestrator.ToolSetup) invocation.callRealMethod(); + return new AgentOrchestrator.ToolSetup(real.toolSpecs(), real.toolExecutors(), real.toolSources(), + real.builtInSpecs(), real.toolCanonicalNames(), real.toolEndpoints(), Map.of("calculate", movedResolver)); + }).when(spied).buildToolSetup(any(), any()); + + ChatModel chatModel = mock(ChatModel.class); + var captor = ArgumentCaptor.forClass(ChatRequest.class); + when(chatModel.chat(captor.capture())).thenReturn(text("I could not perform that action.")); + + var result = spied.resumeToolLoop(chatModel, task, memory, batch, approveAll(), true); + + assertEquals("I could not perform that action.", result.response()); + // The three things the refusal must actually do, none of which the unit + // tests of the predicate could observe: + verify(calculatorTool, never()).calculate(anyString()); + verify(journalStore, never()).tryClaim(anyString(), anyString(), anyString(), anyString(), anyString()); + var refusal = captor.getValue().messages().stream() + .filter(m -> m instanceof ToolExecutionResultMessage) + .map(m -> ((ToolExecutionResultMessage) m).text()) + .filter(t -> t.contains("NOT_EXECUTED")) + .findFirst().orElse(null); + assertNotNull(refusal, "the model must be told the call did not run"); + assertTrue(refusal.contains("changed after it was approved"), refusal); + } + + @Test + @DisplayName("a pinned call whose request is UNCHANGED still executes β€” the guard is not a blanket refusal") + void pinnedCallWithMatchingRequestStillExecutes() throws Exception { + // The mirror direction. Without it, a guard that refused everything would + // pass the test above and silently break every gated write in production. + var task = twoToolTask(); + var r1 = ToolExecutionRequest.builder().id("c1").name("calculate").arguments("{\"expression\":\"6*7\"}").build(); + var gated = gatedCall("c1", "calculate", "{\"expression\":\"6*7\"}"); + + AgentOrchestrator.ToolRequestResolver stableResolver = req -> ai.labs.eddi.modules.apicalls.impl.ResolvedRequest.of("POST", + "https://eddi.example/agentstore/agents/a1", Map.of(), Map.of(), "{}", true); + // Pin it to whatever that resolver actually produces, so gate time and + // resume time genuinely agree. + gated.setRequestFingerprint(stableResolver.resolve(r1).fingerprint()); + var batch = batchWith(0, List.of(gated), List.of(r1)); + + var spied = spy(orchestrator); + doAnswer(invocation -> { + var real = (AgentOrchestrator.ToolSetup) invocation.callRealMethod(); + return new AgentOrchestrator.ToolSetup(real.toolSpecs(), real.toolExecutors(), real.toolSources(), + real.builtInSpecs(), real.toolCanonicalNames(), real.toolEndpoints(), Map.of("calculate", stableResolver)); + }).when(spied).buildToolSetup(any(), any()); + + when(journalStore.tryClaim(anyString(), anyString(), anyString(), anyString(), anyString())).thenReturn(true); + ChatModel chatModel = mock(ChatModel.class); + when(chatModel.chat(any(ChatRequest.class))).thenReturn(text("42")); + + spied.resumeToolLoop(chatModel, task, memory, batch, approveAll(), true); + + verify(journalStore).tryClaim(anyString(), anyString(), anyString(), anyString(), anyString()); + } + + @Test + @DisplayName("unresolved verdict (no top-level, no per-call override) fails closed: treated as REJECTED, not executed") + void unresolvedVerdictFailsClosed() throws Exception { + // ConversationService.resumeConversation rejects a null decision.verdict + // before this method is ever reached in production β€” every real caller + // (REST, Slack, MCP, timeout auto-resolution) already guarantees one. This + // constructs the otherwise-unreachable case directly (decision.verdict left + // unset, no per-call override for the pending call) to prove + // resumeToolLoop's OWN fallback also fails closed rather than trusting that + // upstream guarantee alone β€” the not-REJECTED-so-must-be-approved shape is + // exactly the fail-open Copilot flagged on AgentOrchestrator.java. + var task = twoToolTask(); + var r1 = ToolExecutionRequest.builder().id("c1").name("calculate").arguments("{\"expression\":\"6*7\"}").build(); + var batch = batchWith(0, List.of(gatedCall("c1", "calculate", "{\"expression\":\"6*7\"}")), List.of(r1)); + + ChatModel chatModel = mock(ChatModel.class); + when(chatModel.chat(any(ChatRequest.class))).thenReturn(text("I could not perform that action.")); + + var unresolved = new HitlDecision(); + unresolved.setDecidedBy("reviewer-1"); + // verdict deliberately left null. + + var result = orchestrator.resumeToolLoop(chatModel, task, memory, batch, unresolved, true); + + assertEquals("I could not perform that action.", result.response()); + verify(calculatorTool, never()).calculate(anyString()); + verify(journalStore, never()).tryClaim(anyString(), anyString(), anyString(), anyString(), anyString()); + } + @Test @DisplayName("mixed + amendment: approved executes with amended args, envelope argsAmendedByReviewer:true; rejected gets note") void mixedWithAmendment() throws Exception { diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java index 1154b7dc10..ca21fb3581 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java @@ -741,7 +741,7 @@ void httpCallToolsResult_recordCreation() { List specs = List.of(spec); Map executors = Map.of(); - var result = new AgentOrchestrator.HttpCallToolsResult(specs, executors, Map.of()); + var result = new AgentOrchestrator.HttpCallToolsResult(specs, executors, Map.of(), Map.of()); assertNotNull(result); assertEquals(1, result.toolSpecs().size()); @@ -903,7 +903,7 @@ void safeTemplateMerge_emptyArgs_noChange() throws Exception { @Test void httpCallToolsResult_emptySpecs() { - var result = new AgentOrchestrator.HttpCallToolsResult(List.of(), Map.of(), Map.of()); + var result = new AgentOrchestrator.HttpCallToolsResult(List.of(), Map.of(), Map.of(), Map.of()); assertTrue(result.toolSpecs().isEmpty()); assertTrue(result.executors().isEmpty()); diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorToolGovernanceTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorToolGovernanceTest.java index 0816431f47..7113c38158 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorToolGovernanceTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorToolGovernanceTest.java @@ -117,6 +117,50 @@ void skipsSpecWithoutExecutor() { assertTrue(specs.isEmpty()); assertFalse(executors.containsKey("orphan")); } + + @Test + @DisplayName("an http tool that LOSES a name collision does not keep its request resolver") + void droppedHttpToolLosesItsResolver() { + // Otherwise the builtin that won the name would be pinned against the + // dropped http tool's request: the approver is shown a preview of a + // request that will never run, and the pre-execution re-check compares + // against that same fabricated request and passes. + List specs = new ArrayList<>(List.of(ToolSpecification.builder().name("calculator").build())); + Map executors = new HashMap<>(Map.of("calculator", executor("builtin"))); + Map sources = new HashMap<>(Map.of("calculator", "builtin")); + + AgentOrchestrator.mergeExternalTools(List.of(ToolSpecification.builder().name("calculator").build()), + Map.of("calculator", executor("http")), "http", specs, executors, sources); + Map resolvers = new HashMap<>(); + resolvers.put("calculator", req -> { + throw new AssertionError("the dropped http tool's resolver must never be consulted"); + }); + + AgentOrchestrator.pruneResolversToSurvivingHttpTools(resolvers, sources); + + assertFalse(resolvers.containsKey("calculator"), "the losing http tool's resolver must be pruned"); + } + + @Test + @DisplayName("an http tool that WINS its name keeps its resolver β€” pruning is not a blanket wipe") + void survivingHttpToolKeepsItsResolver() { + List specs = new ArrayList<>(); + Map executors = new HashMap<>(); + Map sources = new HashMap<>(); + + AgentOrchestrator.mergeExternalTools(List.of(ToolSpecification.builder().name("deployAgent").build()), + Map.of("deployAgent", executor("http")), "http", specs, executors, sources); + // A later mcp tool of the same name is the one dropped here. + AgentOrchestrator.mergeExternalTools(List.of(ToolSpecification.builder().name("deployAgent").build()), + Map.of("deployAgent", executor("mcp")), "mcp", specs, executors, sources); + + Map resolvers = new HashMap<>(); + resolvers.put("deployAgent", req -> null); + + AgentOrchestrator.pruneResolversToSurvivingHttpTools(resolvers, sources); + + assertTrue(resolvers.containsKey("deployAgent"), "the http tool owns the name, so pinning must stay available"); + } } @Nested