Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: f84226e The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
✅ Summary
This is a well-executed bugfix that addresses a regex parsing issue where > characters inside quoted attribute values (e.g., JMESPath expressions like docs[?title=='Platform > llms.txt']) were incorrectly interpreted as tag terminators.
What's Good:
- The fix consistently applies the quote-aware pattern
(?:"[^"]*"|'[^']*'|[^>])+?across all 5 regex instances - Comprehensive test coverage (271 lines) with excellent edge case coverage including:
- Standard tags, double-quoted values with
>, single-quoted values with> - Compound JMESPath expressions with
&& - Real-world reproduction case
- Streaming boundary scenarios
- Standard tags, double-quoted values with
- Changeset included with appropriate scope
Regex Pattern Explanation:
The new pattern (?:"[^"]*"|'[^']*'|[^>])+? works by:
"[^"]*"— Match double-quoted strings (any chars except")'[^']*'— Match single-quoted strings (any chars except')[^>]— Match any char except>(for unquoted portions)
This allows > characters inside quoted values while still correctly detecting tag boundaries.
💭 Consider (2) 💭
The following are optional improvements for test coverage completeness. They're noted as inline comments:
💭 1) ArtifactParser.test.ts:270 artifact:ref parsing test
Tests cover artifact:create thoroughly but artifact:ref data retrieval via getArtifactSummary is only verified at detection level.
💭 2) ArtifactParser.test.ts:256-261 Exception handling test
The null return case is tested, but the thrown exception code path (lines 283-291 of ArtifactParser.ts) is untested.
Inline Comments:
- 💭 Consider:
ArtifactParser.test.ts:270artifact:ref parsing test suggestion - 💭 Consider:
ArtifactParser.test.ts:256-261exception handling test suggestion
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
parseObject |
Missing tests for parseObject method | Pre-existing untested code — method wasn't modified in this PR |
parseCreateAttributes |
Missing validation test for required attributes | Valid but nitpick — the validation is straightforward and failures would surface via other test failures |
nested quotes |
Missing test for complex nested quote patterns | Edge case with low likelihood — current prompts enforce quote usage patterns |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
5 | 0 | 2 | 0 | 2 | 0 | 3 |
| Total | 5 | 0 | 2 | 0 | 2 | 0 | 3 |
✅ APPROVE
Summary: Clean bugfix with solid test coverage. The regex changes are correct and consistently applied. The two "Consider" items are optional test coverage enhancements — they don't block approval. Ship it! 🚀
Note: Unable to submit formal approval due to repository access permissions. This review recommends approval.
| const parts = await parser.parseText(text); | ||
| expect(parts.filter((p) => p.kind === 'data')).toHaveLength(1); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💭 Consider: Missing artifact:ref parsing test
Issue: The tests thoroughly cover artifact:create tags but don't include a test for artifact:ref tag parsing in parseText. The hasArtifactMarkers test at line 99 only verifies detection, not the actual data retrieval path.
Why: Lines 340-342 of ArtifactParser.ts handle artifact:ref parsing via getArtifactSummary, and this code path could regress silently.
Fix: Consider adding:
it('parses artifact:ref tags via getArtifactSummary', async () => {
mockArtifactService.getArtifactSummary.mockResolvedValue(mockArtifactData);
const text = `<artifact:ref id='a1' tool='t1' />`;
const parts = await parser.parseText(text);
expect(parts).toHaveLength(1);
expect(parts[0].kind).toBe('data');
expect(mockArtifactService.getArtifactSummary).toHaveBeenCalledWith('a1', 't1', undefined);
});| it('removes the artifact tag when the service returns null', async () => { | ||
| mockArtifactService.createArtifact.mockResolvedValue(null); | ||
| const text = `Before <artifact:create id='a1' tool='t1' type='citation' base='result' /> After`; | ||
| const parts = await parser.parseText(text); | ||
| const dataParts = parts.filter((p) => p.kind === 'data'); | ||
| expect(dataParts).toHaveLength(0); |
There was a problem hiding this comment.
💭 Consider: Missing error handling test for thrown exceptions
Issue: This test covers when createArtifact returns null, but doesn't test when it throws an exception. Lines 283-291 of ArtifactParser.ts have a catch block that removes failed annotations and logs errors.
Why: Different code path than null return — exception handling removes the tag from output, which should be verified.
Fix: Consider adding:
it('removes the artifact tag and continues when createArtifact throws', async () => {
mockArtifactService.createArtifact.mockRejectedValue(new Error('Service error'));
const text = `Before <artifact:create id='a1' tool='t1' type='citation' base='result' /> After`;
const parts = await parser.parseText(text);
const dataParts = parts.filter((p) => p.kind === 'data');
expect(dataParts).toHaveLength(0);
});
amikofalvy
left a comment
amikofalvy
left a comment
There was a problem hiding this comment.
The changes look good. The new test suite is much needed.
Do an audit to see if there are other character issues that need to be addressed, not just the > one that we discovered.
Ito Test Report ✅22 test cases ran. 22 passed. This test run verified the ArtifactParser regex update in PR #2278, which fixes parsing of ✅ Passed (22)
📋 View Recording |
2 participants
No description provided.