fix(review): operator-stack audit — cross-version placeholders, contract leaks, live-path self-guard

docs/changelog.md

@@ -7,6 +7,80 @@
 
 
 
+## 🛡️ fix(review): four-agent audit of the operator stack — cross-version placeholders, contract widenings, live-path guard (2026-08-15)
+
+**Repo:** EDDI (`fix/operator-review-findings`)
+
+A four-reviewer audit of everything merged into `chore/remove-agent-father` (#679–#689) plus an
+end-to-end trace of the operator paths. Confirmed findings, all fixed here:
+
+**Cross-version placeholder stranding (the audit's sharpest catch).** `dropPendingApprovalPlaceholder`
+removes the pending-approval bubble by recomputing `resolvePendingMessage` and matching the exact
+string — which silently assumed pause and resume run the same build. Two releases changed the
+default wording (tool-named, then the repeat ordinal), so a conversation paused under the previous
+build recomputes a string that is not in its output, the removal no-ops, and the resolved turn
+renders [stale placeholder, answer] — the artifact those changes exist to kill, once per in-flight
+pause on the first post-upgrade resume. The resume path now recognises its predecessors' wording
+(`pendingPlaceholderCandidates`): the current rendering, the suffix-less variant, and the legacy
+constant. Two upgrade-boundary tests simulate a pre-upgrade pause and resume with current code.
+
+**Self-conversation guard now holds WITHOUT a pause.** #689's rule ("an agent may not send a request
+to its own conversation") was enforced only in `ToolLoopResumer` — the approval-execution path — so
+a call the gate let through live (ungated method, or the HITL kill-switch off) executed with no
+check anywhere. The rule is absolute; `ToolLoopRunner`'s live loop now runs the same check
+(`targetsOwnConversationLive`, shared core extracted) and refuses with the same `NOT_EXECUTED`
+envelope and `hitl_self_conversation` trace. A second review round then caught that the FIRST
+version of this fix missed the mixed-batch pause branch — ungated calls executed before the pause
+is thrown, frozen into the batch, never rechecked — so the guard runs there too. Accepted cost,
+documented in code: resolver-less tools (built-ins, MCP) fall back to raw-argument containment,
+where a mere MENTION of the id refuses the call; kept because that fallback is the only check
+covering `converse_with_agent` handed the agent's own conversationId.
+
+**#684 contract widenings, narrowed.** The tool-result contract (`body`/`httpCode` on failures)
+leaked past its intent in three places: (1) `ApiCallsTask` merged FAILED results into cross-call
+template data, where a failed call's error text could overwrite a previous success's `{body}` for a
+later call in the same step — failures no longer merge (`isFailureResult`); the scoped
+`{name}Error`/`{name}HttpCode` keys are unchanged. (2) The RAG path pasted a failed retrieval's
+error body into the SYSTEM prompt as "## Search Results" — up to 2KB of proxy/WAF error page,
+attacker-influenced in some architectures, masquerading as retrieved knowledge; failed retrievals
+now contribute nothing, as pre-contract. (3) The error body itself is now REDACTED
+(`SecretRedactionFilter`) before entering the tool result — a 401 routinely echoes the credential
+that failed, and the body flows into the transcript, pause batches and traces. The memory-side
+`{name}Error` entry keeps the raw text as before.
+
+**Test-drive read-back returned nothing to quote.** Every generated tool parameter is REQUIRED, so a
+model with no field filter to express sends `returningFields=""` — which bound as `[""]` and nulled
+steps, outputs AND properties from the snapshot: a working agent looked broken to the operator.
+Blank entries now mean NO filter (`ConversationMemoryUtilities`).
+
+**A guessed say-body was silently swallowed.** The say tool's body schema is a `$ref` the parser
+leaves unresolved, so its description carried zero field names; a guessed `{"message": ...}` bound
+to `InputData`'s defaults (empty input), answered 200, and a human-approved test message was never
+delivered. Body `$refs` now resolve one level (`resolveComponentRef`), so the description names
+`input`/`context` and requiredness — for every generated tool, not just say.
+
+**Enum values and defaults now reach parameter descriptions** (`appendSchemaHints`): the generated
+schema types every parameter as a required string, so the description is the model's only view of
+the value space. Observed with `environment`, where a guessed value silently fell back to production
+on the lenient server-side enum parse — a test-drive quietly exercising the wrong deployment.
+
+**Smaller items:** `padDataLines` normalises bare `\r` so its continuation line stays padded
+(RESTEasy starts a new `data:` line on either); `Authentication-Info`/`Proxy-Authentication-Info`
+join the credential response-header deny-list (RFC 7615 challenge material). Disclosure owed from
+#688: the credential-header stripping sits on the SHARED executor path, so a hand-authored config
+that captured `Set-Cookie`/`Authorization` from a response now reads them as absent — deliberate
+(those values are never data), but it is a behaviour change for such configs.
+
+Verified sound by the same audit, no change needed: `maxPausesPerTurn` exhaustion is fail-closed
+(synthetic DENIED, never ungated execution); every resume entry point restores the full persisted
+batch, so the ordinal is deterministic across REST/Slack/MCP/timeout/group resumes; #687's JSON
+guard runs post-approval by design and cannot diverge from the pinned fingerprint; conversation ids
+are globally unique across environments, so test-environment conversations read back fine.
+
+---
+
+
+
 ## 🔒 fix(hitl): an agent may not send a request to its own conversation (2026-08-15)
 
 **Repo:** EDDI (`fix/self-conversation-tool-call`)

src/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.java

@@ -300,7 +300,13 @@ static String padDataLines(String data) {
         if (data == null || data.isEmpty()) {
             return data;
         }
-        return " " + data.replace("\n", "\n ");
+        // \r is a line break to RESTEasy's SSE serializer too (SseUtil starts a
+        // new data: line on either), so a payload with a bare \r would get an
+        // UNPADDED continuation line. Normalise \r\n and \r to \n first; the
+        // consumer reassembles data lines with \n regardless, so the
+        // normalisation is invisible to it.
+        String normalised = data.replace("\r\n", "\n").replace('\r', '\n');
+        return " " + normalised.replace("\n", "\n ");
     }
 
     private final class SseStream {

src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java

@@ -115,7 +115,7 @@ public static ApiBuildResult parseAndBuild(String openApiSpec, String endpointFi
                     // Determine group (first tag, or "General")
                     String group = (operation.getTags() != null && !operation.getTags().isEmpty()) ? operation.getTags().get(0) : DEFAULT_GROUP;
 
-                    ApiCall httpCall = buildApiCall(method, path, operation, apiAuth);
+                    ApiCall httpCall = buildApiCall(method, path, operation, apiAuth, openAPI);
                     callsByGroup.computeIfAbsent(group, k -> new ArrayList<>()).add(httpCall);
                     endpointCount++;
 
@@ -210,7 +210,7 @@ static boolean looksLikeInlineSpec(String specInput) {
     /**
      * Build a single ApiCall from an OpenAPI operation.
      */
-    private static ApiCall buildApiCall(String method, String path, Operation operation, String apiAuth) {
+    private static ApiCall buildApiCall(String method, String path, Operation operation, String apiAuth, OpenAPI openAPI) {
         var httpCall = new ApiCall();
 
         // Name: operationId or generated slug
@@ -284,6 +284,14 @@ private static ApiCall buildApiCall(String method, String path, Operation operat
             for (Parameter param : operation.getParameters()) {
                 String paramName = param.getName();
                 String paramDesc = param.getDescription() != null ? param.getDescription() : paramName;
+                // The description is the model's ONLY view of the value space —
+                // the generated tool schema types every parameter as a plain
+                // string, and every one is REQUIRED. Without the allowed values
+                // and the default spelled out, the model guesses: observed with
+                // `environment`, where a guessed value silently falls back to
+                // production on the lenient server-side enum parse — a
+                // test-drive that quietly exercises the wrong deployment.
+                paramDesc = appendSchemaHints(paramDesc, param.getSchema());
 
                 if ("query".equals(param.getIn())) {
                     // Query params use Qute template for LLM-provided values
@@ -303,7 +311,18 @@ private static ApiCall buildApiCall(String method, String path, Operation operat
             MediaType jsonMedia = content.get("application/json");
             if (jsonMedia != null) {
                 request.setContentType("application/json");
-                var body = buildBodyTemplate(jsonMedia.getSchema());
+                // One level of $ref resolution, HERE and deliberately not resolveFully():
+                // the parser leaves component references unresolved, so a body
+                // declared as $ref: InputData reached describeBodySchema as a
+                // nameless shell and the parameter description degraded to "a
+                // single JSON object" with ZERO field names. The model then
+                // guesses keys — observed with the say tool, where a guessed
+                // {"message": ...} bound to InputData's DEFAULTS, returned 200,
+                // and the approved test message was silently never delivered.
+                // Nested property refs stay unresolved: the top-level field
+                // names and requiredness are what the model needs to write a
+                // correct body.
+                var body = buildBodyTemplate(resolveComponentRef(jsonMedia.getSchema(), openAPI));
                 // The body template's variables must be declared as tool parameters,
                 // or the model has no documented way to fill them: the tool schema is
                 // built from getParameters() alone (AgentOrchestrator), and with
@@ -441,6 +460,29 @@ private record BodyTemplate(String template, Map<String, String> variables) {
     /** Name of the whole-body variable used when the schema has no properties. */
     static final String WHOLE_BODY_VARIABLE = "requestBody";
 
+    /**
+     * Resolves a top-level {@code $ref: #/components/schemas/X} to its component
+     * schema, one level deep. Anything else — no ref, unknown name, no components —
+     * returns the input unchanged.
+     */
+    private static Schema<?> resolveComponentRef(Schema<?> schema, OpenAPI openAPI) {
+        if (schema == null || schema.get$ref() == null || openAPI == null
+                || openAPI.getComponents() == null || openAPI.getComponents().getSchemas() == null) {
+            return schema;
+        }
+        String ref = schema.get$ref();
+        // Schemas namespace ONLY: a malformed ref into another components
+        // namespace (requestBodies, parameters) must degrade to the safe
+        // nameless form rather than resolving a same-named SCHEMA and
+        // describing the wrong type's fields.
+        String prefix = "#/components/schemas/";
+        if (!ref.startsWith(prefix)) {
+            return schema;
+        }
+        Schema<?> resolved = openAPI.getComponents().getSchemas().get(ref.substring(prefix.length()));
+        return resolved != null ? resolved : schema;
+    }
+
     private static BodyTemplate buildBodyTemplate(Schema<?> schema) {
         if (schema == null) {
             // A declared body with no schema still needs a variable, or the model has
@@ -478,6 +520,29 @@ private static BodyTemplate buildBodyTemplate(Schema<?> schema) {
      * and marks which are required. Types are included because the model must
      * produce real JSON — an integer field unquoted, a string field quoted.
      */
+    /**
+     * Appends the schema's allowed values and default to a parameter description,
+     * when it declares them. See the call site for why this is the model's only
+     * channel for either.
+     */
+    private static String appendSchemaHints(String description, Schema<?> schema) {
+        if (schema == null) {
+            return description;
+        }
+        var sb = new StringBuilder(description);
+        List<?> allowed = schema.getEnum();
+        if (allowed != null && !allowed.isEmpty()) {
+            sb.append(" Allowed values: ");
+            sb.append(String.join(", ", allowed.stream().map(String::valueOf).toList()));
+            sb.append(".");
+        }
+        Object defaultValue = schema.getDefault();
+        if (defaultValue != null && !String.valueOf(defaultValue).isBlank()) {
+            sb.append(" Default: ").append(defaultValue).append(".");
+        }
+        return sb.toString();
+    }
+
     private static String describeBodySchema(Schema<?> schema) {
         // Name the container the schema actually declares. Saying "a single JSON
         // object" for a top-level array makes the model wrap the payload in braces,

src/main/java/ai/labs/eddi/engine/memory/ConversationMemoryUtilities.java

@@ -492,6 +492,18 @@ public static SimpleConversationMemorySnapshot convertSimpleConversationMemorySn
 
         var memorySnapshot = convertSimpleConversationMemory(conversationMemorySnapshot, returnDetailed, returnCurrentStepOnly);
 
+        // Blank entries mean NO filter, not "select nothing". A present-but-empty
+        // query parameter (?returningFields=) binds as [""], and LLM-generated
+        // tools make that shape routine: every generated parameter is required,
+        // so a model with no filter to express sends the empty string — and the
+        // branches below would then null out steps, outputs AND properties,
+        // leaving the operator's test-drive read-back with nothing to quote.
+        // "" selects no field under any reading, so dropping blanks recovers the
+        // caller's intent on every interpretation.
+        if (returningFields != null) {
+            returningFields = returningFields.stream().filter(f -> f != null && !f.isBlank()).toList();
+        }
+
         if (returnCurrentStepOnly) {
             if (isNullOrEmpty(returningFields) || returningFields.contains(KEY_CONVERSATION_STEPS)) {
                 var conversationSteps = memorySnapshot.getConversationSteps();

src/main/java/ai/labs/eddi/engine/runtime/internal/Conversation.java

@@ -878,6 +878,57 @@ private String resolvePendingMessage(IConversationMemory memory) {
         return rendered;
     }
 
+    /**
+     * Every string {@link #pauseConversation} may have written as the pending
+     * placeholder for the CURRENT batch — under this build or a previous one. First
+     * entry is what this build writes; the rest are legacy renderings.
+     * <p>
+     * Exists because the determinism argument ("recompute the exact string on
+     * resume") silently assumed pause and resume run the same build. Two releases
+     * changed the DEFAULT wording — the tool-named default, then the repeat-pause
+     * ordinal — so a conversation paused under the previous build recomputes a
+     * string that is not in its output list, the removal no-ops, and the resolved
+     * turn renders [stale placeholder, answer]: the exact artifact those changes
+     * exist to kill, once per in-flight pause on the first post-upgrade resume.
+     * There is no schema migration for conversation output, so the resume path
+     * itself must recognise its predecessors' wording.
+     * <p>
+     * Only DEFAULT renderings accumulate variants — a configured
+     * {@code pendingMessage} has never been rewritten by a release, so it stays a
+     * single candidate. (An operator editing their template between pause and
+     * resume strands the placeholder exactly as before; that is config drift, not a
+     * version boundary, and predates all of this.)
+     */
+    private List<String> pendingPlaceholderCandidates(IConversationMemory memory) {
+        String current = resolvePendingMessage(memory);
+        var candidates = new ArrayList<String>();
+        candidates.add(current);
+
+        var batch = memory.getHitlPendingToolCalls();
+        var cfg = batch != null && batch.getEffectiveToolApprovals() != null
+                ? batch.getEffectiveToolApprovals()
+                : memory.getAgentToolApprovalsConfig();
+        var rule = batch != null ? batch.getEffectiveRule() : null;
+        boolean configured = (rule != null && rule.getPendingMessage() != null && !rule.getPendingMessage().isBlank())
+                || (cfg != null && !isNullOrEmpty(cfg.getPendingMessage()));
+        if (!configured) {
+            // Pre-ordinal build: the tool-named default without the suffix. The
+            // ordinal itself predates the suffix (it was persisted for cap
+            // enforcement), so a repeat pause persisted by that build re-reads
+            // its ordinal today and gains a suffix the stored text never had.
+            var suffix = java.util.regex.Pattern.compile("^(.*) \\(approval \\d+ this turn\\)$",
+                    java.util.regex.Pattern.DOTALL).matcher(current);
+            if (suffix.matches()) {
+                candidates.add(suffix.group(1));
+            }
+            // Pre-tool-named build: the constant, regardless of names.
+            if (!candidates.contains(DEFAULT_PENDING_MESSAGE)) {
+                candidates.add(DEFAULT_PENDING_MESSAGE);
+            }
+        }
+        return candidates;
+    }
+
     /**
      * Removes the pending-approval placeholder that {@link #pauseConversation}
      * added to the current step on a TOOL_CALL pause, so the resumed step renders
@@ -902,11 +953,18 @@ private String resolvePendingMessage(IConversationMemory memory) {
      * writer replaced it we leave it alone.
      */
     private void dropPendingApprovalPlaceholder(IWritableConversationStep currentStep) {
-        String pending = resolvePendingMessage(conversationMemory);
-        currentStep.removeConversationOutputListItem(MemoryKeys.OUTPUT_PREFIX, pending);
+        // ALL renderings this or a previous build may have written for this batch
+        // — see pendingPlaceholderCandidates. At most one of them is actually in
+        // the list (each pause writes exactly one placeholder), so removing every
+        // candidate removes exactly the placeholder and cannot touch legitimate
+        // output: a candidate that was never written simply no-ops.
+        List<String> candidates = pendingPlaceholderCandidates(conversationMemory);
+        for (String pending : candidates) {
+            currentStep.removeConversationOutputListItem(MemoryKeys.OUTPUT_PREFIX, pending);
+        }
 
         IData<?> outputData = currentStep.getData(MemoryKeys.OUTPUT_PREFIX);
-        if (outputData != null && List.of(pending).equals(outputData.getResult())) {
+        if (outputData != null && candidates.stream().anyMatch(p -> List.of(p).equals(outputData.getResult()))) {
             var blanked = new Data<>(MemoryKeys.OUTPUT_PREFIX, new ArrayList<>());
             blanked.setPublic(true);
             currentStep.storeData(blanked);