feat: add retry, health command, output controls, and 429 exit code (DIS-136)#39
feat: add retry, health command, output controls, and 429 exit code (DIS-136)#39
Conversation
…DIS-136) - Add exponential backoff retry logic with jitter for 429/5xx errors - Add --retries option (default 3) with Retry-After header support - Add health command for API connectivity diagnostics - Add --fields and --max-items output control options - Use exit code 3 for 429 rate limit errors (vs 1 for other API errors) - Add comprehensive tests for all new functionality 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:
|
Co-Authored-By: Chris K <ckorhonen@gmail.com>
…mpty fields, consistent 429 exit code in health Co-Authored-By: Chris K <ckorhonen@gmail.com>
…mand mocks) Co-Authored-By: Chris K <ckorhonen@gmail.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| import { Command } from "commander" | ||
| import { OpenSeaAPIError, type OpenSeaClient } from "../client.js" | ||
|
|
||
| export function healthCommand(getClient: () => OpenSeaClient): Command { |
There was a problem hiding this comment.
🔴 Health command violates mandatory command file pattern from rules.md
The repository's .agents/rules.md (lines 92-118) explicitly states: "Every command file in src/commands/ follows this pattern" — the pattern requires the factory function to receive both getClient and getFormat thunks, and route all output through formatOutput() to console.log(). The healthCommand factory only receives getClient, never calls formatOutput(), and instead constructs JSON directly with JSON.stringify(). This also means --format table and --format toon are silently ignored for the health command, producing inconsistent behavior compared to all other commands.
Prompt for agents
In src/commands/health.ts, update the healthCommand factory function to accept getFormat (and optionally getFilters) thunks matching the pattern of all other command files. Use formatOutput() for the success output at line 13 instead of raw JSON.stringify. For the error paths (lines 17-42), those write to stderr which is acceptable per the rules (stderr for errors). Also update src/cli.ts line 113 to pass getFormat (and getFilters) when wiring the health command: program.addCommand(healthCommand(getClient, getFormat, getFilters)).
Was this helpful? React with 👍 or 👎 to provide feedback.
| .action(async () => { | ||
| const client = getClient() | ||
| const start = performance.now() | ||
| try { | ||
| await client.get("/api/v2/collections", { limit: 1 }) | ||
| const ms = Math.round(performance.now() - start) | ||
| console.log(JSON.stringify({ status: "ok", latency_ms: ms }, null, 2)) | ||
| } catch (error) { | ||
| const ms = Math.round(performance.now() - start) | ||
| if (error instanceof OpenSeaAPIError) { | ||
| console.error( | ||
| JSON.stringify( | ||
| { | ||
| status: "error", | ||
| latency_ms: ms, | ||
| http_status: error.statusCode, | ||
| message: error.responseBody, | ||
| }, | ||
| null, | ||
| 2, | ||
| ), | ||
| ) | ||
| process.exit(error.statusCode === 429 ? 3 : 1) | ||
| } | ||
| console.error( | ||
| JSON.stringify( | ||
| { | ||
| status: "error", | ||
| latency_ms: ms, | ||
| message: (error as Error).message, | ||
| }, | ||
| null, | ||
| 2, | ||
| ), | ||
| ) | ||
| process.exit(1) | ||
| } |
There was a problem hiding this comment.
🔴 Health command violates Design Rule #3: 'Commands are thin wrappers'
.agents/rules.md line 171 states: "Commands are thin wrappers — CLI commands should only parse arguments, call the client, and format output. No business logic in command files." The health command at src/commands/health.ts contains substantial business logic: latency measurement (performance.now() at lines 9, 12, 15), custom error handling with process.exit() calls (lines 29, 42), and structured error JSON construction (lines 17-28, 31-40). Other commands let errors propagate to the global handler in src/cli.ts:118-148. This means the health command's error output has a different JSON structure (http_status, latency_ms) than the global handler's format (status, path), creating an inconsistency for script consumers.
Prompt for agents
Refactor src/commands/health.ts so the command action is a thin wrapper: it should call client.get(), format the success result through formatOutput(), and let errors propagate to the global error handler in src/cli.ts. Latency measurement could be moved to the client layer or done externally. The process.exit() calls for 429/error should be removed from the command file — the global handler in cli.ts already handles OpenSeaAPIError with exit code 3 for 429 and exit code 1 for other errors.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
feat: add retry, health command, output controls, and 429 exit code (DIS-136)
Summary
Implements the four subtasks from DIS-136:
Retry logic (
src/client.ts): Refactorsget()andpost()to share a privatefetchWithRetry()method with exponential backoff + jitter for 429/5xx errors. Configurable via--retries(default 3). RespectsRetry-Afterheader. Logs retry attempts in verbose mode.Health command (
src/commands/health.ts): Newopensea healthcommand that hitsGET /api/v2/collections?limit=1and reports JSON withstatus,latency_ms, and error details. Exits 3 on 429, exits 1 on other failures.Output controls (
src/output.ts+ all command files): New--fields <fields>(comma-separated field selection) and--max-items <n>(array truncation) global options. Filtering is applied at the data level before formatting, so it works with json, table, and toon formats. All 9 command files updated to accept and pass through agetFiltersthunk.429 exit code:
process.exit(3)for rate-limit errors vsprocess.exit(1)for other API errors, consistent across bothcli.tserror handler and health command.Confidence: 🟡 MEDIUM — All tests pass (172/172), lint and type-check clean, but the retry logic refactors the core HTTP path and was only verified via unit tests, not against the live API.
Review & Testing Checklist for Human
fetchWithRetryreplaces the previous inline fetch in bothget()andpost(). Verify the refactor didn't change behavior for non-retry cases (especiallyAbortSignal.timeoutand verbose logging). Thethrow lastError!non-null assertion at the end of the retry loop is logically unreachable but triggers a Biome warning..agents/rules.md(which only lists codes 0, 1, 2). Consider updating rules.md after merge.Recommended Test Plan
Notes
test/mocks.ts):mockResolvedValue→mockImplementationensures each retry attempt gets a freshResponseobject (bodies can only be read once). This is correct but changes behavior for all existing tests—all 172 still pass.filterDatashallow-clones the input object before truncating nested arrays to avoid mutating caller data.--fieldstokens (e.g.--fields ""or trailing commas) are filtered out via.filter(Boolean).Updates since last revision
filterDatanow shallow-clones the object before truncating nested arrays (src/output.ts:23).getFilters()now filters empty strings from--fieldssplits (src/cli.ts:93).cli.tsbehavior (src/commands/health.ts:29).getBaseUrl()andgetApiKey()public methods onOpenSeaClientwere removed.Link to Devin Session: https://app.devin.ai/sessions/96ff332df20a42459b338655d36d8447
Requested by: @ckorhonen