fix: audit follow-ups — LLM-provider binding, hint-mode deser, handler dedup, tc26 flake - #1144
Merged
Merged
Conversation
#1134) The three tc26_cancel_posts_synthetic_event variants (Python/TS/Java) gated the producer-log marker assertions on a fixed 3s wait, which races the async stdout->log capture under CI load (observed FAIL,FAIL,FAIL,PASS on Java). Replace the fixed wait with a poll-until-marker loop (~30x1s) that captures the producer log only once the cancelled_gracefully marker is present; diagnostics to stderr, fail loud on timeout. All registry-side + producer-log assertions retained unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ansform hook (Closes #1136) Two behavior-preserving dedups in the spring-ai vendor handlers: - Message assembly: the three handlers duplicated the user-content loop, the non-system-message extraction, and the system-message replacement loop, differing only in the assistant-prefix literal. Hoisted into LlmProviderHandler defaults (buildUserContent / extractNonSystemMessages / replaceSystemMessage) + a previousResponsePrefix() hook (default "[Previous Assistant Response]\n"; Gemini overrides to "[Previous Response]\n"). - Tool-callback transform: GeminiHandler re-implemented the whole base createToolCallback / createToolCallbacksForSchema just to upper-case the tool input schema. Added a transformToolInputSchema(Map) hook (default identity) to the base; Gemini now overrides only that hook (convertSchemaTypesToUpperCase), dropping both full overrides. Byte-identical per vendor (only the code path is shared). Validated: mvn compile BUILD SUCCESS; spring-ai 135 tests green (incl. GeminiHandler tool-callback + BuildToolNoExecute). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…1141) process_llm_providers_changes inserted the resolved provider into self.topology.llm_providers BEFORE sending the event, and discarded the send result. Once a function_id is recorded, the diff gate never re-emits it — so a dropped send (back-pressure/first-heartbeat-burst race) permanently suppressed re-emission, leaving that @Meshllm function's proxy unbound. The failure scaled with the number of @Meshllm functions on an agent. Reorder to send-before-insert, mirroring process_dependency_changes: the function_id is recorded as "seen" only after a successful send, so a dropped send leaves it un-inserted and the next heartbeat re-emits (self-healing, <= one heartbeat-interval delay). Replaced the `let _ = ...send()` discard with an explicit Err warn + continue. Happy path unchanged. Shared-core fix (benefits Python/TS/Java). Validated: cargo build/test green (420 passed, --no-default-features per CI), zero new clippy warnings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d output (Closes #1142) Under output_mode=hint the provider embeds the schema in the prompt but does not enforce it, so the LLM can return a scalar where the schema declares a list (e.g. "insights": "x" instead of ["x"]). Strict deserialization rejected it (Java threw MismatchedInputException → fallback). Add scoped single-value-as- array coercion at each runtime's response-model deserialization boundary; no-op for well-shaped (strict) output. - Java: dedicated responseModelMapper with ACCEPT_SINGLE_VALUE_AS_ARRAY, used only by generate(Class) (shared objectMapper untouched). - Python: ResponseParser._coerce_scalar_list_fields (list[...] / List[...] / Optional[List[...]]) before Pydantic validation. - TypeScript: ResponseParser.coerceScalarArrayFields via zodToJsonSchema introspection before safeParse. Coercion is top-level (Jackson also handles nesting); scoped to structured- output parsing only. Validated: Java 13, Python 31, TS 843 tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 22 minutes and 23 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR addresses "hint-mode drift" by implementing scalar-to-list coercion across Rust, Java, Python, and TypeScript runtimes, refactoring shared message-handling in Java handlers, and improving integration test reliability with polling instead of fixed waits. ChangesScalar Coercion & Message Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@tests/integration/suites/uc21_meshjob/tc26_cancel_posts_synthetic_event/test.yaml`:
- Around line 103-106: The detection step greps the full output of "meshctl logs
long-task-provider" for "[run_until_cancel] cancelled_gracefully" but the
subsequent capture uses "tail -200" which can omit older markers; make the
capture use the same log source as the detection so they match (either remove
the "tail -200" and run the same "meshctl logs long-task-provider 2>&1 | grep
\"[run_until_cancel] cancelled_gracefully\"" or capture the full output into a
variable and grep that variable) and ensure the grep pattern uses the same
escaped marker string as in the detection.
In
`@tests/integration/suites/uc22_meshjob_ts/tc26_cancel_posts_synthetic_event_ts/test.yaml`:
- Around line 96-99: The test intermittently misses the marker because it calls
meshctl logs long-task-provider-ts twice (once for detection with grep -qF and
again with tail -200 for capture); instead capture the logs once into a variable
or temporary file (run meshctl logs long-task-provider-ts > /tmp/logs or assign
to LOGS="$(meshctl logs long-task-provider-ts)") and then run grep -qF
"[run_until_cancel] cancelled_gracefully" against that single snapshot and, on
success, print the relevant lines from the same snapshot (e.g., use echo "$LOGS"
| tail -200 | grep "\[run_until_cancel\] cancelled_gracefully") so both
detection and capture operate on identical log content.
In
`@tests/integration/suites/uc23_meshjob_java/tc26_cancel_posts_synthetic_event_java/test.yaml`:
- Around line 126-129: The test currently calls meshctl logs
long-task-provider-java twice (once with grep -qF and then again piped to tail
-200) which can miss the marker due to different reads; change the block to read
the logs once into a variable (or a temp file) and use that same captured output
for both the presence check and the tail extraction so both grep and the tail
operate on the identical log snapshot (reference: the commands using meshctl
logs long-task-provider-java, the grep -qF "[run_until_cancel]
cancelled_gracefully" check, and the tail -200 extraction).
🪄 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
Run ID: 58cf4c6a-24b9-4cd7-be67-3fefd01ceba4
📒 Files selected for processing (14)
src/runtime/core/src/runtime.rssrc/runtime/java/mcp-mesh-spring-ai/src/main/java/io/mcpmesh/ai/handlers/AnthropicHandler.javasrc/runtime/java/mcp-mesh-spring-ai/src/main/java/io/mcpmesh/ai/handlers/GeminiHandler.javasrc/runtime/java/mcp-mesh-spring-ai/src/main/java/io/mcpmesh/ai/handlers/LlmProviderHandler.javasrc/runtime/java/mcp-mesh-spring-ai/src/main/java/io/mcpmesh/ai/handlers/OpenAiHandler.javasrc/runtime/java/mcp-mesh-spring-boot-starter/src/main/java/io/mcpmesh/spring/MeshLlmAgentProxy.javasrc/runtime/java/mcp-mesh-spring-boot-starter/src/test/java/io/mcpmesh/spring/MeshLlmAgentProxyResponseModelLenientTest.javasrc/runtime/python/_mcp_mesh/engine/response_parser.pysrc/runtime/python/tests/unit/test_response_parser_scalar_list.pysrc/runtime/typescript/src/__tests__/response-parser.test.tssrc/runtime/typescript/src/response-parser.tstests/integration/suites/uc21_meshjob/tc26_cancel_posts_synthetic_event/test.yamltests/integration/suites/uc22_meshjob_ts/tc26_cancel_posts_synthetic_event_ts/test.yamltests/integration/suites/uc23_meshjob_java/tc26_cancel_posts_synthetic_event_java/test.yaml
…1134) The tc26 poll loop read `meshctl logs <provider>` twice per iteration: full-log detection (grep -qF) vs a second read piped through `tail -200` for capture. If >200 log lines followed the marker, detection passed but the tail-200 capture missed it -> empty producer_log -> assertion failure. Snapshot the log once into $LOGS and use that single snapshot for both detection and extraction (drop tail -200; grep -F emits only the marker line, which carries the events_seen payload). Same fixed-string pattern for both; timeout diagnostics reuse the last snapshot. Assertions unchanged. Validated: all three tc26 variants GREEN (Java x2); events_seen content confirmed on the captured marker line. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Four independent audit follow-ups, each a clean per-issue commit, validated individually and by one comprehensive integration run.
process_llm_providers_changesinserted the resolved provider intoself.topology.llm_providersbefore sendingLLM_PROVIDER_AVAILABLEand discarded the send result. Once afunction_idwas recorded, the diff gate never re-emitted it — so a dropped send (first-heartbeat-burst back-pressure/race) left that@MeshLlmfunction's proxy permanently unbound (worsening with more@MeshLlmfunctions per agent). Reordered to send-before-insert (mirroringprocess_dependency_changes): the funcId is recorded only after a successful send, so a dropped send re-emits next heartbeat (self-healing); thelet _ = …send()discard is now anErrwarn +continue. Happy path unchanged; benefits all runtimes.LlmProviderHandlerdefaults with apreviousResponsePrefix()hook (default[Previous Assistant Response]\n; Gemini overrides[Previous Response]\n); collapsed Gemini's two fullcreateToolCallback(sForSchema)overrides into a singletransformToolInputSchema(Map)hook (default identity; Gemini =convertSchemaTypesToUpperCase). Byte-identical per vendor.hintmode embeds the schema in the prompt without vendor enforcement, so the LLM can return a scalar where the schema declares a list (e.g."insights":"x"vs["x"]); strict deserialization rejected it. Added scoped single-value-as-array coercion at each runtime's response-model boundary (Java: dedicatedresponseModelMapperwithACCEPT_SINGLE_VALUE_AS_ARRAY, shared mapper untouched; Python:ResponseParser._coerce_scalar_list_fieldsforlist/Optional[List]; TS:coerceScalarArrayFieldsviazodToJsonSchema). No-op for well-shaped/strict output.tc26_cancel_posts_synthetic_eventvariants (Python/TS/Java) replaced a fixed 3 s producer-log-flush wait with a poll-until-marker loop (~30×1s, diagnostics to stderr, fail-loud on timeout). All registry-side + producer-log assertions retained. Fixes the observed FAIL,FAIL,FAIL,PASS flake on the Java variant.Scope note
The two high-risk structural refactors were deliberately split out for their own focused PRs (per maintainer): #1131 (TS
MeshRuntimeBaseevent-loop dedup, incl. theregistry_disconnectedbehavioral reconciliation) and #1137 (MeshAutoConfigurationgod-config split). Also deferred: theMeshAutoConfigurationfuncId-source AOP hardening noted during #1141 (latent, not the #1141 cause).Review Notes
Independent review: 0 blocker / 0 warning / 3 INFO (all non-issues). All four fixes VERIFIED; the four touch disjoint areas (core emitter / Java handlers / response parsers / test yaml) with no cross-fix coupling. #1141 reorder confirmed correct (insert only after a successful send;
continueskips only the failed funcId; happy path behavior-preserving). #1136 byte-identical per vendor. #1142 confirmed a genuine no-op when the value already matches the schema (valid arrays/None/nested untouched).Test plan
--no-default-featuresper CI) · Python 31 (Hint-mode structured output: lenient deserialization for loosely-shaped LLM responses (Java strict-deser throws; verify Python/TS parity) #1142) · TS 843 — all compile/tsc cleantsuite-mesh:local)@MeshLlmanalyst binding [Java: per-funcId LLM-provider proxy binding unreliable on agents with many @MeshLlm functions #1141 surface] + output_mode tcs), uc08 9/9 (handler assembly), uc14 31/31 (Gemini tool callbacks/media), uc15 15/15 (parallel tools), uc16_schema_filtering 3/3 + uc16_schema_registry 16/16 (Hint-mode structured output: lenient deserialization for loosely-shaped LLM responses (Java strict-deser throws; verify Python/TS parity) #1142 strict no-op), tc26 ×3 GREEN (Java stable on 2 runs — Flaky test: tc26_cancel_posts_synthetic_event_java — fixed 3s log-flush wait races producer stdout capture #1134)Closes #1134
Closes #1136
Closes #1141
Closes #1142
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests