Skip to content

[BUG]: modify_payload mutations to non-Text ContentParts are silently discarded by AplRouteHandler #151

Description

@jonpspri

Summary

A plugin that mutates a message via PluginResult::modify_payload has its mutation silently discarded unless the mutation happens to change ContentPart::Text content. Mutations to ToolCall, ToolResult, Resource, or Thinking parts are invisible to the detection logic and never reach the host.

This is a silent-data-loss bug: no warning is logged, the plugin reports success, and the unmutated payload is forwarded.

Versions: apl-cpex 0.2.2, cpex-core 0.2.2 (crates.io).

Root cause

AplRouteHandler decides whether to emit modified_payload using three branches (apl-cpex-0.2.2/src/route_handler.rs:426-458):

  1. route_payload.args != pre_args — an APL args: pipeline changed args
  2. Post-phase result: pipeline changed the result
  3. msg_payload.message.get_text_content() != final_payload.message.get_text_content() (line 450)

Branch 3 is explicitly intended to catch direct modify_payload mutations. Its own comment says so:

} else if msg_payload.message.get_text_content() != final_payload.message.get_text_content()
{
    // A `pre_invocation:` plugin mutated the message directly via
    // `modify_payload` (not through a field pipeline). Pass
    // the invoker's view through unchanged.
    Some(Box::new(final_payload) as Box<dyn PluginPayload>)
} else {
    None
};

But Message::get_text_content() matches only ContentPart::Text (cpex-core-0.2.2/src/cmf/message.rs:86-94):

pub fn get_text_content(&self) -> String {
    let mut texts = Vec::new();
    for part in &self.content {
        if let ContentPart::Text { text } = part {
            texts.push(text.as_str());
        }
    }
    texts.join("")
}

ContentPart (cpex-core-0.2.2/src/cmf/content.rs:239+) has Text, Thinking, ToolCall, ToolResult, and Resource. So four of five variants can be mutated with no observable effect, and the branch meant to honour modify_payload only works when the plugin coincidentally also edits text.

Impact

This is most severe for redaction / sanitisation plugins, which are precisely the plugins that mutate ToolResult.content. A plugin that correctly detects and redacts sensitive data in a tool result returns a mutated payload, the invoker accepts it, and the handler then drops it — so the unredacted data is forwarded downstream while the plugin's telemetry says it redacted.

We hit this building a PII redactor: the redaction silently did not reach the wire.

Reproduction

  1. Write a plugin whose pre_invocation/post_invocation returns PluginResult::modify_payload with a MessagePayload whose ContentPart::ToolResult { content } has been altered (and whose Text parts are untouched).
  2. Route it through AplRouteHandler.
  3. Observe the host receives the original, unmutated ToolResult. No warning is emitted.

Changing any Text part in the same mutation makes it work, which is the tell.

Workaround (for others hitting this)

When our redactor mutates, it also appends a throwaway ContentPart::Text summary so branch 3 fires and the real mutation rides along. That is obviously a hack and depends on downstream re-serializers ignoring stray text parts.

Suggested fix

The ground truth already exists and is being thrown away. CmfPluginInvoker knows definitively whether the plugin returned a mutation (apl-cpex-0.2.2/src/cmf_invoker.rs:332) and already writes it into the shared payload:

let modified_value = if let Some(mp_boxed) = result.modified_payload.as_ref() {
    match mp_boxed.as_any().downcast_ref::<MessagePayload>() {
        Some(modified) => {
            *self.payload.lock().await = modified.clone();

Rather than have AplRouteHandler re-derive "was this modified?" heuristically from text, the invoker could record a payload_modified flag that the handler consults. That is exact instead of approximate, and removes the content-shape dependency entirely.

A narrower alternative is to compare the full content vector rather than only text, which requires PartialEq over ContentPart. That fixes the symptom but keeps the diff-based approach.

Related

Same family as #135 and #54 (mutations silently dropped due to an equality/detection quirk), though the mechanism here is the get_text_content() heuristic rather than copy-on-write equality. Worth considering together, since the pattern is "mutation detection inferred rather than signalled".

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    Projects

    Status
    Backlog

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions