examples: strict arcade example #408
Conversation
@modelcontextprotocol/ext-apps
@modelcontextprotocol/server-arcade
@modelcontextprotocol/server-basic-react
@modelcontextprotocol/server-basic-vanillajs
@modelcontextprotocol/server-budget-allocator
@modelcontextprotocol/server-cohort-heatmap
@modelcontextprotocol/server-customer-segmentation
@modelcontextprotocol/server-map
@modelcontextprotocol/server-pdf
@modelcontextprotocol/server-scenario-modeler
@modelcontextprotocol/server-shadertoy
@modelcontextprotocol/server-sheet-music
@modelcontextprotocol/server-system-monitor
@modelcontextprotocol/server-threejs
@modelcontextprotocol/server-transcript
@modelcontextprotocol/server-video-resource
@modelcontextprotocol/server-wiki-explorer
commit: |
There was a problem hiding this comment.
Pull request overview
Tightens the arcade example’s copyright/distribution guardrails by introducing explicit rights modeling, a curated allowlist, and runtime rights checks before allowing games to be discovered/loaded.
Changes:
- Added rights-related domain types, a curated allowlist, and a metadata-based rights checker with caching/concurrency limits.
- Updated search flow to prioritize allowlist matches and filter archive.org results through rights verification.
- Updated server tooling + documentation to communicate and enforce “verified-rights only” behavior (plus stricter no-store caching headers).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/arcade-server/types.ts | Introduces shared rights/allowlist result types used across search/checker/server. |
| examples/arcade-server/server.ts | Updates tool copy, returns rights fields in search results, and blocks loading unless rights are allowed. |
| examples/arcade-server/search.ts | Adds allowlist-first search and rights-filtering for archive.org results with timeouts. |
| examples/arcade-server/rights-checker.ts | Implements rights checking via allowlist + archive.org metadata with caching and concurrency control. |
| examples/arcade-server/index.ts | Tightens HTTP response caching headers for embedded HTML and proxied scripts. |
| examples/arcade-server/allowlist.ts | Adds the curated allowlist dataset plus lookup helpers. |
| examples/arcade-server/README.md | Adds legal disclaimer and documents the allowlist/rights-checking behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## Available Games (18 Verified Titles) | ||
|
|
||
| All games have verified distribution rights. See [`allowlist.ts`](allowlist.ts) for legal documentation. |
There was a problem hiding this comment.
The README claims there are “18 Verified Titles”, but searchArchiveOrgGames can also return non-allowlist items when archive.org metadata matches permissive patterns, so the available set is not necessarily limited to this table. Clarify that this section lists the curated allowlist (and that additional metadata-verified results may appear), or change the implementation to only ever return allowlisted games.
| ## Available Games (18 Verified Titles) | |
| All games have verified distribution rights. See [`allowlist.ts`](allowlist.ts) for legal documentation. | |
| ## Curated Allowlisted Games (18 Verified Titles) | |
| The games listed below are the curated allowlist entries with verified distribution rights. See [`allowlist.ts`](allowlist.ts) for legal documentation. At runtime, additional titles may also be available when archive.org metadata clearly indicates permissive rights (e.g., shareware, freeware, public domain, Creative Commons), so the overall playable set is not limited strictly to this table. |
| /released\s*as\s*free/i, | ||
| /no\s*known\s*copyright/i, | ||
| /copyright\s*not\s*renewed/i, | ||
| /abandonware/i, |
There was a problem hiding this comment.
Including "abandonware" as a permissive rights indicator will mark items as allowed when metadata contains that term, but "abandonware" is not a license or distribution permission. Remove this pattern (or treat it as unknown/restricted) to keep the checker aligned with the “verified rights only” goal.
| /abandonware/i, |
| const metadataCache = new Map<string, CacheEntry>(); | ||
|
|
||
| // Concurrency control | ||
| const MAX_CONCURRENT_REQUESTS = 5; | ||
| let activeRequests = 0; |
There was a problem hiding this comment.
metadataCache never evicts entries; the TTL is only checked on read, so the Map can grow without bound over time as new identifiers are requested. Add pruning of expired entries (on read/write or via a periodic sweep) and/or enforce a max size (LRU) to avoid unbounded memory growth.
| identifier: "wolfenstein-3d", | ||
| displayTitle: "Wolfenstein 3D", | ||
| rightsNote: "Shareware - id Software (1992)", | ||
| sourceUrl: "https://wolfenstein.fandom.com/wiki/Wolfenstein_3D#Shareware", | ||
| legalBasis: |
There was a problem hiding this comment.
This entry’s sourceUrl points to a fandom wiki, but the file header says sourceUrl should be an authoritative external source (publisher announcement/official site/press release). Either replace this with a primary source, or relax/update the allowlist requirements to reflect what sources are acceptable.
| identifier: "Epic-pinball-sw1", | ||
| displayTitle: "Epic Pinball (Shareware)", | ||
| rightsNote: "Shareware - Epic MegaGames (1993)", | ||
| sourceUrl: "https://en.wikipedia.org/wiki/Epic_Pinball", | ||
| legalBasis: |
There was a problem hiding this comment.
This entry uses Wikipedia as the sourceUrl, but the allowlist header requires authoritative, externally verifiable sources. Consider replacing with a primary source (publisher/rights-holder statement) or adjusting the allowlist criteria to match what’s actually being used.
| // Common shareware/freeware indicators in identifiers | ||
| if ( | ||
| idLower.includes("shareware") || | ||
| idLower.includes("-sw") || | ||
| idLower.endsWith("sw") | ||
| ) { | ||
| return { | ||
| status: "allowed", | ||
| note: "Identifier indicates shareware", | ||
| }; | ||
| } | ||
|
|
||
| if (idLower.includes("freeware") || idLower.includes("free-")) { | ||
| return { | ||
| status: "allowed", | ||
| note: "Identifier indicates freeware", | ||
| }; | ||
| } | ||
|
|
||
| if (idLower.includes("public-domain") || idLower.includes("publicdomain")) { | ||
| return { | ||
| status: "allowed", | ||
| note: "Identifier indicates public domain", | ||
| }; | ||
| } | ||
|
|
||
| if (idLower.includes("demo")) { | ||
| return { | ||
| status: "allowed", | ||
| note: "Identifier indicates demo version", |
There was a problem hiding this comment.
checkIdentifierForRights() returns status:"allowed" based only on identifier substrings ("-sw", "free-", "demo", etc.). Identifiers are not reliable evidence of distribution permission, so this can incorrectly allow copyrighted content and undermines the “verified rights” guarantee. Consider removing these allow-returns (or downgrading them to status:"unknown" and only using them as hints alongside allowlist/metadata evidence).
| // Common shareware/freeware indicators in identifiers | |
| if ( | |
| idLower.includes("shareware") || | |
| idLower.includes("-sw") || | |
| idLower.endsWith("sw") | |
| ) { | |
| return { | |
| status: "allowed", | |
| note: "Identifier indicates shareware", | |
| }; | |
| } | |
| if (idLower.includes("freeware") || idLower.includes("free-")) { | |
| return { | |
| status: "allowed", | |
| note: "Identifier indicates freeware", | |
| }; | |
| } | |
| if (idLower.includes("public-domain") || idLower.includes("publicdomain")) { | |
| return { | |
| status: "allowed", | |
| note: "Identifier indicates public domain", | |
| }; | |
| } | |
| if (idLower.includes("demo")) { | |
| return { | |
| status: "allowed", | |
| note: "Identifier indicates demo version", | |
| // Common shareware/freeware indicators in identifiers (heuristic only) | |
| if ( | |
| idLower.includes("shareware") || | |
| idLower.includes("-sw") || | |
| idLower.endsWith("sw") | |
| ) { | |
| return { | |
| status: "unknown", | |
| note: "Identifier suggests shareware (heuristic only)", | |
| }; | |
| } | |
| if (idLower.includes("freeware") || idLower.includes("free-")) { | |
| return { | |
| status: "unknown", | |
| note: "Identifier suggests freeware (heuristic only)", | |
| }; | |
| } | |
| if (idLower.includes("public-domain") || idLower.includes("publicdomain")) { | |
| return { | |
| status: "unknown", | |
| note: "Identifier suggests public domain (heuristic only)", | |
| }; | |
| } | |
| if (idLower.includes("demo")) { | |
| return { | |
| status: "unknown", | |
| note: "Identifier suggests demo version (heuristic only)", |
| // If we have some allowlist results but not enough, try to supplement with archive.org | ||
| // But if allowlist already found matches, return those to avoid slow network calls | ||
| if (allowlistResults.length > 0) { | ||
| return allowlistResults; | ||
| } |
There was a problem hiding this comment.
These comments say the code will "supplement" allowlist results with archive.org results, but the implementation returns early whenever allowlistResults.length > 0, so supplementation never happens. Either update the comments to match the behavior, or actually supplement up to maxResults.
| } | ||
|
|
||
| // If no allowlist matches, check metadata for verified rights | ||
| const limitedGames = games.slice(0, 10); |
There was a problem hiding this comment.
This hard-caps metadata rights checks to the first 10 archive results, so searchArchiveOrgGames can return at most 10 games even when maxResults is higher (the tool allows up to 50). Either respect maxResults here (possibly with a documented max cap), or document that non-allowlist searches are limited to 10 results.
| const limitedGames = games.slice(0, 10); | |
| const rightsCheckLimit = Math.min(games.length, maxResults, 50); | |
| const limitedGames = games.slice(0, rightsCheckLimit); |
| const entry = getAllowlistEntry(game.identifier)!; | ||
| return { | ||
| identifier: game.identifier, | ||
| title: game.title, |
There was a problem hiding this comment.
For allowlisted matches found via archive.org search, the returned title uses the archive doc title (game.title) rather than the allowlist entry’s displayTitle. This can make titles inconsistent between allowlist search vs archive search. Prefer entry.displayTitle when isInAllowlist(game.identifier) is true.
| title: game.title, | |
| title: entry.displayTitle ?? game.title, |
| for (const outcome of fetchResults) { | ||
| if (outcome.status === "fulfilled") { | ||
| const { id, result } = outcome.value; | ||
| results.set(id, result); | ||
| metadataCache.set(id, { result, timestamp: Date.now() }); | ||
| } else { | ||
| // On failure, mark as unknown | ||
| const id = needsFetch[fetchResults.indexOf(outcome)]; |
There was a problem hiding this comment.
In checkRightsBatch(), the rejected-case maps the id via needsFetch[fetchResults.indexOf(outcome)], which is O(n^2) and can mis-associate ids if there are duplicates. Iterate with an index (for (let i=0; i<fetchResults.length; i++)) or carry the id alongside the promise so failures can be attributed deterministically.
| for (const outcome of fetchResults) { | |
| if (outcome.status === "fulfilled") { | |
| const { id, result } = outcome.value; | |
| results.set(id, result); | |
| metadataCache.set(id, { result, timestamp: Date.now() }); | |
| } else { | |
| // On failure, mark as unknown | |
| const id = needsFetch[fetchResults.indexOf(outcome)]; | |
| for (let i = 0; i < fetchResults.length; i++) { | |
| const outcome = fetchResults[i]; | |
| const id = needsFetch[i]; | |
| if (outcome.status === "fulfilled") { | |
| const { result } = outcome.value; | |
| results.set(id, result); | |
| metadataCache.set(id, { result, timestamp: Date.now() }); | |
| } else { | |
| // On failure, mark as unknown |
|
Closing. Removed to avoid potential issues |
1 participant
Tightens copyright guardrails around the arcade example.