fix(review): operator-stack audit — cross-version placeholders, contract leaks, live-path self-guard - #690
Merged
Conversation
… live guard A four-reviewer audit of #679-#689 plus an end-to-end operator-path trace, then a second adversarial round over this fix itself. Everything confirmed is addressed here; docs/changelog.md carries the full account. - Cross-version placeholder stranding: dropPendingApprovalPlaceholder now recognises the previous builds' default wordings (legacy constant, suffix-less tool-named), so the first post-upgrade resume of an in-flight pause no longer renders [stale placeholder, answer]. Two upgrade-boundary tests simulate a pre-upgrade pause. - Self-conversation guard is now enforced on the LIVE path, including the mixed-batch pause branch the second review round caught (ungated calls execute before the pause is thrown and are never rechecked). Shared core extracted; same NOT_EXECUTED envelope and trace everywhere. - #684 contract narrowed: failed results stay out of ApiCallsTask's cross-call template merge and out of the RAG system prompt; error bodies (and the status-message fallback) are redacted before reaching the model. Memory-side *Error keys unchanged. - Test-drive read-back: a blank returningFields entry means NO filter - [""] no longer nulls steps/outputs/properties out of the snapshot. - Generated tools: body $refs resolve one level (schemas namespace only), so descriptions name real fields - a guessed say-body bound to InputData defaults and silently sent an empty message; enum values and defaults now reach parameter descriptions (the environment typo->production trap). - padDataLines normalises bare CR; RFC 7615 headers join the credential response deny-list; #688's shared-path stripping disclosed in changelog. 264 tests across the affected suites, including mutation-informed pins: same-tool ordinal drop, refused-mid-batch pairing, blank-filter recovery, redaction survival of the failure reason.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
pull Bot
pushed a commit
to Stars1233/EDDI
that referenced
this pull request
Aug 17, 2026
…dit changes ImportStyleTest.noInlineFullyQualifiedNames (AGENTS.md 4.7) went red on chore/remove-agent-father right after labsai#690 merged. Three files from that PR used inline fully-qualified names. Conversation.java spelled out java.util.regex.Pattern twice inside pendingPlaceholderCandidates(). The compiled pattern becomes an ORDINAL_SUFFIX constant beside the other pending-message constants, so it is compiled once at class-init instead of on every resume. ApiCallsTaskTest used java.util.Map.of(...) six times; the import was already there. ConversationMemoryUtilitiesTest used java.util.Arrays.asList, now List.of. No behaviour change: the regex text and the DOTALL flag are byte-identical to what they replaced.
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.
A four-reviewer audit of everything merged into this branch (#679–#689) plus an end-to-end trace of the operator paths — then a second adversarial round over the fix itself, which caught a HIGH defect in my own first version. The changelog entry carries the full account; the highlights:
Confirmed findings, fixed
Cross-version placeholder stranding.
dropPendingApprovalPlaceholderremoves the pending bubble by recomputing the exact string — silently assuming pause and resume run the same build. Two releases changed the default wording, so every in-flight pause across the upgrade would have rendered[stale placeholder, answer]on its first resume: the precise artifact those releases were written to kill. Resume now recognises its predecessors' wording; two upgrade-boundary tests simulate a pre-upgrade pause.Self-conversation guard now holds without a pause — including the branch round two caught. #689's rule was enforced only on the approval path, so an ungated call executed with no check anywhere. My first fix guarded the main live loop; the second review round found the mixed-batch pause branch (ungated calls execute before the pause is thrown, frozen into the batch, never rechecked — one gated
deleteplus one ungated self-sayin a single model turn slipped through). Both loops now refuse with the sameNOT_EXECUTEDenvelope. Accepted cost documented in code: resolver-less tools fall back to raw-argument containment, kept because it is the only check coveringconverse_with_agenthanded the agent's own conversationId.#684 contract leaks narrowed. Failed results no longer enter
ApiCallsTask's cross-call template merge (a failed call's error text could overwrite a previous success's{body}for a later call's payload) nor the RAG system prompt (up to 2KB of proxy/WAF error page masquerading as "## Search Results"). Error bodies — and the status-message fallback — are redacted before reaching the model; a 401 routinely echoes the credential that failed. Memory-side*Errorkeys unchanged.Test-drive actually works end-to-end now. Two silent breaks:
returningFields=""(the natural fill for a required parameter) nulled steps/outputs/properties from the read-back, so a working agent looked broken; and the say tool's$refbody description carried zero field names, so a guessed{"message": …}bound toInputDatadefaults and a human-approved test message was never delivered, with a 200. Body$refsresolve one level (schemas namespace only — round two caught the wrong-namespace resolution risk); enum values and defaults reach parameter descriptions (theenvironmenttypo→production trap).Smaller: bare-
\rSSE padding, RFC 7615 headers in the credential deny-list, #688's shared-path stripping disclosed.Verified sound by the audit, no change needed
maxPausesPerTurnexhaustion is fail-closed; the pause ordinal is deterministic across all six resume entry points; #687's guard cannot diverge from the pinned fingerprint; conversation ids are globally unique across environments, so test-env conversations read back fine.Tests
264 across the affected suites — including upgrade-boundary resumes, live-path refusals, refused-mid-batch behaviour, blank-filter recovery, and redaction survival of the failure reason. Local full unit baseline unchanged (
GroupHitlITneeds Docker, as always).