feat: add opensea health diagnostic command (DIS-144)#35
feat: add opensea health diagnostic command (DIS-144)#35
Conversation
Co-Authored-By: Chris K <ckorhonen@gmail.com>
Original prompt from Chris K |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
ckorhonen
left a comment
There was a problem hiding this comment.
Code Review
Overall: Request changes — All 160 tests pass, but the health check doesn't actually validate API keys.
Critical Issue: Health check passes with invalid API keys
Confirmed via E2E testing:
$ npx . health --api-key invalidkey123
{"status":"ok","key_prefix":"inva...","message":"API key is valid and connectivity is working"}
GET /api/v2/collections?limit=1 returns 200 regardless of key validity. The health check doesn't validate authentication. The success message should be updated to reflect that this verifies connectivity, not key validity.
Other Issues
-
SDK test mocking breaks
instanceof— Intest/sdk.test.ts,OpenSeaAPIErroris mocked asvi.fn(), soinstanceof OpenSeaAPIErrornever matches in the SDK'sHealthAPI.check(). The auth/API error branches are untested at the SDK level. Fix: usevi.importActualto pass the real class through. -
type HealthResultshould beinterface HealthResult— Every other object shape insrc/types/usesinterface. This should match for consistency. -
MockClient type hack — The health test patches
getApiKeyPrefixonto the mock viaRecord<string, unknown>cast. Should be added to theMockClienttype intest/mocks.tsinstead. -
Duplicated error-handling logic between
src/commands/health.tsandsrc/sdk.tsHealthAPI.check(). Consider having the CLI command be a thin wrapper per the project's design rules.
I've pushed a commit addressing these issues.
- Fix misleading success message: says 'Connectivity is working' instead of claiming API key validation (endpoint returns 200 for any key) - Extract shared checkHealth() into src/health.ts to DRY up duplicated logic between CLI command and SDK HealthAPI - Change 'type HealthResult' to 'interface HealthResult' for consistency with other types in src/types/ - Add getApiKeyPrefix to MockClient type instead of using Record cast - Fix SDK test: use vi.importActual for OpenSeaAPIError so instanceof works correctly, enabling auth/API error branch coverage - Add SDK tests for auth error (401) and API error (500) branches Co-Authored-By: Chris K <ckorhonen@gmail.com>
|
Addressed all feedback in 5a7d6c2:
All 162 tests pass, lint and type-check clean. |
The health check now performs two steps: 1. Connectivity check via /api/v2/collections (public endpoint) 2. Auth validation via /api/v2/listings (requires valid API key) Previously the health check only hit a public endpoint that returned 200 regardless of API key validity, giving false "ok" results for invalid keys. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| message: `Authentication failed (${error.statusCode}): invalid API key`, | ||
| } | ||
| } | ||
| // Non-auth error on events endpoint — connectivity works but auth is unverified |
There was a problem hiding this comment.
🟡 Stale comment references "events endpoint" but code calls listings endpoint
The comment on line 50 says "Non-auth error on events endpoint" but the actual API call on line 29 is to /api/v2/listings/collection/boredapeyachtclub/all — a listings endpoint, not an events endpoint. This appears to be left over from a previous iteration where the health check used the events endpoint.
Root Cause
The comment was written when the Step 2 auth check used an events endpoint, but the implementation was later changed to use the listings endpoint (/api/v2/listings/collection/boredapeyachtclub/all at src/health.ts:29). The comment was not updated to match.
Impact: Minor — misleading for developers maintaining this code. No runtime effect.
| // Non-auth error on events endpoint — connectivity works but auth is unverified | |
| // Non-auth error on listings endpoint — connectivity works but auth is unverified |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — this is in @ckorhonen's commit (b3c0300). I can fix it if desired.
feat: add
opensea healthdiagnostic command (DIS-144)Summary
Adds a new
opensea healthcommand that checks API connectivity before running expensive operations. Makes a lightweightGET /api/v2/collections?limit=1call and outputs structured JSON:{ "status": "ok", "key_prefix": "8d02...", "message": "Connectivity is working" }On failure (auth error, API error, or network error), outputs the same shape with
"status": "error"and a descriptive message, then exits with code 1.Changes:
src/client.ts: AddedgetApiKeyPrefix()to safely expose first 4 chars of the API keysrc/health.ts: New sharedcheckHealth(client)function containing all health-check logic (used by both CLI and SDK)src/commands/health.ts: Thin CLI wrapper — delegates tocheckHealth, formats output, exits with code 1 on errorsrc/sdk.ts: AddedHealthAPIclass withcheck()method for programmatic consumers, also delegates tocheckHealthsrc/types/index.ts: AddedHealthResultinterfacesrc/commands/index.ts), CLI (src/cli.ts), and package entry (src/index.ts)test/mocks.ts: AddedgetApiKeyPrefixtoMockClienttype (no moreRecord<string, unknown>cast)Confidence: 🟡 Medium-High — All 162 tests pass, lint and type-check clean. However, the command hasn't been verified against the live OpenSea API.
Resolves DIS-144
Updates since last revision
Addressed all code review feedback from @ckorhonen:
checkHealth()intosrc/health.ts; both CLI command and SDKHealthAPIdelegate to ittype→interface—HealthResultnow usesinterfacefor consistency with other types insrc/types/getApiKeyPrefixtoMockClientintest/mocks.tsinstead of usingRecord<string, unknown>castvi.importActualto pass through realOpenSeaAPIErrorclass soinstanceofworks; added SDK tests for auth error (401) and API error (500) branchesReview & Testing Checklist for Human
npx . health --api-key <valid_key>andnpx . health --api-key invalidkeyagainst the live API. Both should return"status": "ok"(since the endpoint doesn't validate keys). Confirm the output shape matches{ status, key_prefix, message }OpenSeaCLIand callsdk.health.check()to verify the sharedcheckHealthfunction works through the SDK as well as the CLINotes