From 818fc56e6adfd711973e572a5f658e49992f7cc9 Mon Sep 17 00:00:00 2001 From: skyflow-bharti Date: Mon, 1 Jun 2026 10:50:01 +0530 Subject: [PATCH] SK-2708 add claude setup --- .claude/commands/code-review.md | 132 +++++++++++++++++++++++++++ .claude/commands/security-review.md | 96 ++++++++++++++++++++ .claude/skills/code-review/SKILL.md | 134 ++++++++++++++++++++++++++++ 3 files changed, 362 insertions(+) create mode 100644 .claude/commands/code-review.md create mode 100644 .claude/commands/security-review.md create mode 100644 .claude/skills/code-review/SKILL.md diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md new file mode 100644 index 0000000..1cc59ad --- /dev/null +++ b/.claude/commands/code-review.md @@ -0,0 +1,132 @@ +--- +description: SDK-focused Go code review covering API design, idioms, backward compatibility, and test coverage +paths: !/v2/internal/generated/ +--- + +Perform a thorough code review from an **SDK maintainer's perspective**. + +**Always exclude files under `internal/generated/` from the review — these are auto-generated from OpenAPI specs and must not be edited manually.** + +Determine the scope based on the argument `$ARGUMENTS`: + +- **No argument** (default): Review only the files changed on this branch. Run `git diff main...HEAD --name-only` to get the list, filter out any path containing `internal/generated/`, then run `git diff main...HEAD` for the full diff and read each non-generated changed file in full. +- **`full review`**: Review the entire SDK codebase. Run `find v2 -name "*.go" -not -path "*/generated/*" -not -name "*_test.go"` to enumerate all non-generated source files, then read them all. +- **A file or directory path**: Review only that path. If it is a directory, run `find -name "*.go" -not -path "*/generated/*"` to list all Go files within it (excluding generated), then read them all. + +After reading the relevant files, apply the checklist below. + +Structure the review under these headings. Under each heading list findings as `[BLOCKER]`, `[WARNING]`, or `[SUGGESTION]`. Skip any heading that has no findings. + +--- + +## 1. Public API Surface + +- Are new exported types, functions, and methods named following Go conventions? + - PascalCase for exported identifiers; camelCase for unexported + - Acronyms fully capitalised when exported (`VaultID`, `HTTPClient`), fully lower when unexported + - `With*` prefix for functional option constructors + - Receiver names short (1-2 letters), consistent across all methods of a type +- Does any change break existing callers? (renamed export, removed field, changed signature) +- Are new options/params added via functional options (`With*`) rather than positional arguments? +- Do all public operations accept `ctx context.Context` as the first parameter? +- Are new interfaces minimal and named for behaviour (verb/noun — `TokenProvider`, not `ITokenProvider`)? +- Do exported types have godoc comments that describe what, not how? + +## 1a. Cross-SDK Nomenclature (Go-specific) + +Verify these specific naming requirements from the cross-SDK specification. Flag any violation as `[BLOCKER]` since they are part of the public API contract across all Skyflow SDKs. + +**Credential JSON key fields** (in `Credentials` struct and related types): +- Must use `ClientId` (not `ClientID`), `TokenUri` (not `TokenURI`), `KeyId` (not `KeyID`) +- Go struct field tags must produce JSON keys `clientId`, `tokenUri`, `keyId` +- Check all credential-related structs in `utils/common/` and `internal/` + +**Response map keys — universal across all vault operations**: +- Skyflow record identifier key must be `SkyflowId` in all response structs (Insert, Update, Get, Bulk Delete, Query) +- Must NOT be `skyflow_id` (snake_case), `skyflowId` (camelCase), or `SkyflowID` (all-caps acronym) +- Check `InsertResponse`, `UpdateResponse`, `GetResponse`, `QueryResponse`, `BulkDeleteResponse`, and any other response types + +**Query response**: +- Tokenized data field must be `TokenizedData` (not `tokenized_data`, `tokenizedData`, or `TokenizedDataField`) +- `TokenizedData` must always be present in query response records (empty slice, not absent/nil) + +**BearerTokenOptions**: +- Role list field must be `RoleIds` (not `RoleIDs`, `RoleId`, or `Roles`) + +**Always-present response fields**: +- All response structs must always include an `Errors` field (never omit it, even on success — emit empty slice, not nil/absent) +- Verify the field is exported (`Errors []SkyflowError`) and included in JSON serialization + +## 2. Error Handling + +- Are all public methods returning `*error.SkyflowError` (never raw `error` or `nil` on failure)? +- Are errors wrapped with cause context (`Cause: err`)? +- Are new error message strings added to `utils/messages/` rather than inlined? +- Does any error message expose sensitive data (tokens, credentials, PII)? +- Is `panic` used in library code? (never acceptable in an SDK — convert to error return) + +## 3. Go Idioms and Best Practices + +- No naked `return` in long functions; named returns only when they substantially aid clarity +- No use of `fmt.Print*` or `log.*` in library code (use project logger) +- Goroutines: are all goroutines guarded by context cancellation or a done channel? +- No unexported global mutable state (race conditions in concurrent SDK use) +- Nil guards on receivers and map/slice fields before use +- `defer` used correctly — no deferred calls inside loops +- No shadowed `err` variables across if-blocks +- Proper use of `errors.Is` / `errors.As` rather than type-asserting raw errors +- Unused imports, unused variables — run `go vet` confirms clean? + +## 4. Internal vs Public Boundary + +- Is new business logic placed in `internal/` rather than exported packages? +- Are generated files under `internal/generated/` untouched (edited by hand)? +- Does `utils/common/` only contain data types, not logic? + +## 5. Input Validation + +- Are all user-supplied inputs (vault IDs, field names, tokens, file paths) validated in `internal/validation/` before use? +- Validation errors returned via `SkyflowError`, not panics +- No use of raw user input in log messages without sanitisation + +## 6. Test Coverage + +- Does every new exported function/method have at least one `It` block (happy path)? +- Are all validation-error branches covered? +- Are external HTTP calls mocked via `httptest.Server`, not by patching internal helpers? +- New `DescribeTable` entries for any table-driven scenario added? +- Tests compilable and passing: `cd v2 && go test ./...` + +## 7. Backward Compatibility + +- Are any exported identifiers removed or renamed? (BLOCKER — requires major version bump) +- Are struct fields that callers may embed or copy affected? +- Is `go.mod` `require` bumped for a dependency with a breaking change in its own API? + +## 8. Concurrency and Resource Safety + +- HTTP clients and vault configs: are they safely shared across goroutines (no mutation after construction)? +- File uploads: are temporary files and open handles cleaned up even on error paths? +- Context propagation: is `ctx` passed down to every HTTP call and blocking operation? + +--- + +End with a **Summary** table: + +| Category | Blockers | Warnings | Suggestions | +|---|---|---|---| +| Public API Surface | | | | +| Cross-SDK Nomenclature | | | | +| Error Handling | | | | +| Go Idioms | | | | +| Internal/Public Boundary | | | | +| Input Validation | | | | +| Test Coverage | | | | +| Backward Compatibility | | | | +| Concurrency/Resources | | | | + +Then give an overall verdict: **Approve**, **Approve with minor changes**, or **Request changes**. + +--- + +After completing the code review above, also run `/security-review` to perform a security audit of the same scope, and append its findings below the code review output. diff --git a/.claude/commands/security-review.md b/.claude/commands/security-review.md new file mode 100644 index 0000000..3982d10 --- /dev/null +++ b/.claude/commands/security-review.md @@ -0,0 +1,96 @@ +--- +description: SDK-focused security audit covering credentials, tokens, input validation, HTTP hardening, and data leakage +--- + +Perform a security-focused review of the changes on the current branch. Run `git diff main...HEAD --name-only` to get the list of changed files, filter out any path containing `internal/generated/` (auto-generated OpenAPI code — do not review), then run `git diff main...HEAD` and read each remaining file in full. For each finding use severity labels: `[CRITICAL]`, `[HIGH]`, `[MEDIUM]`, `[LOW]`, or `[INFO]`. + +Focus exclusively on security impact. Do not comment on style, naming, or non-security logic unless it directly enables an attack. + +--- + +## 1. Credential and Secret Handling + +- Are credentials (API keys, bearer tokens, private keys, passwords) ever logged — even at DEBUG level? +- Are credential structs or maps printed via `%v` / `%+v` in log lines or error messages? +- Are secrets held in `string` fields when they should be zeroed after use? (`string` cannot be zeroed; use `[]byte` and `defer` wipe for high-sensitivity material) +- Is any credential written to a temp file or disk path that a low-privileged process could read? +- Is the JWT private key (RSA/EC) used for service-account token signing properly scoped and not stored beyond the signing operation? +- Are Skyflow credentials passed through `context.Context`? (Acceptable only if the context scope is controlled; flag if request-scoped contexts leak upward) + +## 2. Token Security (JWT / Bearer) + +- Is token expiry checked **before** each API call, not just at construction? +- Is the JWT signature verified using the correct algorithm? (`alg: none` or algorithm confusion?) +- Are `exp`, `iat`, and `iss` claims validated on tokens received from external sources? +- Is clock skew handled with a small tolerance (e.g., 10–30 s) to avoid replay near expiry boundary? +- Are bearer tokens transmitted only over TLS (never over plain HTTP)? +- Are tokens cached in memory only — never written to disk, logs, or error messages? + +## 3. Input Validation and Injection + +- Is every caller-supplied string (vault ID, table name, column name, query string, file path) validated before it reaches an HTTP request or file operation? +- Could a malformed vault ID or field name inject unexpected path segments into the API URL? (path traversal / SSRF via URL construction) +- Are SQL-like query parameters passed to the Skyflow Query API sanitised or parameterised? +- Are file paths for `UploadFile` validated to prevent directory traversal (`../` sequences)? +- Is user-controlled data ever placed in a log format string (format string injection)? +- Is any `os.Exec` or shell command constructed from user input? + +## 4. HTTP and TLS Hardening + +- Is `tls.Config.InsecureSkipVerify` ever set to `true`? (CRITICAL — removes MITM protection) +- Is a request timeout set on all HTTP clients (`http.Client{Timeout: ...}`)? (missing timeout enables slowloris / resource exhaustion) +- Are redirect policies controlled? (default Go client follows redirects; redirects to `http://` from `https://` downgrade protection) +- Are response bodies always closed via `defer resp.Body.Close()`? Unclosed bodies cause connection leaks. +- Is response body size bounded before reading into memory (`io.LimitReader`)? Unbounded read enables memory exhaustion. +- Are custom headers set by callers sanitised to prevent header injection (CRLF in values)? + +## 5. Error and Log Leakage + +- Do error messages returned to callers ever include raw server responses that may contain sensitive data? +- Are stack traces exposed in `SkyflowError.Message` visible to end users? +- Do log lines at any level emit token values, field-level data, or PII record content? +- Are internal error codes/states safe to expose externally (no internal system paths, DB details)? + +## 6. Concurrency and State Safety + +- Is any credential or token cache shared across goroutines without proper synchronisation (`sync.Mutex`, `sync.RWMutex`, or atomic)? +- Could a race condition cause two goroutines to simultaneously refresh a token, generating duplicate network calls or a TOCTOU window on expiry? +- Are maps used as shared configuration mutated after the client is constructed? (concurrent map read/write = data race + undefined behaviour) + +## 7. Dependency and Supply Chain + +- Does `go.mod` introduce a new direct dependency? If so: + - Is it from a reputable source with an active maintainer? + - Does it have known CVEs? (check `govulncheck` or OSV) + - Is the minimum version pinned (`require` without a floating pseudo-version)? +- Are indirect dependencies updated in a way that pulls in a vulnerable version? + +## 8. File and OS Operations + +- Are temp files created with `os.CreateTemp` (random suffix, not predictable path)? +- Are temp files cleaned up with `defer os.Remove(...)` even on error paths? +- Are file permissions restricted (`0600` for sensitive files, never world-readable)? +- Is any user-supplied filename used in `os.Open` / `os.Create` without path sanitisation? + +## 9. Context and Cancellation + +- Can a cancelled context cause a partial write to the Skyflow vault that leaves data in an inconsistent state? Document the SDK's guarantee (or lack thereof) in godoc. +- Are context values used to pass security-sensitive data (credentials, tokens)? If so, is the key an unexported type to prevent collision? + +--- + +## Summary Table + +| Category | Critical | High | Medium | Low | Info | +|---|---|---|---|---|---| +| Credentials & Secrets | | | | | | +| Token Security | | | | | | +| Input Validation / Injection | | | | | | +| HTTP / TLS Hardening | | | | | | +| Error & Log Leakage | | | | | | +| Concurrency / State | | | | | | +| Dependencies | | | | | | +| File / OS Operations | | | | | | +| Context & Cancellation | | | | | | + +**Overall risk**: Critical / High / Medium / Low — and a one-sentence recommendation. diff --git a/.claude/skills/code-review/SKILL.md b/.claude/skills/code-review/SKILL.md new file mode 100644 index 0000000..5b6105b --- /dev/null +++ b/.claude/skills/code-review/SKILL.md @@ -0,0 +1,134 @@ +--- +name: code-review +description: SDK-focused Go code review covering API design, idioms, backward compatibility, and test coverage. Use when asked to review code, review a PR, check a branch, or audit changes in the skyflow-go SDK. +argument-hint: "[full review | ]" +allowed-tools: [Bash, Read, Glob, Grep] +--- + +Perform a thorough code review from an **SDK maintainer's perspective**. + +**Always exclude files under `internal/generated/` from the review — these are auto-generated from OpenAPI specs and must not be reviewed or edited manually.** + +Determine the scope based on the argument `$ARGUMENTS`: + +- **No argument** (default): Review only the files changed on this branch. Run `git diff main...HEAD --name-only` to get the list, filter out any path containing `internal/generated/`, then run `git diff main...HEAD` for the full diff and read each non-generated changed file in full. +- **`full review`**: Review the entire SDK codebase. Run `find v2 -name "*.go" -not -path "*/generated/*" -not -name "*_test.go"` to enumerate all non-generated source files, then read them all. +- **A file or directory path**: Review only that path. If it is a directory, run `find -name "*.go" -not -path "*/generated/*"` to list all Go files within it (excluding generated), then read them all. + +After reading the relevant files, apply the checklist below. + +Structure the review under these headings. Under each heading list findings as `[BLOCKER]`, `[WARNING]`, or `[SUGGESTION]`. Skip any heading that has no findings. + +--- + +## 1. Public API Surface + +- Are new exported types, functions, and methods named following Go conventions? + - PascalCase for exported identifiers; camelCase for unexported + - Acronyms fully capitalised when exported (`VaultID`, `HTTPClient`), fully lower when unexported + - `With*` prefix for functional option constructors + - Receiver names short (1-2 letters), consistent across all methods of a type +- Does any change break existing callers? (renamed export, removed field, changed signature) +- Are new options/params added via functional options (`With*`) rather than positional arguments? +- Do all public operations accept `ctx context.Context` as the first parameter? +- Are new interfaces minimal and named for behaviour (verb/noun — `TokenProvider`, not `ITokenProvider`)? +- Do exported types have godoc comments that describe what, not how? + +## 1a. Cross-SDK Nomenclature (Go-specific) + +Verify these specific naming requirements from the cross-SDK specification. Flag any violation as `[BLOCKER]` since they are part of the public API contract across all Skyflow SDKs. + +**Credential JSON key fields** (in `Credentials` struct and related types): +- Must use `ClientId` (not `ClientID`), `TokenUri` (not `TokenURI`), `KeyId` (not `KeyID`) +- Go struct field tags must produce JSON keys `clientId`, `tokenUri`, `keyId` +- Check all credential-related structs in `utils/common/` and `internal/` + +**Response map keys — universal across all vault operations**: +- Skyflow record identifier key must be `SkyflowId` in all response structs (Insert, Update, Get, Bulk Delete, Query) +- Must NOT be `skyflow_id` (snake_case), `skyflowId` (camelCase), or `SkyflowID` (all-caps acronym) +- Check `InsertResponse`, `UpdateResponse`, `GetResponse`, `QueryResponse`, `BulkDeleteResponse`, and any other response types + +**Query response**: +- Tokenized data field must be `TokenizedData` (not `tokenized_data`, `tokenizedData`, or `TokenizedDataField`) +- `TokenizedData` must always be present in query response records (empty slice, not absent/nil) + +**BearerTokenOptions**: +- Role list field must be `RoleIds` (not `RoleIDs`, `RoleId`, or `Roles`) + +**Always-present response fields**: +- All response structs must always include an `Errors` field (never omit it, even on success — emit empty slice, not nil/absent) +- Verify the field is exported (`Errors []SkyflowError`) and included in JSON serialization + +## 2. Error Handling + +- Are all public methods returning `*error.SkyflowError` (never raw `error` or `nil` on failure)? +- Are errors wrapped with cause context (`Cause: err`)? +- Are new error message strings added to `utils/messages/` rather than inlined? +- Does any error message expose sensitive data (tokens, credentials, PII)? +- Is `panic` used in library code? (never acceptable in an SDK — convert to error return) + +## 3. Go Idioms and Best Practices + +- No naked `return` in long functions; named returns only when they substantially aid clarity +- No use of `fmt.Print*` or `log.*` in library code (use project logger) +- Goroutines: are all goroutines guarded by context cancellation or a done channel? +- No unexported global mutable state (race conditions in concurrent SDK use) +- Nil guards on receivers and map/slice fields before use +- `defer` used correctly — no deferred calls inside loops +- No shadowed `err` variables across if-blocks +- Proper use of `errors.Is` / `errors.As` rather than type-asserting raw errors +- Unused imports, unused variables — run `go vet` confirms clean? + +## 4. Internal vs Public Boundary + +- Is new business logic placed in `internal/` rather than exported packages? +- Are generated files under `internal/generated/` untouched (edited by hand)? +- Does `utils/common/` only contain data types, not logic? + +## 5. Input Validation + +- Are all user-supplied inputs (vault IDs, field names, tokens, file paths) validated in `internal/validation/` before use? +- Validation errors returned via `SkyflowError`, not panics +- No use of raw user input in log messages without sanitisation + +## 6. Test Coverage + +- Does every new exported function/method have at least one `It` block (happy path)? +- Are all validation-error branches covered? +- Are external HTTP calls mocked via `httptest.Server`, not by patching internal helpers? +- New `DescribeTable` entries for any table-driven scenario added? +- Tests compilable and passing: `cd v2 && go test ./...` + +## 7. Backward Compatibility + +- Are any exported identifiers removed or renamed? (BLOCKER — requires major version bump) +- Are struct fields that callers may embed or copy affected? +- Is `go.mod` `require` bumped for a dependency with a breaking change in its own API? + +## 8. Concurrency and Resource Safety + +- HTTP clients and vault configs: are they safely shared across goroutines (no mutation after construction)? +- File uploads: are temporary files and open handles cleaned up even on error paths? +- Context propagation: is `ctx` passed down to every HTTP call and blocking operation? + +--- + +End with a **Summary** table: + +| Category | Blockers | Warnings | Suggestions | +|---|---|---|---| +| Public API Surface | | | | +| Cross-SDK Nomenclature | | | | +| Error Handling | | | | +| Go Idioms | | | | +| Internal/Public Boundary | | | | +| Input Validation | | | | +| Test Coverage | | | | +| Backward Compatibility | | | | +| Concurrency/Resources | | | | + +Then give an overall verdict: **Approve**, **Approve with minor changes**, or **Request changes**. + +--- + +After completing the code review above, also run `/security-review $ARGUMENTS` to perform a security audit of the same scope, and append its findings below the code review output.