fix: extract action params from standalone XML blocks in comma-separated format#6692
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
Claude finished @HaruHunab1320's task in 1m 5s —— View job PR Review: fix/comma-separated-action-params
✅ Overall: LGTMThe fix correctly addresses the issue where action params were dropped when the LLM outputs actions in comma-separated format with standalone XML blocks. Changes Summarypackages/typescript/src/services/message.ts
packages/typescript/src/tests/comma-separated-action-params.test.ts
Feedback
💡 SUGGESTION: Source code string matching tests are brittle 💡 SUGGESTION: Consider extracting reserved keys to a constant const RESERVED_XML_KEYS = ["actions", "thought", "text", "simple"] as const;
// Then use: !RESERVED_XML_KEYS.includes(matchingKey as any)This improves maintainability if more reserved keys are added later. Security✅ No security issues found - input is already parsed XML from LLM output Type Safety✅ Proper type checking with Test Coverage✅ 7 tests covering the fix, using vitest Verdict: Ready to merge after considering the unrelated indentation changes. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| describe("comma-separated action params extraction (message service)", () => { | ||
| it("scans parsedXml for standalone action blocks in the comma-split path", () => { | ||
| // The fix must look for top-level parsedXml keys matching action names | ||
| expect(messageServiceSource).toContain("standaloneParamsFragments"); | ||
| expect(messageServiceSource).toContain("commaSplitActions"); | ||
| }); | ||
|
|
||
| it("skips reserved keys (actions, thought, text, simple)", () => { | ||
| expect(messageServiceSource).toMatch(/matchingKey\s*!==\s*"actions"/); | ||
| expect(messageServiceSource).toMatch(/matchingKey\s*!==\s*"thought"/); | ||
| expect(messageServiceSource).toMatch(/matchingKey\s*!==\s*"text"/); | ||
| }); | ||
|
|
||
| it("only populates params when not already set", () => { | ||
| // Guard: don't overwrite explicit params from structured <action> blocks | ||
| expect(messageServiceSource).toMatch( | ||
| /!parsedXml\.params\s*\|\|\s*parsedXml\.params\s*===\s*""/, | ||
| ); | ||
| }); | ||
|
|
||
| it("assembles params in legacy flat format for parseActionParams", () => { | ||
| // The assembled string wraps action content: <UPPER_NAME>content</UPPER_NAME> | ||
| expect(messageServiceSource).toContain("standaloneParamsFragments.push"); | ||
| expect(messageServiceSource).toContain( | ||
| "parsedXml.params = standaloneParamsFragments", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The first describe block reads message.ts as a raw string and asserts on internal variable names like "standaloneParamsFragments" and "commaSplitActions". These tests pass only as long as those exact identifiers remain unchanged. Any safe rename/refactor of those variables would break the tests without changing any observable behaviour.
A more robust approach is to test the observable output of the normalised-action path directly by invoking a thin helper (or temporarily exporting the extraction logic) and asserting on the returned action list and resulting parsedXml.params value. That way the tests survive any internal refactoring and also catch regressions that the source-scan approach cannot (e.g. the logic running but producing the wrong output).
| if ( | ||
| matchingKey && | ||
| matchingKey !== "actions" && | ||
| matchingKey !== "thought" && | ||
| matchingKey !== "text" && | ||
| matchingKey !== "simple" && | ||
| typeof parsedXml[matchingKey] === "string" && | ||
| (parsedXml[matchingKey] as string).includes("<") | ||
| ) { | ||
| standaloneParamsFragments.push( | ||
| `<${upperName}>${parsedXml[matchingKey]}</${upperName}>`, | ||
| ); |
There was a problem hiding this comment.
matchingKey comes directly from Object.keys(parsedXml), which preserves whatever casing the XML parser emits. If the LLM ever outputs <THOUGHT>, <ACTIONS>, or <TEXT> in uppercase (or mixed case), the guards below will not fire:
matchingKey !== "actions" // won't catch "ACTIONS" or "Actions"
matchingKey !== "thought" // won't catch "THOUGHT" or "Thought"
matchingKey !== "text" // won't catch "TEXT"
matchingKey !== "simple" // won't catch "SIMPLE"Since the look-up itself already uses toUpperCase() for matching, applying .toLowerCase() to matchingKey in the guards would make the filtering consistent:
| if ( | |
| matchingKey && | |
| matchingKey !== "actions" && | |
| matchingKey !== "thought" && | |
| matchingKey !== "text" && | |
| matchingKey !== "simple" && | |
| typeof parsedXml[matchingKey] === "string" && | |
| (parsedXml[matchingKey] as string).includes("<") | |
| ) { | |
| standaloneParamsFragments.push( | |
| `<${upperName}>${parsedXml[matchingKey]}</${upperName}>`, | |
| ); | |
| if ( | |
| matchingKey && | |
| matchingKey.toLowerCase() !== "actions" && | |
| matchingKey.toLowerCase() !== "thought" && | |
| matchingKey.toLowerCase() !== "text" && | |
| matchingKey.toLowerCase() !== "simple" && | |
| typeof parsedXml[matchingKey] === "string" && | |
| (parsedXml[matchingKey] as string).includes("<") | |
| ) { |
HaruHunab1320
force-pushed
the
fix/comma-separated-action-params
branch
from
268170e to
7141b4f
Compare
|
Claude finished @HaruHunab1320's task in 1m 9s —— View job PR Review: fix/comma-separated-action-params
✅ LGTM - Safe to mergeThe fix correctly addresses the parameter-loss bug when LLM outputs actions in comma-separated format with standalone XML blocks. Changes Summarypackages/typescript/src/services/message.ts
packages/typescript/src/tests/comma-separated-action-params.test.ts
Feedback💡 SUGGESTION: Source-inspection tests are brittle 💡 SUGGESTION: Separate unrelated changes Verification
Verdict: The core logic is correct and well-guarded. The P2 findings from Greptile (brittle tests, case-sensitive guards) have been addressed. Ready to merge. |
…ted format When the LLM outputs actions as a comma-separated list (`<actions>REPLY,START_CODING_TASK</actions>`), the parser extracted action names but dropped their parameters. The LLM also outputs standalone blocks like `<START_CODING_TASK><repo>...</repo></START_CODING_TASK>` as sibling XML elements — the XML parser puts these as top-level keys on parsedXml, but the message service never collected them into parsedXml.params for downstream consumption by parseActionParams. This caused action handlers to receive empty parameters, breaking features like coding agent swarm spawning where task/agents/repo params are required for coordinator registration. The fix scans parsedXml for top-level keys matching comma-split action names and assembles them into the legacy flat params format that parseActionParams already handles. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
HaruHunab1320
force-pushed
the
fix/comma-separated-action-params
branch
from
7141b4f to
48f5650
Compare
|
Claude finished @HaruHunab1320's task in 58s —— View job PR Review: fix/comma-separated-action-params
✅ LGTM - ApprovedPrevious review feedback has been addressed. The fix correctly solves the parameter-loss bug. Changes Summarypackages/typescript/src/services/message.ts
packages/typescript/src/tests/comma-separated-action-params.test.ts
Previous Review Issues - RESOLVED
Verification
💡 Minor: The TTS indentation cleanup (lines 377-420) is unrelated to the param extraction fix. Consider separate commits for cleaner history, but this is non-blocking. |
1 participant
Summary
<actions>REPLY,START_CODING_TASK</actions>(comma-separated), the message service extracted action names but dropped all parameters<START_CODING_TASK><repo>...</repo></START_CODING_TASK>— the XML parser puts these as top-level keys onparsedXml, but the comma-split path never collected them intoparsedXml.paramsprocessActionsto call handlers with emptyoptions.parameters, breaking any action that relies on params (e.g., coding agent spawning wheretask/agents/repoare needed)Fix
After comma-splitting action names, scan
parsedXmlfor top-level keys matching those names. If found and they contain XML children, assemble them intoparsedXml.paramsin the legacy flat format (<ACTION_NAME>...</ACTION_NAME>) thatparseActionParamsalready consumes.Guards:
parsedXml.paramsis not already set (structured<action>blocks take priority)actions,thought,text,simple)<(actual XML, not plain text)Test plan
comma-separated-action-params.test.ts— 7 tests covering source verification +parseActionParamsintegration with the flat format🤖 Generated with Claude Code
Greptile Summary
This PR fixes a bug where action parameters were silently dropped when the LLM used the comma-separated
<actions>REPLY,START_CODING_TASK</actions>format instead of the structured<action>block format. The fix scansparsedXmlfor top-level keys matching the comma-split action names and assembles them into theparsedXml.paramsstring thatparseActionParamsalready knows how to consume, mirroring what the structured-action path already did.\n\nChanges:\n-message.ts– After comma-splitting action names, iteratesparsedXmlfor matching top-level keys (skipping reserved ones and non-XML values), wraps them into the legacy flat<ACTION_NAME>…</ACTION_NAME>format, and assigns the result toparsedXml.params(only when params is not already populated). Also cleans up excessive indentation in the TTS block.\n- New test file – 7 tests: 4 source-inspection assertions (variable names, patterns) and 3 properparseActionParamsintegration tests covering single-action, multi-action, and empty-input cases.\n\nObservations:\n- The core logic is correct:parsedXml[matchingKey]holds inner XML content (the XML parser strips the outer wrapper), so re-wrapping with<UPPER_NAME>before joining produces exactly the legacy flat format expected byparseActionParams.\n- The!parsedXml.params || parsedXml.params === \"\"guard correctly prevents overwriting params already populated by the structured-action path.\n- The reserved-key exclusions (actions,thought,text,simple) use case-sensitive string comparisons againstmatchingKey, which could miss uppercase/mixed-case variants emitted by some XML parsers.\n- The source-inspection tests in the firstdescribeblock are brittle: they assert on internal variable names and will break on any rename, even safe ones.Confidence Score: 5/5
Safe to merge; the fix correctly addresses the parameter-loss bug and all remaining findings are P2 style/robustness suggestions.
The functional change is correct and well-guarded: it only activates when params are absent, skips reserved keys, validates for XML content, and the assembled string is in the exact format that the existing parseActionParams already handles. The two P2 findings (brittle source-inspection tests and case-sensitive reserved-key guards) are non-blocking quality improvements. No P0 or P1 issues found.
packages/typescript/src/tests/comma-separated-action-params.test.ts — the source-inspection test block should ideally be replaced with behavioural tests.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[LLM response parsed into parsedXml] --> B{parsedXml.actions type?} B -- "string containing action" --> C[Structured path: extract action entries and inline params] C --> D{actionEntries found?} D -- yes --> E[Build inlineParamsXml string if parsedXml.params is empty] E --> F[Return action names list] D -- no --> G[Fall through to comma-split path] B -- "plain comma-separated string" --> G G --> H[Split by comma → commaSplitActions] H --> I{parsedXml.params empty?} I -- yes --> J[For each action name: find matching top-level key in parsedXml] J --> K{Key found and not reserved and value contains '<'?} K -- yes --> L[Push ACTION_NAME inner content to standaloneParamsFragments] K -- no --> M[Skip] L --> N{Any fragments?} N -- yes --> O[Set parsedXml.params = joined fragments] O --> P[Return commaSplitActions] N -- no --> P I -- no --> P B -- "array" --> Q[Return parsedXml.actions directly] F --> R[responseContent = spread parsedXml + overrides] P --> R Q --> R R --> S[parseActionParams reads responseContent.params → Map of action to params]Reviews (1): Last reviewed commit: "fix: extract action params from standalo..." | Re-trigger Greptile