feat(hitl): approval binds to the resolved request, not the tool name - #627
Merged
Conversation
Groundwork for binding a human approval to the request that actually
executes, rather than to a tool name. Nothing calls resolve() yet.
Today an approver of a gated tool call sees the tool's name and the model's
raw arguments. For a client generated from an OpenAPI spec that is close to
meaningless: the name comes from an operationId and says nothing about
which resource is written or with what body. Method, path, query and body
are only produced inside ApiCallExecutor#execute, after approval.
IApiCallExecutor#resolve now builds exactly that request and returns it
redacted, alongside a fingerprint.
Two design points worth stating, because both look like compromises:
The fingerprint covers the REDACTED request. That is the point, not a
concession. ApiCallExecutor resolves ${caller:token} into Authorization,
and on a resumed turn the caller is whoever approved the pause — routinely
not the person whose turn raised it. Fingerprinting the live header would
mismatch on every cross-user approval, i.e. on correct behaviour, until
someone switched the guard off. Redacting first makes the fingerprint
answer what approval is actually about: what the request does. Whose
credentials carry it is authentication's business.
resolve() does NOT run pre-request property instructions, because those
write to conversation memory and previewing a call must not change the
conversation. A call that has them therefore cannot be resolved to the
request execute() will build, so it comes back with a null fingerprint and
will simply not be enforced — rather than being failed on a comparison that
was never sound. Tools generated from a spec never carry them, so the
operator's writes are always pinned.
Canonicalisation is length-prefixed rather than delimiter-separated: a JSON
body can contain any delimiter, and without prefixes a body carrying a
newline could impersonate an extra header field and collide. Header names
are lowercased and both maps sorted, so casing and ordering — neither of
which changes what the request does — cannot change the hash.
Also extracts the header redaction ApiCallExecutor already did privately
into RequestRedactor, now shared by the memory scrub and the approval
preview. Two copies of "what counts as a credential" would eventually
disagree, and the one that drifted would leak. The toMap() key names move
onto IRequest for the same reason — readers of that map should not
re-spell the strings.
Each gated httpcall tool now resolves to the request it would send, and the pause carries both a redacted preview of it and its fingerprint. Nothing enforces the fingerprint yet — that is the next commit; this one only makes the pause record the truth. The preview replaces guessing. An approver previously saw a tool name and the model's raw arguments, and the Manager reconstructed a method and path client-side by looking the operationId up in a spec it fetched separately. That reconstruction is a guess from a document that can drift, and it was labelled as such because it could not be anything better. The backend knows the answer exactly, so it now says it. Headers ride along in the preview even though they are mostly dull, because the fingerprint covers them: a header the approver never saw could otherwise be the thing that later fails the check, and "approve what you are shown" has to mean the whole of what is checked. Three deliberate non-failures, all of which leave a call simply unpinned rather than breaking anything: - Non-http tools have no resolver. There is no HTTP request on this side of the boundary to pin, so builtin/mcp/a2a calls are approved on name and arguments exactly as before. - A call whose pre-request property instructions would have to run first cannot be resolved without writing to conversation memory, so it is left unpinned rather than pinned to a request execution will not build. - A resolver that throws is logged and skipped. Letting a template error abort the batch would turn a display feature into a way to kill a turn. Null therefore means "unenforced", never "rejected" — no call is ever refused on a comparison that was never sound. The executor and the resolver now share templateDataFor(): the fingerprint is only meaningful if it was computed from the inputs execution will use, and two copies of that merge would eventually disagree — rejecting correct calls, or worse, passing altered ones. Body truncation in the preview is display-only and cannot weaken the check: the fingerprint is computed over the whole body before capping, which the oversize-body test pins by asserting a change past the cut-off still moves the hash.
Closes the loop opened by the previous two commits. A pinned call is re-resolved immediately before execution and refused if its fingerprint no longer matches the one the human approved. Approval now binds to a request, not to a tool name. Checked before the journal claim, so a refusal consumes nothing and the call stays replayable. The refusal returns a synthetic NOT_EXECUTED result rather than throwing: the model sees that the call did not run and can say so, and the rest of the batch proceeds normally. Fails closed on anything unverifiable, which is a different question from unchanged: - the tool is gone from the workflow (the agent was reconfigured while a human was deciding), - re-resolution throws, - the call can no longer be pinned at all (config gained a pre-request property instruction across the pause). All three refuse. A pin we can no longer check is exactly the situation this guard exists for; treating "cannot verify" as "unchanged" would make reconfiguring an agent mid-pause the way around it. Two deliberate exemptions, both returning "proceed": - A call that was never pinned. Every non-http tool, and anything unresolvable at gate time. There is no comparison to make, and inventing a failure here would break every builtin/mcp/a2a approval. - An amended call. The approver rewrote the arguments themselves, so the pin describes the request they replaced — 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. The audit line carries the tool, the call id and a fixed reason string, and deliberately no argument, body or header. Mutation-verified: disabling the check kills four tests, covering the tamper case and all three fail-closed paths.
The rubber-stamping signal the plan calls for: approvals far outnumbering rejections over time means the approval step has stopped being read. Emitted once per gated call the instant its verdict is resolved, before any of the downstream branches (truncated args, the changed-request refusal from the previous two commits, execution itself) — all of those are still an instance of a decision having been made, human or automatic. "write" names the mechanism, not the transport: every call reaching this loop was gated by toolApprovals.requireApproval, whether it dispatches over http, mcp, or a2a. Scoping the tag to http-sourced calls only would silently drop a gated MCP write from the signal. decidedBy distinguishes an actual human decision from one the timeout policy made (HitlTimeoutHandler, decidedBy = "system:timeout") and tags it "timeout" rather than folding it into approved/rejected — an unattended timeout auto-approval inflating "approved" would defeat the metric's whole purpose. Tagged only with the decision outcome: no tool name, argument, or conversation id. Follows the existing AgentOrchestrator idiom (recordRuleMatches, recordPauseCapGuard): Metrics.globalRegistry, not an injected MeterRegistry, since this class is not CDI-managed; best-effort, swallowing any emission failure rather than letting it break the LLM loop. Testing this against Metrics.globalRegistry needed one extra thing: outside a running Quarkus app the global registry is a bare CompositeMeterRegistry with no backing store attached, so meters register and increment without throwing but every read-back is silently 0. Attaching a SimpleMeterRegistry in @BeforeAll (guarded, so repeat attachment across test classes in the same fork is a no-op) is what makes the counter observable at all — without it all four tests below would report a false pass. Mutation-verified: disabling the timeout/verdict distinction fails exactly the two timeout-tagging tests, leaving approved/rejected untouched.
…rics
Completes the plan's metrics table. eddi.operator.write.approval (prior
commit) is genuinely backend-native — the orchestrator observes every
decision directly. The other three are not: the write canary is a synthetic
conversation the Manager drives in the browser, and gate verification is
the Manager re-reading every version of the operator agent document. This
codebase has no first-class notion of "the operator" at all — it's an
agent like any other with a particular hitlConfig — so neither fact has a
server-side event to hang a meter on.
POST /administration/operator/{canary-result,gate-status} exists purely to
relay those already-established facts onto this deployment's /q/metrics,
so an on-call engineer watching Grafana does not need a Manager tab open
to see whether the write gate is currently sound. It is NOT a verification
endpoint — a report is trusted at face value — which is exactly why it
sits behind eddi-admin, the same tier that can provision the operator in
the first place. Whoever could misreport through it could reconfigure the
operator directly instead.
eddi.operator.gate.verified defaults to 0 before any report ever arrives,
matching "fail closed on an inconclusive signal" — a deployment that has
never activated an operator therefore also reads 0, indistinguishable from
one whose gate broke. That ambiguity is real and not solved here; it needs
a separate activation signal if an alerting rule has to tell the two apart.
Duration and outcome are recorded independently: a negative or absent
durationMs still counts the outcome, since a malformed timing value says
nothing about whether the gate held.
Testing needed one thing the CDI-managed path gets for free: the gate
gauge is registered once in @PostConstruct, which never fires when a test
constructs the service directly. Made public (not package-private) and
called explicitly in @beforeeach — same shape as RestDocsTest, which
already establishes the "construct the collaborator directly, no mocking
framework" pattern this class follows.
Full 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.
Belated per AGENTS.md §2 rule 8 — should have landed alongside each of the four preceding commits on this branch rather than after all of them. docs/hitl.md: new "Request pinning" subsection under Tool-Level Approval Gating, covering what gets resolved and fingerprinted, why the fingerprint covers the redacted request rather than the live one, and the deliberate unpinned/amended exemptions versus the three fail-closed cases. Operations metrics list extended with all four new meters. docs/changelog.md: one entry covering the whole branch to date (the four commits already pushed), including the honest "what's left" list — the Manager-side canary, populating WRITE_ENDPOINTS, real scope selection, and rendering the server preview in place of the client-side reconstruction — so the entry doesn't read as though writes are already reachable.
The gate-time pinning from three commits ago persisted requestPreview and requestFingerprint on PendingToolCallBatch.PendingToolCall, but nothing external ever read them back — buildToolCallPauseDetails builds its response as an explicit LinkedHashMap, field by field, so a new model field does not appear in the API just because it exists on the entity. This is the other half: an approver's GET .../approval-status now receives the actual resolved request (method, URI, query, redacted headers, body) for any pinned call, replacing what the Manager has so far had to guess by reconstructing an operationId against a separately-fetched spec. requestPinned rides alongside requestPreview so a client can tell "nothing to preview" (every non-http tool, or an http call that could not be resolved without side effects) apart from a resolution that failed silently. The raw fingerprint itself is deliberately NOT exposed — it is an internal comparison value with no meaning to a human approver, and there is no reason to hand it out. Explicit field-by-field again, matching every other field this method already builds (arguments, gateReason, ...) rather than handing the POJO to Jackson: keeps the exposed shape under the same review as the redacted arguments field right above it. The OTHER read path — namesOnlyPendingToolCalls, the security-motivated projection used by the generic conversation-read surfaces (MCP read_conversation, REST simple conversation log) — needed no code change: it is an explicit allow-list copy, so a field it was never told to copy is absent by construction, the same way argumentsRaw and argumentsRedacted already are. Only its doc comment needed updating to name the two new fields explicitly, and a test now pins that guarantee for them the same way the existing test already pinned it for the older fields. Verification note: this repo's `@Nested`-only JUnit test classes report `Tests run: 0` in the plain-text surefire report even when they pass — documented in memory before this session, re-confirmed the hard way during it (see updated `surefire-nested-test-filter`). The real result for both touched test classes, read from the XML `<testsuite>` attribute rather than the ambiguous .txt: RestAgentEngineToolPauseDetailsTest tests="11" errors="0" failures="0"; ConversationMemoryUtilitiesHitlTest tests="8" errors="0" failures="0". Both include the three new pinned/unpinned/ fingerprint-exclusion cases plus the one securing the redaction boundary.
Addendum to the request-pinning changelog entry: the approval-status REST surface now returns requestPinned/requestPreview per pending call, and the write-approval decision metric plus the two Manager metrics-relay endpoints are live. Updates the stale "what's left" note now that WRITE_ENDPOINTS is populated on the Manager side.
RequestRedactor only ever touched "headers", so both consumers of a resolved request — the debug record persisted to the conversation document and the approval preview shown to a human — carried the body verbatim. A config write carries its credential in the body, and the approver is routinely a different admin than whoever's turn raised the pause. Adds RequestRedactor.redactBody, delegating to SecretRedactionFilter (the same value-shape scan already behind argumentsRedacted, so the two cannot drift), wired into redactRequestMap and ResolvedRequest#of. Headers stay fingerprinted redacted, for the cross-user-approval reason already documented. The body is fingerprinted RAW and only the stored copy is redacted: a body has no equivalent legitimate variance, and redacting first would hash two different credentials to one marker and so to one fingerprint, letting a swapped secret pass the pre-execution re-check as unchanged. ResolvedRequest#of does the redaction itself so no call site can invert that order.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds HTTP request pinning for HITL approvals, redacted request previews, SHA-256 fingerprints, fail-closed resume verification, approval metrics, and admin-protected operator metric relay endpoints. ChangesHITL security and metrics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AgentOrchestrator
participant ApiCallExecutor
participant PendingToolCallBatch
participant RestAgentEngine
AgentOrchestrator->>ApiCallExecutor: resolve and redact gated request
ApiCallExecutor-->>AgentOrchestrator: preview and fingerprint
AgentOrchestrator->>PendingToolCallBatch: persist request binding
RestAgentEngine->>PendingToolCallBatch: return approval details
AgentOrchestrator->>ApiCallExecutor: re-resolve request on resume
ApiCallExecutor-->>AgentOrchestrator: current fingerprint
AgentOrchestrator->>AgentOrchestrator: continue or fail closed
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR strengthens HITL (human-in-the-loop) tool approvals by binding approvals to the resolved HTTP request (method/URI/query/headers/body) rather than just the tool name + raw model arguments, and adds operator-facing metrics plus request redaction to prevent credential leakage in persisted/debug/approval surfaces.
Changes:
- Add
IApiCallExecutor.resolve()and aResolvedRequest(+ fingerprint) mechanism to preview/pin/redact HTTP requests at gate time and re-check them before execution. - Extend HITL pause persistence and REST approval-status payloads with
requestPinned+requestPreview, and enforce “request changed since approval” fail-closed behavior for pinned calls. - Add operator metrics (
eddi.operator.write.approval{decision}+ client-reported canary/gate relays) and new tests/docs to cover the new HITL behavior and redaction rules.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java | Pins resolved request previews/fingerprints at gate time; re-resolves and refuses execution if a pinned request changed; emits write-approval decision metrics. |
| src/main/java/ai/labs/eddi/modules/apicalls/impl/IApiCallExecutor.java | Adds resolve() contract for side-effect-free request resolution to support approver previews and pinning. |
| src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java | Implements resolve(), wires centralized request redaction, and reuses redaction for persisted request debug maps. |
| src/main/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequest.java | New value type for redacted request preview + SHA-256 fingerprinting with canonicalization. |
| src/main/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactor.java | New shared redaction utility for headers and body (value-shape based) used by both persistence and approval preview paths. |
| src/main/java/ai/labs/eddi/engine/memory/model/PendingToolCallBatch.java | Extends pending tool-call persistence with requestFingerprint and requestPreview plus preview body size cap. |
| src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.java | Ensures “names-only” snapshot projections exclude request previews/fingerprints (security boundary). |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.java | Surfaces requestPinned and redacted requestPreview in approval-status pauseDetails payloads. |
| src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java | Adds KEY_* constants and clarifies toMap() contract (used by resolve/redaction). |
| src/main/java/ai/labs/eddi/engine/httpclient/impl/HttpClientWrapper.java | Uses IRequest.KEY_* constants for request map shaping (ensures consistent keys). |
| src/main/java/ai/labs/eddi/engine/api/OperatorMetricsService.java | New service to back operator canary + gate-verification metrics in Micrometer. |
| src/main/java/ai/labs/eddi/engine/api/IRestOperatorMetrics.java | New admin-only REST interface for reporting client-observed operator canary/gate outcomes into /q/metrics. |
| src/main/java/ai/labs/eddi/engine/rest/RestOperatorMetrics.java | REST implementation validating and delegating canary/gate metric reports. |
| src/main/java/ai/labs/eddi/engine/api/model/OperatorCanaryReport.java | New DTO for canary outcome + duration reporting. |
| src/main/java/ai/labs/eddi/engine/api/model/OperatorGateStatusReport.java | New DTO for gate verification reporting. |
| src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java | Updates tests for new HttpCallToolsResult record shape. |
| src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.java | Updates tests for new HttpCallToolsResult record shape. |
| src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.java | Adds coverage for request pinning behavior, “changed since approval” enforcement, and write-approval metric tagging. |
| src/test/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequestTest.java | New test suite validating fingerprint stability/discrimination and body redaction vs hashing order. |
| src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.java | Wires RequestRedactor and adds regression test ensuring secrets in request bodies are redacted before persistence. |
| src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorExtendedTest.java | Updates executor construction to include RequestRedactor. |
| src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorBranchCoverageTest.java | Updates executor construction to include RequestRedactor. |
| src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorValidationErrorTest.java | Updates executor construction to include RequestRedactor. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.java | Adds tests for requestPinned/requestPreview exposure rules and ensures fingerprints do not leak via REST responses. |
| src/test/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilitiesHitlTest.java | Extends snapshot security tests to ensure request preview/fingerprint do not leak through generic snapshots. |
| src/test/java/ai/labs/eddi/engine/api/OperatorMetricsServiceTest.java | New unit tests for canary counters/timer, outcome vocabulary validation, and gate gauge behavior. |
| src/test/java/ai/labs/eddi/engine/rest/RestOperatorMetricsTest.java | New REST boundary tests for validation + metric emission delegation. |
| docs/hitl.md | Documents request pinning semantics, preview exposure, enforcement rules, and new metrics. |
| docs/changelog.md | Adds detailed changelog entry explaining motivation, design, enforcement, and verification notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+273
to
+278
| var requestMap = buildRequest(targetServerUrl, call, templateDataObjects).toMap(); | ||
| var headers = requestMap.get(IRequest.KEY_HEADERS) instanceof Map<?, ?> h ? (Map<String, ?>) h : Map.<String, Object>of(); | ||
| var queryParams = requestMap.get(IRequest.KEY_QUERY_PARAMS) instanceof Map<?, ?> q | ||
| ? (Map<String, String>) q | ||
| : Map.<String, String>of(); | ||
| Object body = requestMap.get(IRequest.KEY_BODY); |
Comment on lines
+42
to
+47
| /** Key of the {@code Map<String, String>} of headers in {@link #toMap()}. */ | ||
| String KEY_HEADERS = "headers"; | ||
| /** | ||
| * Key of the {@code Map<String, String>} of query params in {@link #toMap()}. | ||
| */ | ||
| String KEY_QUERY_PARAMS = "queryParams"; |
|
|
||
| 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 pinned, `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. `requestPinned: false` with `requestPreview: null` means exactly what it says — nothing to preview, not a resolution failure the caller should treat as an error — see the unpinned/fail-closed cases above. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/java/ai/labs/eddi/engine/api/IRestOperatorMetrics.java (1)
39-62: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOperator metrics endpoints skip the required
AsyncResponsepattern. Both the contract and its implementation declarereportCanaryResult/reportGateStatusas synchronous methods returningResponsedirectly. As per coding guidelines: "Backend code must be thread-safe and non-blocking; use AsyncResponse for REST endpoints and avoid extended blocking in tasks."
src/main/java/ai/labs/eddi/engine/api/IRestOperatorMetrics.java#L39-L62: change both method signatures to accept anAsyncResponseparameter and returnvoid, per the codebase's REST endpoint convention.src/main/java/ai/labs/eddi/engine/rest/RestOperatorMetrics.java#L30-L46: update both implementations to callasyncResponse.resume(Response.noContent().build())(or the equivalent error path) instead of returningResponsesynchronously.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/engine/api/IRestOperatorMetrics.java` around lines 39 - 62, Update src/main/java/ai/labs/eddi/engine/api/IRestOperatorMetrics.java lines 39-62 so reportCanaryResult and reportGateStatus accept AsyncResponse and return void. Update src/main/java/ai/labs/eddi/engine/rest/RestOperatorMetrics.java lines 30-46 to match these signatures and complete success or error handling via asyncResponse.resume(...) rather than synchronous Response returns.Source: Coding guidelines
src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java (1)
838-853: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the new javadoc so
auditOutcomeUnknownkeeps its documentation.Lines 838-846 document
auditOutcomeUnknown. The new javadoc block at lines 847-853 was inserted between that comment and the method it described. The result is two consecutive javadoc comments beforerequestChangedSinceApproval, andauditOutcomeUnknownat line 905 now has no javadoc. Move theauditOutcomeUnknownjavadoc back down to line 905.♻️ Proposed reordering
- /** - * Records an at-most-once outcome-unknown event. No lightweight - * {`@code` hitl.tool.*} audit collector is reachable from this task (the - * {`@link` ai.labs.eddi.engine.audit.model.AuditEntry} record is built by the - * LifecycleManager per-task with HMAC context we do not have here), so — - * exactly as the config-drift path does — this WARN-logs with a distinctive - * marker that operators can alert on. Package-private + overridable so tests - * can assert it fired. - */ /** * 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.Then add the moved javadoc immediately above
auditOutcomeUnknownat line 905.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java` around lines 838 - 853, Move the at-most-once outcome-unknown Javadoc currently preceding requestChangedSinceApproval so it appears immediately above auditOutcomeUnknown. Keep the requestChangedSinceApproval Javadoc directly attached to that method, and preserve the existing auditOutcomeUnknown documentation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java`:
- Around line 284-290: Update the resolved preview construction in
ApiCallExecutor around ResolvedRequest.of to redact every query-parameter value
with the existing requestRedactor before passing queryParams. Preserve the
parameter names and structure, and apply the same scalar redaction behavior used
for other request components so secrets are not persisted or displayed.
- Around line 273-278: The query-parameter flow in ApiCallExecutor.resolve must
unwrap RequestWrapper.toMap() values from Map<String, List<String>> into the
scalar representation expected by ResolvedRequest before constructing it. Update
the queryParams normalization at the ApiCallExecutor site, and revise the
IRequest.KEY_QUERY_PARAMS contract in IRequest to accurately document the
emitted or normalized shape; preserve existing behavior for absent parameters.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java`:
- Around line 1005-1017: Update the http resolver registration around
mergeExternalTools so toolRequestResolvers retains entries only when toolSources
confirms the http tool won that name. Filter httpCallTools.resolvers() against
the post-merge winner mapping before adding them, ensuring duplicate names keep
the incumbent tool’s behavior and cannot resolve or pin the dropped HTTP
request.
- Around line 2116-2122: Update the WARN in the request-resolution catch block
around resolver.resolve and setRequestFingerprint to pass req.name() through the
existing sanitize(...) helper before logging it, matching the handling used by
other log statements in AgentOrchestrator.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/engine/api/IRestOperatorMetrics.java`:
- Around line 39-62: Update
src/main/java/ai/labs/eddi/engine/api/IRestOperatorMetrics.java lines 39-62 so
reportCanaryResult and reportGateStatus accept AsyncResponse and return void.
Update src/main/java/ai/labs/eddi/engine/rest/RestOperatorMetrics.java lines
30-46 to match these signatures and complete success or error handling via
asyncResponse.resume(...) rather than synchronous Response returns.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java`:
- Around line 838-853: Move the at-most-once outcome-unknown Javadoc currently
preceding requestChangedSinceApproval so it appears immediately above
auditOutcomeUnknown. Keep the requestChangedSinceApproval Javadoc directly
attached to that method, and preserve the existing auditOutcomeUnknown
documentation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 948491e3-5941-4e5b-aabd-67cf2b31fcc8
📒 Files selected for processing (29)
docs/changelog.mddocs/hitl.mdsrc/main/java/ai/labs/eddi/engine/api/IRestOperatorMetrics.javasrc/main/java/ai/labs/eddi/engine/api/OperatorMetricsService.javasrc/main/java/ai/labs/eddi/engine/api/model/OperatorCanaryReport.javasrc/main/java/ai/labs/eddi/engine/api/model/OperatorGateStatusReport.javasrc/main/java/ai/labs/eddi/engine/httpclient/IRequest.javasrc/main/java/ai/labs/eddi/engine/httpclient/impl/HttpClientWrapper.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngine.javasrc/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.javasrc/main/java/ai/labs/eddi/engine/memory/model/PendingToolCallBatch.javasrc/main/java/ai/labs/eddi/engine/rest/RestOperatorMetrics.javasrc/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.javasrc/main/java/ai/labs/eddi/modules/apicalls/impl/IApiCallExecutor.javasrc/main/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactor.javasrc/main/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequest.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/test/java/ai/labs/eddi/engine/api/OperatorMetricsServiceTest.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineToolPauseDetailsTest.javasrc/test/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilitiesHitlTest.javasrc/test/java/ai/labs/eddi/engine/rest/RestOperatorMetricsTest.javasrc/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorBranchCoverageTest.javasrc/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorExtendedTest.javasrc/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.javasrc/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorValidationErrorTest.javasrc/test/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequestTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java
Three review findings, all in the pinning path.
IRequest#toMap returns query params as Map<String, List<String>> —
HttpClientWrapper accumulates repeats — but resolve() cast that to
Map<String, String>. 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,
including POST .../deploy/{agentId}?version=N, a granted write. Both
shapes are now accepted, and the canonical form emits one length-
prefixed field per value so ?tag=a&tag=b cannot be forged by a single
value containing the display separator. The KEY_QUERY_PARAMS javadoc
asserted the wrong type and is corrected.
Query parameters were also not redacted — same class as the body leak,
missed the same way. ?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.
mergeExternalTools resolves a name collision by dropping the incoming
tool, but the resolver was registered before that verdict was known. A
builtin winning a collision against a same-named http tool would 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 owns.
Also sanitizes the model-chosen tool name in the resolve-failure WARN,
and drops the docs claim that requestPinned:false implies
requestPreview:null — a call with preRequest.propertyInstructions is
previewed best-effort AND left unpinnable, so both hold at once.
Comment on lines
+136
to
+159
| /** | ||
| * Redact the {@code headers} and {@code body} entries of a request map in | ||
| * place, as produced by | ||
| * {@link ai.labs.eddi.engine.httpclient.IRequest#toMap()}. | ||
| * <p> | ||
| * Query parameters are deliberately left alone here: this map is the debug | ||
| * record, whose {@code queryParams} entry is the live map the request itself | ||
| * holds ({@code HttpClientWrapper.RequestWrapper#toMap} does not copy it), so | ||
| * rewriting its values in place would corrupt the outgoing request. The | ||
| * approval preview redacts them on its own copy instead — see | ||
| * {@code ApiCallExecutor#resolve}. | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| public void redactRequestMap(Map<String, Object> requestMap) { | ||
| if (requestMap == null) { | ||
| return; | ||
| } | ||
| if (requestMap.get("headers") instanceof Map<?, ?> headers) { | ||
| requestMap.put("headers", redactHeaders((Map<String, ?>) headers)); | ||
| } | ||
| if (requestMap.get("body") instanceof String body) { | ||
| requestMap.put("body", redactBody(body)); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactor.java (1)
136-159: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact the
queryParamsmap without mutating the live request.
RequestWrapper#toMap()storesqueryParamsMapdirectly, andApiCallExecutor.execute()callsredactRequestMap()before creating thehttpCallsmemory entry. Query credentials inqueryParamsare therefore stored unredacted on the execute path, while the same redaction logic is applied inResolvedRequest. Build a new redactedMap<String, List<String>>, replacingrequestMap.get("queryParams")without changing the livequeryParamsMapcontents.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactor.java` around lines 136 - 159, Update redactRequestMap to also process the queryParams entry by creating a new redacted Map<String, List<String>> and replacing the entry in requestMap, rather than mutating the original map referenced by RequestWrapper#toMap(). Reuse the existing query-parameter redaction logic used by ApiCallExecutor#resolve or ResolvedRequest so execute-path memory entries store redacted credentials while the live queryParamsMap remains unchanged.
🧹 Nitpick comments (1)
src/main/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequest.java (1)
99-118: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winLength-prefix the query/header key, not only the value.
In
fingerprintOf, the query field name is built as"query." + entry.getKey() + "[" + i + "]"(Line 112) and the header field name as"header." + entry.getKey()(Line 116).appendFieldonly length-prefixes the VALUE it receives; the NAME string, which embeds the raw key, is written to the canonical buffer unprefixed and unescaped.The class's own threat model (documented at Lines 90-98) exists to stop exactly this class of forgery for values: a
\nor a":"sequence inside an unprotected field lets one request's canonical bytes imitate two fields instead of one. The same forgery mechanism applies to the key if a query parameter name or header name ever contains those characters, since the key is concatenated directly into the field name rather than passed throughappendFieldas a length-prefixed value.The
Injectionnested test class inResolvedRequestTest.javacovers value-based forgeries (body-as-header, query-separator collision) but does not cover a forged key. Whether this is exploitable depends on whether query/header names can ever carry attacker- or model-influenced content — worth confirming, since httpcall query parameter names are normally developer-configured, but this is not guaranteed by the code itself.🛡️ Proposed fix: move the key into a length-prefixed field
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)); + appendField(canonical, "query[" + i + "].name", entry.getKey()); + appendField(canonical, "query[" + i + "].value", values.get(i)); } } for (var entry : headers.entrySet()) { - appendField(canonical, "header." + entry.getKey(), entry.getValue()); + appendField(canonical, "header.name", entry.getKey()); + appendField(canonical, "header.value", entry.getValue()); }The
"query[" + i + "]"and literal"header.name"/"header.value"segments contain only loop-counter digits or fixed literals, never attacker-controlled text, so they cannot themselves be forged.As per coding guidelines, no specific rule covers this file; this is a general security-hardening finding based on the class's own stated forgery-prevention design.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequest.java` around lines 99 - 118, Update fingerprintOf so query parameter and header names are passed to appendField as length-prefixed data instead of being concatenated into the unescaped field name. Keep only fixed labels and value indexes in canonical field names, and preserve separate indexed query values while ensuring attacker-controlled keys cannot forge canonical field boundaries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/changelog.md`:
- Line 57: Update the `WRITE_ENDPOINTS` changelog entry so it states first that
the Manager-side population is complete, then clearly lists only the remaining
Manager-side tasks: rendering this backend’s `requestPreview` in the approval
banner and completing the agent/group authoring UI.
In `@docs/hitl.md`:
- Line 381: The documentation’s claim that hashing the raw body “reveals
nothing” is too absolute. Update the fingerprint explanation to state that it is
not exposed through any client API, while treating the persisted SHA-256 digest
as sensitive internal data because predictable values may permit offline
guessing or equality correlation.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactor.java`:
- Around line 136-159: Update redactRequestMap to also process the queryParams
entry by creating a new redacted Map<String, List<String>> and replacing the
entry in requestMap, rather than mutating the original map referenced by
RequestWrapper#toMap(). Reuse the existing query-parameter redaction logic used
by ApiCallExecutor#resolve or ResolvedRequest so execute-path memory entries
store redacted credentials while the live queryParamsMap remains unchanged.
---
Nitpick comments:
In `@src/main/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequest.java`:
- Around line 99-118: Update fingerprintOf so query parameter and header names
are passed to appendField as length-prefixed data instead of being concatenated
into the unescaped field name. Keep only fixed labels and value indexes in
canonical field names, and preserve separate indexed query values while ensuring
attacker-controlled keys cannot forge canonical field boundaries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c608776-a2e0-4c9d-8bfc-7bca4db8d725
📒 Files selected for processing (11)
docs/changelog.mddocs/hitl.mdsrc/main/java/ai/labs/eddi/engine/httpclient/IRequest.javasrc/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.javasrc/main/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactor.javasrc/main/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequest.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.javasrc/test/java/ai/labs/eddi/modules/apicalls/impl/ResolvedRequestTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorToolGovernanceTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java
- src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java
- src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.java
The preview redacts them, the conversation document did not — so a credential passed as ?api_key=… was persisted to MongoDB in the clear. The previous doc comment claimed this was unavoidable because RequestWrapper#toMap hands back its LIVE queryParamsMap, so rewriting it would corrupt the outgoing request. That reasoning was wrong: the entry can be REPLACED with a redacted copy in the freshly-built outer map, exactly as redactHeaders already does, leaving the nested live map untouched. A test asserts both halves — the persisted record is redacted AND the original map still holds its real value. Also corrects two doc claims: SHA-256 is not encryption, so the persisted fingerprint is described as sensitive internal data rather than as revealing nothing, and the changelog no longer asks what is left before WRITE_ENDPOINTS can be populated immediately before saying it already has been.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/main/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactor.java:165
redactRequestMap()is still keying into the request map with string literals ("headers"/"queryParams"/"body"), even thoughIRequestnow defines these keys as part of thetoMap()contract. Using the shared constants avoids future drift/typos if the map contract evolves, and matches the convention established inHttpClientWrapper/ApiCallExecutor#resolve.
if (requestMap.get("headers") instanceof Map<?, ?> headers) {
requestMap.put("headers", redactHeaders((Map<String, ?>) headers));
}
if (requestMap.get("queryParams") instanceof Map<?, ?> queryParams) {
requestMap.put("queryParams", redactQueryParams((Map<String, ?>) queryParams));
…ing literals redactRequestMap spelled "headers"/"queryParams"/"body" itself, so a rename on the IRequest side would leave it silently redacting nothing. Those constants were promoted onto the interface earlier in this branch precisely so callers stop duplicating them — and a disagreement between the interface's stated contract and what a caller assumed is exactly what produced the query-param defect this PR already had to fix.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/main/java/ai/labs/eddi/engine/rest/RestOperatorMetrics.java:37
reportCanaryResulttreats a missing/empty request body the same as an invalidoutcomeand always throws "outcome must be one of: pass, fail, unknown". Fornullbodies this is misleading (and inconsistent withreportGateStatus, which explicitly reports that the request body is required). Split the validation so a null body yields a body-required error, and only invalid outcomes yield the outcome-vocabulary error.
public Response reportCanaryResult(OperatorCanaryReport report) {
if (report == null || !OperatorMetricsService.isValidOutcome(report.outcome())) {
throw new BadRequestException("outcome must be one of: pass, fail, unknown");
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 44 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.java:355
- The Javadoc starting here is not attached to any declaration because it’s immediately followed by another Javadoc block. This can trigger Javadoc/checkstyle warnings and makes the intended documentation easy to miss. Convert this first block to a regular block comment (or merge it into the method/field Javadoc) so only one Javadoc applies to the next member.
/**
src/main/java/ai/labs/eddi/modules/apicalls/impl/RequestRedactor.java:158
- This Javadoc block is immediately followed by another Javadoc block, so it doesn’t attach to any member (and may violate Javadoc/checkstyle rules). Convert it to a regular block comment (or merge it with the method’s Javadoc) so documentation is applied predictably.
/**
…rest Third review comment about the same mistake, so this fixes the class rather than the instance. Adding a method (or constant) directly above an existing documented one leaves the existing javadoc stranded: two comment blocks back to back, the first describing something further down the file. Repaired here — ConversationMemoryUtilities (REDACTED_FINGERPRINT split stripRequestFingerprintsForRead from its doc) — plus three more the first two review comments had not reached, found by scanning every .java file this branch touches for a `*/` immediately followed by a `/**`: AgentSetupService, RequestRedactor, AgentOrchestrator. Comment lines only. The single non-comment line in the diff is REDACTED_FINGERPRINT changing position; no logic, no reordering of code. Full suite at the documented baseline (8 pre-existing EmbeddingModelFactoryBranchTest failures, 313 environmental).
Repo: EDDI-Manager (feat/operator-write-scope) — see that repo's HANDOFF.md and commit 578c4587 for the full write-up. Landing the entry here because this changelog tracks all repos in this arc, per its own stated purpose.
Comment on lines
+48
to
+55
| 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"); | ||
| } |
…redaction
isSensitiveHeaderName's name check covered authorization/api-key/token/
secret/credential but not "password" — despite this class's own javadoc,
two lines above the query-param version of this same method, already
listing password among the generic rule's recognized credential names.
The gap was real: SecretRedactionFilter's shape rule needs the credential
name INSIDE the value ("password=hunter2"), so a value that's just the
bare password with the name sitting in a separate header/param name
(X-Password: hunter2, ?password=hunter2) matched neither the name check
nor the shape rule, and reached the approval preview and the debug
record in plaintext.
isSensitiveHeaderName backs both redactHeaderValue and
redactQueryParamValue, so one fix closes both channels. Mutation-verified
one test per channel: reverting the check fails both new tests, restored
and re-verified green (16/16, full suite otherwise at the documented
baseline).
Found by an automated review comment on PR #627.
The "a call can be unpinned" paragraph listed non-http tools and
preRequest.propertyInstructions. canExecuteDivergeFromResolve has two
more branches: fireAndForget+batchRequests, and — the one that matters —
any call carrying a retryApiCallInstruction with maxRetries >= 1.
That last one is easy to trip over. RetryApiCallInstruction defaults
maxRetries to 3, so `"retryApiCallInstruction": {}` alone unpins an
otherwise-pinnable write, and a retry can fire on a 2xx when
responseValuePathMatchers matches rather than only on retryOnHttpCodes.
So a realistic gated POST could ship with requestPinned:false while this
document told the operator the request is re-checked before it runs.
Nothing is wrong at runtime — requestPinned is reported honestly per
call on approval-status, and the code has been correct since the retry
guard landed. But this document is the contract, and config that
silently does something other than what it says is precisely the class
of bug this whole feature exists to remove.
Restated as a table keyed on the actual invariant ("never pin what
cannot be honoured") rather than a list of features, so a future branch
that adds a fifth divergence path has somewhere obvious to add it. The
fail-closed paragraph gets the same correction.
Found by an adversarial review pass over the branch.
…-fingerprint # Conflicts: # docs/changelog.md
rolandpickl
approved these changes
Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes a human approval bind to the actual HTTP request a tool call will send, instead of to its name. Until now an approver of a gated
httpcall saw the tool's name and the model's raw arguments — never the method, path or body — because those are only produced insideApiCallExecutor#execute, after approval. So what someone signed off on and what ran could differ. This is the precondition EDDI-Manager#129 needs before granting the operator any write at all.hitlConfig.toolApprovalsset.requestPreview/requestPinned, thehitlConfigon setup, and/administration/operator/*.Where to look
Ranked — the first three are where a bug would actually matter:
ResolvedRequest.java— the fingerprint canonicalisation. If two different requests can hash alike, the whole guard is decorative. Note the deliberate asymmetry: headers are hashed redacted, body is hashed raw.ApiCallExecutor.java—resolve(), andcanExecuteDivergeFromResolve(), which decides what may be pinned at all. Getting that predicate wrong pins something we cannot honour, and then refuses a legitimate call.RequestRedactor.java— the single definition of "redacted request", shared by the conversation debug record and the approver preview so they cannot drift. Four separate leaks were found here during review; all four are in the tests.AgentOrchestrator.java— gate-time pinning and the pre-execution re-check.AgentSetupService/SetupAgentRequest— the newhitlConfigon the standard agent-setup path.What it does
Resolve without sending.
IApiCallExecutor.resolvebuilds the request anApiCallwould send and returns method, URI, query, redacted headers, body and a SHA-256 fingerprint — without sending it.Pinned at gate time, re-checked before execution. The fingerprint and a redacted preview are stored on the pause. On resume the call is re-resolved and compared immediately before it runs; a mismatch refuses it with a synthetic
NOT_EXECUTEDand an audit line (hitl.tool.request_changed— tool and callId only, never the request), and the rest of the batch proceeds.Not everything can be pinned, and that is deliberate. The rule is never pin what cannot be honoured — four cases where
execute()may legitimately build a requestresolve()did not:httppreRequest.propertyInstructionsfireAndForget+batchRequestsretryApiCallInstructionwithmaxRetries >= 1buildRequestis inside the retry loop, and each attempt re-renders against a memory the previous attempt wrote to.The retry row is the easy one to miss:
maxRetriesdefaults to 3, so"retryApiCallInstruction": {}alone unpins a write. ReadrequestPinnedper call; do not infer it from the endpoint.Unpinned and amended calls pass through unchanged — nothing is refused on a comparison that was never sound. Three cases fail closed, because "cannot verify" is not "unchanged": the tool vanished mid-pause, re-resolution throws, or a pinned call became unpinnable.
Gate provisioning.
SetupAgentRequestgainedhitlConfig— the standard agent-setup path previously had no way to install a gate, so every agent it created shipped ungated. Validated up front and set on v1, never via a later PUT, so an ungated v1 cannot stay reachable by redeploy. Deliberately absent from the MCPsetup_agenttool, which would otherwise let a caller pick its own gate.Redaction. Four leaks closed, each mutation-verified: the body was never redacted at all; header values were judged only by name (so
X-Client-Auth: Bearer …reached the approver in full); percent-encoding defeated the shape rules; andpasswordwas missing from the name list despite this class's own javadoc listing it.Plus: a resume verdict that resolved to
nullwas one comparison away from executing as approved, with the metric tagging it"rejected"while it ran. Fixed at both ends.What to be sceptical about
${caller:token}, which legitimately differs between requester and approver — but the exemption is any header matching the sensitive-name list or a secret shape. A header carrying ordinary business data under a credential-ish name therefore does not participate in change detection. URI, query and body are all covered as-resolved.eddi.operator.write.approval{decision=approved}fires before the refusal checks, so an approved-then-refused call incrementsapproved. Don't alert on it as "writes executed".Verification
Every security-relevant fix is mutation-tested — the fix reverted, the specific test confirmed red, restored. The one worth naming: two different API keys must produce different fingerprints, which fails if body redaction is hoisted above the hash.
CI green including Integration Tests, which cannot run in a sandbox (no loopback sockets) and is therefore the first true end-to-end confirmation — and it ran on the post-merge commit, so it covers the merge with
main.Docs:
docs/hitl.md,docs/changelog.md.