-
Notifications
You must be signed in to change notification settings - Fork 9
[LC-1304] Feat: LinkedClaims Plugin - Endorsements #789
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: c8b4213 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
✅ Deploy Preview for learncarddocs canceled.
|
…ng sanitization Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@claude please review this PR and recommend merging / not |
Claude finished @Custard7's task —— View job PR Review CompleteI've completed my review of the LinkedClaims plugin PR. Here's my comprehensive analysis: ✅ Strengths
|
🥷 Code experts: TaylorBeeston Custard7 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ PR Review
The PR introduces a new LinkedClaims plugin implementing Open Badges v3 EndorsementCredential functionality. The core implementation is solid, but there are several potential runtime issues related to method existence validation and weak UUID generation fallbacks.
3 issues detected:
🐞 Bug - Calling methods on objects without verifying they exist can cause runtime failures.
Details: The code assumes store providers have an
uploadEncrypted
method without checking if it exists. If a store provider doesn't implement this method, the code will throw a runtime error, making the plugin unusable with certain providers.
File:packages/plugins/linked-claims/src/index.ts (106-106)
🐞 Bug - Weak random ID generation can lead to ID collisions and potential data corruption.
Details: The UUID generation uses a weak fallback (
Math.random().toString(36).slice(2)
) whencrypto.randomUUID()
is unavailable. This fallback produces much shorter, less random strings that significantly increase collision risk and could lead to data integrity issues.
File:packages/plugins/linked-claims/src/index.ts (111-111)
🐞 Bug - Assumptions about method availability can lead to runtime errors with different provider implementations.
Details: Multiple method calls on index and read providers assume methods exist without validation (e.g.,
add
,get
methods). While less critical than the store issue, these could still cause runtime errors with incompatible providers.
File:packages/plugins/linked-claims/src/index.ts
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using. We'd love your feedback! 🚀
storeEndorsement: async (_learnCard, endorsement, options) => { | ||
const storeName = pickStoreProvider(learnCard, options?.storeName); | ||
if (!storeName) throw new Error('No store plane provider available'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐞 Bug - Missing Method Validation: Add runtime validation to check if the uploadEncrypted
method exists on the store provider before calling it. Throw a more descriptive error if the method is not available.
const uri: string = await (learnCard.store as any)[storeName].uploadEncrypted(endorsement); | |
if (!learnCard.store[storeName].uploadEncrypted || typeof learnCard.store[storeName].uploadEncrypted !== 'function') { | |
throw new Error(`Store provider '${storeName}' does not support uploadEncrypted method`); | |
} |
|
||
const indexName = pickIndexProvider(learnCard, options?.indexName); | ||
let indexed = false; | ||
let id = (endorsement as any).id || `urn:uuid:${(globalThis as any).crypto?.randomUUID?.() || Math.random().toString(36).slice(2)}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐞 Bug - Weak UUID Fallback: Use a proper UUID library or implement a more robust fallback that generates cryptographically secure random IDs with sufficient entropy. Consider throwing an error if secure random generation is unavailable.
let id = (endorsement as any).id || `urn:uuid:${(globalThis as any).crypto?.randomUUID?.() || Math.random().toString(36).slice(2)}`; | |
let id = (endorsement as any).id || `urn:uuid:${(globalThis as any).crypto?.randomUUID?.() || (() => { throw new Error('Secure random UUID generation is not available. Please use an environment that supports crypto.randomUUID().'); })()}`; |
@Custard7 I think this would be better done as a new template, and then the endorsement plugin can be just a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ PR Review
The LinkedClaims plugin implementation looks solid overall with good test coverage. However, there are previous issues that remain unresolved and one new potential bug around query construction.
1 issues detected:
🐞 Bug - Query parameter can be undefined leading to unpredictable index behavior
Details: When retrieving endorsements, if the original credential has no ID and no credentialSubject.id, the query will be constructed with
endorsedId: undefined
. This could cause the index query to fail or behave unexpectedly, preventing endorsement retrieval.
File:packages/plugins/linked-claims/src/index.ts (138-138)
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using. We'd love your feedback! 🚀
|
||
const subjectIds = getSubjectIds(original); | ||
const originalId = (original as any).id; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐞 Bug - Undefined Query Parameter: Add validation to ensure at least one identifier exists before constructing the query. Return an empty array or throw a descriptive error if neither originalId nor subjectIds[0] are available.
if (!originalId && !subjectIds[0]) { | |
return []; | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ PR Review
The LinkedClaims plugin implementation looks well-structured with comprehensive test coverage. However, the previous provider method validation issues remain unaddressed, and there's a missing dependency in the build configuration.
1 issues detected:
🐞 Bug - Build script imports a module that isn't declared as a dependency.
Details: The build script imports 'rimraf' but it's not listed as a dependency in package.json. This will cause the build process to fail when the script attempts to run.
File:packages/plugins/linked-claims/scripts/build.mjs (4-4)
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using. We'd love your feedback! 🚀
import path from 'path'; | ||
|
||
import esbuild from 'esbuild'; | ||
import rimraf from 'rimraf'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐞 Bug - Missing Dependency: Add 'rimraf' as a devDependency in the package.json file, or replace the rimraf usage with Node.js built-in fs.rmSync() for a dependency-free solution.
import rimraf from 'rimraf'; | |
import fs from 'fs'; |
Overview
🎟 Relevant Jira Issues
LC-1304
📚 What is the context and goal of this PR?
Migrate the LinkedClaims plugin to use Open Badges v3 EndorsementCredential (OBv3) with VC v2, eliminating our custom LinkedClaims context and partner-specific fields. Provide simple APIs to:
🥴 TL; RL:
Replaced our custom endorsement schema/context with the OBv3 native EndorsementCredential:
credentialSubject.id
points to the original VC or its subject.endorsementComment
,name
,description
.💡 Feature Breakdown (screenshots & videos encouraged!)
type
includesEndorsementCredential
,credentialSubject.type
includesEndorsementSubject
,credentialSubject.id
references the endorsed target.EndorsementCredential
type,EndorsementSubject
subject with id.uploadEncrypted
). Index record includesendorsedId
,subjectId
, andoriginalCredentialId
set to the endorsed subject id;category: "Endorsement"
.originalCredentialId
(if original has id) or byendorsedId
(first subject id).{ endorsementComment?: string; name?: string; description?: string }
endorsementComment
rather than legacy fields.🛠 Important tradeoffs made:
recommendationText
,portfolio
,howKnow
,qualifications
,rating
,tags
,reference
,metadata
) to prevent schema drift and ensure standards compliance.evidence
field in EndorsementDetails to keep the input surface minimal; can be revisited in a follow-up if needed by use cases.uploadEncrypted
.🔍 Types of Changes
💳 Does This Create Any New Technical Debt? ( If yes, please describe and add JIRA TODOs )
gradient-string
, REPL options) we may address separately.Testing
🔬 How Can Someone QA This?
Build:
pnpm nx start cli
In the CLI REPL: