Make SDK response types honest, harden commit retries#1
Merged
Conversation
Response types now match what the server actually returns: upsertCustomer apiKey is optional (only present on first creation), the authorize quote carries no customerLocalId and permits zero credits, and reservation is nullable on authorize/commit/release. meterToolCall retries transient commit failures (network/5xx, never 4xx) because commit is idempotent on requestId; exhausted retries throw MeterCommitFailedError carrying the tool result and requestId so callers can re-commit instead of losing an expensive result. Anthropic cache_creation_input_tokens stays in the billable input bucket (no distinct field exists) and the bucketing is locked with priced adapter tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ydudzw4rEgoBK3MaRmrub
The example Next.js handler let verifyMeterWebhookSignature throw on a bad or forged signature, surfacing as an unhandled 500. Catch it and reject with 400 so copy-pasted deployments fail closed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011ydudzw4rEgoBK3MaRmrub
Reviewer's GuideAligns SDK response types with actual server behavior, adds type-level regression tests, introduces robust commit retry semantics with a new error type, extends HTTP timeout to cover body reads, exposes 429 retry-after metadata, corrects Anthropic adapter token accounting, and hardens the example webhook error handling, with corresponding tests and a changeset. Sequence diagram for meterToolCall commit retry and MeterCommitFailedErrorsequenceDiagram
actor Caller
participant MeterPublicApiClient
participant ExternalTool
participant MeterServer
Caller->>MeterPublicApiClient: meterToolCall(usage, options)
MeterPublicApiClient->>ExternalTool: execute tool
ExternalTool-->>MeterPublicApiClient: result
MeterPublicApiClient->>MeterServer: commit(commitInput)
loop up to COMMIT_MAX_ATTEMPTS (3)
alt commit succeeds
MeterServer-->>MeterPublicApiClient: MeterCommitResponse
MeterPublicApiClient-->>Caller: result
else commit fails (network or 5xx)
MeterServer-->>MeterPublicApiClient: error
MeterPublicApiClient->>MeterPublicApiClient: sleep(COMMIT_RETRY_BASE_DELAY_MS * 2**n)
else commit fails with 4xx
MeterServer-->>MeterPublicApiClient: MeterPublicApiError(status<500)
MeterPublicApiClient->>Caller: throw MeterCommitFailedError(result, requestId, error)
end
end
Note over MeterPublicApiClient,MeterServer: commit is idempotent server-side keyed on requestId
Sequence diagram for webhook POST route with signature validationsequenceDiagram
actor WebhookSender
participant RouteHandler as POST
participant SDK as verifyMeterWebhookSignature
participant Processor as processEventIdempotently
WebhookSender->>RouteHandler: POST request
RouteHandler->>RouteHandler: payload = request.text()
RouteHandler->>SDK: verifyMeterWebhookSignature({payload, signature, secret})
alt signature valid
SDK-->>RouteHandler: ok
RouteHandler->>RouteHandler: event = JSON.parse(payload)
RouteHandler->>Processor: processEventIdempotently(event)
Processor-->>RouteHandler: done
RouteHandler-->>WebhookSender: Response.json({received:true})
else signature invalid
SDK-->>RouteHandler: throw
RouteHandler-->>WebhookSender: Response.json({error:"invalid_signature"}, {status:400})
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- MeterCommitFailedError assumes a non-null requestId but meterToolCall passes usage.requestId directly, so if callers omit requestId you’ll end up throwing an error with an undefined identifier despite the type, which might be better handled via a runtime assertion or by making requestId optional in the error’s shape.
- The retry-after parsing in MeterPublicApiClient.request treats the header as a numeric seconds value only, but HTTP allows a date format as well; consider handling both forms or documenting that only delta-seconds headers are respected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- MeterCommitFailedError assumes a non-null requestId but meterToolCall passes usage.requestId directly, so if callers omit requestId you’ll end up throwing an error with an undefined identifier despite the type, which might be better handled via a runtime assertion or by making requestId optional in the error’s shape.
- The retry-after parsing in MeterPublicApiClient.request treats the header as a numeric seconds value only, but HTTP allows a date format as well; consider handling both forms or documenting that only delta-seconds headers are respected.
## Individual Comments
### Comment 1
<location path="packages/sdk/src/client.ts" line_range="878-887" />
<code_context>
}
}
if (!res.ok) {
+ const retryAfterRaw = res.headers.get("retry-after");
+ const retryAfterSeconds =
+ retryAfterRaw && Number.isFinite(Number(retryAfterRaw))
+ ? Number(retryAfterRaw)
+ : undefined;
throw new MeterPublicApiError(
res.status,
path,
body,
- res.headers.get("x-request-id") ?? undefined
+ res.headers.get("x-request-id") ?? undefined,
+ retryAfterSeconds
);
}
</code_context>
<issue_to_address>
**suggestion:** The `retry-after` parsing only supports numeric seconds and silently ignores HTTP-date values.
The implementation only handles numeric `Retry-After` values and discards valid HTTP-date headers, so callers miss useful backoff information when servers use the date form. To better align with the spec and improve interoperability, consider parsing non-numeric values with `Date.parse` and converting them to seconds relative to `Date.now()`, while still treating malformed headers as `undefined`.
Suggested implementation:
```typescript
}
if (!res.ok) {
```
```typescript
if (!res.ok) {
const retryAfterRaw = res.headers.get("retry-after");
let retryAfterSeconds: number | undefined;
if (retryAfterRaw) {
const numericValue = Number(retryAfterRaw);
if (Number.isFinite(numericValue)) {
// Numeric seconds form: Retry-After: 120
retryAfterSeconds = numericValue;
} else {
// HTTP-date form: Retry-After: Wed, 21 Oct 2015 07:28:00 GMT
const parsedDateMs = Date.parse(retryAfterRaw);
if (!Number.isNaN(parsedDateMs)) {
const diffMs = parsedDateMs - Date.now();
if (diffMs > 0) {
retryAfterSeconds = diffMs / 1000;
}
// If diffMs <= 0, the date is in the past; treat as no backoff (undefined)
}
// Malformed HTTP-date leaves retryAfterSeconds as undefined
}
}
throw new MeterPublicApiError(
res.status,
path,
body,
res.headers.get("x-request-id") ?? undefined,
retryAfterSeconds
);
}
```
1. Ensure the `MeterPublicApiError` constructor accepts the new `retryAfterSeconds?: number` parameter and stores/exposes it appropriately.
2. Callers that consume `retryAfterSeconds` should be prepared for fractional seconds (e.g., 1.5) and for `undefined` when the header is absent or malformed, or when the HTTP-date is already in the past.
</issue_to_address>
### Comment 2
<location path="examples/meter-sdk/webhook-route.ts" line_range="7-13" />
<code_context>
- });
+ // A bad/forged signature must be a 400, not an unhandled 500: verifyMeterWebhookSignature
+ // throws on mismatch, so catch it and reject explicitly before trusting the body.
+ try {
+ await verifyMeterWebhookSignature({
+ payload,
+ signature: request.headers.get("x-meter-signature") ?? "",
+ secret: process.env.METER_WEBHOOK_SECRET!,
+ });
+ } catch {
+ return Response.json({ error: "invalid_signature" }, { status: 400 });
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching all errors from `verifyMeterWebhookSignature` and mapping them to a 400 may hide server-side issues.
This bare `catch` turns all failures from `verifyMeterWebhookSignature`—including runtime/configuration errors—into a 400 `invalid_signature`, which both obscures real server problems and misleads clients. Prefer catching only the specific error that indicates a bad signature, and allow other errors to surface as 500s (with appropriate logging) so genuine infrastructure issues aren’t masked as client errors.
</issue_to_address>
### Comment 3
<location path="packages/sdk/test/client.test.ts" line_range="97-95" />
<code_context>
assert.equal(calls[1]?.body.reason, "upstream failed");
});
+test("meterToolCall retries a transient commit failure and returns the result", async () => {
+ const calls: string[] = [];
+ const client = new MeterPublicApiClient({
+ baseUrl: "https://meter.example",
+ serviceId: "service_1",
+ serviceApiKey: "secret",
+ fetchImpl: async (input) => {
+ const path = new URL(String(input)).pathname;
+ calls.push(path);
+ if (path === "/api/v1/meter/commit" && calls.filter((p) => p === path).length === 1) {
+ return Response.json({ error: "internal" }, { status: 503 });
+ }
+ return Response.json({});
+ },
+ });
+
+ const result = await client.meterToolCall(
</code_context>
<issue_to_address>
**suggestion (testing):** Add parallel tests for `withUsage` to cover the new commit retry and `MeterCommitFailedError` behavior
The new `meterToolCall` tests cover transient commit failures, 4xx aborts, and `requestId`/result preservation, but `withUsage` now shares this retry logic and error behavior without any targeted tests. Please add at least two `withUsage` tests: one where a transient 5xx commit eventually succeeds (asserting commit call count and returned result), and one where all commit attempts fail (asserting `MeterCommitFailedError` with the correct `result`/`requestId` and that reservations are not released). This will help prevent regressions in the `withUsage` path.
Suggested implementation:
```typescript
test("meterToolCall retries a transient commit failure and returns the result", async () => {
const calls: string[] = [];
const client = new MeterPublicApiClient({
baseUrl: "https://meter.example",
serviceId: "service_1",
serviceApiKey: "secret",
fetchImpl: async (input) => {
const path = new URL(String(input)).pathname;
calls.push(path);
if (path === "/api/v1/meter/commit" && calls.filter((p) => p === path).length === 1) {
return Response.json({ error: "internal" }, { status: 503 });
}
return Response.json({});
},
});
const result = await client.meterToolCall(
{ customerLocalId: "customer_1", tool: "search", credits: 3 },
() => "expensive result"
);
assert.equal(result, "expensive result");
assert.deepEqual(calls, [
"/api/v1/meter/authorize",
"/api/v1/meter/commit",
"/api/v1/meter/commit",
]);
});
test("withUsage retries a transient commit failure and returns the result", async () => {
const calls: string[] = [];
const client = new MeterPublicApiClient({
baseUrl: "https://meter.example",
serviceId: "service_1",
serviceApiKey: "secret",
fetchImpl: async (input, init) => {
const url = new URL(String(input));
const path = url.pathname;
calls.push(path);
// Simulate an authorize call that returns a requestId
if (path === "/api/v1/meter/authorize") {
return Response.json({ requestId: "req_1" });
}
// First commit attempt fails with a transient 5xx error, second succeeds
if (path === "/api/v1/meter/commit") {
const commitAttempts = calls.filter((p) => p === path).length;
if (commitAttempts === 1) {
return Response.json({ error: "internal" }, { status: 503 });
}
return Response.json({ requestId: "req_1" });
}
return Response.json({});
},
});
const result = await client.withUsage(
{ customerLocalId: "customer_1", credits: 3 },
async () => {
return "expensive result";
}
);
assert.equal(result, "expensive result");
assert.deepEqual(calls, [
"/api/v1/meter/authorize",
"/api/v1/meter/commit",
"/api/v1/meter/commit",
]);
});
test("withUsage throws MeterCommitFailedError when all commit attempts fail and does not release reservations", async () => {
const calls: string[] = [];
const client = new MeterPublicApiClient({
baseUrl: "https://meter.example",
serviceId: "service_1",
serviceApiKey: "secret",
fetchImpl: async (input, init) => {
const url = new URL(String(input));
const path = url.pathname;
calls.push(path);
if (path === "/api/v1/meter/authorize") {
// Return a requestId that should be surfaced on the error
return Response.json({ requestId: "req_2" });
}
if (path === "/api/v1/meter/commit") {
// All commit attempts fail with a transient 5xx error
return Response.json({ error: "internal" }, { status: 503 });
}
// There should be no release calls on commit failure
if (path === "/api/v1/meter/release") {
return Response.json({});
}
return Response.json({});
},
});
let caught: unknown;
try {
await client.withUsage(
{ customerLocalId: "customer_2", credits: 5 },
async () => {
return "failed result";
}
);
} catch (err) {
caught = err;
}
assert.ok(caught instanceof MeterCommitFailedError);
const error = caught as MeterCommitFailedError;
assert.equal(error.requestId, "req_2");
assert.equal(error.result, "failed result");
// Authorize happens once, commit is retried, and no release is performed
assert.ok(calls.includes("/api/v1/meter/authorize"));
const commitCalls = calls.filter((p) => p === "/api/v1/meter/commit").length;
assert.ok(commitCalls >= 2);
assert.ok(!calls.includes("/api/v1/meter/release"));
});
```
The `withUsage` tests above assume the following based on typical SDK patterns and the new `meterToolCall` behavior:
1. `MeterPublicApiClient.withUsage` accepts `(options, fn)` where `fn` returns the "result" that should be preserved on `MeterCommitFailedError`. Adjust the call signature if your actual `withUsage` API differs (e.g., if it receives a usage context argument).
2. The commit retry count is asserted as `>= 2` in the failure test to avoid coupling to an exact retry limit. If your implementation has a known fixed retry count, you may tighten this assertion to an exact number and/or assert the full `calls` array contents.
3. The error shape is assumed to be `MeterCommitFailedError` with `requestId` and `result` properties. If your error type exposes these differently (e.g., `error.data.requestId` or `error.commit.requestId`), update the assertions accordingly.
4. If `withUsage` performs additional API calls (for example, a separate reservation or usage endpoint before commit), you may want to extend the `calls` assertions to cover those paths and ensure they behave correctly under commit failure scenarios.
</issue_to_address>
### Comment 4
<location path="packages/sdk/test/client.test.ts" line_range="187-99" />
<code_context>
+ assert.deepEqual(calls, ["/api/v1/meter/authorize", "/api/v1/meter/commit"]);
+});
+
test("HTTP errors expose status, response body, path, and request ID", async () => {
const client = new MeterPublicApiClient({
baseUrl: "https://meter.example",
</code_context>
<issue_to_address>
**suggestion (testing):** Extend HTTP error tests to assert `retryAfterSeconds` on 429 responses
Since `MeterPublicApiError` now exposes `retryAfterSeconds` from the `retry-after` header, please extend this test (or add a new one) to cover a 429 response with a numeric `retry-after` and assert the parsed `retryAfterSeconds` value. Also consider a case with a non-numeric `retry-after` to confirm `retryAfterSeconds` remains `undefined` when parsing fails, so backoff behavior is properly validated.
Suggested implementation:
```typescript
await assert.rejects(
client.meterToolCall({ customerLocalId: "customer_1", tool: "search" }, () => "ok"),
MeterCommitFailedError
);
assert.deepEqual(calls, ["/api/v1/meter/authorize", "/api/v1/meter/commit"]);
});
// HTTP error tests
test("HTTP errors expose status, response body, path, request ID, and retryAfterSeconds for numeric retry-after on 429", async () => {
const calls: string[] = [];
const client = new MeterPublicApiClient({
baseUrl: "https://meter.example",
serviceId: "service_1",
serviceApiKey: "secret",
fetchImpl: async (input) => {
const url = new URL(String(input));
calls.push(url.pathname);
// Return a 429 with numeric retry-after and request ID headers
return new Response(
JSON.stringify({
error: "rate_limit",
message: "Too many requests",
}),
{
status: 429,
headers: {
"retry-after": "10",
"x-request-id": "req_429_numeric",
},
}
);
},
});
const error = await assert.rejects(
client.meterToolCall({ customerLocalId: "customer_1", tool: "search" }, () => "ok"),
(err: unknown) => err instanceof MeterPublicApiError
);
assert.ok(error instanceof MeterPublicApiError);
assert.strictEqual(error.status, 429);
assert.strictEqual(error.retryAfterSeconds, 10);
assert.strictEqual(error.requestId, "req_429_numeric");
assert.strictEqual(error.path, "/api/v1/meter/commit");
assert.deepEqual(calls, ["/api/v1/meter/authorize", "/api/v1/meter/commit"]);
});
test("HTTP 429 with non-numeric retry-after leaves retryAfterSeconds undefined", async () => {
const calls: string[] = [];
const client = new MeterPublicApiClient({
baseUrl: "https://meter.example",
serviceId: "service_1",
serviceApiKey: "secret",
fetchImpl: async (input) => {
const url = new URL(String(input));
calls.push(url.pathname);
// Return a 429 with non-numeric retry-after; parsing should fail
return new Response(
JSON.stringify({
error: "rate_limit",
message: "Too many requests",
}),
{
status: 429,
headers: {
"retry-after": "not-a-number",
"x-request-id": "req_429_non_numeric",
},
}
);
},
});
const error = await assert.rejects(
client.meterToolCall({ customerLocalId: "customer_1", tool: "search" }, () => "ok"),
(err: unknown) => err instanceof MeterPublicApiError
);
assert.ok(error instanceof MeterPublicApiError);
assert.strictEqual(error.status, 429);
assert.strictEqual(error.retryAfterSeconds, undefined);
assert.strictEqual(error.requestId, "req_429_non_numeric");
assert.strictEqual(error.path, "/api/v1/meter/commit");
assert.deepEqual(calls, ["/api/v1/meter/authorize", "/api/v1/meter/commit"]);
});
```
The above edit assumes:
1. `MeterPublicApiClient`, `MeterPublicApiError`, `assert`, `Response`, and `URL` are already imported in `client.test.ts`, which appears consistent with the existing tests.
2. The `meterToolCall` path used by the client is `/api/v1/meter/commit` (matching the preceding test); if the actual path differs, update the `assert.strictEqual(error.path, "...")` expectations accordingly.
3. The existing "HTTP errors expose status, response body, path, and request ID" test is elsewhere in the file; the added tests are independent and specifically target 429 responses with numeric and non-numeric `retry-after` headers to validate `retryAfterSeconds` parsing and backoff behavior.
If the client uses a different method to trigger HTTP errors (e.g., another endpoint or operation), you can adapt the body of these tests to call that method instead, but keep the 429 + `retry-after` header setup and the `retryAfterSeconds` assertions intact.
</issue_to_address>
### Comment 5
<location path="packages/adapters/test/index.test.ts" line_range="32" />
<code_context>
+ const anthropic = aiUsageFromAnthropic({
</code_context>
<issue_to_address>
**suggestion (testing):** Add a companion Anthropic test where usage is nested under `message.usage` to mirror the adapter’s fallback behavior
The adapter also handles responses where usage is under `response.message.usage`. Please add a test that sends a payload with `message: { usage: ... }` and no top-level `usage`, and asserts the same token bucket and `costMicrousd` values. This will keep both code paths covered and guard against regressions in the nested `message.usage` handling.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+878
to
+887
| const retryAfterRaw = res.headers.get("retry-after"); | ||
| const retryAfterSeconds = | ||
| retryAfterRaw && Number.isFinite(Number(retryAfterRaw)) | ||
| ? Number(retryAfterRaw) | ||
| : undefined; | ||
| throw new MeterPublicApiError( | ||
| res.status, | ||
| path, | ||
| body, | ||
| res.headers.get("x-request-id") ?? undefined | ||
| res.headers.get("x-request-id") ?? undefined, |
There was a problem hiding this comment.
suggestion: The retry-after parsing only supports numeric seconds and silently ignores HTTP-date values.
The implementation only handles numeric Retry-After values and discards valid HTTP-date headers, so callers miss useful backoff information when servers use the date form. To better align with the spec and improve interoperability, consider parsing non-numeric values with Date.parse and converting them to seconds relative to Date.now(), while still treating malformed headers as undefined.
Suggested implementation:
}
if (!res.ok) { if (!res.ok) {
const retryAfterRaw = res.headers.get("retry-after");
let retryAfterSeconds: number | undefined;
if (retryAfterRaw) {
const numericValue = Number(retryAfterRaw);
if (Number.isFinite(numericValue)) {
// Numeric seconds form: Retry-After: 120
retryAfterSeconds = numericValue;
} else {
// HTTP-date form: Retry-After: Wed, 21 Oct 2015 07:28:00 GMT
const parsedDateMs = Date.parse(retryAfterRaw);
if (!Number.isNaN(parsedDateMs)) {
const diffMs = parsedDateMs - Date.now();
if (diffMs > 0) {
retryAfterSeconds = diffMs / 1000;
}
// If diffMs <= 0, the date is in the past; treat as no backoff (undefined)
}
// Malformed HTTP-date leaves retryAfterSeconds as undefined
}
}
throw new MeterPublicApiError(
res.status,
path,
body,
res.headers.get("x-request-id") ?? undefined,
retryAfterSeconds
);
}- Ensure the
MeterPublicApiErrorconstructor accepts the newretryAfterSeconds?: numberparameter and stores/exposes it appropriately. - Callers that consume
retryAfterSecondsshould be prepared for fractional seconds (e.g., 1.5) and forundefinedwhen the header is absent or malformed, or when the HTTP-date is already in the past.
Comment on lines
+7
to
+13
| try { | ||
| await verifyMeterWebhookSignature({ | ||
| payload, | ||
| signature: request.headers.get("x-meter-signature") ?? "", | ||
| secret: process.env.METER_WEBHOOK_SECRET!, | ||
| }); | ||
| } catch { |
There was a problem hiding this comment.
issue (bug_risk): Catching all errors from verifyMeterWebhookSignature and mapping them to a 400 may hide server-side issues.
This bare catch turns all failures from verifyMeterWebhookSignature—including runtime/configuration errors—into a 400 invalid_signature, which both obscures real server problems and misleads clients. Prefer catching only the specific error that indicates a bad signature, and allow other errors to surface as 500s (with appropriate logging) so genuine infrastructure issues aren’t masked as client errors.
| @@ -93,6 +94,96 @@ test("meterToolCall releases the generated reservation when protected work fails | |||
| assert.equal(calls[1]?.body.reason, "upstream failed"); | |||
| }); | |||
There was a problem hiding this comment.
suggestion (testing): Add parallel tests for withUsage to cover the new commit retry and MeterCommitFailedError behavior
The new meterToolCall tests cover transient commit failures, 4xx aborts, and requestId/result preservation, but withUsage now shares this retry logic and error behavior without any targeted tests. Please add at least two withUsage tests: one where a transient 5xx commit eventually succeeds (asserting commit call count and returned result), and one where all commit attempts fail (asserting MeterCommitFailedError with the correct result/requestId and that reservations are not released). This will help prevent regressions in the withUsage path.
Suggested implementation:
test("meterToolCall retries a transient commit failure and returns the result", async () => {
const calls: string[] = [];
const client = new MeterPublicApiClient({
baseUrl: "https://meter.example",
serviceId: "service_1",
serviceApiKey: "secret",
fetchImpl: async (input) => {
const path = new URL(String(input)).pathname;
calls.push(path);
if (path === "/api/v1/meter/commit" && calls.filter((p) => p === path).length === 1) {
return Response.json({ error: "internal" }, { status: 503 });
}
return Response.json({});
},
});
const result = await client.meterToolCall(
{ customerLocalId: "customer_1", tool: "search", credits: 3 },
() => "expensive result"
);
assert.equal(result, "expensive result");
assert.deepEqual(calls, [
"/api/v1/meter/authorize",
"/api/v1/meter/commit",
"/api/v1/meter/commit",
]);
});
test("withUsage retries a transient commit failure and returns the result", async () => {
const calls: string[] = [];
const client = new MeterPublicApiClient({
baseUrl: "https://meter.example",
serviceId: "service_1",
serviceApiKey: "secret",
fetchImpl: async (input, init) => {
const url = new URL(String(input));
const path = url.pathname;
calls.push(path);
// Simulate an authorize call that returns a requestId
if (path === "/api/v1/meter/authorize") {
return Response.json({ requestId: "req_1" });
}
// First commit attempt fails with a transient 5xx error, second succeeds
if (path === "/api/v1/meter/commit") {
const commitAttempts = calls.filter((p) => p === path).length;
if (commitAttempts === 1) {
return Response.json({ error: "internal" }, { status: 503 });
}
return Response.json({ requestId: "req_1" });
}
return Response.json({});
},
});
const result = await client.withUsage(
{ customerLocalId: "customer_1", credits: 3 },
async () => {
return "expensive result";
}
);
assert.equal(result, "expensive result");
assert.deepEqual(calls, [
"/api/v1/meter/authorize",
"/api/v1/meter/commit",
"/api/v1/meter/commit",
]);
});
test("withUsage throws MeterCommitFailedError when all commit attempts fail and does not release reservations", async () => {
const calls: string[] = [];
const client = new MeterPublicApiClient({
baseUrl: "https://meter.example",
serviceId: "service_1",
serviceApiKey: "secret",
fetchImpl: async (input, init) => {
const url = new URL(String(input));
const path = url.pathname;
calls.push(path);
if (path === "/api/v1/meter/authorize") {
// Return a requestId that should be surfaced on the error
return Response.json({ requestId: "req_2" });
}
if (path === "/api/v1/meter/commit") {
// All commit attempts fail with a transient 5xx error
return Response.json({ error: "internal" }, { status: 503 });
}
// There should be no release calls on commit failure
if (path === "/api/v1/meter/release") {
return Response.json({});
}
return Response.json({});
},
});
let caught: unknown;
try {
await client.withUsage(
{ customerLocalId: "customer_2", credits: 5 },
async () => {
return "failed result";
}
);
} catch (err) {
caught = err;
}
assert.ok(caught instanceof MeterCommitFailedError);
const error = caught as MeterCommitFailedError;
assert.equal(error.requestId, "req_2");
assert.equal(error.result, "failed result");
// Authorize happens once, commit is retried, and no release is performed
assert.ok(calls.includes("/api/v1/meter/authorize"));
const commitCalls = calls.filter((p) => p === "/api/v1/meter/commit").length;
assert.ok(commitCalls >= 2);
assert.ok(!calls.includes("/api/v1/meter/release"));
});The withUsage tests above assume the following based on typical SDK patterns and the new meterToolCall behavior:
MeterPublicApiClient.withUsageaccepts(options, fn)wherefnreturns the "result" that should be preserved onMeterCommitFailedError. Adjust the call signature if your actualwithUsageAPI differs (e.g., if it receives a usage context argument).- The commit retry count is asserted as
>= 2in the failure test to avoid coupling to an exact retry limit. If your implementation has a known fixed retry count, you may tighten this assertion to an exact number and/or assert the fullcallsarray contents. - The error shape is assumed to be
MeterCommitFailedErrorwithrequestIdandresultproperties. If your error type exposes these differently (e.g.,error.data.requestIdorerror.commit.requestId), update the assertions accordingly. - If
withUsageperforms additional API calls (for example, a separate reservation or usage endpoint before commit), you may want to extend thecallsassertions to cover those paths and ensure they behave correctly under commit failure scenarios.
|
|
||
| test("meterToolCall retries a transient commit failure and returns the result", async () => { | ||
| const calls: string[] = []; | ||
| const client = new MeterPublicApiClient({ |
There was a problem hiding this comment.
suggestion (testing): Extend HTTP error tests to assert retryAfterSeconds on 429 responses
Since MeterPublicApiError now exposes retryAfterSeconds from the retry-after header, please extend this test (or add a new one) to cover a 429 response with a numeric retry-after and assert the parsed retryAfterSeconds value. Also consider a case with a non-numeric retry-after to confirm retryAfterSeconds remains undefined when parsing fails, so backoff behavior is properly validated.
Suggested implementation:
await assert.rejects(
client.meterToolCall({ customerLocalId: "customer_1", tool: "search" }, () => "ok"),
MeterCommitFailedError
);
assert.deepEqual(calls, ["/api/v1/meter/authorize", "/api/v1/meter/commit"]);
});
// HTTP error tests
test("HTTP errors expose status, response body, path, request ID, and retryAfterSeconds for numeric retry-after on 429", async () => {
const calls: string[] = [];
const client = new MeterPublicApiClient({
baseUrl: "https://meter.example",
serviceId: "service_1",
serviceApiKey: "secret",
fetchImpl: async (input) => {
const url = new URL(String(input));
calls.push(url.pathname);
// Return a 429 with numeric retry-after and request ID headers
return new Response(
JSON.stringify({
error: "rate_limit",
message: "Too many requests",
}),
{
status: 429,
headers: {
"retry-after": "10",
"x-request-id": "req_429_numeric",
},
}
);
},
});
const error = await assert.rejects(
client.meterToolCall({ customerLocalId: "customer_1", tool: "search" }, () => "ok"),
(err: unknown) => err instanceof MeterPublicApiError
);
assert.ok(error instanceof MeterPublicApiError);
assert.strictEqual(error.status, 429);
assert.strictEqual(error.retryAfterSeconds, 10);
assert.strictEqual(error.requestId, "req_429_numeric");
assert.strictEqual(error.path, "/api/v1/meter/commit");
assert.deepEqual(calls, ["/api/v1/meter/authorize", "/api/v1/meter/commit"]);
});
test("HTTP 429 with non-numeric retry-after leaves retryAfterSeconds undefined", async () => {
const calls: string[] = [];
const client = new MeterPublicApiClient({
baseUrl: "https://meter.example",
serviceId: "service_1",
serviceApiKey: "secret",
fetchImpl: async (input) => {
const url = new URL(String(input));
calls.push(url.pathname);
// Return a 429 with non-numeric retry-after; parsing should fail
return new Response(
JSON.stringify({
error: "rate_limit",
message: "Too many requests",
}),
{
status: 429,
headers: {
"retry-after": "not-a-number",
"x-request-id": "req_429_non_numeric",
},
}
);
},
});
const error = await assert.rejects(
client.meterToolCall({ customerLocalId: "customer_1", tool: "search" }, () => "ok"),
(err: unknown) => err instanceof MeterPublicApiError
);
assert.ok(error instanceof MeterPublicApiError);
assert.strictEqual(error.status, 429);
assert.strictEqual(error.retryAfterSeconds, undefined);
assert.strictEqual(error.requestId, "req_429_non_numeric");
assert.strictEqual(error.path, "/api/v1/meter/commit");
assert.deepEqual(calls, ["/api/v1/meter/authorize", "/api/v1/meter/commit"]);
});The above edit assumes:
MeterPublicApiClient,MeterPublicApiError,assert,Response, andURLare already imported inclient.test.ts, which appears consistent with the existing tests.- The
meterToolCallpath used by the client is/api/v1/meter/commit(matching the preceding test); if the actual path differs, update theassert.strictEqual(error.path, "...")expectations accordingly. - The existing "HTTP errors expose status, response body, path, and request ID" test is elsewhere in the file; the added tests are independent and specifically target 429 responses with numeric and non-numeric
retry-afterheaders to validateretryAfterSecondsparsing and backoff behavior.
If the client uses a different method to trigger HTTP errors (e.g., another endpoint or operation), you can adapt the body of these tests to call that method instead, but keep the 429 +retry-afterheader setup and theretryAfterSecondsassertions intact.
| assert.equal(anthropic.inputTokens, 80); | ||
| assert.equal(anthropic.cachedInputTokens, 20); | ||
| assert.equal(anthropic.totalTokens, 105); | ||
| assert.equal(anthropic.costMicrousd, 321); |
There was a problem hiding this comment.
suggestion (testing): Add a companion Anthropic test where usage is nested under message.usage to mirror the adapter’s fallback behavior
The adapter also handles responses where usage is under response.message.usage. Please add a test that sends a payload with message: { usage: ... } and no top-level usage, and asserts the same token bucket and costMicrousd values. This will keep both code paths covered and guard against regressions in the nested message.usage handling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit of the published `@meter-mcp/*` SDK against the server contract found response types that claimed shapes the server never returns, silently breaking consumers. This branch makes them honest and hardens the commit path. Full gate green: build, 25 tests, publint, attw, `verify:openapi`, examples typecheck.
Type accuracy (silent consumer bugs)
Correctness
A changeset is included (sdk minor + adapters patch). No publish.
🤖 Generated with Claude Code
https://claude.ai/code/session_011ydudzw4rEgoBK3MaRmrub
Summary by Sourcery
Align SDK response types with actual server behavior and improve reliability of usage commit flows.
New Features:
Bug Fixes:
Enhancements:
Tests:
Chores: