fix(llm): a malformed workflow version must not abort tool discovery - #621
Merged
Merged
Conversation
Two findings Copilot raised against #618 after it was already approved. Kept out of that PR so they land small enough for CodeRabbit to review — #618 grew to 120 files, past CodeRabbit's 100-file limit, and merged without ever receiving a CodeRabbit pass. - WorkflowTraversal: every other malformed-URI branch in that loop warns, marks the traversal degraded and continues. The version parse did not. String.replaceAll returns its input UNCHANGED when the pattern does not match, so a workflow URI with "?version=abc" passed the contains("version=") guard and reached Integer.parseInt as the literal "version=abc". The NumberFormatException escaped discoverConfigs, so one bad workflow URI took out httpcall, mcpcall AND RAG tool discovery for the whole turn instead of skipping the single broken workflow. Now matched explicitly, treating "present but unusable" exactly like "absent"; a digit run too large for an int folds into the same path. - MemoryItemConverter logged raw exception messages in both catch blocks. That text can carry user-controlled values — the same CWE-117 class CodeQL flagged five times in #618. Routed through LogSanitizer. 90 tests pass across WorkflowTraversal, MemoryItemConverter and Deployment.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
📝 WalkthroughWalkthroughThe changes make workflow version parsing resilient to malformed query parameters and sanitize exception messages logged during memory resolution. Tests cover non-numeric, misleading, and overflowing version values. ChangesWorkflow discovery parsing
Memory conversion log sanitization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Fixes two robustness/security issues in the LLM tool-discovery path so that a single malformed workflow URI can’t abort discovery for an entire turn, and log messages don’t include unsanitized exception text.
Changes:
WorkflowTraversal: parseversion=query param defensively (explicit match + degrade-and-continue on malformed/overflowing versions).MemoryItemConverter: sanitize exception messages before logging to prevent CWE-117 style log injection.- Tests: add regression coverage ensuring malformed workflow versions return an empty result and do not query the workflow store.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/test/java/ai/labs/eddi/modules/llm/impl/WorkflowTraversalCacheTest.java | Adds regression tests for non-numeric and overflowing version= queries to ensure discovery degrades safely. |
| src/main/java/ai/labs/eddi/modules/llm/impl/WorkflowTraversal.java | Prevents tool discovery from aborting on malformed workflow version= query parsing. |
| src/main/java/ai/labs/eddi/engine/memory/MemoryItemConverter.java | Sanitizes logged exception messages via LogSanitizer. |
| docs/changelog.md | Documents the follow-up fixes and their rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+45
to
+47
| /** Matches the numeric value of a {@code version=} query param. */ | ||
| private static final Pattern VERSION_PARAM = Pattern.compile("version=(\\d+)"); | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/changelog.md (1)
8-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the changelog focused on user-visible changes.
Remove the internal review-tooling, PR-size, and approval-history details; retain only the workflow-discovery and log-sanitization behavior changes.
🤖 Prompt for 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. In `@docs/changelog.md` around lines 8 - 18, Revise the changelog entry to remove internal review-tooling, PR-size, approval-history, repository, and branch details. Retain only the user-visible changes describing resilient workflow version parsing during tool discovery and sanitized exception logging in MemoryItemConverter.
🤖 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 `@src/main/java/ai/labs/eddi/modules/llm/impl/WorkflowTraversal.java`:
- Around line 45-46: Update the VERSION_PARAM pattern used by WorkflowTraversal
to require complete query-parameter boundaries, preventing matches embedded in
other parameter names, values, or trailing non-delimiter characters while
preserving valid version parameters. Apply the same boundary-aware parsing
behavior in the version extraction block around the existing Matcher.find()
logic, and add regression coverage for malformed query strings such as
version=1abc, someversion=1, and foo=version=7.
---
Nitpick comments:
In `@docs/changelog.md`:
- Around line 8-18: Revise the changelog entry to remove internal
review-tooling, PR-size, approval-history, repository, and branch details.
Retain only the user-visible changes describing resilient workflow version
parsing during tool discovery and sanitized exception logging in
MemoryItemConverter.
🪄 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 Plus
Run ID: f182a23b-6f61-4e85-966e-ec9b1d21556e
📒 Files selected for processing (4)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/memory/MemoryItemConverter.javasrc/main/java/ai/labs/eddi/modules/llm/impl/WorkflowTraversal.javasrc/test/java/ai/labs/eddi/modules/llm/impl/WorkflowTraversalCacheTest.java
…rsion
CodeRabbit and Copilot independently flagged the same defect in the previous
commit, and both are right: "version=(\d+)" is an unanchored substring search,
so a query of "?subversion=123" matched and parsed as version 123 — reading a
workflow version out of a query that carries none.
The pre-existing code had the same flaw (contains("version=") plus
".*version=(\d+).*"), so this is not a regression, but a fix that leaves the
hole open is not a fix. Anchored to real parameter boundaries with
(?:^|&)version=(\d+)(?:&|$).
Two tests: "?subversion=123" degrades instead of parsing a version, and
"?foo=x&version=1" still reads 1 — the anchoring must not break the ordinary
multi-param case, which is the obvious way to get this wrong in the other
direction.
aisabella-ai
approved these changes
Jul 29, 2026
#614 merged to main and left this PR CONFLICTING — which matters beyond tidiness, because a conflicting PR has no computable merge ref and so cannot run CI at all. One conflict, docs/changelog.md: both sides added a top entry. Both kept, nothing dropped. Verified past the textual resolution, since a clean auto-merge has silently broken compilation twice in this stack when main changed a constructor this branch's tests were written against: the merged tree compiles, and 208 tests across both sides of the merge pass (WorkflowTraversal and MemoryItemConverter from this branch, the OpenAI adapter surface from #614).
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/changelog.md (1)
681-681: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a fenced code block here.
The indented snippet triggers markdownlint MD046. Replace the four-space indentation with fenced Markdown syntax so documentation checks pass consistently.
🤖 Prompt for 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. In `@docs/changelog.md` at line 681, Update the MongoDB health-check configuration snippet in the changelog to use a fenced Markdown code block instead of four-space indentation, preserving the configuration text and ensuring it satisfies markdownlint MD046.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@docs/changelog.md`:
- Line 681: Update the MongoDB health-check configuration snippet in the
changelog to use a fenced Markdown code block instead of four-space indentation,
preserving the configuration text and ensuring it satisfies markdownlint MD046.
ginccc
added a commit
that referenced
this pull request
Jul 29, 2026
Changelog-only conflict, both entries kept — the collision I flagged when both PRs carried a top entry. #621 merged first, so this branch absorbs it. Compiles clean; no source overlap between the two PRs.
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.
Two findings Copilot raised against #618 after it was already approved. Deliberately kept out of that PR so they arrive small enough for CodeRabbit to review — #618 reached 120 files, past CodeRabbit's 100-file limit, and merged without ever receiving a CodeRabbit pass.
WorkflowTraversalaborted tool discovery for a whole turn over one malformed URIEvery other malformed-URI branch in that loop warns, marks the traversal degraded and continues. The version parse did not.
String.replaceAllreturns its input unchanged when the pattern doesn't match. So a workflow URI carrying?version=abcpassed thecontains("version=")guard and then reachedInteger.parseIntas the literal string"version=abc". The resultingNumberFormatExceptionescapeddiscoverConfigsentirely — meaning a single bad workflow URI took out httpcall, mcpcall and RAG tool discovery for that turn, rather than skipping the one workflow that was actually broken.Now matched explicitly, with "present but unusable" treated exactly like "absent". A digit run too large for an
intfolds into the same path.MemoryItemConverterlogged raw exception messagesBoth the prompt-snippet and global-variable catch blocks interpolated
e.getMessage()directly. That text can carry user-controlled values, making it the same CWE-117 class CodeQL flagged five times in #618. Routed through the existingLogSanitizer.Tests
Two new regression tests pin the degrade behaviour (non-numeric version, and a version too large for an
int), asserting discovery returns a usable empty result and never reaches the workflow store. 90 tests pass acrossWorkflowTraversal,MemoryItemConverterandDeployment.Summary by CodeRabbit
versionhandling so malformed or oversized/non-numeric values no longer abort discovery; they now degrade gracefully and skip only the affected URI.versionquery parameter scenarios during discovery.