-
Notifications
You must be signed in to change notification settings - Fork 5
feat(databind): improve validation error messages with location and context #597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
david-waltermire
merged 17 commits into
metaschema-framework:develop
from
david-waltermire:feature/validation-error-improvements
Dec 29, 2025
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
4d0f0ac
docs: add PRD for validation error message improvements
david-waltermire 70bea19
test(databind): add tests for validation error message improvements
david-waltermire 85f3e1a
feat(databind): improve validation error messages with location and c…
david-waltermire fc7d594
docs: update implementation plan with completed tasks
david-waltermire 22b5879
docs: add PRD update requirement to conventions
david-waltermire 52ed0b7
docs: add PRD completion rule and mark validation-errors PRD complete
david-waltermire a130e99
fix: add @Nullable annotations per code review
david-waltermire 98d8487
docs: add Javadoc for null URI handling and fix hyphenation
david-waltermire 2143c4a
refactor: align deserializer contracts with @NonNull URI parameters
david-waltermire 6bfebfe
docs: add rule for responding to automated reviewers
david-waltermire 4f328aa
refactor: consolidate IMetaschemaData implementations into SimpleReso…
david-waltermire 07a2cb4
feat: add address-pr-feedback command for systematic review handling
david-waltermire 3599fa7
fix: include issue comments in address-pr-feedback command
david-waltermire 279e602
fix: replace {owner}/{repo} placeholders with dynamic REPO variable
david-waltermire 810f5c7
fix: add blank line before markdown table
david-waltermire 56f1f1d
feat: add Rejected status icon for incorrect feedback
david-waltermire e6c1031
fix: use dedicated /replies endpoint for PR comment responses
david-waltermire File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,244 @@ | ||
| # Address PR Feedback Command | ||
|
|
||
| Address review feedback on the current PR, fix issues, and close resolved conversations. | ||
|
|
||
| ## Instructions | ||
|
|
||
| Execute the following steps to systematically address PR feedback: | ||
|
|
||
| ### Step 1: Identify the PR | ||
|
|
||
| Determine the current PR number: | ||
|
|
||
| ```bash | ||
| # Get current branch | ||
| git branch --show-current | ||
|
|
||
| # Find PR for current branch | ||
| gh pr list --head $(git branch --show-current) --json number,title,url --jq '.[0]' | ||
| ``` | ||
|
|
||
| If no PR exists, STOP and inform the user. | ||
|
|
||
| ### Step 2: Fetch All Comments | ||
|
|
||
| Retrieve all comments on the PR (both file-level and general): | ||
|
|
||
| ```bash | ||
| # Get PR number and repository info | ||
| PR_NUMBER=$(gh pr list --head $(git branch --show-current) --json number --jq '.[0].number') | ||
| REPO=$(gh repo view --json nameWithOwner --jq '.nameWithOwner') | ||
|
|
||
| # Fetch file-level review comments (line-specific) | ||
| echo "=== File Review Comments ===" | ||
| gh api repos/$REPO/pulls/$PR_NUMBER/comments \ | ||
| --jq '.[] | {id: .id, type: "review", path: .path, line: .line, body: .body[0:300], user: .user.login, in_reply_to_id: .in_reply_to_id}' | ||
|
|
||
| # Fetch general PR comments (not tied to specific lines) | ||
| echo "=== General PR Comments ===" | ||
| gh api repos/$REPO/issues/$PR_NUMBER/comments \ | ||
| --jq '.[] | {id: .id, type: "issue", body: .body[0:300], user: .user.login}' | ||
| ``` | ||
|
|
||
| **Comment Types:** | ||
|
|
||
| | Type | API Endpoint | Description | | ||
| |------|--------------|-------------| | ||
| | Review comments | `/pulls/{id}/comments` | Line-specific comments on files | | ||
| | Issue comments | `/issues/{id}/comments` | General PR-level discussion | | ||
|
|
||
| ### Step 3: Categorize Comments | ||
|
|
||
| **Reference**: See `.claude/skills/pr-feedback.md` for detailed categorization guidance. | ||
|
|
||
| Before implementing any feedback, **verify technical claims** (Step 2 in the skill): | ||
| - Automated reviewers can be wrong | ||
| - Check official docs or schema introspection before implementing | ||
| - Note incorrect feedback for the summary | ||
|
|
||
| Organize comments into categories: | ||
|
|
||
| | Category | Description | Action Required | | ||
| |----------|-------------|-----------------| | ||
| | **Blocking** | Critical issues, bugs, security | Must fix before merge | | ||
| | **Suggestions** | Recommended improvements | Should address | | ||
| | **Nits** | Minor style/formatting | Address (shows attention to detail) | | ||
| | **Questions** | Needs clarification | Reply with answer | | ||
| | **Incorrect** | Verified as wrong | Explain why in response | | ||
|
|
||
| For each root comment (where `in_reply_to_id` is null), determine: | ||
| 1. Is the feedback technically correct? (verify before implementing) | ||
| 2. Is this already addressed in the code? | ||
| 3. What action is needed? | ||
| 4. What file/line needs changes? | ||
|
|
||
| ### Step 4: Address Each Issue | ||
|
|
||
| **Reference**: See `.claude/skills/pr-feedback.md` Step 4 for detailed guidance. | ||
|
|
||
| For each unresolved comment: | ||
|
|
||
| 1. **Read the relevant code** to understand the current state | ||
| 2. **Verify the feedback** is technically correct before implementing | ||
| 3. **Determine the fix** based on the feedback and project conventions | ||
| 4. **Follow TDD for code changes**: | ||
| - Write/update tests first if the fix involves code behavior | ||
| - Verify tests fail for the right reason | ||
| - Implement the fix | ||
| - Verify tests pass | ||
| 5. **Verify the fix** compiles and all tests pass | ||
|
|
||
| Track progress with a checklist: | ||
| - [ ] Comment 1: [description] - [status] | ||
| - [ ] Comment 2: [description] - [status] | ||
| - ... | ||
|
|
||
| **Batch related fixes** into a single commit when possible. | ||
|
david-waltermire marked this conversation as resolved.
|
||
|
|
||
| ### Step 5: Reply to Close Conversations | ||
|
|
||
| For EACH addressed comment, reply to close the conversation. | ||
|
|
||
| **Use the reviewer's actual handle** from the comment's `user.login` field: | ||
|
|
||
| **For automated reviewers:** | ||
| ```text | ||
| @<reviewer_handle> Addressed in commit <hash>. <Brief explanation of the resolution> | ||
| ``` | ||
|
|
||
| Examples: | ||
| - `@coderabbitai Addressed in commit abc123. Added @Nullable annotation.` | ||
| - `@copilot Addressed in commit def456. Fixed the null check.` | ||
| - `@sonarcloud Addressed in commit ghi789. Resolved the code smell.` | ||
|
|
||
| **For human reviewers:** | ||
| ```text | ||
| Addressed in commit <hash>. <Brief explanation> | ||
| ``` | ||
| (No @ mention needed for human reviewers) | ||
|
|
||
| Use the GitHub CLI to reply: | ||
| ```bash | ||
| # Get repository info (if not already set) | ||
| REPO=$(gh repo view --json nameWithOwner --jq '.nameWithOwner') | ||
|
|
||
| # Get the reviewer's login from the comment (available from Step 2 output) | ||
| COMMENT_ID=<comment_id> | ||
| REVIEWER_LOGIN=$(gh api repos/$REPO/pulls/$PR_NUMBER/comments/$COMMENT_ID --jq '.user.login') | ||
|
|
||
| # Reply using the dedicated replies endpoint | ||
| gh api repos/$REPO/pulls/$PR_NUMBER/comments/$COMMENT_ID/replies \ | ||
| -X POST \ | ||
| -f body="@$REVIEWER_LOGIN Addressed in commit abc1234. <explanation>" | ||
| ``` | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| ### Step 6: Generate Summary Report | ||
|
|
||
| After addressing all feedback, produce a summary table: | ||
|
|
||
| ```markdown | ||
| ## PR Feedback Summary | ||
|
|
||
| | File | Comment | Status | | ||
| |------|---------|--------| | ||
| | `path/to/file.java` | Description of issue | ✅ Addressed (commit abc123) | | ||
| | `path/to/other.java` | Another issue | ✅ Addressed (commit def456) | | ||
| | `path/to/file.java` | Nitpick suggestion | ⏭️ Deferred (out of scope) | | ||
|
|
||
| **Actions Taken:** | ||
| - Fixed X by doing Y | ||
| - Updated Z to address W | ||
| - Deferred A because B | ||
| ``` | ||
|
|
||
| ### Step 7: Verify and Commit | ||
|
|
||
| After all changes: | ||
|
|
||
| ```bash | ||
| # Run tests to verify fixes don't break anything | ||
| mvn -pl <affected-modules> test | ||
|
|
||
| # Check for any style issues | ||
| mvn checkstyle:check | ||
|
|
||
| # Stage and commit changes | ||
| git add -A | ||
| git commit -m "fix: address PR review feedback | ||
|
|
||
| - <list of changes made> | ||
| " | ||
|
|
||
| # Push to update PR | ||
| git push | ||
| ``` | ||
|
|
||
| ## Status Icons | ||
|
|
||
| Use these icons in the summary table: | ||
|
|
||
| | Icon | Meaning | | ||
| |------|---------| | ||
| | ✅ | Addressed - fix committed | | ||
| | ❌ | Rejected - feedback incorrect, explained why | | ||
| | ⏭️ | Deferred - out of scope, will address later | | ||
| | 🔄 | In Progress - working on it | | ||
| | ❓ | Needs Clarification - asked reviewer | | ||
|
|
||
| ## Automated Reviewer Conventions | ||
|
|
||
| When responding to automated reviewers, **use the reviewer's actual handle** from the comment. | ||
|
|
||
| ### General Pattern | ||
| ```text | ||
| @<reviewer_handle> Addressed in commit <hash>. <Brief explanation> | ||
| ``` | ||
|
|
||
| ### Common Automated Reviewers | ||
|
|
||
| | Reviewer | Handle | Notes | | ||
| |----------|--------|-------| | ||
| | CodeRabbit | `@coderabbitai` | Auto-resolves when it detects fixes | | ||
| | GitHub Copilot | `@copilot` | May require manual resolution | | ||
| | SonarCloud | `@sonarcloud` | Links to SonarCloud dashboard | | ||
| | Codecov | `@codecov` | Coverage-related comments | | ||
|
|
||
| ### Example Responses | ||
| ```text | ||
| @coderabbitai Addressed in commit e45cabea. Aligned with the @NonNull contract from AbstractDeserializer. | ||
| @sonarcloud Addressed in commit abc1234. Extracted method to reduce complexity. | ||
| ``` | ||
|
|
||
| ## Safety Checks | ||
|
|
||
| Before making changes: | ||
|
|
||
| 1. **Understand the feedback** - Don't make changes you don't understand | ||
| 2. **Check project conventions** - Align fixes with existing patterns | ||
| 3. **Run tests** - Verify fixes don't introduce regressions | ||
| 4. **Don't over-fix** - Address only what was requested | ||
|
|
||
| ## Example Workflow | ||
|
|
||
| ```text | ||
| 1. Fetch PR #597 comments | ||
| 2. Found 5 root comments from @coderabbitai: | ||
| - PathTracker.java: Add @Nullable to pop() | ||
| - DefaultXmlDeserializer.java: Null-safety annotation | ||
| - ValidationErrorMessageTest.java: Inconsistent null handling | ||
| - DefaultXmlDeserializer.java: Document null behavior | ||
| - DefaultJsonDeserializer.java: Null-safety annotation | ||
|
|
||
| 3. Address each: | ||
| - Added @Nullable annotation to pop() | ||
| - Aligned with @NonNull contract from AbstractDeserializer | ||
| - Updated test to use valid URI | ||
| - Removed null-handling code (aligned with contract) | ||
| - Removed null-handling code (aligned with contract) | ||
|
|
||
| 4. Reply to each with @coderabbitai mention | ||
|
|
||
| 5. Generate summary table | ||
|
|
||
| 6. Commit and push | ||
| ``` | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| # Error Message Terminology | ||
|
|
||
| ## User-Friendly Language Required | ||
|
|
||
| Error messages displayed to users must use format-appropriate, user-friendly terminology. Users should NOT need to understand Metaschema concepts to understand error messages. | ||
|
|
||
| ## Terminology Mapping | ||
|
|
||
| ### Property Type Names | ||
|
|
||
| | Metaschema Concept | XML Term | JSON/YAML Term | | ||
| |--------------------|----------|----------------| | ||
| | Flag | "attribute" | "property" | | ||
| | Field | "element" | "property" | | ||
| | Assembly | "element" | "property" | | ||
|
|
||
| ### Container Names | ||
|
|
||
| | Metaschema Concept | XML Term | JSON/YAML Term | | ||
| |--------------------|----------|----------------| | ||
| | Assembly (as parent) | "element" | "object" | | ||
| | Field (as parent) | "element" | "object" | | ||
|
|
||
| ## Examples | ||
|
|
||
| ### DO (Correct) | ||
|
|
||
| ```text | ||
| Missing required property 'title' in 'metadata' | ||
| Missing required attribute 'id' in 'party' | ||
| Missing required element 'description' in 'control' | ||
| ``` | ||
|
|
||
| ### DON'T (Incorrect - Metaschema jargon) | ||
|
|
||
| ```text | ||
| Missing required flag 'id' in assembly 'party' | ||
| Missing required field 'description' in assembly 'control' | ||
| ``` | ||
|
|
||
| ## Rationale | ||
|
|
||
| 1. **Accessibility**: Most users work with specific formats (XML or JSON) and understand those concepts | ||
| 2. **Actionable**: Users can immediately locate and fix issues using familiar terminology | ||
| 3. **No prerequisite knowledge**: Error messages should not require reading Metaschema documentation | ||
| 4. **Format-appropriate**: XML users expect "attribute" and "element", JSON users expect "property" | ||
|
|
||
| ## Implementation | ||
|
|
||
| Use explicit switch statements on `Format` to handle terminology. Do NOT use `if (format != XML)` patterns as this doesn't accommodate future formats. | ||
|
|
||
| ### Correct Pattern | ||
|
|
||
| ```java | ||
| switch (format) { | ||
| case XML: | ||
| return isFlag ? "attribute" : "element"; | ||
| case JSON: | ||
| case YAML: | ||
| return "property"; | ||
| default: | ||
| // Fallback for future formats - use generic terminology | ||
| return "property"; | ||
| } | ||
| ``` | ||
|
|
||
| ### Incorrect Pattern | ||
|
|
||
| ```java | ||
| // DON'T do this - doesn't handle future formats explicitly | ||
| if (format == Format.XML) { | ||
| return isFlag ? "attribute" : "element"; | ||
| } else { | ||
| return "property"; | ||
| } | ||
| ``` | ||
|
|
||
| ## Adding New Formats | ||
|
|
||
| When adding a new format to the `Format` enum: | ||
|
|
||
| 1. Update `AbstractProblemHandler.getFormatPropertyTypeName()` with explicit case | ||
| 2. Update `AbstractProblemHandler.getFormatPropertyGroupLabel()` with explicit case | ||
| 3. Consider what terminology is appropriate for the new format's users |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.