Code Review: 24 findings across develop branch (3 critical, 6 high, 9 medium, 6 low)

[ccp-manash]

Code Review Report: Crowdsplit SDK

Date: 2026-02-20
Branch: develop
Commit: 3d3993c
Reviewer: Claude Opus 4.6
Validation: 4 independent Opus 4.6 agents verified all findings (27/28 TRUE, 1/28 PARTIALLY TRUE, 0/28 FALSE)

---

Table of Contents

• Executive Summary
• Severity Matrix
• Critical Issues
• High Severity Issues
• Medium Severity Issues
• Low Severity Issues
• Appendix: Validation Results

---

Executive Summary

This is a pre-launch SDK (README states "WORK IN PROGRESS - Launch expected March 2026") with reasonable architectural foundations — Result types, factory pattern, monorepo structure — but significant execution gaps that would be embarrassing in production.

The three critical bugs alone are enough to block any release: tokens re-authenticate on every API call (doubling latency), a test mocking library ships to every consumer, and client secrets are exposed on public objects.

Total findings: 24 issues across 4 severity levels.

Severity	Count	Summary
Critical	3	Token expiration bug, prod dependency leak, secret exposure
High	6	Missing service, any types, silent test skips, code duplication, CI gaps
Medium	9	Type design flaws, dead code, inconsistencies, security gaps
Low	6	Useless wrappers, naming inconsistencies, config contradictions

---

Severity Matrix

#	Severity	Issue	File(s)
1	Critical	Token expires_in seconds/ms mismatch	authManager.ts
2	Critical	nock in production dependencies	package.json
3	Critical	clientSecret exposed on public client object	client.ts, types/client.ts
4	High	RefundService missing from product facade	crowdsplit/index.ts
5	High	any types on HTTP client and retry interfaces	httpClient.ts, retryHandler.ts
6	High	Integration tests silently pass when skipped	__tests__/integration/*
7	High	Token-fetch pattern duplicated 35 times	All 11 service files
8	High	CI lint not enforced	ci.yml
9	High	test-sdk.ts committed with 15+ hardcoded UUIDs	test-sdk.ts
10	Medium	ReturnType<typeof> instead of named interfaces	crowdsplit/index.ts
11	Medium	Response types extend request union types	types/payment.ts, types/transfer.ts
12	Medium	Dead exported function getErrorBodyMessage	services/helpers.ts
13	Medium	Dual lockfiles (npm + pnpm)	Root directory
14	Medium	No webhook signature verification utility	N/A
15	Medium	Trailing slash inconsistency on payment URL	paymentService.ts
16	Medium	Unpinned npm@latest in release CI	release.yml
17	Medium	@types/jest@^30 with ts-jest@^29 version mismatch	package.json
18	Medium	contracts package is a published placeholder	contracts/src/index.ts
19	Low	createAuthService is a zero-value wrapper	authService.ts
20	Low	36 hardcoded URL constructions across services	All service files
21	Low	SDKConfig is an unused type alias	types/config.ts
22	Low	experimentalDecorators enabled for dead code	tsconfig.json
23	Low	Customer.Data has ambiguous id/customer_id	types/customer.ts
24	Low	Engine/packageManager/prepublish inconsistencies	package.json (root + api)

---

Critical Issues

C1: Token Expiration Bug — Every API Call Re-authenticates

Impact: Every SDK consumer makes a redundant auth request on every API call, doubling latency and hammering the auth server.

Root cause: OAuth expires_in is in seconds per RFC 6749. Date.now() returns milliseconds. A standard 3600-second token expires in ~3.6 seconds.

Location:
packages/api/src/authManager.ts:L49

this.tokenExpiration = Date.now() + response.value.expires_in;
// Should be: Date.now() + response.value.expires_in * 1000

The 60-second buffer confirms the code intends millisecond arithmetic:
packages/api/src/authManager.ts:L62

currentTime >= this.tokenExpiration - 60000

Tests mask the bug instead of catching it:
packages/api/__tests__/unit/authService.test.ts:L39 — uses expires_in: 3300000 (already milliseconds, not seconds)
packages/api/__tests__/unit/authService.test.ts:L92 — uses expires_in: 1 which only triggers re-fetch because of the bug

Fix: Multiply expires_in by 1000 in authManager.ts. Rewrite tests to mock Date.now() instead of relying on real timers and buggy values.

Caveat: If the upstream API returns expires_in in milliseconds (deviating from OAuth spec), the code is correct. This should be verified against the actual API response.

---

C2: Test Mocking Library Ships as Production Dependency

Impact: Every consumer of the SDK installs nock (HTTP mocking library) and dotenv as transitive runtime dependencies.

Location:
packages/api/package.json:L55-L58

"dependencies": {
    "dotenv": "^17.2.1",
    "nock": "^14.0.10"
}

Neither is imported by any production source file. Both are only used in tests and configuration.

Fix: Move both to devDependencies.

---

C3: Client Secret Exposed on Public Object

Impact: Any code holding a reference to the SDK client can extract credentials via client.config.clientSecret. Secrets can leak through logging, serialization, or debugging.

Location:
packages/api/src/client.ts:L46 — config: resolvedConfig returned on the public object
packages/api/src/types/client.ts:L9 — clientSecret: string in the exported config type

The test validates this as expected behavior:
packages/api/__tests__/unit/client.test.ts:L78

expect(client.config.clientSecret).toBe(baseConfig.clientSecret);

Fix: Remove clientSecret from ResolvedOakClientConfig. Pass the secret directly to AuthManager via constructor and store it as a private closure variable. Remove the test assertion.

---

High Severity Issues

H1: RefundService Missing from Product Facade

Impact: Consumers using the Crowdsplit() product facade cannot access refund functionality despite the service being fully implemented.

Location:
packages/api/src/services/index.ts:L34 — createRefundService is exported
packages/api/src/products/crowdsplit/index.ts:L15-L26 — CrowdsplitProduct interface has no refunds property
packages/api/src/products/crowdsplit/index.ts:L32-L43 — Crowdsplit() factory does not instantiate refund service

Fix: Import createRefundService/RefundService, add refunds: RefundService to the interface, add refunds: createRefundService(client) to the factory.

---

H2: any Types on HTTP Client and Retry Interfaces

Impact: The most critical type safety boundary — request bodies and error handlers — has no type checking. Consumers get zero TypeScript protection for the data they send.

Locations:
packages/api/src/utils/httpClient.ts:L157 — post<T>(url: string, data: any, ...)
packages/api/src/utils/httpClient.ts:L195 — put<T>(url: string, data: any, ...)
packages/api/src/utils/httpClient.ts:L212 — patch<T>(url: string, data: any, ...)
packages/api/src/utils/retryHandler.ts:L20-L21 — retryOnError?: (error: any) => boolean and onRetry?: (attempt: number, error: any) => void

Additional any usage in type definitions:
packages/api/src/types/payment.ts:L13 — Metadata: [key: string]: any
packages/api/src/types/payment.ts:L36 — ProviderResponse: [key: string]: any
packages/api/src/types/webhook.ts:L38 — Notification.data: any

Fix: Change data: any to data: unknown on HTTP client methods. Change retry callbacks to accept unknown. Replace [key: string]: any with Record<string, unknown> across type definitions.

---

H3: Integration Tests Silently Pass When Skipped

Impact: When a prerequisite test fails, dependent tests report as passed (green) in CI despite executing zero assertions. 23 instances across 4 test files.

Example:
packages/api/__tests__/integration/customerService.test.ts:L67-L70

if (!createdCustomerId) {
  console.warn("Skipping: createdCustomerId not available from previous test");
  return; // Jest sees: test PASSED ✓ (no assertions failed)
}

Affected files:

• paymentMethodService.test.ts — 12 instances
• webhookService.test.ts — 7 instances
• customerService.test.ts — 2 instances
• transferService.test.ts — 2 instances

Fix: Use beforeAll for setup with proper error propagation, and it.skip() or conditional describe blocks for dependent tests.

---

H4: Token-Fetch Pattern Duplicated 35 Times

Impact: Maintainability nightmare. Any change to auth handling requires editing 35 locations across 11 files.

Example (first occurrence):
packages/api/src/services/customerService.ts:L24-L27

const token = await client.getAccessToken();
if (!token.ok) {
  return err(token.error);
}

Distribution: webhookService.ts (8), planService.ts (6), customerService.ts (4), paymentMethodService.ts (4), transactionService.ts (3), providerService.ts (3), paymentService.ts (3), transferService.ts (1), refundService.ts (1), buyService.ts (1), sellService.ts (1).

Fix: Extract a withAuth helper and an authHeaders builder into packages/api/src/services/helpers.ts:

export const withAuth = async <T>(
  client: OakClient,
  fn: (token: string) => Promise<Result<T, OakError>>
): Promise<Result<T, OakError>> => {
  const token = await client.getAccessToken();
  if (!token.ok) return err(token.error);
  return fn(token.value);
};

---

H5: CI Doesn't Enforce Linting

Impact: Lint rules exist but violations never block PRs or builds.

Location:
.github/workflows/ci.yml:L62

continue-on-error: true

Fix: Remove continue-on-error: true.

---

H6: Manual Test File Committed With 15+ Hardcoded UUIDs

Impact: Repository pollution, potential exposure of sandbox entity identifiers.

Location:
packages/api/test-sdk.ts — 200+ lines of commented-out code with UUIDs at lines 39, 49, 65, 74, 92-93, 102-103, 143, 169, 185, 199, 226, 233, 238.

Fix: git rm packages/api/test-sdk.ts and add to .gitignore.

---

Medium Severity Issues

M1: ReturnType<typeof> Used Instead of Named Interfaces

Impact: The product facade type is coupled to factory implementations rather than the explicitly defined service interfaces.

Location:
packages/api/src/products/crowdsplit/index.ts:L15-L26

export interface CrowdsplitProduct {
  customers: ReturnType<typeof createCustomerService>;
  payments: ReturnType<typeof createPaymentService>;
  // ...
}

Fix: Replace with customers: CustomerService, payments: PaymentService, etc.

---

M2: Response Types Extend Request Union Types

Impact: Complex distributed intersection types that are hard to narrow and confusing for consumers.

Locations:
packages/api/src/types/payment.ts:L154 — export type Request = MercadoPagoRequest | PagarMeRequest | StripeRequest
packages/api/src/types/payment.ts:L159-L166 — export type Transaction = Request & { id, status, ... }
packages/api/src/types/transfer.ts:L63 — export type Request = BrlaRequest | PagarMeRequest | StripeRequest
packages/api/src/types/transfer.ts:L68-L75 — export type Data = Request & { id, status, ... }

Fix: Define Transaction and Transfer.Data as standalone interfaces with all fields explicitly declared.

---

M3: Dead Exported Function getErrorBodyMessage

Impact: Dead code that ships to consumers, increases bundle size, and creates maintenance burden.

Location:
packages/api/src/services/helpers.ts:L28

Grep confirmed: zero imports or calls from any file under src/. Only referenced in test files.

Fix: Delete the function and its tests.

---

M4: Dual Lockfiles

Impact: Mixed package manager signals. The project uses pnpm but npm lockfiles are committed.

Files:

• /sdk/package-lock.json
• /sdk/packages/api/package-lock.json
• /sdk/pnpm-lock.yaml

Fix: git rm package-lock.json packages/api/package-lock.json and add package-lock.json to .gitignore.

---

M5: No Webhook Signature Verification Utility

Impact: The SDK provides a secret field on webhook registration but no utility to verify incoming webhook payloads. Consumers have no guidance on HMAC verification, which is a critical security concern for payment webhooks.

Location:
packages/api/src/types/webhook.ts:L11 — secret: string in Data interface

Fix: Create packages/api/src/utils/webhookVerifier.ts with a verifyWebhookSignature(payload, signature, secret) function. Export from index.ts.

---

M6: Trailing Slash Inconsistency on Payment URL

Impact: Potential routing differences depending on server configuration. Inconsistent with all other services.

Location:
packages/api/src/services/paymentService.ts:L23

`${client.config.baseUrl}/api/v1/payments/`  // trailing slash

All other services (customers, transactions, webhooks, plans, etc.) omit the trailing slash.

Fix: Remove trailing slash, or add a comment explaining why it's required if the API demands it.

---

M7: Unpinned npm@latest in Release CI

Impact: Non-deterministic CI. A breaking npm release could fail publishing.

Location:
.github/workflows/release.yml:L55

npm install -g npm@latest

Fix: Pin to a specific version, e.g., npm install -g npm@10.9.2.

---

M8: Jest Type Version Mismatch

Impact: Potential type errors between the Jest runner (v30) and TypeScript transformer (ts-jest v29).

Location:
packages/api/package.json:L38 — "@types/jest": "^30.0.0"
packages/api/package.json:L41 — "ts-jest": "^29.4.1"

Fix: Upgrade ts-jest to ^30.x or downgrade @types/jest to ^29.5.0.

---

M9: contracts Package Is a Published Placeholder

Impact: A package that does nothing is published to npm, runs in CI, and adds noise to the monorepo.

Location:
packages/contracts/src/index.ts:L8-L10

export function placeholder(): void {
  // This ensures coverage is calculated
}

Fix: Exclude from CI/publish until it has real content, or remove from the workspace.

---

Low Severity Issues

L1: createAuthService Is a Zero-Value Wrapper

Location:
packages/api/src/services/authService.ts:L1-L15

Wraps client.grantToken() and client.getAccessToken() with zero transformation. Consumers already have these methods on the client.

Fix: Remove the service and its usage in the product facade.

---

L2: 36 Hardcoded URL Constructions Across Services

Every service builds URLs with inline template literals: ${client.config.baseUrl}/api/v1/.... Any API versioning change requires a global search-replace across all 11 service files.

Fix: Extract a buildUrl(baseUrl, ...segments) helper into helpers.ts.

---

L3: SDKConfig Is an Unused Type Alias

Location:
packages/api/src/types/config.ts:L3

export type SDKConfig = OakClientConfig;

Never imported or used anywhere in the codebase. Dead code.

Fix: Delete the file and its re-export from types/index.ts.

---

L4: experimentalDecorators Enabled for Dead Production Code

Location:
packages/api/tsconfig.json:L13-L14

"experimentalDecorators": true,
"emitDecoratorMetadata": true

packages/api/src/decorators/sandboxOnly.ts:L49-L95 — SandboxOnly decorator is defined but never applied in any production source file. Only used in test files.

Fix: Remove the class decorator. Keep only sandboxOnlyFn (the function wrapper). Remove both tsconfig flags.

---

L5: Customer.Data Has Ambiguous id/customer_id Fields

Location:
packages/api/src/types/customer.ts:L21-L22

id?: string;
customer_id?: string;

Both optional, no documentation explaining when each is populated.

Fix: Add TSDoc comments explaining the difference, or consolidate into one field if they're always equivalent.

---

L6: Engine/PackageManager/Prepublish Inconsistencies

Locations:
package.json:L25 — packageManager: "pnpm@10.17.1+sha512..."
package.json:L28 — engines.pnpm: ">=8.0.0" (allows incompatible v8/v9)
packages/api/package.json:L30 — prepublishOnly: "npm run build" (uses npm in a pnpm project)

Fix:

• Change engines.pnpm to ">=10.0.0"
• Change prepublishOnly to "pnpm run build"

---

Appendix: Validation Results

All findings were independently verified by 4 Claude Opus 4.6 agents running in parallel, each reviewing a different severity category against the actual source code.

Agent	Scope	Claims Checked	TRUE	PARTIALLY TRUE	FALSE
1	Critical bugs	4	3	1	0
2	High severity	6	6	0	0
3	Medium severity	9	9	0	0
4	Low + security	9	9	0	0
Total		28	27	1	0

The one partial finding (C1 test masking): Agent 1 found the claim was actually understated. Both expires_in: 3300000 and expires_in: 1 mask the bug through different mechanisms. Fixing the multiplication would break both tests.

Corrections from agents:

• Token duplication: claimed ~30, actual count 35
• Hardcoded URLs: claimed ~12 services, actual count 36 constructions
• Silent test skips: actual count 23 instances with zero test.skip() usage
• Singular endpoints: /buy and /sell are also singular, not just /transfer
• SandboxOnly decorator: never applied in production source — fully dead code
• SDKConfig: never imported anywhere — dead type and dead file