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>
|
@devin CI lint is failing on this PR. Please investigate the lint errors and push a fix. You can reproduce locally with |
Co-Authored-By: Chris K <ckorhonen@gmail.com>
|
@devin Several issues to fix on this PR:
getApiKeyPrefix(): string {
if (this.apiKey.length < 8) return '***'
return `${this.apiKey.slice(0, 4)}...`
}
|
- getApiKeyPrefix() returns '***' for keys shorter than 8 chars - checkHealth() detects 429 responses and sets rate_limited flag - CLI exits with code 3 on rate limiting (vs 1 for other errors) - HealthResult interface gains rate_limited boolean field - Added tests for 429 handling in both CLI and SDK Co-Authored-By: Chris K <ckorhonen@gmail.com>
The healthCommand was added to src/commands/index.ts via PR #35 but these test files were not updated to include it in their mock. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat: add retry with exponential backoff in client.ts (DIS-143) Co-Authored-By: Chris K <ckorhonen@gmail.com> * fix: cancel response body on retryable errors to prevent resource leak Co-Authored-By: Chris K <ckorhonen@gmail.com> * fix: create fresh AbortSignal per retry attempt to prevent stale timeout Co-Authored-By: Chris K <ckorhonen@gmail.com> * fix: address code review feedback (POST idempotency, SDK default, stream cancel safety) Co-Authored-By: Chris K <ckorhonen@gmail.com> * fix: add healthCommand mock to CLI error handler tests The healthCommand was added to src/commands/index.ts via PR #35 but these test files were not updated to include it in their mock. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
feat: add
opensea healthdiagnostic command (DIS-144)Summary
Adds a new
opensea healthcommand that validates API connectivity and authentication before running expensive operations. Performs a two-step check:GET /api/v2/collections?limit=1(public endpoint — succeeds regardless of key validity)GET /api/v2/listings/collection/boredapeyachtclub/all?limit=1(requires valid API key)Outputs structured JSON:
{ "status": "ok", "key_prefix": "8d02...", "authenticated": true, "rate_limited": false, "message": "Connectivity and authentication are working" }Exit codes:
0= success,1= error (auth failure, API error, network error),3= rate limited (429).If step 1 succeeds but step 2 fails with a non-auth error (e.g., 500), the command returns
"status": "ok"with"authenticated": falseand a message indicating auth could not be verified.Changes:
src/client.ts: AddedgetApiKeyPrefix()— returns first 4 chars of API key, or"***"for keys shorter than 8 chars to prevent leaking short keyssrc/health.ts: New sharedcheckHealth(client)function with two-step connectivity + auth validation (used by both CLI and SDK)src/commands/health.ts: Thin CLI wrapper — delegates tocheckHealth, formats output, exits with code 1 or 3src/sdk.ts: AddedHealthAPIclass withcheck()method for programmatic consumers, delegates tocheckHealthsrc/types/index.ts: AddedHealthResultinterface withstatus,key_prefix,authenticated,rate_limited,messagesrc/commands/index.ts), CLI (src/cli.ts), and package entry (src/index.ts)test/mocks.ts: AddedgetApiKeyPrefixtoMockClienttypeConfidence: 🟡 Medium-High — All 167 tests pass, lint and type-check clean. Not verified against the live OpenSea API.
Resolves DIS-144
Review & Testing Checklist for Human
npx . health --api-key <valid_key>— should return"status": "ok","authenticated": truenpx . health --api-key invalidkey123— should return"status": "error","authenticated": false, exit code 1boredapeyachtclub— verify this collection still exists and requires auth. If it's ever removed/renamed, the health check's auth step will break. Consider whether this should be configurable or use a different endpoint.OpenSeaCLIand callsdk.health.check()to verify the sharedcheckHealthfunction works through the SDKNotes