Skip to content

fix invalid union types and add automated checks to CI - #132

Merged
aspiers merged 6 commits into
developfrom
union-objects
Jan 26, 2026
Merged

fix invalid union types and add automated checks to CI#132
aspiers merged 6 commits into
developfrom
union-objects

Conversation

@aspiers

@aspiers aspiers commented Jan 26, 2026

Copy link
Copy Markdown
Contributor

Note

Introduces schema changes to comply with ATProto union rules and adds automated style validation.

  • Converts app.certified.defs#did to an object ({ did: string }, maxLength 256)
  • In org.hypercerts.claim.activity, converts contributorIdentity, contributorRole, and workScopeString to objects (identity/role/scope) with reduced string limits
  • Adds scripts/check-lexicon-style.js with ref resolution and union-type validation; updates LEXICON_STYLE_GUIDE.md and enables npm run style:check in opencode.json
  • Regenerates SCHEMAS.md to document new object shapes

Written by Cursor Bugbot for commit 9bd413b. This will update automatically on new commits. Configure here.

Summary by CodeRabbit

  • Breaking Changes

    • Contributor fields in activity claims (identity, role, scope) and the certified DID are now objects with nested string properties; integrations must read those nested properties. New stricter max-length limits may affect existing records.
  • Documentation

    • Added schema docs for the structured contributor and DID fields.
    • Updated style guide: union variants must be object/record types (no primitives).
  • Chores

    • Improved lexicon/style validation and updated CI style-check command.

✏️ Tip: You can customize this high-level summary in your review settings.

Copilot AI review requested due to automatic review settings January 26, 2026 21:54
@changeset-bot

changeset-bot Bot commented Jan 26, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9bd413b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@hypercerts-org/lexicon Minor

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

@coderabbitai

coderabbitai Bot commented Jan 26, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@aspiers has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 11 minutes and 56 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📝 Walkthrough

Walkthrough

Converts several primitive string union members into single-property object schemas (contributorIdentity, contributorRole, workScopeString, app.certified.defs#did), adds a lexicon style rule forbidding primitive union variants, and extends the style checker to index lexicons, resolve local/external $refs, and validate union constituents.

Changes

Cohort / File(s) Summary
Docs & Changelog
\.changeset/fix-union-string-types.md, \.changeset/fix-certified-did-type.md, LEXICON_STYLE_GUIDE.md, SCHEMAS.md
Adds changesets documenting breaking changes; documents new public defs; adds rule that union variants must be objects (no primitives).
Activity lexicon
lexicons/org/hypercerts/claim/activity.json
Replaces contributorIdentity, contributorRole, workScopeString string variants with object shapes { identity:string }, { role:string }, { scope:string } and applies tightened maxLength/maxGraphemes constraints.
Certified defs
lexicons/app/certified/defs.json
Changes defs.did from a primitive string (format: did) to an object { did: string } with maxLength and required property.
Style checker
scripts/check-lexicon-style.js
Adds lexiconIndex and prebuild indexing; threads lexicon context through checks; adds resolveLocalRef(), resolveExternalRef(), checkUnionRefForPrimitiveType(); strengthens union validation and skips third-party lexicons.
Config
opencode.json
Updates allowed npm script from "npm run style" to "npm run style:check" under bash permissions.

Sequence Diagram(s)

sequenceDiagram
  participant Dev as Developer
  participant SC as StyleChecker
  participant LI as LexiconIndex
  participant LX as Lexicon File(s)

  rect rgba(200,200,255,0.5)
  Dev->>SC: run style check (npm run style:check)
  SC->>LI: buildLexiconIndex(dir)
  LI-->>SC: lexicon index (id -> parsed lexicons)
  end

  rect rgba(200,255,200,0.5)
  SC->>LX: load & parse lexicon file
  LX-->>SC: definitions (including local `#refs` and external refs)
  SC->>SC: resolveLocalRef(ref, lexicon)
  SC->>LI: resolveExternalRef(externalRef)
  LI-->>SC: external definition
  SC->>SC: checkUnionProperty(..., lexicon)
  SC->>SC: checkUnionRefForPrimitiveType(ref, ...)
  SC-->>Dev: report issues/warnings
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Suggested reviewers

  • s-adamantine

Poem

🐇
I hopped through schemas, tidy and spry,
Wrapped lone strings in boxes high.
The checker learned to trace each thread,
Refs resolved before flags were spread.
Carrots structured — tidy sky! 🥕


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error)
Check name Status Explanation Resolution
Lexicon Documentation Sync ❌ Error Documentation files are out of sync with JSON lexicon changes; SCHEMAS.md requires regeneration and README.md examples need updates. Regenerate SCHEMAS.md, update README.md examples to reflect object structures, and change changeset version bump to major for breaking changes.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the PR's main objectives: fixing invalid union types (strings converted to objects) and adding automated checks to CI.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Lexicons Styleguide Compliance ✅ Passed The npm run style:check validation script executed successfully with exit code 0, confirming all lexicon changes comply with ATProto Style Guide requirements.

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

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request fixes invalid union types in the activity lexicon and adds automated validation checks to the CI pipeline. The changes ensure compliance with the ATProto specification requirement that all union variants must be object or record types, not primitive types like strings.

Changes:

  • Converted three string type definitions (contributorIdentity, contributorRole, workScopeString) to object types with nested string properties to satisfy union type requirements
  • Added automated validation logic to check union types and reject primitive types in unions
  • Updated documentation to reflect the new object structures

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
scripts/check-lexicon-style.js Added union validation logic including resolveLocalRef and checkUnionRefForPrimitiveType methods, threaded lexicon parameter through validation methods, and added skip logic for third-party lexicons
lexicons/org/hypercerts/claim/activity.json Converted contributorIdentity, contributorRole, and workScopeString from primitive string types to object types with nested string properties
SCHEMAS.md Added documentation tables for the three new object type definitions
LEXICON_STYLE_GUIDE.md Added item 11 documenting the new union type validation check
.changeset/fix-union-string-types.md Added changeset documenting the breaking changes to union type definitions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

"role": {
"type": "string",
"description": "The contribution role or details.",
"maxLength": 1000,

Copilot AI Jan 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maxLength constraint has been significantly reduced from 10000 to 1000 (a 10x decrease). This is a breaking change that may cause existing valid data to be rejected. If this reduction is intentional due to the ATProto spec requirements for union types, it should be explicitly noted in the changeset and carefully considered for backward compatibility. Consider whether this stricter limit is necessary or if a less restrictive limit would still satisfy the requirements.

Suggested change
"maxLength": 1000,
"maxLength": 10000,

Copilot uses AI. Check for mistakes.
Comment thread lexicons/org/hypercerts/claim/activity.json
Comment thread .changeset/fix-union-string-types.md
Comment thread scripts/check-lexicon-style.js Outdated
Comment thread scripts/check-lexicon-style.js
Comment thread lexicons/org/hypercerts/claim/activity.json
Comment thread scripts/check-lexicon-style.js

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@scripts/check-lexicon-style.js`:
- Around line 504-545: In checkUnionRefForPrimitiveType, non-# string refs like
"string" or "integer" are currently treated as external refs and thus bypass
validation; add an explicit primitive-literal check (using a list of primitive
type names, e.g., "string","integer","number","boolean","bytes", etc.) before
the external-ref skip so that when ref is a string and matches one of those
primitives you push an error into fileResult.issues (using the same SEVERITY/
rule "union-invalid-type" and a message indicating union variants cannot be
primitive types) — reference the validUnionTypes constant and fileResult.issues
and place the check in the branch that handles typeof ref === "string" before
treating non-# strings as external refs.

Comment thread scripts/check-lexicon-style.js Outdated
Comment on lines +504 to +545
checkUnionRefForPrimitiveType(ref, index, path, fileResult, lexicon) {
const validUnionTypes = ["object", "record"];

// Check if this is an inline type definition
if (typeof ref === "object" && ref.type) {
if (!validUnionTypes.includes(ref.type)) {
fileResult.issues.push({
severity: SEVERITY.ERROR,
rule: "union-invalid-type",
message: `Union variants must be object or record types. Inline type "${ref.type}" is not allowed in unions by ATProto spec`,
location: `${path}.refs[${index}]`,
});
}
return;
}

// Check if this is a local ref that we can resolve
if (typeof ref === "string" && ref.startsWith("#")) {
const resolvedDef = this.resolveLocalRef(ref, lexicon);
if (resolvedDef) {
if (!resolvedDef.type) {
fileResult.issues.push({
severity: SEVERITY.WARNING,
rule: "union-invalid-type",
message: `Union variant local ref "${ref}" resolves to a definition without a type field`,
location: `${path}.refs[${index}]`,
});
} else if (!validUnionTypes.includes(resolvedDef.type)) {
fileResult.issues.push({
severity: SEVERITY.ERROR,
rule: "union-invalid-type",
message: `Union variants must be object or record types. Local ref "${ref}" resolves to type "${resolvedDef.type}" which is not allowed in unions by ATProto spec`,
location: `${path}.refs[${index}]`,
});
}
}
return;
}

// For external refs (e.g., "com.atproto.repo.strongRef" or "org.hypercerts.defs#uri"),
// we cannot validate without loading external lexicons, so we skip validation.
// These should be validated by the ATProto lexicon validator at build/runtime.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Union primitive string refs can bypass the new rule.

checkUnionRefForPrimitiveType treats non-# strings as external refs, so refs: ["string"] / ["integer"] won’t be flagged. Add an explicit primitive-literal check before the external-ref skip.

🐛 Proposed fix
   checkUnionRefForPrimitiveType(ref, index, path, fileResult, lexicon) {
     const validUnionTypes = ["object", "record"];
+    const primitiveTypeNames = new Set([
+      "string",
+      "integer",
+      "boolean",
+      "bytes",
+      "cid-link",
+    ]);
+
+    if (typeof ref === "string" && primitiveTypeNames.has(ref)) {
+      fileResult.issues.push({
+        severity: SEVERITY.ERROR,
+        rule: "union-invalid-type",
+        message: `Union variants must be object or record types. Primitive "${ref}" is not allowed in unions by ATProto spec`,
+        location: `${path}.refs[${index}]`,
+      });
+      return;
+    }
 
     // Check if this is an inline type definition
     if (typeof ref === "object" && ref.type) {
🤖 Prompt for AI Agents
In `@scripts/check-lexicon-style.js` around lines 504 - 545, In
checkUnionRefForPrimitiveType, non-# string refs like "string" or "integer" are
currently treated as external refs and thus bypass validation; add an explicit
primitive-literal check (using a list of primitive type names, e.g.,
"string","integer","number","boolean","bytes", etc.) before the external-ref
skip so that when ref is a string and matches one of those primitives you push
an error into fileResult.issues (using the same SEVERITY/ rule
"union-invalid-type" and a message indicating union variants cannot be primitive
types) — reference the validUnionTypes constant and fileResult.issues and place
the check in the branch that handles typeof ref === "string" before treating
non-# strings as external refs.

Copilot AI review requested due to automatic review settings January 26, 2026 22:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +139 to +140
"maxLength": 1000,
"maxGraphemes": 100

Copilot AI Jan 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maxLength and maxGraphemes constraints for the role property have been significantly reduced from the original values. The original contributorRole string type had maxLength: 10000 and maxGraphemes: 1000, but the new nested role property only has maxLength: 1000 and maxGraphemes: 100 (a 10x reduction). This represents an additional breaking change beyond just the structural conversion from string to object. Consider whether these tighter constraints are intentional, or if they should match the original values to minimize breaking changes.

Suggested change
"maxLength": 1000,
"maxGraphemes": 100
"maxLength": 10000,
"maxGraphemes": 1000

Copilot uses AI. Check for mistakes.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@lexicons/app/certified/defs.json`:
- Around line 6-16: Update the .changeset/initial-version-post-repo-migration.md
to document the DID schema change by adding a "Breaking Changes" section that
explicitly lists app.certified.defs#did as changed from string to an object
(required property "did"), include the complete object shape (object with
required ["did"], properties: did: string, format: "did", maxLength: 256, and
description), and add a short migration example showing old usage (plain DID
string) and the new usage (object with { "did": "did:..." }) and a note to
update any code that reads/writes the DID value accordingly.

Comment on lines 6 to +16
"did": {
"type": "string",
"format": "did",
"description": "A Decentralized Identifier (DID) string."
"type": "object",
"description": "A Decentralized Identifier (DID) string.",
"required": ["did"],
"properties": {
"did": {
"type": "string",
"format": "did",
"description": "The DID string value.",
"maxLength": 256
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check for .changeset directory and look for relevant changesets
echo "=== Checking .changeset directory ==="
if [ -d .changeset ]; then
  echo ".changeset directory found"
  echo ""
  echo "=== Contents of .changeset directory ==="
  ls -la .changeset
  echo ""
  echo "=== Searching for changeset files mentioning DID or defs ==="
  find .changeset -type f \( -name "*.md" -o -name "*.json" \) -exec grep -l "did\|defs\|DID" {} \; 2>/dev/null | head -20
  echo ""
  echo "=== Content of changeset files ==="
  find .changeset -type f \( -name "*.md" \) ! -name "README*" -exec echo "File: {}" \; -exec cat {} \; 2>/dev/null
else
  echo "No .changeset directory found"
fi

Repository: hypercerts-org/hypercerts-lexicon

Length of output: 24894


🏁 Script executed:

#!/bin/bash
# Examine the defs.json file to understand the DID definition context
echo "=== Reading lexicons/app/certified/defs.json ==="
cat lexicons/app/certified/defs.json | head -100

echo ""
echo "=== Checking initial-version-post-repo-migration.md for DID details ==="
grep -A 5 -B 5 "did" .changeset/initial-version-post-repo-migration.md

Repository: hypercerts-org/hypercerts-lexicon

Length of output: 1143


Expand the changeset to fully document the DID definition change.

The DID definition in app.certified.defs is documented in .changeset/initial-version-post-repo-migration.md, but the changeset lacks detail about the object structure and breaking change. Expand the changeset entry to include:

  • Explicit "Breaking Changes" section marking the DID shape change (string → object)
  • The complete object structure with required fields and property definitions
  • A migration example showing how existing code using the DID should be updated

Example format:

**Breaking Changes:**
- `app.certified.defs#did`: Changed from string to object type with required `did` property
🤖 Prompt for AI Agents
In `@lexicons/app/certified/defs.json` around lines 6 - 16, Update the
.changeset/initial-version-post-repo-migration.md to document the DID schema
change by adding a "Breaking Changes" section that explicitly lists
app.certified.defs#did as changed from string to an object (required property
"did"), include the complete object shape (object with required ["did"],
properties: did: string, format: "did", maxLength: 256, and description), and
add a short migration example showing old usage (plain DID string) and the new
usage (object with { "did": "did:..." }) and a note to update any code that
reads/writes the DID value accordingly.

Copilot AI review requested due to automatic review settings January 26, 2026 22:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +614 to +618
// Could not resolve the external ref - might be a typo or the lexicon might not be loaded
fileResult.issues.push({
severity: SEVERITY.ERROR,
rule: "union-unresolved-ref",
message: `Union variant external ref "${ref}" cannot be resolved (possible typo or missing lexicon)`,

Copilot AI Jan 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error handling for unresolved external refs may be too strict. When an external ref like "pub.leaflet.pages.linearDocument" (a third-party lexicon not in the repo) is used in a union, it will trigger an ERROR-level issue even though it's a legitimate external reference. This could cause false positives when checking lexicons that reference external third-party lexicons.

Consider downgrading this to a WARNING with a message indicating that the ref could not be validated because the lexicon is not available in the repository, or add logic to skip validation for known third-party lexicon prefixes (pub.leaflet., app.bsky., etc.).

Suggested change
// Could not resolve the external ref - might be a typo or the lexicon might not be loaded
fileResult.issues.push({
severity: SEVERITY.ERROR,
rule: "union-unresolved-ref",
message: `Union variant external ref "${ref}" cannot be resolved (possible typo or missing lexicon)`,
// Could not resolve the external ref - might be a typo or the lexicon might not be loaded in this repository
fileResult.issues.push({
severity: SEVERITY.WARNING,
rule: "union-unresolved-ref",
message: `Union variant external ref "${ref}" cannot be resolved or validated because the referenced lexicon is not available in this repository (possible typo or external third-party lexicon)`,

Copilot uses AI. Check for mistakes.
Comment thread scripts/check-lexicon-style.js Outdated
Comment on lines +124 to +131
// Skip style checks for third-party lexicons
if (
lexicon.id &&
(lexicon.id.startsWith("pub.leaflet.") ||
lexicon.id.startsWith("app.bsky."))
) {
return;
}

Copilot AI Jan 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The skip logic only checks for lexicons starting with "pub.leaflet." or "app.bsky." but does not include "com.atproto." in the skip list. This means that com.atproto lexicons (like com.atproto.repo.strongRef) will be checked for style compliance.

While this may be intentional (since com.atproto is a standard ATProto lexicon), consider documenting this decision in a comment to clarify why com.atproto lexicons are not skipped like other third-party lexicons.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings January 26, 2026 23:03
aspiers and others added 6 commits January 27, 2026 12:03
Without this patch, the lexicon style checker did not validate that
union types only contain object or record type references. The ATProto
spec requires that all union variants must be object or record types,
e.g. primitive types like string, integer, and boolean are not allowed
directly in unions.

This is a problem because developers could accidentally create invalid
lexicons with primitive types in unions, which would not conform to the
ATProto specification.

This patch solves the problem by:

- Changing the validation logic to check for valid union types (object, record)
- Reporting errors when a resolved type is NOT object or record
- Adding a warning when a local ref resolves to a definition without a type
- Clarifying that external refs cannot be validated without loading external
  lexicons
- Improving error messages to focus on the requirement (must be object/record)
  rather than what's forbidden

This makes the validation more aligned with the ATProto specification and
more maintainable as it explicitly checks for valid types.

Co-authored-by: Claude Code <noreply@anthropic.com>
Without this patch, the contributorIdentity, contributorRole, and
workScopeString definitions in org.hypercerts.claim.activity were defined
as primitive string types, which violates the ATProto specification
requirement that all union variants must be object or record types.

This is a problem because the lexicon style checker now correctly detects
and reports these violations as errors, and the lexicons would not be
valid according to the ATProto spec.

This patch solves the problem by:
- Converting contributorIdentity from string to object with identity property
- Converting contributorRole from string to object with role property
- Converting workScopeString from string to object with scope property
- Adding appropriate maxLength and maxGraphemes constraints to each property
- Creating a changeset documenting the breaking changes

Breaking changes:
- Code using contributorIdentity must now access .identity property
- Code using contributorRole must now access .role property
- Code using workScopeString must now access .scope property

Co-authored-by: Claude Code <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Without this patch, the union type validator only checked local refs
(starting with #) and could not validate external refs like
"org.hypercerts.defs#smallImage" or "com.atproto.repo.strongRef". This
meant that typos or invalid external refs would not be caught by the style
checker.

This is a problem because developers could reference non-existent external
definitions or reference definitions that resolve to invalid types (like
primitives), and these errors would only be caught at runtime or by the
ATProto lexicon validator.

This patch solves the problem by:
- Adding buildLexiconIndex() to load all lexicons into a Map for lookup
- Adding resolveExternalRef() to resolve external refs to their definitions
- Enhancing checkUnionRefForPrimitiveType() to validate external refs
- Reporting errors for unresolved refs (possible typos or missing lexicons)
- Reporting errors when external refs resolve to non-object/record types

The checker now validates all three types of union refs:
1. Inline definitions (e.g., {type: "string"})
2. Local refs (e.g., "#contributorIdentity")
3. External refs (e.g., "org.hypercerts.defs#uri")

This enhancement caught a real issue: app.certified.defs#did was a string
type being used in a union, which violates the ATProto spec.

Co-authored-by: Claude Code <noreply@anthropic.com>
Without this patch, the app.certified.defs#did definition was a primitive
string type, which violates the ATProto specification when used in a union.
The app.certified.badge.award lexicon uses this definition in a union type
for the subject property, which should only contain object or record types.

This is a problem because the enhanced style checker now correctly detects
and reports this as an error, and the lexicon would not be valid according
to the ATProto spec.

This patch solves the problem by:
- Converting the did definition from type: string to type: object
- Wrapping the DID string value in a required 'did' property
- Adding maxLength constraint of 256 (appropriate for DID strings)
- Preserving the format: did constraint on the inner string property

Breaking change:
- Code using app.certified.defs#did must now access the .did property
  instead of using the value directly

Co-authored-by: Claude Code <noreply@anthropic.com>
@aspiers
aspiers enabled auto-merge January 26, 2026 23:04
@aspiers
aspiers merged commit fa918b9 into develop Jan 26, 2026
5 checks passed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +7 to +17
"type": "object",
"description": "A Decentralized Identifier (DID) string.",
"required": ["did"],
"properties": {
"did": {
"type": "string",
"format": "did",
"description": "The DID string value.",
"maxLength": 256
}
}

Copilot AI Jan 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The from field uses app.certified.defs#did as a required field, but the description states "Leave empty if sender wants to stay anonymous." With the conversion of app.certified.defs#did from a string to an object type with a required did property, this creates an inconsistency: the field cannot be truly "empty" anymore since it must be an object with a did property if present.

This is not directly caused by this PR, but the conversion highlights the issue. The field should probably be marked as optional (removed from the required array) if anonymity is intended to be supported. Consider whether this breaking change affects the intended semantics of the funding receipt schema.

Copilot uses AI. Check for mistakes.
Comment on lines +552 to +561
// Check if this is an inline type definition
if (typeof ref === "object" && ref.type) {
if (!validUnionTypes.includes(ref.type)) {
fileResult.issues.push({
severity: SEVERITY.ERROR,
rule: "union-invalid-type",
message: `Union variants must be object or record types. Inline type "${ref.type}" is not allowed in unions by ATProto spec`,
location: `${path}.refs[${index}]`,
});
}

Copilot AI Jan 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition checks if ref is an object with a type property to detect inline type definitions in unions. However, this check could incorrectly flag valid ref objects that happen to have a type property for other reasons.

In ATProto lexicons, refs in unions are typically string references to definitions (e.g., "#contributorIdentity" or "com.atproto.repo.strongRef"), not inline object definitions. The check for typeof ref === "object" might be overly broad.

Consider verifying that inline object definitions are actually valid in the union refs array according to the ATProto spec. If they're not valid at all, this check might be catching something that shouldn't exist in the first place, making the error message misleading.

Suggested change
// Check if this is an inline type definition
if (typeof ref === "object" && ref.type) {
if (!validUnionTypes.includes(ref.type)) {
fileResult.issues.push({
severity: SEVERITY.ERROR,
rule: "union-invalid-type",
message: `Union variants must be object or record types. Inline type "${ref.type}" is not allowed in unions by ATProto spec`,
location: `${path}.refs[${index}]`,
});
}
// Union refs should be string references to definitions per ATProto spec.
// Any non-string (e.g. inline object) ref is invalid as a union variant.
if (typeof ref === "object" && ref !== null) {
fileResult.issues.push({
severity: SEVERITY.ERROR,
rule: "union-invalid-ref",
message: `Union variants must be string references to object or record definitions; inline or non-string refs are not allowed in unions by ATProto spec`,
location: `${path}.refs[${index}]`,
});

Copilot uses AI. Check for mistakes.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In @.changeset/fix-certified-did-type.md:
- Around line 1-2: The changeset currently marks "@hypercerts-org/lexicon" as a
minor bump but changing app.certified.defs#did from a string to an object is a
breaking change; update the changeset so the version bump is "major" (replace
minor with major in the changeset header for "@hypercerts-org/lexicon") and
save/commit the updated .changeset entry so consumers get a major release.

In `@lexicons/app/certified/defs.json`:
- Around line 7-17: The generated docs are out of sync: the JSON schema for
app.certified.defs#did now defines an object with a required nested "did" string
property (properties.did.type = "string") but SCHEMAS.md still shows a direct
string; re-run the SCHEMAS.md generation step to regenerate documentation so it
reflects the new structure for app.certified.defs#did (an object with required
"did" string property, maxLength 256 and format "did"), or update the doc
generation script to pick up lexicons/app/certified/defs.json changes and commit
the regenerated SCHEMAS.md.

Comment on lines +1 to +2
---
"@hypercerts-org/lexicon": minor

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use a major bump for this breaking change.
Changing app.certified.defs#did from a string to an object is a breaking change for consumers, so the changeset should use a major bump.

🔧 Proposed fix
--- 
-"@hypercerts-org/lexicon": minor
+"@hypercerts-org/lexicon": major
---
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
---
"@hypercerts-org/lexicon": minor
---
"@hypercerts-org/lexicon": major
🤖 Prompt for AI Agents
In @.changeset/fix-certified-did-type.md around lines 1 - 2, The changeset
currently marks "@hypercerts-org/lexicon" as a minor bump but changing
app.certified.defs#did from a string to an object is a breaking change; update
the changeset so the version bump is "major" (replace minor with major in the
changeset header for "@hypercerts-org/lexicon") and save/commit the updated
.changeset entry so consumers get a major release.

Comment thread lexicons/app/certified/defs.json

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

const validUnionTypes = ["object", "record"];

// Check if this is an inline type definition
if (typeof ref === "object" && ref.type) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null ref in union causes style checker crash

Low Severity

The condition typeof ref === "object" && ref.type doesn't account for null, since JavaScript's typeof null returns "object". If a union's refs array contains a null value (valid JSON), evaluating null.type throws a TypeError, crashing the style checker instead of gracefully reporting a validation error.

Fix in Cursor Fix in Web

@aspiers
aspiers deleted the union-objects branch January 28, 2026 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants