feat(language): Allow including partial model in loadDocument when there is error

[jiashengguo]

Summary by CodeRabbit

• New Features
  • Document loader can now optionally include a partial model when parsing fails, improving error context for diagnostics and recovery workflows.
  • Error reports may now include partial parsing results alongside errors and warnings to aid troubleshooting and more robust handling of problematic documents.

[claude]

Claude finished @jiashengguo's task —— View job

---

PR Review Complete ✅

Todo List

• Read repository guidelines from CLAUDE.md
• Analyze the changed file (packages/language/src/document.ts)
• Review code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Evaluate security concerns
• Review test coverage needs
• Provide comprehensive feedback

Code Review Feedback

This PR adds a new optional parameter returnPartialModelForError to the loadDocument function that allows returning a partial model even when there are errors during document loading.

✅ Code Quality & Best Practices

• Good: The implementation follows existing code patterns and maintains backward compatibility
• Good: Parameter has a descriptive name and clear boolean type
• Good: Proper TypeScript type updates in return type union
• Good: Uses conditional logic that's easy to understand

✅ Potential Bugs & Issues

• No issues found: The implementation correctly checks returnPartialModelForError before accessing document.parseResult.value
• Safe casting: The as Model cast is consistent with the successful path (line 126, 151) and follows existing patterns
• Error handling preserved: All existing error handling logic remains intact

✅ Performance Considerations

• Minimal impact: No performance overhead when parameter is false (default)
• No additional parsing: Only returns already parsed result, no extra work performed

✅ Security Concerns

• No security issues: The change doesn't introduce any new attack vectors
• Safe defaults: Default false value maintains existing secure behavior

⚠️ Test Coverage Needs

The main concern is missing test coverage for the new functionality:

• Missing tests: No tests verify the new returnPartialModelForError parameter behavior
• Recommended additions:

  // Test that partial model is returned when flag is true
  const result = await loadDocument('invalid-schema.zmodel', [], true, true);
  expect(result.success).toBe(false);
  expect(result.model).toBeDefined(); // Should have partial model
  
  // Test that no model is returned when flag is false (existing behavior)  
  const result2 = await loadDocument('invalid-schema.zmodel', [], true, false);
  expect(result2.success).toBe(false);
  expect(result2.model).toBeUndefined(); // Should not have model

📝 Minor Suggestions

1. Documentation: Consider adding JSDoc comment explaining when/why to use returnPartialModelForError
2. Type safety: The current implementation assumes document.parseResult.value is always a valid Model even with errors - consider adding runtime validation

Overall Assessment

This is a clean, backward-compatible addition that enables error-tolerant document loading. The implementation is solid, but would benefit from comprehensive test coverage to ensure the new behavior works correctly across various error scenarios.

Recommendation: ✅ Approve after adding tests for the new parameter behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 071c3764-6018-4cd9-8392-217971ea8f76

📥 Commits

Reviewing files that changed from the base of the PR and between a17e184 and f02da69.

📒 Files selected for processing (1)

• packages/language/src/document.ts

---

📝 Walkthrough

Walkthrough

The loadDocument function now accepts an optional returnPartialModelForError: boolean (default false). On failure paths it may return an optional model?: Model populated from document.parseResult.value when returnPartialModelForError is true; success path unchanged.

Changes

Cohort / File(s)	Summary
Document Loading Enhancement packages/language/src/document.ts	Added optional returnPartialModelForError parameter to loadDocument. Failure return type extended to include an optional model?: Model, populated from document.parseResult.value when returnPartialModelForError is true and errors occur. Success path unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A tiny flag hopped into place,
When parsing trips, it leaves a trace,
A partial model, snug and small,
Returned when errors loudly call.
Default is false, the old path stays—hooray!

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding a feature to include a partial model in loadDocument when errors occur.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jiasheng-dev

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/language/src/document.ts`:
- Around line 117-123: The failure-return path is inconsistent: when
returnPartialModelForError is true you currently include
document.parseResult.value as Model in the first errors return block but omit it
in the later additionalErrors failure path; update the later error-return to
mirror the first by returning model: returnPartialModelForError ?
(document.parseResult.value as Model) : undefined (or equivalent) so callers
consistently receive the partial Model for both the initial errors branch and
the additionalErrors branch; look for the additionalErrors handling in this file
and add the same conditional model field to its returned object.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5e37f734-51cf-4d04-964e-f42764f38075

📥 Commits

Reviewing files that changed from the base of the PR and between 826cc4e and a17e184.

📒 Files selected for processing (1)

• packages/language/src/document.ts

[claude]

Claude finished @jiashengguo's task —— View job

---

PR Code Review Complete ✅

Todo List

• Read repository guidelines from CLAUDE.md
• Analyze the current state of the changed file
• Review CodeRabbit's feedback about inconsistent error paths
• Evaluate code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Evaluate security concerns
• Review test coverage needs
• Provide comprehensive feedback

Review Summary

This PR adds an optional returnPartialModelForError parameter to the loadDocument function (packages/language/src/document.ts:36), allowing callers to receive a partial model even when document loading encounters errors.

✅ CodeRabbit Issue Resolution

FIXED: The inconsistency identified by CodeRabbit has been resolved! Both error return paths now correctly include the conditional model field:

• Lines 120: Initial error path includes model: returnPartialModelForError ? (document.parseResult.value as Model) : undefined
• Lines 144: Additional errors path also includes model: returnPartialModelForError ? (document.parseResult.value as Model) : undefined

✅ Code Quality & Best Practices

• Excellent: Clean, backward-compatible API design with sensible defaults
• Good: Parameter name returnPartialModelForError is descriptive and self-documenting
• Good: Proper TypeScript union type for conditional model field in return type (line 39)
• Good: Consistent with existing code patterns and follows repository conventions
• Good: Maintains existing function behavior when parameter is omitted (default false)

✅ Potential Bugs & Issues

• No critical issues: Implementation correctly handles both error scenarios
• Safe casting: The document.parseResult.value as Model cast is consistent throughout the codebase
• Proper conditionals: Both error paths correctly check returnPartialModelForError before including model
• Error handling preserved: All existing error detection and reporting logic remains intact

✅ Performance Considerations

• Excellent: Zero performance overhead when returnPartialModelForError is false (default)
• Minimal impact: When true, only returns already-parsed data without additional processing
• No memory leaks: No retention of unnecessary references or objects

✅ Security Concerns

• No security issues: The change doesn't introduce any new attack vectors
• Safe defaults: Default false behavior maintains existing secure patterns
• No data exposure: Only returns data that was already being processed

⚠️ Test Coverage Gap

The primary concern remains insufficient test coverage for the new functionality:

Missing test scenarios:

• Verify partial model returned when returnPartialModelForError: true and parsing errors occur
• Verify no model returned when returnPartialModelForError: false and parsing errors occur
• Verify behavior consistency across both error paths (initial diagnostics vs additional errors)
• Validate that returned partial model contains expected structure even with errors

Suggested test location: Based on repository structure, tests should be added to packages/language/src/ test files or tests/regression/test/ following the pattern in CLAUDE.md.

📋 Additional Recommendations

1. Documentation: Consider adding JSDoc comments explaining when/why to use this parameter
2. Type safety: Current implementation assumes document.parseResult.value is always a valid Model - consider runtime validation if needed

Overall Assessment

This is a well-implemented, backward-compatible enhancement that addresses error-tolerant document loading needs. The code quality is excellent and the CodeRabbit-identified inconsistency has been properly resolved.

Recommendation: ✅ Approve after adding comprehensive test coverage for the new parameter behavior.