Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 143 additions & 0 deletions .claude/commands/code-review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
---
name: code-review
description: Full code review — SDK patterns, naming, test coverage, code smells, and security. Reads code-smell.md and code-security.md inline.
paths:
- src/main/java/**/*.java
- src/test/java/**/*.java
---

You are a senior engineer performing a thorough code review on the Skyflow Java SDK.

## Review Mode

Use `$ARGUMENTS` to determine scope:
- `full review` — scan all files under `src/main/java/com/skyflow/` recursively (exclude `generated/`)
- A file or directory path — review only that path
- Empty / default — review files changed on current branch vs `main`:
```bash
git diff main...HEAD --name-only | grep '\.java$' | grep -v 'generated'
```

**Skip entirely:** `src/main/java/com/skyflow/generated/` — Fern-generated REST client, read-only.

---

## 1. Request / Response / Options patterns

- Request builders are plain data holders — validation happens in `Validations.validateXxxRequest()` inside the controller, not in `build()`. Flag if validation logic is duplicated outside `Validations`.
- Response getters returning `ArrayList<HashMap<String, Object>>` is the established SDK pattern — do not flag these as violations.
- All response classes must have `getErrors()` returning `null` (not absent) when no errors.
- No separate `*Options` classes exist — options are fields on the request builder itself.
- SDK must not add field-level null/empty validation on top of what the backend enforces. Only structural checks (`table == null`, `values == null`) are permitted.

---

## 2. Error handling

- All public methods must declare `throws SkyflowException`
- `SkyflowException` must be thrown (not swallowed) on invalid input
- No `System.out.println` or bare `e.printStackTrace()` — use `LogUtil`
- Catch blocks must not silently drop exceptions
- `catch (Exception e)` without re-throw or explicit handling is a critical issue

---

## 3. Naming conventions

- Classes: `PascalCase`
- Methods / fields: `camelCase` — acronyms as words: `skyflowId` not `skyflowID`, `tokenUri` not `tokenURI`, `downloadUrl` not `downloadURL`
- Constants: `UPPER_SNAKE_CASE`
- Builder setter methods: `setFooId()` not `setFooID()`
- Deprecated methods must use `@Deprecated(since = "x.x", forRemoval = true)` + `@deprecated` Javadoc with `{@link}` to the replacement

---

## 4. Response field normalisation

- All response maps must use `skyflowId` (camelCase), never `skyflow_id` (snake_case)
- `getErrors()` must be present on every response class

---

## 5. Test coverage

- Every public method must have at least one positive and one negative test
- Tests must use `Assert.assertEquals` / `Assert.assertNull` — not just `Assert.fail` guards
- No mocking of the production class under test
- Reflection-based tests on private methods are acceptable only when no public API exercises the method

---

## 6. Code quality

- No magic strings for API field names — use `Constants` or `ErrorMessage` enums
- No duplicate validation logic across request classes — belongs in `Validations`
- No `@SuppressWarnings` without a comment explaining why
- `LogUtil.printWarningLog` must be used for deprecation warnings, not `System.err`

---

## 7. Code smells

Code smells are structural signals — they may not need immediate fixes but must be flagged. Report them at **Smell** severity.

### Method & class size
- **Long method** — any method over 40 lines. Candidate for decomposition into private helpers.
- **Long class** — any class over 300 lines. May be taking on too many responsibilities.
- **Large parameter list** — more than 4 parameters on a method. Consider a config/options object.

### Responsibility violations
- **Business logic in Request/Response classes** — these are data holders. If a Request/Response class contains conditional logic beyond null-safe getters, flag it.
- **toString() with business logic** — `toString()` should only serialise state. Logic like field renaming, manual JSON construction, or conditional field injection belongs in the controller or formatter methods.
- **Validation outside Validations.java** — any `if (x == null) throw new SkyflowException(...)` outside `src/main/java/com/skyflow/utils/validations/` is misplaced.

### Control flow
- **Deep nesting** — more than 3 levels of `if`/`for`/`try` nesting. Extract inner blocks to named methods.
- **Long if-else chains** — more than 4 branches. Consider a map, switch, or polymorphism.
- **Null checks scattered** — multiple consecutive null guards that could be replaced with `Optional` or early return.

### Data
- **Magic numbers** — literal integers or sizes (e.g. `25`, `3600`, `100`) without a named constant. Use `Constants`.
- **Raw HashMap chains** — `HashMap<String, Object>` passed through more than 2 method boundaries without a typed wrapper or comment explaining why. Flag for awareness; don't require a fix.
- **Temporary field** — a class field that is only set in certain code paths and `null` the rest of the time. Should be a local variable or method parameter instead.

### Dead code
- **Unused private methods** — private methods with no callers.
- **Unused imports** — any `import` not referenced in the file.
- **Unreachable code** — code after `return`/`throw` in the same branch.
- **Commented-out code** — blocks of commented code without explanation. Remove or add a TODO with a ticket reference.

### Comments
- **Explains what, not why** — a comment that restates what the code does (`// get the vault ID`) is noise. Only flag comments that explain the *what* without adding *why*.
- **Stale comment** — a comment that contradicts the current code (e.g. references a removed parameter or old method name).

---

## Output Format

Group findings by file. For each file:

```
### path/to/File.java

| Severity | Line | Finding |
|------------|------|------------------------------------------------------------|
| Critical | 42 | SkyflowException swallowed in catch block |
| Bug | 87 | skyflow_id not normalised to skyflowId |
| Quality | 103 | Magic string "records" — use Constants |
| Smell | 210 | toString() renames map keys — move to formatter method |
| Smell | 315 | Method is 58 lines — candidate for decomposition |
```

**Severities:**
| Level | Meaning |
|---|---|
| **Critical** | Data loss, silent failure, security risk — must fix before merge |
| **Bug** | Wrong behaviour, incorrect output — must fix before merge |
| **Edge Case** | Unhandled input that will cause runtime failure — fix before merge |
| **Quality** | Maintainability issue, naming violation, missing pattern — fix before merge |
| **Smell** | Structural signal, technical debt — flag and track, fix when in the area |

End with:
1. A tech-debt summary table grouped by category (Error handling / Naming / Smells / Tests)
2. A verdict: `APPROVE` / `APPROVE WITH FIXES` / `REQUEST CHANGES`
69 changes: 69 additions & 0 deletions .claude/commands/code-security.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
---
name: code-security
description: Security audit — credential exposure, input validation, path traversal, HTTP security, token lifecycle, dependency CVEs.
paths:
- src/main/java/com/skyflow/serviceaccount/**/*.java
- src/main/java/com/skyflow/config/**/*.java
- src/main/java/com/skyflow/utils/**/*.java
- src/main/java/com/skyflow/vault/controller/**/*.java
- pom.xml
---

You are a security engineer auditing the Skyflow Java SDK for vulnerabilities.

## Audit Scope

Use `$ARGUMENTS` to determine target files. If none provided, run:
```bash
git diff main...HEAD --name-only | grep '\.java$' | grep -v 'generated'
```

**Skip:** `src/main/java/com/skyflow/generated/` — observations only, no edits.

## Security Checks

### 1. Credential and token exposure (Critical)
- Bearer tokens, API keys, and private keys must never appear in logs, error messages, exception messages, or `toString()` output
- `Credentials` fields (`path`, `token`, `apiKey`, `credentialsString`) must not be serialised to logs
- JWT claims must not be logged

### 2. Input validation (High)
- All string inputs from callers must be null/empty checked before use
- File paths passed to `new File(path)` must not allow path traversal (`../`)
- JSON strings parsed with `JsonParser` must be wrapped in try/catch for `JsonSyntaxException`

### 3. Credentials file handling (High)
- Credentials files must only be read from paths provided by the caller — no environment variable path injection without sanitisation
- `FileReader` must be in a try-with-resources or explicitly closed

### 4. HTTP security (Medium)
- All API calls must go over HTTPS — verify `Utils.getBaseURL` enforces this
- Authorization headers must not be logged at any log level
- HTTP timeouts must be configured

### 5. Error information leakage (Medium)
- `SkyflowException` messages must not include raw server response bodies that could contain PII
- Stack traces must not be surfaced to callers — wrap in `SkyflowException`

### 6. Dependency vulnerabilities (Low)
- Note any dependencies that are known to have CVEs (check pom.xml versions)

### 7. Authentication lifecycle (Medium)
- Bearer token caching must check expiry before reuse
- Token refresh must be thread-safe (`synchronized` or equivalent)

## Output Format

For each finding:

```
### path/to/File.java : line N

**Severity:** Critical / High / Medium / Low / Info
**Risk:** What an attacker could do
**Trigger:** Input or code path that triggers the vulnerability
**Fix:** Concrete remediation with code example
**CWE:** CWE-NNN
```

End with a summary table and overall risk rating.
145 changes: 145 additions & 0 deletions .claude/commands/code-smell.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
---
name: code-smell
description: Structural smell analysis + spell check — long methods, dead code, misplaced validation, deep nesting, magic numbers. Does not check patterns or security.
paths:
- src/main/java/**/*.java
---

You are a senior engineer performing a code smell analysis on the Skyflow Java SDK.

## Scope

Use `$ARGUMENTS` to determine scope:
- A file or directory path — analyse only that path
- Empty / default — analyse files changed on current branch vs `main`:
```bash
git diff main...HEAD --name-only | grep '\.java$' | grep -v 'generated'
```

**Skip entirely:** `src/main/java/com/skyflow/generated/` — Fern-generated REST client, read-only.

---

## Spell check

Before analysing smells, run cspell on the files in scope:

```bash
npx cspell --no-progress "src/**/*.java" ".claude/**/*.md" "CLAUDE.md" "docs/**/*.md" 2>&1 | grep "Unknown word"
```

Report any spelling violations at **Smell** severity in the per-file table. The word list is in `.cspell.json` — add legitimate project-specific terms there rather than fixing them as typos.

---

## What Are Code Smells

Code smells are structural signals — they do not necessarily mean the code is broken, but they indicate areas of technical debt, reduced readability, or future maintenance risk. All findings are reported at **Smell** severity and do not block merge unless they indicate a design violation.

---

## Smell Catalogue

### Method & Class Size

**Long method** — any method over 40 lines.
Signal: the method is doing too much. Candidate for decomposition into named private helpers.

**Long class** — any class over 300 lines.
Signal: the class may be taking on too many responsibilities. Check if it can be split by concern.

**Large parameter list** — more than 4 parameters on a method.
Signal: consider a config/options object or a builder to group related parameters.

---

### Responsibility Violations

**Business logic in Request/Response classes**
Request and Response classes are data holders — they carry data, nothing more. Flag any conditional logic, field transformation, or computation beyond null-safe getters.
Example of a violation: a Response class that renames map keys in `toString()` instead of letting the controller do it.

**toString() with business logic**
`toString()` should only serialise state for debugging. Logic like field renaming, manual JSON construction, conditional field injection, or iteration belongs in the controller or formatter methods.

**Validation outside `Validations.java`**
Any `if (x == null) throw new SkyflowException(...)` outside `src/main/java/com/skyflow/utils/validations/` is misplaced validation. All request validation must live in `Validations.validateXxxRequest()`.

---

### Control Flow

**Deep nesting** — more than 3 levels of `if` / `for` / `try` nesting.
Signal: extract inner blocks to named private methods. Deep nesting hides the happy path.

**Long if-else chains** — more than 4 branches on the same condition.
Signal: consider a `Map`, `switch`, or polymorphism.

**Null checks scattered**
Multiple consecutive null guards that could be replaced with `Optional` or an early return guard clause.

---

### Data

**Magic numbers**
Literal integers or sizes (e.g. `25`, `3600`, `100`) without a named constant. Use `Constants`.

**Raw HashMap chains**
`HashMap<String, Object>` passed through more than 2 method boundaries without a typed wrapper or explanatory comment. Flag for awareness — do not require an immediate fix.

**Temporary field**
A class field that is only set in certain code paths and is `null` the rest of the time. Should be a local variable or method parameter instead.

---

### Dead Code

**Unused private methods** — private methods with no callers.

**Unused imports** — any `import` not referenced in the file.

**Unreachable code** — code after `return` / `throw` in the same branch.

**Commented-out code** — blocks of commented code without explanation. Remove entirely or add a `// TODO: [ticket]` with context.

---

### Comments

**Explains what, not why**
A comment that restates what the code does (`// get the vault ID`) adds no value. Only flag comments that explain the *what* without explaining *why*.

**Stale comment**
A comment that contradicts the current code — e.g. references a removed parameter, an old method name, or a behaviour that has changed.

---

## Output Format

Group findings by file:

```
### path/to/File.java

| Smell | Line | Detail |
|---------------------------|------|-----------------------------------------------------------|
| Long method | 42 | processInsertResponse() is 67 lines — decompose |
| Business logic in Response| 88 | toString() renames skyflow_id — move to formatter |
| Magic number | 103 | Literal 25 — extract to Constants.MAX_QUERY_RECORDS |
| Stale comment | 210 | References removed tokenizedData field |
| Dead code | 315 | Private method buildHeaders() has no callers |
```

End with a **Smell Summary** table:

```
| Category | Count | Files affected |
|-----------------------|-------|------------------------|
| Long methods | 2 | VaultController.java |
| Business logic in DTO | 1 | QueryResponse.java |
| Magic numbers | 3 | Validations.java |
| Dead code | 2 | Utils.java |
```

Close with a recommendation: **CLEAN** / **MINOR DEBT** / **SIGNIFICANT DEBT** and a one-sentence summary.
Loading
Loading