Conversation
🦋 Changeset detectedLatest commit: f01f294 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds string/array validation metadata and new common defs across lexicons, converts a badge response enum to knownValues, updates style-check and schema generation scripts, and adds documentation and a changeset to reflect ATProto Lexicon style-guide compliance. (48 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
s-adamantine
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@SCHEMAS.md`:
- Around line 177-191: Create a new ATProto lexicon v1 JSON file named
lexicons/org/hypercerts/claim/acknowledgement.json that defines the record
"org.hypercerts.acknowledgement" with the properties listed in SCHEMAS.md:
subjects (ref, required), context (ref, required), given (boolean, required),
comment (string, optional, maxLength 1000), and createdAt (string, required);
ensure the lexicon includes proper ids, defs, record key, and required array per
ATProto lexicon schema v1 and mirrors the table types and descriptions. Then
update README.md to list and briefly describe the new acknowledgement lexicon
alongside the existing claim-related records so it appears in the lexicon
index/documentation.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SCHEMAS.md (1)
186-194: Update SCHEMAS.md to document the URI maxLength constraint.The defs.json file correctly contains
maxLength: 2048for the URI definition, but SCHEMAS.md does not reflect this constraint in the Comments column. The table shows only"Has 'uri' property (string, format uri)"without documenting the maxLength. Regenerate or update SCHEMAS.md to include this constraint and keep it in sync with the source lexicon.
|
@copilot This needs rebasing |
There was a problem hiding this comment.
Pull request overview
This PR aligns the repository’s ATProto lexicons and schema documentation with the ATProto Lexicon Style Guide (per #66), primarily by tightening string constraints and improving schema evolution compatibility.
Changes:
- Replaced
enumwithknownValuesfor badge response values to support schema evolution. - Added
maxLengthconstraints to multiple unconstrained string fields across badge, rights, and funding receipt lexicons. - Updated schema docs (
SCHEMAS.md) and added a changeset entry describing the compliance changes.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lexicons/org/hypercerts/funding/receipt.json | Adds maxLength constraints to several string properties for tighter validation. |
| lexicons/org/hypercerts/defs.json | Updates URI definition to use maxLength (instead of maxGraphemes). |
| lexicons/org/hypercerts/claim/rights.json | Adds maxLength and maxGraphemes constraints to rightsDescription. |
| lexicons/app/certified/badge/response.json | Switches from enum to knownValues and constrains weight length. |
| lexicons/app/certified/badge/definition.json | Adds maxLength constraints to badge text fields. |
| lexicons/app/certified/badge/award.json | Adds a maxLength constraint to the optional note. |
| SCHEMAS.md | Updates generated schema documentation tables to reflect new constraints/known values. |
| .changeset/style-guide-compliance-fixes.md | Adds release notes for the style guide compliance updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Synthesize correct parts of PRs #103 and #112 into a single commit: - Fix style checker to skip format-typed, enum, and knownValues fields - Fix rightsDescription byte:grapheme ratio to 10:1 (was 5:1) - Add maxLength/maxGraphemes to all unconstrained string and array fields - Add app.certified.defs to schema generator categorization - Change badge response enum to knownValues - Add top-level description to org.hypercerts.defs - Upgrade changeset severity from patch to minor - Add STRING_CONSTRAINTS.md documenting rationale for every constraint Co-Authored-By: Sharfy Adamantine <s-adamantine@users.noreply.github.com> Co-Authored-By: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
aspiers
changed the title
Synthesize correct parts of PRs #103 and #112 into a single commit: - Fix style checker to skip format-typed, enum, and knownValues fields - Fix rightsDescription byte:grapheme ratio to 10:1 (was 5:1) - Add maxLength/maxGraphemes to all unconstrained string and array fields - Add app.certified.defs to schema generator categorization - Change badge response enum to knownValues - Add top-level description to org.hypercerts.defs - Upgrade changeset severity from patch to minor - Add STRING_CONSTRAINTS.md documenting rationale for every constraint Co-Authored-By: Sharfy Adamantine <s-adamantine@users.noreply.github.com> Co-Authored-By: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
scripts/check-lexicon-style.js:389
checkStringPropertynow skips the maxLength/maxGraphemes warning whenformat,enum, orknownValuesare present, but the repo’sLEXICON_STYLE_GUIDE.mdcurrently documents that the checker enforces "String properties have maxLength constraints". Please update the style guide documentation (or adjust the checker) so the documented automated checks match actual behavior.
checkStringProperty(prop, path, fileResult) {
// Check for maxLength
if (
!prop.maxLength &&
!prop.maxGraphemes &&
!prop.format &&
!prop.enum &&
!prop.knownValues
) {
fileResult.issues.push({
severity: SEVERITY.WARNING,
rule: "string-max-length",
message: "String property should have maxLength or maxGraphemes",
location: `${path}.maxLength`,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Change enum to knownValues in badge/response.json for schema evolution - Add maxLength constraints to string fields without format: - badge/definition.json: badgeType (100), title (256), description (1000) - badge/award.json: note (500) - badge/response.json: weight (50) - rights.json: rightsDescription (5000/1000 graphemes) - funding/receipt.json: to, amount, currency, paymentRail, paymentNetwork, transactionId - Remove maxGraphemes from format-typed URI field in defs.json
Synthesize correct parts of PRs #103 and #112 into a single commit: - Fix style checker to skip format-typed, enum, and knownValues fields - Fix rightsDescription byte:grapheme ratio to 10:1 (was 5:1) - Add maxLength/maxGraphemes to all unconstrained string and array fields - Add app.certified.defs to schema generator categorization - Change badge response enum to knownValues - Add top-level description to org.hypercerts.defs - Upgrade changeset severity from patch to minor - Add STRING_CONSTRAINTS.md documenting rationale for every constraint - Document format/enum/knownValues exception in LEXICON_STYLE_GUIDE.md Co-Authored-By: Sharfy Adamantine <s-adamantine@users.noreply.github.com> Co-Authored-By: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace mutating gen-schemas-md call with a validate-only check that regenerates, diffs, and restores if stale. The rest of the pipeline (npm run check) already runs prettier --check, so no writes are needed. Added comments explaining the rationale so this doesn't get reverted. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add missing descriptions, replace vague text with concrete guidance and examples, clarify numeric string fields, fix punctuation spacing, and standardize createdAt wording. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Without this patch, the style checker output showed aggregate severity counts but didn't break down which specific rules were triggered and how many times, making it harder to identify the most common issues. This is a problem because developers need to prioritize which style issues to address first, and a per-rule breakdown makes patterns more visible. This patch solves the problem by: - Adding formatRuleBreakdown() method showing count per rule type (e.g., 'strongref-documentation: 6'), sorted by severity then count - Refactoring monolithic formatResults() into 5 focused helpers: formatHeader(), formatSummary(), formatFileDetails(), formatRuleBreakdown(), formatFooter() - Extracting SEVERITY_ICONS and SEPARATOR as shared module constants, eliminating duplicated icon lookup table Co-authored-by: Claude Code <claude-code@noreply.anthropic.com>
Summary
Best-of-breed synthesis of #112 and the original work in this PR, fixing issues in both:
check-lexicon-style.js): skipformat,enum, andknownValuesfields — stops false warnings that caused Fix lexicon style guide violations #112's overcorrectionrightsDescriptionratio: corrected to 10:1 (maxLength: 10000, maxGraphemes: 1000) per style guide — was 5:1 in the original PRmaxLength/maxGraphemesto all unconstrained string and array fields across badge, activity, collection, rights, receipt, and contributor lexiconsenum→knownValuesfor schema evolution complianceapp.certified.defscategorization in schema generatorpatchtominor(new constraints are additive but affect validation)STRING_CONSTRAINTS.mddocumenting rationale for every constraint valueCloses #111
Closes #112
What was wrong with #112?
maxLengthto 24 format-typed fields (datetime,at-uri, etc.) which the style guide explicitly forbidscom.atproto.repo.strongRef)What was wrong with the original PR?
rightsDescriptionhad 5:1 byte:grapheme ratio (maxLength: 5000, maxGraphemes: 1000) instead of style guide's 10:1patchseverity instead ofminorTest plan
node scripts/check-lexicon-style.js— 0 errors, 0string-max-lengthwarningsnpm run gen-schemas-md— SCHEMAS.md regenerated and up to datenpm test— 7/7 tests passnpm run check— full pipeline (gen-api, lint, typecheck, build, test) passesmaxLength(verified via diff)🤖 Generated with Claude Code