diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md new file mode 100644 index 00000000..54461707 --- /dev/null +++ b/.claude/commands/code-review.md @@ -0,0 +1,210 @@ +--- +description: Full code review and smell detection for the Skyflow Node SDK — correctness, SDK patterns, async, duplication, dead code, magic values, naming, and complexity. +constraints: + - "NEVER edit, create, or delete any file under src/_generated_/. Filter it out at the git diff step with: git diff --name-only | grep -v '_generated_'. If a finding relates to generated code, report it as an observation only." +--- + +You are a senior engineer reviewing the Skyflow Node SDK — a TypeScript client library for Skyflow's data privacy vault. Perform a combined correctness review and code smell analysis on the target. + +## Mode selection — pick exactly one + +Inspect `$ARGUMENTS` and choose the review mode: + +| Argument | Mode | +|---|---| +| `full review` (case-insensitive) | **Full** — scan all files under `src/` recursively | +| A file or directory path (starts with `/`, `./`, `src/`, `samples/`, `test/`, etc.) | **Path** — review only that file or directory | +| Empty / anything else | **Branch** — review files changed on the current branch vs `main` | + +### Full mode +Scan all files under `src/` recursively, grouped by layer: +``` +src/vault/controller/ +src/vault/model/ +src/utils/validations/ +src/utils/ +src/service-account/ +src/error/ +src/index.ts +``` +Read each file fully before reporting findings. Work layer by layer — controllers first, then validators, then model, then utilities. + +### Path mode +Restrict the scan to the path given in `$ARGUMENTS`. Read every file under that path before reporting. + +### Branch mode +Review all files changed on the current branch vs main: +``` +git diff main...HEAD --name-only | grep -v '_generated_' +git diff main...HEAD +git log main...HEAD --oneline +``` +Summarise what the branch does in 2–3 sentences. List files grouped by layer: model / controller / validation / tests / samples / exports / docs. + +> **IMPORTANT — Generated code boundary** +> `src/_generated_/` contains Fern-generated REST client code. **Never modify any file inside `src/_generated_/`**. If a finding relates to generated code, report it as an observation only — do not edit, create, or delete any file under that path. + +--- + +## What to review + +### Basic checks +- Identify issues and unhandled edge cases that can break the code at runtime. + +### 1. Request / Response / Options pattern +- Every public operation must use dedicated classes: `XxxRequest`, `XxxOptions`, `XxxResponse` +- Options must use setters (`options.setFoo(val)`), never direct property assignment +- Response objects must be readonly data containers — no business logic inside them +- Flag any operation that accepts or returns plain objects instead of typed classes + +### 2. Validation completeness +- Every public method must call its `validateXxx()` function from `src/utils/validations/index.ts` **before** any API call +- Validators must throw `SkyflowError` with a code from `src/error/codes/index.ts` +- Validators must call `printLog` with `MessageType.ERROR` before throwing — without the log, there is no trace of the failure in production +- Check for missing edge cases: null/undefined inputs, empty strings, wrong types, empty arrays, negative numbers +- No truthy guard `if (!x)` for values where `0`, `""`, or `false` could be valid — use `=== undefined || === null` instead +- Consistent null guard style across all validators — no mixing of `!x`, `=== undefined`, `=== null` +- Flag validators that both validate and transform data — these should be separate concerns + +### 3. Error handling +- All `async` methods must have `try/catch` that rejects with the original error or a `SkyflowError` +- Never swallow errors silently +- No-op `catch` blocks (`catch (e) { throw e }` with no added value) — flag and remove +- `catch` blocks that wrap in `SkyflowError` must preserve the original error message or cause — losing it makes production debugging impossible +- `catch` blocks that catch a broad `Error` but only handle one specific subtype, silently ignoring all others — every unhandled case must at minimum re-throw +- `SkyflowError` must use the correct `http_code` for the failure type (400 for validation, 500 for server) + +### 4. Async and Promise patterns +- No `new Promise(async (resolve, reject) => …)` — the `async` executor swallows rejections; use a plain executor or a top-level `async` function +- `async` functions that never use `await` — the `async` keyword is misleading; remove it +- Every `resolve()` call inside a `.then()` chain must have a `return` to prevent fall-through +- Floating promises: async function called without `await` and without `.catch()` +- Every async state machine (polling, retry, status tracking) must handle **all** possible status values — an unhandled value must resolve, reject, or throw; never silently do nothing (leaves the Promise hanging forever) +- No `fs.readFileSync` / `fs.writeFileSync` — use `fs.promises.*` +- Instance state (`this.xxx`) must not be mutated with per-call configuration — causes state leakage when the same controller is reused +- Promise constructor wrapping an already-thenable value + +### 5. TypeScript quality +- No `var` — use `const` or `let` +- No untyped `any` outside `src/_generated_/` +- No `Function` type — use explicit signatures like `(value: T) => void` +- Optional chaining applied consistently: `a?.b?.c` not `a?.b.c` +- No duplicate null-coalescing typos (`x ?? x ?? y`) — the second expression is dead code +- `as SomeType` casts that bypass type safety — especially `as unknown as X` +- All public controller and utility methods must have explicit return type annotations + +### 6. State and side effects +- Instance variables must not be mutated inside per-call methods +- Use local variables for per-call configuration, not `this.xxx = ...` in method bodies + +### 7. Exports and public API surface +- All public types/classes must be exported from `src/index.ts` +- Internal helpers must not be exported +- No circular imports + +### 8. Logging +- Use `printLog(message, MessageType.LOG | ERROR, this.client.getLogLevel())` — never `console.log` +- Sensitive values (tokens, credentials, PII) must never appear in log messages + +### 9. Naming conventions + +| Identifier type | Required style | Example | +|---|---|---| +| Field / variable / parameter | `camelCase` | `vaultId`, `tokenUri` | +| Constant | `UPPER_SNAKE_CASE` | `SDK_METRICS_HEADER_KEY` | +| Class / Interface / Type | `PascalCase` | `InsertRequest`, `VaultConfig` | +| Source file | `camelCase.ts` or `kebab-case.ts` | `deidentify-text.ts` | + +**Acronym rule — title-case, not ALL-CAPS:** +- `ID` → `Id` (e.g. `skyflowId`, not `skyflowID`) +- `URI` → `Uri` (e.g. `tokenUri`, not `tokenURI`) +- `URL` → `Url` (e.g. `callbackUrl`, not `callbackURL`) +- `API` → `Api` (e.g. `apiKey`, not `APIKey`) +- Exception: standalone environment variable names follow OS convention (`SKYFLOW_ID`, `TOKEN_URI`) + +**What to flag:** +- Vague variable names (`req`, `res`, `data`, `obj`, `temp`) in non-trivial logic — name after the domain concept +- Method names that don't match their return type (e.g. a `buildXxx` that returns a `Promise`) +- Typos in published class, enum, or type names — flag as breaking-change risk +- Any `snake_case` field or method name in the public API +- Mixed conventions within the same class or module + +### 10. Function size and complexity +- Flag any function exceeding 50 lines — include the actual line count +- Flag nesting deeper than 3 levels — suggest early returns or extracted helpers +- Flag long `if (valid) { ...entire body... }` blocks where an inverted guard + early return would flatten the code + +### 11. Duplication +- Identical or near-identical object literals constructed in multiple methods — extract to a shared helper +- The same validation logic (type check + array check + element check) copy-pasted across multiple `validateXxx` functions — extract a reusable validator +- Repeated `try { ... } catch (e) { throw new SkyflowError(...) }` wrappers with the same error code — consolidate +- Builder methods that follow the exact same structure (read file → encode → populate fields) duplicated per file type — extract a shared builder + +### 12. Dead / no-op code +- `catch (error) { throw error; }` — no-op catch; remove it, it obscures the real stack trace +- Redundant null-coalescing: `x ?? x` where both sides are identical expressions +- Unused imports — check for TS6133 unused variable errors +- Variables assigned once and only used in the assignment expression + +### 13. Magic values +- Hardcoded status strings (`"IN_PROGRESS"`, `"SUCCESS"`, `"FAILED"`, `"UNKNOWN"`) scattered in controller logic — define typed constants or use enums +- Hardcoded numeric limits (timeouts, max sizes, retry counts) with no named constant explaining the constraint +- Hardcoded file extensions or MIME type strings in routing/switch logic — use enum values from the generated API types +- Hardcoded HTTP codes (200, 400, 500) in business logic — use `SKYFLOW_ERROR_CODE` constants + +### 14. Single Responsibility +- Controller methods that mix transport concerns (HTTP call) with business logic (polling, file writing, response transformation) — each concern belongs in its own private method +- `handleRequest` or equivalent dispatcher methods that switch on a string type to apply post-processing — the post-processing belongs in the callers, not the dispatcher +- Response parsers that also write to the file system — parsing and I/O should be separate + +### 15. Cross-cutting consistency +- All options classes must use private fields + getters + setters consistently +- Controller methods must use a consistent async style (`async/await` vs `.then()/.catch()`) — flag inconsistency within the same controller +- All validators must call `printLog` before throwing — flag any that throw directly without logging +- All public methods must have explicit return type annotations + +--- + +## Output format + +### Part 1 — Findings (grouped by file) + +For each issue: + +``` +[SEVERITY] file:line — Description + Problem: + Fix: +``` + +Severity levels: **CRITICAL** | **BUG** | **EDGE CASE** | **QUALITY** | **SMELL** + +### Part 2 — Tech-debt summary (grouped by category) + +| Category | Findings | Debt level | +|---|---|---| +| Async / Promise | n | High / Medium / Low | +| TypeScript quality | n | High / Medium / Low | +| Duplication | n | High / Medium / Low | +| Dead code | n | High / Medium / Low | +| Magic values | n | High / Medium / Low | +| Single Responsibility | n | High / Medium / Low | +| Naming | n | High / Medium / Low | +| Consistency | n | High / Medium / Low | + +### Part 3 — Summary + +**Findings table** +| Severity | Count | +|---|---| +| CRITICAL | n | +| BUG | n | +| EDGE CASE | n | +| QUALITY | n | +| SMELL | n | +| **Total** | n | + +**Top 5 highest-priority fixes** (by risk, not count) + +**Verdict**: `APPROVE` | `APPROVE WITH FIXES` | `REQUEST CHANGES` +One sentence explaining the verdict. diff --git a/.claude/commands/code-security.md b/.claude/commands/code-security.md new file mode 100644 index 00000000..1a371d08 --- /dev/null +++ b/.claude/commands/code-security.md @@ -0,0 +1,93 @@ +--- +description: Security audit for the Skyflow Node SDK — credentials, injection, file I/O, deps, auth lifecycle, HTTP safety. +constraints: + - "NEVER edit, create, or delete any file under src/_generated_/. Filter it out at the git diff step with: git diff --name-only | grep -v '_generated_'. If a finding relates to generated code, report it as an observation only." +--- + +You are a security engineer auditing the Skyflow Node SDK — a TypeScript library that handles sensitive PII and credentials. Perform a security review on the target: $ARGUMENTS + +If no argument is given, scan all files changed on the current branch vs main: +``` +git diff main...HEAD --name-only +``` + +> **IMPORTANT — Generated code boundary** +> `src/_generated_/` contains Fern-generated REST client code. **Never modify any file inside `src/_generated_/`**. If a security finding relates to generated code, report it as an observation only — do not edit, create, or delete any file under that path. + +Read each target file fully before reporting findings. For any file that touches authentication, file I/O, or HTTP calls, also read the related controller and validation file. + +--- + +## Security checks + +### 1. Credential and token exposure +- Bearer tokens, API keys, and service account credentials must **never** appear in: + - Log messages (even at DEBUG level) + - Error messages or `SkyflowError` details + - Response objects returned to callers + - File names or paths written to disk +- Check that `getBearerToken()` results are used immediately and not stored on `this` or in module-level variables +- Verify that credentials passed in `Credentials` objects are not echoed back in any response or log +- Check that error catch blocks don't accidentally log the full error object (which may contain auth headers) + +### 2. Input validation and injection +- All user-supplied strings passed to file system APIs (`readFileSync`, `writeFileSync`, `mkdirSync`, `existsSync`, `lstatSync`, `readdirSync`) must be validated before use — check for path traversal (`../`), null bytes, and absolute paths escaping expected directories +- Regex patterns supplied by users (e.g. allow/restrict lists) must be wrapped in `try/catch` on `new RegExp()` construction — malformed or catastrophic patterns cause ReDoS or crashes +- Any `Buffer.from(x, 'base64')` call on data from an API response or user input must have a size check before decoding — unbounded base64 can cause OOM +- Ensure no user-controlled string reaches `eval`, `new Function()`, `child_process.exec/spawn`, or dynamic `require()`/`import()` +- SQL-like query strings passed to the vault API (e.g. `QueryRequest`) must be treated as opaque — verify no string interpolation of user data happens server-side by checking that the SDK passes values as parameters, not concatenated strings + +### 3. File system security +- User-supplied directory paths (e.g. output directories) must be validated to be existing, accessible directories — the SDK must not create arbitrary directory trees from user input +- Output file paths must be constructed with `path.join(validatedBaseDir, sanitizedFileName)` — never raw string concatenation +- File name components that come from API responses (extensions, type names) must be sanitized before use in a path — they could contain `../`, leading dots, or special characters if the response is tampered with +- Temporary or intermediate files written during processing must be cleaned up on both success and error paths + +### 4. Dependency and supply chain +- Run `npm audit --json` and report all HIGH and CRITICAL severity findings with CVE IDs +- Check `package.json` for any dependency that has not been pinned to a specific version (floating `^` or `~` versions on security-sensitive packages) +- Flag any dynamic `require()` or `import()` where the module path is derived from user input or environment variables + +### 5. Error message information leakage +- `SkyflowError` messages surfaced to callers must not include: internal file system paths, raw stack traces, server-side SQL or query details, or internal service names +- Catch blocks that log errors must use `removeSDKVersion(error.message)` — never `error.stack` in production log paths +- HTTP response bodies from API errors must be stripped of internal server details (stack traces, internal hostnames) before being wrapped in `SkyflowError` +- Ensure `SkyflowError.error.details` only contains information safe to expose to SDK consumers + +### 6. Authentication and token lifecycle +- Bearer tokens fetched via `getBearerToken()` must be checked as non-expired immediately before the API call +- Check for TOCTOU (time-of-check-time-of-use): if token validity is checked in a validator but the actual call happens later, a token could expire in between +- API key validation (`isValidAPIKey()`) must check format meaningfully — not just truthiness +- Service account JWT generation must use a secure signing algorithm — verify RS256 or ES256, never HS256 with a weak secret +- Check that no credential type silently falls back to a less secure method without logging a warning + +### 7. HTTP and network security +- All HTTP calls made through the generated REST client (`src/_generated_/`) must use HTTPS — flag any `http://` hardcoded URLs +- Auth headers must be forwarded correctly and not leaked in redirect responses +- Verify that `x-request-id` and other response headers are read but not echoed back in subsequent requests (header injection risk) +- Check that connection timeouts are configured — no indefinite blocking calls + +### 8. Generated code (`src/_generated_/`) +- Do not modify — but do verify it uses HTTPS only, applies auth headers from the SDK layer, and does not hardcode environment-specific endpoints +- Flag if the generated client silently retries on auth failure without refreshing the token (could mask credential issues) + +--- + +## Output format + +For each finding: + +``` +[SEVERITY] file:line — Finding title + Risk: + Trigger: + Fix: + CWE: +``` + +Severity: **CRITICAL** | **HIGH** | **MEDIUM** | **LOW** | **INFO** + +End with: +1. Overall risk rating (Critical / High / Medium / Low) +2. Top 3 highest-priority fixes +3. Whether the code is safe to ship as-is diff --git a/.claude/commands/sdk-sample.md b/.claude/commands/sdk-sample.md new file mode 100644 index 00000000..57bfa584 --- /dev/null +++ b/.claude/commands/sdk-sample.md @@ -0,0 +1,96 @@ +--- +description: Generate a new Skyflow Node SDK sample file demonstrating a specific feature with proper patterns, error handling, and type safety. +constraints: + - "NEVER edit, create, or delete any file under src/_generated_/. Samples must import only from 'skyflow-node' (the public package), never directly from src/_generated_/." +--- + +Create a new Skyflow Node SDK sample. Feature to demonstrate: $ARGUMENTS + +> **IMPORTANT — Generated code boundary** +> `src/_generated_/` contains Fern-generated REST client code. **Never modify any file inside `src/_generated_/`**. Samples must import only from `'skyflow-node'` (the public package), never directly from `src/_generated_/`. + +Read the following before writing anything: +1. An existing sample in `samples/vault-api/` to understand structure and style +2. `src/index.ts` to see what is exported +3. `samples/package.json` to understand available imports + +--- + +## Sample requirements + +### File location +- Vault operations → `samples/vault-api/.ts` +- Service account / auth → `samples/service-account/.ts` +- Detect / PII operations → `samples/vault-api/detect-.ts` + +### Required structure (in this order) +1. Import block — only from `'skyflow-node'` (not from `src/`) +2. Credentials configuration with `Credentials` type +3. `VaultConfig` (and `ConnectionConfig` if needed) +4. `SkyflowConfig` with `logLevel: LogLevel.ERROR` +5. `Skyflow` client initialization +6. Request object construction using the appropriate `XxxRequest` class +7. Options object using setters (not direct property assignment) +8. `async` function wrapping the SDK call +9. `try/catch` with `SkyflowError` check +10. `main()` call at the bottom + +### Code style rules +- Use realistic placeholder values (`'your-vault-id'`, `'your-api-key'`, table names matching the feature) +- Every non-obvious step gets a short inline comment explaining WHY (not what) +- Show the full error handling pattern — both `SkyflowError` and generic `Error` +- Keep the sample under 100 lines — if it needs more, split into focused examples +- No `console.log` on success paths other than printing the response + +### Template +```typescript +import Skyflow, { + Credentials, + LogLevel, + SkyflowConfig, + // add relevant Request/Options/Response imports +} from 'skyflow-node'; + +const credentials: Credentials = { + apiKey: 'your-skyflow-api-key', +}; + +const primaryVaultConfig = { + vaultId: 'your-vault-id', + clusterId: 'your-cluster-id', + credentials, +}; + +const skyflowConfig: SkyflowConfig = { + vaultConfigs: [primaryVaultConfig], + logLevel: LogLevel.ERROR, +}; + +const skyflowClient = new Skyflow(skyflowConfig); + +async function main() { + try { + // Build request + // Execute + // Log response + } catch (error) { + if (error instanceof SkyflowError) { + console.error('Skyflow error:', { + code: error.error?.http_code, + message: error.message, + details: error.error?.details, + }); + } else { + console.error('Unexpected error:', error); + } + } +} + +main(); +``` + +--- + +After creating the file: +1. Run `npx tsc --noEmit --project samples/tsconfig.json 2>/dev/null || npx tsc --noEmit` to verify it type-checks +2. Report the file path created and any type errors to fix diff --git a/.claude/commands/test.md b/.claude/commands/test.md new file mode 100644 index 00000000..4f8e9afa --- /dev/null +++ b/.claude/commands/test.md @@ -0,0 +1,122 @@ +--- +description: Full quality pipeline for the Skyflow Node SDK — type check, lint, build, tests, coverage, and edge case analysis. +constraints: + - "NEVER edit, create, or delete any file under src/_generated_/. Always filter generated files before passing any file list to lint or prettier: git diff --name-only | grep -E '\\.(ts|js)$' | grep -v '_generated_'. If analysis touches generated code, report it as an observation only." +--- + +Run the full Skyflow Node SDK quality pipeline and report results. Target (optional — specific test file or pattern): $ARGUMENTS + +> **IMPORTANT — Generated code boundary** +> `src/_generated_/` contains Fern-generated REST client code. **Never modify any file inside `src/_generated_/`**. If analysis or test generation touches generated code, report it as an observation only — do not edit, create, or delete any file under that path. + +Execute the following steps in order. Stop and report clearly if any step fails. + +--- + +## Step 1 — TypeScript type check +```bash +npx tsc --noEmit +``` +Report any type errors. These indicate broken contracts in the SDK's type system. + +--- + +## Step 2 — Lint + +Lint only the files that have changed, not the entire repo. + +First, get the list of changed `.ts` / `.js` files: +```bash +git diff --name-only HEAD | grep -E '\.(ts|js)$' +``` +If that returns nothing (e.g. all changes are staged or the target is a specific file from `$ARGUMENTS`), fall back to: +```bash +git diff --name-only --cached | grep -E '\.(ts|js)$' +``` +If `$ARGUMENTS` is a file path, use that file directly. + +Then run prettier and eslint only on those files: +```bash +npx prettier --check +npx eslint +``` + +Report any formatting or ESLint violations, including the file name and line number for each. + +--- + +## Step 3 — Build +```bash +npm run build +``` +Verify the TypeScript compiles cleanly to `lib/`. Report any build errors. + +--- + +## Step 4 — Tests + +If `$ARGUMENTS` is provided, run only matching tests: +```bash +npx jest "$ARGUMENTS" --coverage --verbose +``` + +Otherwise run the full suite: +```bash +npm test +``` + +--- + +## Step 5 — Coverage analysis + +After tests complete, analyze coverage output: +- Report overall line / branch / function / statement coverage % +- **Line coverage must be 100%** for public interfaces in `src/` — flag any file below 100% line coverage +- Flag any file across the entire `src/` directory with branch coverage above or equal 90% +- For every flagged file, list the exact uncovered line numbers and what scenario they represent + +--- + +## Step 6 — Edge case analysis and test generation + +For every file across the entire `src/` directory that has branch coverage below 80%, or for the target file if `$ARGUMENTS` is provided: + +### 6a — Identify uncovered edge cases +Read the source file and the corresponding test file. For each uncovered branch or line, identify what scenario is missing. Focus on: +- `null` / `undefined` inputs to public methods +- Empty strings, empty arrays, zero/negative numbers +- Async state machine values not exercised (e.g. unhandled poll statuses) +- Error paths that are never triggered in tests +- Boundary conditions (e.g. exactly at a limit vs. one over) +- Concurrent or reuse scenarios (same controller called twice) + +List each gap as: +``` +UNCOVERED: : — +``` + +### 6b — Write the missing unit tests +For each identified gap, write a concrete Jest test case using the project's conventions: +- Test files live in `test/vault/controller/` or `test/utils/` and use `.test.js` extension +- Follow the existing `describe` / `it('should ...')` structure in that file +- Mock external dependencies (API calls, `getBearerToken`) the same way the existing tests do +- Each test must: arrange inputs → act (call the method) → assert the outcome (resolve value or rejection error code) + +Output the new tests as a ready-to-paste code block, clearly labelled with the target test file path. Do **not** remove or modify existing tests. + +--- + +## Step 7 — Report + +Produce a summary table: + +| Step | Status | Notes | +|------|--------|-------| +| Type check | PASS/FAIL | error count | +| Lint | PASS/FAIL | violation count | +| Build | PASS/FAIL | warning count | +| Tests | PASS/FAIL | X passed, Y failed, Z skipped | +| Coverage | % | files below threshold | +| Edge cases | n found | n tests written | + +End with: **READY TO MERGE** or **NEEDS FIXES** with a bullet list of what must be fixed. diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000..8cabd5ab --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,54 @@ +{ + "hooks": { + "PostToolUse": [ + { + "matcher": "Edit|Write", + "hooks": [ + { + "type": "command", + "command": "python3 -c \"\nimport sys, json, subprocess\nd = json.load(sys.stdin)\nf = d.get('tool_input', {}).get('file_path', d.get('file_path', ''))\nif f and f.endswith(('.ts', '.js')) and '_generated_' not in f:\n subprocess.run(['npx', 'prettier', '--write', f], capture_output=True)\n subprocess.run(['npx', 'eslint', '--fix', f], capture_output=True)\n\"" + } + ] + }, + { + "matcher": "Edit|Write", + "hooks": [ + { + "type": "command", + "command": "cd /Users/bhartisagar/Desktop/skyflow_work/skyflow-node && npx tsc --noEmit 2>&1 | tail -20" + } + ] + } + ], + "PreToolUse": [ + { + "matcher": "Edit|Write", + "hooks": [ + { + "type": "command", + "command": "python3 -c \"import sys,json; d=json.load(sys.stdin); p=d.get('tool_input',{}).get('file_path',d.get('file_path','')); banned='_generated_'; (sys.stderr.write('BLOCKED: Fern-generated code — do not edit manually\\n'), sys.exit(2)) if banned in p else sys.exit(0)\"" + } + ] + } + ], + "Stop": [ + { + "hooks": [ + { + "type": "command", + "command": "osascript -e 'display notification \"Claude finished\" with title \"Claude Code\"' 2>/dev/null || true" + } + ] + } + ] + }, + "permissions": { + "allow": [ + "Bash(node -e ' *)" + ], + "deny": [ + "Edit(src/_generated_/**)", + "Write(src/_generated_/**)" + ] + } +} diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md new file mode 100644 index 00000000..ac16411f --- /dev/null +++ b/.claude/skills/pr-review/SKILL.md @@ -0,0 +1,209 @@ +--- +name: pr-review +description: Full pull-request review for the Skyflow Node SDK. Covers code quality, SDK patterns, tests, docs, security, breaking-change detection, naming conventions, edge cases, and coverage. +constraints: + - "NEVER edit, create, or delete any file under src/_generated_/. Filter it out at the git diff step with: git diff --name-only | grep -v '_generated_'. If a finding relates to generated code, report it as an observation only." +--- + +You are a senior engineer conducting a pull-request review for the Skyflow Node SDK — a TypeScript client library for Skyflow's data privacy vault. + +Review the target PR or branch: $ARGUMENTS + +If no argument is given, diff the current branch against main: +``` +git diff main...HEAD +git diff main...HEAD --name-only +git log main...HEAD --oneline +``` + +> **IMPORTANT — Generated code boundary** +> `src/_generated_/` contains Fern-generated REST client code. **Never modify any file inside `src/_generated_/`**. If a finding relates to generated code, report it as an observation only — do not edit, create, or delete any file under that path. + +--- + +## Step 1 — Understand the change + +- Summarise what the PR does in 2–3 sentences. +- List every file changed, grouped by layer: model / controller / validation / tests / samples / exports / docs. +- Identify the primary change type: **new feature** | **bug fix** | **refactor** | **docs** | **dependency update**. + +--- + +## Step 2 — Breaking-change detection + +- [ ] No public class or method removed from `src/index.ts` +- [ ] No public method signature changed (parameter added/removed/reordered, return type changed) +- [ ] No Request / Options / Response class field renamed or removed +- [ ] No error code value changed (string/number identity) +- [ ] No `LogLevel` enum value changed +- [ ] If a breaking change exists: is it intentional and documented in `CHANGELOG.md` / `docs/migrate_to_v2.md`? + +Flag any breaking change as **BREAKING** even if intentional. + +--- + +## Step 3 — SDK pattern compliance + +### Request / Response / Options +- [ ] Every new public operation uses `XxxRequest`, `XxxOptions`, `XxxResponse` classes +- [ ] Options use setters (`options.setFoo(val)`), never direct property assignment +- [ ] Response objects are readonly data containers — no business logic + +### Validation +- [ ] Every public method calls `validateXxx()` from `src/utils/validations/index.ts` **before** any API call +- [ ] Validators throw `SkyflowError` with a code from `src/error/codes/index.ts` +- [ ] `printLog(…, MessageType.ERROR, logLevel)` called **before** throwing — never after +- [ ] Edge cases covered: null/undefined, empty string, empty array, wrong type, negative numbers +- [ ] No truthy guard `if (!x)` for values where `0`, `""`, or `false` could be valid — use `=== undefined || === null` instead +- [ ] Consistent null guard style across all validators in the changed files — no mixing of `!x`, `=== undefined`, `=== null` + +### Error handling +- [ ] All `async` methods have `try/catch` that rejects with a `SkyflowError` or re-throws the original +- [ ] No silent error swallowing (`catch` block that does nothing or only logs) +- [ ] No no-op catch (`catch (e) { throw e }` with no added value) +- [ ] `catch` blocks that wrap in `SkyflowError` must preserve the original error message or cause — losing it makes production debugging impossible + +### Async / Promise patterns +- [ ] No `new Promise(async (resolve, reject) => …)` anti-pattern +- [ ] Every `resolve()` inside a `.then()` chain is preceded by `return` +- [ ] All API status/state values are handled — no unhandled enum member that leaves a Promise hanging +- [ ] No `fs.readFileSync` / `fs.writeFileSync` — use `fs.promises.*` + +### TypeScript quality +- [ ] No `var` — use `const` or `let` +- [ ] No untyped `any` outside `src/_generated_/` +- [ ] No `Function` type — use explicit signatures +- [ ] Optional chaining applied consistently: `a?.b?.c` not `a?.b.c` +- [ ] No duplicate null-coalescing typos (`x ?? x ?? y`) +- [ ] All public controller and utility methods have explicit return type annotations + +### Function size and complexity +- [ ] No function exceeds 50 lines — flag with actual line count +- [ ] No nesting deeper than 3 levels — suggest early returns or extracted helpers +- [ ] Long `if (valid) { ...entire body... }` blocks replaced with inverted guard + early return + +### Options class consistency +- [ ] Every options class uses private fields + public getters + public setters — no direct field exposure + +### State / side effects +- [ ] Per-call configuration stored in local variables, not mutated on `this` +- [ ] No instance-level state set inside method bodies that leaks across calls + +--- + +## Step 4 — Exports and public API surface + +- [ ] New public types/classes exported from `src/index.ts` +- [ ] Internal helpers not accidentally exported +- [ ] No circular imports introduced + +--- + +## Step 5 — Tests + +- [ ] Test file exists at `test/vault/controller/.test.js` +- [ ] Happy path covered +- [ ] One test per new error code / validation branch +- [ ] API error response handled +- [ ] Async / polling / retry logic tested if applicable +- [ ] No tests removed without explanation +- [ ] `npm test` passes with no new failures +- [ ] **100% line coverage** on every new or changed file — flag any uncovered lines +- [ ] **Branch coverage ≥ 80%** on every new or changed file + +**Edge case coverage check — for every new/changed source file:** +Read the source and test file together. Flag any of these scenarios that exist in the code but have no test: +- `null` / `undefined` passed to public methods +- Empty string, empty array, zero, negative number inputs +- All possible async status values (e.g. `IN_PROGRESS`, `SUCCESS`, `FAILED`, `UNKNOWN`) each exercised +- Error paths inside `catch` blocks that are never triggered +- Boundary conditions (exactly at a limit vs. one over) +- Same controller/client reused across two consecutive calls (state leakage) + +List each gap as: +``` +UNCOVERED EDGE CASE: : — +``` + +--- + +## Step 6 — Sample code + +- [ ] Sample exists in `samples/vault-api/`, `samples/detect-api/`, or `samples/service-account/` +- [ ] Uses only public exports from `'skyflow-node'` +- [ ] Follows `XxxRequest` / `XxxOptions` / `XxxResponse` pattern +- [ ] Shows `try/catch` with `SkyflowError` handling +- [ ] Type-checks cleanly (`tsc --noEmit`) + +--- + +## Step 7 — Documentation & logging + +- [ ] JSDoc on all new public-facing classes and methods +- [ ] No `console.log` — uses `printLog(…, MessageType, logLevel)` +- [ ] Sensitive values (tokens, credentials, PII) never appear in log messages +- [ ] Spellcheck passes: `npm run spellcheck` +- [ ] `CHANGELOG.md` updated if this is a user-visible change + +--- + +## Step 8 — Security spot-check + +- [ ] No credentials or tokens hard-coded or logged +- [ ] No new external HTTP calls outside the generated REST client +- [ ] No `eval`, `Function()`, or dynamic `require()` with user-controlled input +- [ ] Dependencies added to `package.json` are well-known, maintained packages — flag any unfamiliar ones + +--- + +## Step 9 — Naming conventions (Node / TypeScript) + +| Identifier type | Required style | Example | +|---|---|---| +| Field / variable / parameter | `camelCase` | `vaultId`, `tokenUri` | +| Constant | `UPPER_SNAKE_CASE` | `SDK_METRICS_HEADER_KEY` | +| Class / Interface / Type | `PascalCase` | `InsertRequest`, `VaultConfig` | +| Source file | `camelCase.ts` or `kebab-case.ts` | `deidentify-text.ts` | + +**Acronym rule — title-case, not ALL-CAPS:** +- `ID` → `Id` (e.g. `skyflowId` not `skyflowID`) +- `URI` → `Uri` (e.g. `tokenUri` not `tokenURI`) +- `URL` → `Url` (e.g. `callbackUrl` not `callbackURL`) +- `API` → `Api` (e.g. `apiKey` not `APIKey`) +- Exception: standalone environment variable names stay `ALL_CAPS` (`SKYFLOW_ID`, `TOKEN_URI`) + +- [ ] No ALL-CAPS acronyms in public field, method, parameter, or class names +- [ ] No `snake_case` fields or methods in the public API +- [ ] All classes, interfaces, and types are `PascalCase` +- [ ] All constants are `UPPER_SNAKE_CASE` +- [ ] No mixed conventions within the same class (one method `Id`, another `ID`) + +--- + +## Output format + +For each issue, output: + +``` +[SEVERITY] file:line — Short title + Problem: + Fix: +``` + +Severity levels: **BREAKING** | **CRITICAL** | **BUG** | **EDGE CASE** | **QUALITY** | **NITPICK** + +End with: + +**Summary table** +| Severity | Count | +|---|---| +| BREAKING | n | +| CRITICAL | n | +| BUG | n | +| EDGE CASE | n | +| QUALITY | n | +| NITPICK | n | +| Uncovered edge cases | n | + +**Verdict**: `APPROVE` | `APPROVE WITH FIXES` | `REQUEST CHANGES` +One sentence explaining the verdict.