From 9d8775b0a59d16f76ee1f8228d0bea1a2e494ac2 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 18:50:04 +0530 Subject: [PATCH 01/54] SK-2832 add Claude Code setup (CLAUDE.md, commands, workflows) - Add CLAUDE.md with project conventions, version info, commit guidelines - Add .claude/commands: code-review, code-smell, code-security, sdk-sample, test, commit (Jira-aware) - Add .claude/hooks/checkstyle-on-edit.py for per-edit lint feedback - Add .claude/settings.json with permissions and PostToolUse hook - Add .claude/skills/requesting-code-review for context-fork reviews - Add .github/workflows/claude-pr-review.yml: automated SDK + security review on PRs - Add .github/workflows/claude-changelog.yml: auto-generated release notes on tag push - Add .github/workflows/release-v1.yml: v1 public release targeting v1 branch - Update .cspell.json with British English and project-specific terms Requires ANTHROPIC_API_KEY secret in GitHub Actions for claude-* workflows. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .claude/commands/code-review.md | 107 ++++++++++++ .claude/commands/code-security.md | 69 ++++++++ .claude/commands/code-smell.md | 145 ++++++++++++++++ .claude/commands/commit.md | 55 ++++++ .claude/commands/sdk-sample.md | 70 ++++++++ .claude/commands/test.md | 73 ++++++++ .claude/hooks/checkstyle-on-edit.py | 19 +++ .claude/settings.json | 30 ++++ .../skills/requesting-code-review/SKILL.md | 76 +++++++++ .cspell.json | 33 +++- .github/workflows/claude-changelog.yml | 86 ++++++++++ .github/workflows/claude-pr-review.yml | 157 ++++++++++++++++++ .github/workflows/release-v1.yml | 0 CLAUDE.md | 148 +++++++++++++++++ 14 files changed, 1065 insertions(+), 3 deletions(-) create mode 100644 .claude/commands/code-review.md create mode 100644 .claude/commands/code-security.md create mode 100644 .claude/commands/code-smell.md create mode 100644 .claude/commands/commit.md create mode 100644 .claude/commands/sdk-sample.md create mode 100644 .claude/commands/test.md create mode 100644 .claude/hooks/checkstyle-on-edit.py create mode 100644 .claude/settings.json create mode 100644 .claude/skills/requesting-code-review/SKILL.md create mode 100644 .github/workflows/claude-changelog.yml create mode 100644 .github/workflows/claude-pr-review.yml create mode 100644 .github/workflows/release-v1.yml create mode 100644 CLAUDE.md diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md new file mode 100644 index 00000000..498fcfac --- /dev/null +++ b/.claude/commands/code-review.md @@ -0,0 +1,107 @@ +--- +name: code-review +description: Full code review — SDK patterns, naming, test coverage, then runs /code-smell and /code-security. +paths: + - src/main/java/**/*.java + - src/test/java/**/*.java +--- + +You are a senior engineer performing a thorough code review on the Skyflow Java SDK. + +## Scope + +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. + +--- + +## Step 1 — SDK Pattern Review + +Check the files in scope against the rules below. + +### 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>` 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 and response field normalisation + +Follow the conventions in CLAUDE.md under "Naming Conventions". Key enforcement points: +- Acronyms as words: `skyflowId`, `tokenUri`, `clientId` — never uppercase abbreviations +- Builder setters: `setFooId()` not `setFooID()`; constants: `UPPER_SNAKE_CASE`; classes: `PascalCase` +- Response maps: `skyflowId` (camelCase) only — never `skyflow_id`; `getErrors()` must be present on every response class +- Deprecated methods: `@Deprecated(since = "x.x", forRemoval = true)` + `@deprecated` Javadoc with `{@link}` to replacement + +### 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` + +### Output for Step 1 + +Group findings by 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 | +``` + +**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 | + +--- + +## Step 2 — Code Smell Analysis + +Read the file `.claude/commands/code-smell.md` and follow all of its instructions for the same files in scope. Produce its full output (per-file smell table + smell summary + recommendation). + +--- + +## Step 3 — Security Audit + +Read the file `.claude/commands/code-security.md` and follow all of its instructions for the same files in scope. Produce its full output (per-finding blocks + summary table + overall risk rating). + +--- + +## Final Verdict + +After all three steps, close with: +1. A tech-debt summary table grouped by category (SDK Patterns / Error Handling / Naming / Tests / Smells / Security) +2. A verdict: `APPROVE` / `APPROVE WITH FIXES` / `REQUEST CHANGES` diff --git a/.claude/commands/code-security.md b/.claude/commands/code-security.md new file mode 100644 index 00000000..0fa69923 --- /dev/null +++ b/.claude/commands/code-security.md @@ -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. diff --git a/.claude/commands/code-smell.md b/.claude/commands/code-smell.md new file mode 100644 index 00000000..f456a8d8 --- /dev/null +++ b/.claude/commands/code-smell.md @@ -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` 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. diff --git a/.claude/commands/commit.md b/.claude/commands/commit.md new file mode 100644 index 00000000..6aae4614 --- /dev/null +++ b/.claude/commands/commit.md @@ -0,0 +1,55 @@ +--- +name: commit +description: Stage check + Jira-aware commit — extracts ticket ID from branch name and validates against pr.yml commit-message check. +--- + +Create a git commit for staged changes on the current branch. + +Use `$ARGUMENTS` as the commit message description. If empty, ask the user for a description before proceeding. + +## Step 1 — Extract ticket ID from branch name + +```bash +git rev-parse --abbrev-ref HEAD +``` + +Extract the Jira ticket ID using the pattern `[A-Z]{1,10}-[0-9]+`: +- `devesh/SK-1234-fix-foo` → `SK-1234` +- `karthik/GV-770-ext-auth-json-error` → `GV-770` +- `username/SDK-2814-some-fix` → `SDK-2814` + +If no ticket ID is found, **stop** and ask the user to provide one before continuing. + +## Step 2 — Check what is staged + +```bash +git status --short +git diff --cached --stat +``` + +If nothing is staged, list the unstaged files and ask the user which files to stage. Do not run `git add .` — ask for explicit paths (`.env`, `credentials.json`, and `generated/` must never be staged). + +## Step 3 — Assemble and validate the commit message + +Build the message as: +``` + +``` + +If the user provided a Conventional Commits prefix (`feat`, `fix`, `chore`, `docs`, `refactor`, `test`), prepend it: +``` +feat: SK-1234 add bulk insert support +fix: GV-770 handle null bearer token on refresh +``` + +Validate against the `pr.yml` enforced pattern: `(\[?[A-Z]{1,10}-[1-9][0-9]*)|(\[AUTOMATED\])|(Merge)|(Release)` +- Must contain a Jira ID — a bare description without a ticket ID will fail CI. +- If validation fails, report the exact requirement and stop. + +## Step 4 — Commit + +```bash +git commit -m "" +``` + +Report the resulting commit SHA and the commit message first line. diff --git a/.claude/commands/sdk-sample.md b/.claude/commands/sdk-sample.md new file mode 100644 index 00000000..a7b84a65 --- /dev/null +++ b/.claude/commands/sdk-sample.md @@ -0,0 +1,70 @@ +--- +name: sdk-sample +description: Generate a Skyflow Java SDK sample file for a vault feature or service account operation. Compile-verified after creation. +paths: + - samples/**/*.java + - samples/pom.xml +--- + +Create a Skyflow Java SDK sample file demonstrating: $ARGUMENTS + +## File placement + +| Feature type | Package | Directory | +|---|---|---| +| Vault ops (insert/get/update/delete/query/tokenize) | `com.example.vault` | `samples/src/main/java/com/example/vault/` | +| Service account auth | `com.example.serviceaccount` | `samples/src/main/java/com/example/serviceaccount/` | +| Connection | `com.example.connection` | `samples/src/main/java/com/example/connection/` | +| Detect | `com.example.detect` | `samples/src/main/java/com/example/detect/` | +| Audit event operations | `com.example.audit` | `samples/src/main/java/com/example/audit/` | +| BIN lookup | `com.example.bin` | `samples/src/main/java/com/example/bin/` | + +File name: `Example.java` + +## Structure (follow this order) + +1. Package declaration +2. Imports — only from `com.skyflow.*`, `java.*`; never from `com.skyflow.generated.*` +3. Public class with `main(String[] args) throws SkyflowException` +4. Credentials setup — choose based on feature: + - **Vault ops:** `credentials.setApiKey("")` or `credentials.setCredentialsString("")` + - **Service account:** `credentials.setPath("credentials.json")` (path to the service account JSON file) +5. `VaultConfig` with `setVaultId`, `setClusterId`, `setEnv(Env.PROD)`, `setCredentials(credentials)` +6. Build the Skyflow client: + ```java + Skyflow skyflowClient = Skyflow.builder() + .setLogLevel(LogLevel.DEBUG) + .addVaultConfig(vaultConfig) + .build(); + ``` +7. Request object via `*Request.builder()` — options go directly on the builder (no separate Options class): + ```java + // Example: InsertRequest with tokenMode + InsertRequest request = InsertRequest.builder() + .table("...") + .values(records) + .tokenMode(TokenMode.ENABLE) + .build(); + ``` +8. Call the vault method inside a try/catch for `SkyflowException`: + ```java + InsertResponse response = skyflowClient.vault().insert(request); + System.out.println(response); + ``` + +## Rules + +- Vault IDs / cluster IDs use placeholders: `""`, `""` +- Credential values use placeholders: `""`, `""` +- Credentials file path: `"credentials.json"` (relative — no absolute paths) +- Always catch `SkyflowException` and print `e.getMessage()` +- No separate `*Options` classes — they don't exist in this SDK; use request builder methods +- Keep under 80 lines + +## After creating the file + +```bash +cd samples && mvn compile -q 2>&1 | tail -20 +``` + +Report the file path and any compile errors. diff --git a/.claude/commands/test.md b/.claude/commands/test.md new file mode 100644 index 00000000..e690a355 --- /dev/null +++ b/.claude/commands/test.md @@ -0,0 +1,73 @@ +--- +name: test +description: Quality pipeline — compile, checkstyle, build, tests, coverage analysis. Pass a class name to target a single test class. +paths: + - src/**/*.java + - pom.xml +--- + +Run the Skyflow Java SDK quality pipeline. + +Use `$ARGUMENTS` to target a specific test class (e.g. `BearerTokenTests`). If empty, run the full suite. + +> Baseline failures are listed in CLAUDE.md under "Known Pre-existing Test Failures". +> Do not investigate them unless specifically asked. Only report failures **beyond** that baseline. + +## Pipeline + +### Step 1 — Compile +```bash +mvn compile -q 2>&1 | tail -20 +``` +Expected: no output (clean compile). Report any errors. + +### Step 2 — Checkstyle +```bash +mvn checkstyle:check -q 2>&1 | tail -20 +``` +Note: `failsOnError=false` in pom.xml means the build will not fail even if violations exist — check the output for `[WARN]` checkstyle lines. Violations are excluded from `generated/` by pom config. + +### Step 3 — Build +```bash +mvn package -DskipTests -q 2>&1 | tail -20 +``` +Expected: BUILD SUCCESS. + +### Step 4 — Tests +If `$ARGUMENTS` is set: +```bash +mvn test -Dtest=$ARGUMENTS -q 2>&1 | tail -40 +``` +Otherwise: +```bash +mvn test -q 2>&1 | tail -40 +``` +Report: tests run, failures, errors. Flag any pre-existing failures separately from new ones. + +### Step 5 — Coverage analysis +Flag any public interface class (`src/main/java/com/skyflow/vault/`, `src/main/java/com/skyflow/config/`, `src/main/java/com/skyflow/serviceaccount/`) that has no corresponding test file under `src/test/`. + +For classes that do have tests, check whether each public method has at least one positive and one negative test case. List any gaps. + +### Step 6 — Edge case identification +For any test class below complete coverage, identify missing scenarios: +- Null / empty inputs +- Invalid types / wrong enum values +- Concurrent / reuse scenarios +- Error paths (API rejection, network failure) + +Write concrete JUnit 4 test method stubs (not full implementations) for each gap. + +### Step 7 — Report + +``` +| Step | Status | Notes | +|---|---|---| +| Compile | ✅ / ❌ | ... | +| Checkstyle | ✅ / ❌ | ... | +| Build | ✅ / ❌ | ... | +| Tests | ✅ / ❌ | N passed, M failed | +| Coverage gaps | ... | list classes | +``` + +Conclude with **READY TO MERGE** or **NEEDS FIXES** and a prioritised fix list. diff --git a/.claude/hooks/checkstyle-on-edit.py b/.claude/hooks/checkstyle-on-edit.py new file mode 100644 index 00000000..4fd0787d --- /dev/null +++ b/.claude/hooks/checkstyle-on-edit.py @@ -0,0 +1,19 @@ +import sys, json, subprocess, os + +d = json.load(sys.stdin) +f = d.get('tool_input', {}).get('file_path', d.get('file_path', '')) +if not f or not f.endswith('.java'): + sys.exit(0) + +root = '/home/devb/SDK/skyflow-java' +marker = 'src/main/java/' +if marker in f: + rel = f.split(marker, 1)[1] + args = ['mvn', 'checkstyle:check', '-q', '-Dcheckstyle.includes=' + rel] +else: + args = ['mvn', 'checkstyle:check', '-q'] + +r = subprocess.run(args, capture_output=True, text=True, cwd=root) +out = (r.stdout + r.stderr).strip() +if out: + print('\n'.join(out.splitlines()[-20:])) diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000..1d00baed --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,30 @@ +{ + "hooks": { + "PostToolUse": [ + { + "matcher": "Edit|Write", + "hooks": [ + { + "type": "command", + "command": "python3 .claude/hooks/checkstyle-on-edit.py" + } + ] + } + ] + }, + "permissions": { + "allow": [ + "Bash(mvn *)", + "Bash(java *)", + "Bash(python3 *)", + "Bash(git *)", + "Bash(find *)", + "Bash(grep *)", + "Bash(npx cspell *)" + ], + "deny": [ + "Edit(src/main/java/com/skyflow/generated/**)", + "Write(src/main/java/com/skyflow/generated/**)" + ] + } +} diff --git a/.claude/skills/requesting-code-review/SKILL.md b/.claude/skills/requesting-code-review/SKILL.md new file mode 100644 index 00000000..9f662842 --- /dev/null +++ b/.claude/skills/requesting-code-review/SKILL.md @@ -0,0 +1,76 @@ +--- +name: requesting-code-review +description: Use when completing tasks, implementing major features, or before merging to verify work meets requirements +paths: + - src/main/java/**/*.java + - src/test/java/**/*.java +--- + +# Requesting Code Review + +**Core principle:** Review early, review often. Review after each task — catch issues before they compound. + +## When to Request Review + +**Mandatory:** +- After each task in subagent-driven development +- After completing a major feature +- Before merge to main + +**Optional but valuable:** +- When stuck (fresh perspective) +- Before refactoring (baseline check) +- After fixing a complex bug + +## How to Request + +**1. Pick the right command:** + +| Change type | Command | +|---|---| +| SDK logic, patterns, naming, tests | `/code-review` — SDK checks + smell + security | +| Structural debt only | `/code-smell` — standalone smell analysis | +| Auth, credentials, tokens, HTTP | `/code-security` — standalone security audit | + +For security-sensitive changes, run both: +```bash +/code-review src/main/java/com/skyflow/serviceaccount/ +/code-security src/main/java/com/skyflow/serviceaccount/ +``` + +**2. Fork context — dispatch a subagent reviewer:** + +The commands above run in the current session and share your context. For an independent second opinion (no confirmation bias, preserved main context window), dispatch a fresh subagent: + +``` +Agent tool (general-purpose): + description: "SDK code review" + prompt: | + You are a senior engineer reviewing the Skyflow Java SDK. + + Read CLAUDE.md for project conventions, then read and follow + .claude/commands/code-review.md for the full review process. + + Git range to review: + Base: {BASE_SHA} + Head: {HEAD_SHA} + + Run: + git diff --stat {BASE_SHA}..{HEAD_SHA} + git diff {BASE_SHA}..{HEAD_SHA} + + Description of what was implemented: + {DESCRIPTION} +``` + +Get the SHAs: +```bash +BASE_SHA=$(git merge-base main HEAD) # branch vs main +HEAD_SHA=$(git rev-parse HEAD) +``` + +**3. Act on feedback:** +- Fix Critical issues immediately +- Fix Important issues before proceeding +- Note Minor/Smell issues for later +- Push back with reasoning if you disagree diff --git a/.cspell.json b/.cspell.json index a0ec0be6..a982f837 100644 --- a/.cspell.json +++ b/.cspell.json @@ -73,9 +73,33 @@ "pkcs", "prioritise", "Prioritise", + "prioritised", "Timeto", "Wdex", - "jacoco" + "jacoco", + "serialise", + "serialised", + "serialises", + "serialising", + "normalise", + "Normalise", + "normalised", + "normalises", + "Normalises", + "normalising", + "behaviour", + "Behaviour", + "behaviours", + "sanitisation", + "prioritise", + "recognised", + "unrecognised", + "nocreds", + "nodir", + "detok", + "qhdmceurtnlz", + "ngrok", + "obac" ], "languageSettings": [ { @@ -98,7 +122,9 @@ "src/main/java/com/skyflow/generated/**", "**/*.ts", "**/processed-*", - "samples/src/main/java/com/example/credentials.json" + "samples/src/main/java/com/example/credentials.json", + "RUNNING_SAMPLES.md", + "docs/superpowers/**" ], "ignoreRegExpList": [ "/\\b[A-Z][A-Z0-9_]{2,}\\b/g", @@ -106,6 +132,7 @@ "/(eyJ[A-Za-z0-9+/=_-]+\\.)+[A-Za-z0-9+/=_-]+/g", "/[A-Za-z0-9_.~-]*%[0-9A-Fa-f]{2}[A-Za-z0-9_.~%-]*/g", "/\\b[A-Za-z0-9_]{7,}\\b(?=])/g", - "/\"[A-Za-z0-9+/=]{15,}\"/g" + "/\"[A-Za-z0-9+/=]{15,}\"/g", + "/-D[A-Za-z][A-Za-z0-9.]*/g" ] } diff --git a/.github/workflows/claude-changelog.yml b/.github/workflows/claude-changelog.yml new file mode 100644 index 00000000..aae70fa2 --- /dev/null +++ b/.github/workflows/claude-changelog.yml @@ -0,0 +1,86 @@ +name: Claude Changelog + +on: + push: + tags: + - '[0-9]+.[0-9]+.[0-9]+' + - '*.*.*-beta.*' + +permissions: + contents: write + +jobs: + generate-changelog: + name: Generate Release Notes + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Get previous tag + id: previoustag + uses: WyriHaximus/github-action-get-previous-tag@v1 + with: + fallback: '0.0.0' + + - name: Get commits since previous tag + id: commits + run: | + PREV="${{ steps.previoustag.outputs.tag }}" + CURR="${{ github.ref_name }}" + COMMITS=$(git log "${PREV}..${CURR}" --oneline \ + | grep -v '^\S* \[AUTOMATED\]' \ + | grep -v '^\S* Merge ' \ + | grep -v '^\S* \[AUTOMATED\]') + echo "log<> $GITHUB_OUTPUT + echo "$COMMITS" >> $GITHUB_OUTPUT + echo "EOF" >> $GITHUB_OUTPUT + + - name: Install Claude CLI + run: npm install -g @anthropic-ai/claude-code + + - name: Generate release notes + id: notes + continue-on-error: true + env: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + run: | + PREV="${{ steps.previoustag.outputs.tag }}" + CURR="${{ github.ref_name }}" + COMMITS="${{ steps.commits.outputs.log }}" + NOTES=$(claude --print --model claude-sonnet-4-5 -p " + Generate GitHub Release notes for the Skyflow Java SDK. + + Release: $CURR (previous: $PREV) + + Commits: + $COMMITS + + Rules: + - Group into sections: ## Features, ## Bug Fixes, ## Security, ## Breaking Changes + - Omit any section with no entries + - Each entry: bullet point with a concise one-line description; include the Jira ticket ID if present (e.g. SK-1234) + - Strip PR merge numbers like (#323) — keep the substance + - Skip [AUTOMATED] commits, version bump commits, and bare merge commits + - Breaking Changes section must come first if present + - End with: _Full changelog: https://github.com/skyflowapi/skyflow-java/compare/${PREV}...${CURR}_ + + Output only the markdown. No preamble or explanation. + ") + echo "notes<> $GITHUB_OUTPUT + echo "$NOTES" >> $GITHUB_OUTPUT + echo "EOF" >> $GITHUB_OUTPUT + + - name: Create or update GitHub Release + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + TAG="${{ github.ref_name }}" + NOTES="${{ steps.notes.outputs.notes }}" + if gh release view "$TAG" > /dev/null 2>&1; then + gh release edit "$TAG" --notes "$NOTES" + else + gh release create "$TAG" --notes "$NOTES" --title "Release $TAG" + fi diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml new file mode 100644 index 00000000..821b2d50 --- /dev/null +++ b/.github/workflows/claude-pr-review.yml @@ -0,0 +1,157 @@ +name: Claude PR Review + +on: + pull_request: + branches: [main] + paths: + - 'src/**/*.java' + +permissions: + pull-requests: write + contents: read + +jobs: + sdk-patterns-review: + name: SDK Patterns & Naming Review + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Install Claude CLI + run: npm install -g @anthropic-ai/claude-code + + - name: Get changed Java files + id: changed-files + run: | + FILES=$(git diff --name-only origin/${{ github.base_ref }}...${{ github.sha }} \ + | grep '\.java$' \ + | grep -v 'generated' \ + | tr '\n' ' ') + echo "files=$FILES" >> $GITHUB_OUTPUT + + - name: Run SDK patterns review + if: steps.changed-files.outputs.files != '' + id: review + continue-on-error: true + env: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + run: | + FILES="${{ steps.changed-files.outputs.files }}" + REVIEW=$(claude --print --model claude-sonnet-4-5 -p " + You are a senior engineer reviewing the Skyflow Java SDK. + + Review the following changed Java files for SDK pattern violations: + 1. Request/Response/Options patterns — builders are data holders, validation in Validations.java only + 2. Error handling — all public methods throw SkyflowException, no swallowed exceptions, no bare println/printStackTrace + 3. Naming — acronyms as words (skyflowId not skyflowID, tokenUri not tokenURI); UPPER_SNAKE constants; PascalCase classes + 4. Response normalisation — skyflowId not skyflow_id in response maps; getErrors() present on every response class + 5. Code quality — no magic strings (use Constants), no @SuppressWarnings without comment, deprecation via LogUtil.printWarningLog + + Skip src/main/java/com/skyflow/generated/ entirely. + + Files to review: $FILES + + For each file with findings, produce a markdown table: + | Severity | Line | Finding | + Severities: Critical (data loss/security), Bug (wrong behaviour), Quality (naming/patterns). + Skip Info-level observations. If no findings for a file, omit it. + If no findings at all, write: 'No issues found.' + + End with one of: APPROVE / APPROVE WITH FIXES / REQUEST CHANGES + ") + echo "result<> $GITHUB_OUTPUT + echo "$REVIEW" >> $GITHUB_OUTPUT + echo "EOF" >> $GITHUB_OUTPUT + + - name: Post review comment + if: steps.changed-files.outputs.files != '' + uses: actions/github-script@v7 + with: + script: | + const review = `${{ steps.review.outputs.result }}`; + const files = `${{ steps.changed-files.outputs.files }}`; + await github.rest.issues.createComment({ + ...context.repo, + issue_number: context.payload.pull_request.number, + body: `## Claude SDK Patterns Review\n\n${review}\n\n---\n_Files reviewed: \`${files}\`_` + }); + + security-review: + name: Security Review (serviceaccount changes) + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Check for serviceaccount changes + id: filter + uses: dorny/paths-filter@v3 + with: + filters: | + serviceaccount: + - 'src/**/serviceaccount/**/*.java' + + - name: Install Claude CLI + if: steps.filter.outputs.serviceaccount == 'true' + run: npm install -g @anthropic-ai/claude-code + + - name: Get changed serviceaccount files + if: steps.filter.outputs.serviceaccount == 'true' + id: sa-files + run: | + FILES=$(git diff --name-only origin/${{ github.base_ref }}...${{ github.sha }} \ + | grep 'serviceaccount' \ + | grep '\.java$' \ + | tr '\n' ' ') + echo "files=$FILES" >> $GITHUB_OUTPUT + + - name: Run security audit + if: steps.filter.outputs.serviceaccount == 'true' && steps.sa-files.outputs.files != '' + id: security + continue-on-error: true + env: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + run: | + FILES="${{ steps.sa-files.outputs.files }}" + AUDIT=$(claude --print --model claude-sonnet-4-5 -p " + You are a security engineer auditing the Skyflow Java SDK serviceaccount module. + + Audit the following files for: + 1. Credential and token exposure — bearer tokens, API keys, private keys must never appear in logs, error messages, or toString() output + 2. Path traversal — file paths passed to new File(path) must not allow ../ + 3. JSON parsing — JsonParser calls must be wrapped in try/catch for JsonSyntaxException + 4. HTTP security — all API calls must be HTTPS; Authorization headers must not be logged at any level + 5. Token lifecycle — bearer token caching must check expiry before reuse; token refresh must be thread-safe + + Files: $FILES + + For each finding: + **Severity:** Critical / High / Medium / Low + **File:Line:** path:N + **Risk:** one sentence + **Fix:** one concrete sentence + + If no findings, write: 'No security issues found.' + End with overall risk rating: LOW / MEDIUM / HIGH / CRITICAL + ") + echo "result<> $GITHUB_OUTPUT + echo "$AUDIT" >> $GITHUB_OUTPUT + echo "EOF" >> $GITHUB_OUTPUT + + - name: Post security comment + if: steps.filter.outputs.serviceaccount == 'true' && steps.sa-files.outputs.files != '' + uses: actions/github-script@v7 + with: + script: | + const audit = `${{ steps.security.outputs.result }}`; + const files = `${{ steps.sa-files.outputs.files }}`; + await github.rest.issues.createComment({ + ...context.repo, + issue_number: context.payload.pull_request.number, + body: `## Claude Security Audit (serviceaccount)\n\n${audit}\n\n---\n_Files audited: \`${files}\`_` + }); diff --git a/.github/workflows/release-v1.yml b/.github/workflows/release-v1.yml new file mode 100644 index 00000000..e69de29b diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 00000000..00904528 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,148 @@ +--- +name: skyflow-java-sdk +description: Skyflow Java SDK project context — naming conventions, build commands, known failures, and slash commands. Loaded for all Java source, test, and sample files. +paths: + - src/**/*.java + - samples/**/*.java + - pom.xml + - checkstyle.xml +--- + +# Skyflow Java SDK — Claude Code Instructions + +## Project Overview + +This is the Skyflow Java SDK (`skyflow-java`). It provides a Java interface to the Skyflow Data Privacy Vault API — vault operations (insert, get, update, delete, query, tokenize, detokenize), service account authentication (bearer tokens, signed data tokens), connections, detect, and audit. + +**v1 (maintenance mode, `v1` branch):** Security and bug fixes only — no new features. EOL announced: **October 31, 2026**. + +**Current stable version: v2.1** — supports PDB vaults. This is what customers use. + +**v3 (pre-release, Flow DB only):** v3 is *not* a full replacement for v2. It adds Flow DB-specific operations used by the [Spark wrapper](https://github.com/skyflowapi/vault-workflows): +- `bulkInsert` +- `batchProcessing` (`batchSize` + `concurrencyLimit`) + +v3 does not yet have full parity with v2. Do not treat v3 as the general SDK — scope v3 work strictly to Flow DB features unless explicitly told otherwise. + +## Critical Boundary — Generated Code + +**Never edit files under `src/main/java/com/skyflow/generated/`.** + +These are auto-generated by [Fern](https://buildwithfern.com) from the Skyflow API definition. Manual edits are overwritten on the next generation run. If you find a bug in generated code, report it — do not patch it directly. + +The `pom.xml` checkstyle and test configs already exclude `generated/` from all checks. + +## Project Structure + +``` +src/ + main/java/com/skyflow/ + config/ # VaultConfig, Credentials, ConnectionConfig + vault/ # controller/, data/, tokens/, connection/, audit/, bin/, detect/ + serviceaccount/ # BearerToken, SignedDataTokens (JWT + credential parsing) + enums/ # LogLevel, RedactionType, TokenMode, Env + errors/ # SkyflowException, ErrorCode, ErrorMessage + utils/ # Utils, Constants, HttpUtility, LogUtil, Validations + generated/ # ← FERN-GENERATED, DO NOT EDIT + test/java/com/skyflow/ + ... # JUnit 4 tests mirroring main structure +samples/ # Standalone Maven project — vault / serviceaccount / detect / connection +docs/ + superpowers/specs/ # Design specs + superpowers/plans/ # Implementation plans +``` + +## Naming Conventions + +- **Acronyms as words:** `skyflowId` (not `skyflowID`), `clientId` (not `clientID`), `tokenUri` (not `tokenURI`), `keyId` (not `keyID`) +- **Builder setters:** `setVaultId()`, `setClusterId()`, `setSkyflowId()` — never `setVaultID()` +- **Response maps:** always use `skyflowId` (camelCase) — the raw API returns `skyflow_id` (snake_case) which VaultController normalises before returning to callers +- **Constants class:** use `com.skyflow.utils.Constants` for string literals; `ErrorMessage` enum for error message strings + +## Build and Test + +```bash +mvn compile -q # compile +mvn checkstyle:check -q # lint (config: checkstyle.xml) +mvn test -q # full test suite (JUnit 4) +mvn test -Dtest=ClassName # single test class +mvn package -DskipTests -q # build jar +``` + +Samples (separate Maven project): +```bash +cd samples && mvn compile -q +``` + +## Credentials JSON Format + +The SDK reads a `credentials.json` file for service account authentication. The canonical field names (v3+) are: + +```json +{ + "clientId": "...", + "keyId": "...", + "tokenUri": "...", + "privateKey": "..." +} +``` + +The legacy all-caps forms (`clientID`, `keyID`, `tokenURI`) are accepted as fallbacks for migration. + +## Known Pre-existing Test Failures + +These failures exist on `main` and are **not regressions** — do not investigate them unless specifically asked: + +| Test class | Failure | Cause | +|---|---|---| +| `HttpUtilityTests` | `InaccessibleObject` (all tests) | JDK 21 + PowerMock incompatibility — PowerMock cannot reflect into `java.net` | +| `TokenTests#testExpiredTokenForIsExpiredToken` | Environment error | Requires live credentials | +| `VaultClientTests#testSetBearerTokenWithEnvCredentials` | Environment error | Requires `SKYFLOW_CREDENTIALS` env var | +| `ConnectionClientTests#testSetBearerTokenWithEnvCredentials` | Environment error | Requires `SKYFLOW_CREDENTIALS` env var | + +Run `mvn test -q 2>&1 | grep -E "Tests run|FAIL|ERROR"` to see the current baseline. + +## Active Work + +See `docs/superpowers/specs/` for in-progress design specs and `docs/superpowers/plans/` for implementation plans. + +## Slash Commands + +- `/code-review` — full review: SDK patterns + code smells + security (Steps 2 and 3 read `.claude/commands/code-smell.md` and `.claude/commands/code-security.md` at runtime) +- `/code-smell` — standalone structural smell analysis only (long methods, dead code, misplaced logic) +- `/code-security` — standalone security audit only (credentials, input validation, HTTP security) +- `/sdk-sample ` — generate a sample file for a feature +- `/test [ClassName]` — run quality pipeline (compile → checkstyle → build → test → coverage) +- `/commit ` — stage check + Jira-aware commit (extracts ticket ID from branch name) + +## Commit & PR Guidelines + +### Commit messages +Every commit on a PR branch **must** include a Jira ticket ID — enforced by the `check-commit-message` step in `.github/workflows/pr.yml`. + +Accepted formats: +``` +SK-1234 short description +SK-1234: short description +feat: SK-1234 short description +fix(SK-1234): short description +``` + +Exempt patterns (no ticket needed): +- `[AUTOMATED]` — release version bumps only +- `Merge ...` — merge commits +- `Release ...` — release commits + +Conventional Commits prefixes (`feat:`, `fix:`, `chore:`, `docs:`) are encouraged but only valid alongside a Jira ID. + +### Branch naming +Branch name must include your GitHub username: + +``` +/- +``` + +Example: `karthik/GV-770-ext-auth-json-error` + +### PR template +The `.github/pull_request_template.md` requires: **Why**, **Goal**, **Testing** sections. Tech debt section is optional. From 08df1abe892d1ef94747c1166cda598e778e273b Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 18:50:36 +0530 Subject: [PATCH 02/54] =?UTF-8?q?SK-2832=20remove=20release-v1.yml=20?= =?UTF-8?q?=E2=80=94=20not=20part=20of=20Claude=20setup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/workflows/release-v1.yml | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 .github/workflows/release-v1.yml diff --git a/.github/workflows/release-v1.yml b/.github/workflows/release-v1.yml deleted file mode 100644 index e69de29b..00000000 From 1819b057693a8695222d69e7f4994de63a89febb Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:03:02 +0530 Subject: [PATCH 03/54] SK-2832 split samples context into samples/CLAUDE.md - Move file placement, deprecated folder note, and sample rules into samples/CLAUDE.md (loads only when working on samples) - Remove samples paths and content from root CLAUDE.md to reduce token load on every non-samples session Co-Authored-By: Claude Sonnet 4.6 (1M context) --- CLAUDE.md | 9 +-------- samples/CLAUDE.md | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 8 deletions(-) create mode 100644 samples/CLAUDE.md diff --git a/CLAUDE.md b/CLAUDE.md index 00904528..ad46df3f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,9 +1,8 @@ --- name: skyflow-java-sdk -description: Skyflow Java SDK project context — naming conventions, build commands, known failures, and slash commands. Loaded for all Java source, test, and sample files. +description: Skyflow Java SDK project context — naming conventions, build commands, known failures, and slash commands. Loaded for all Java source files. paths: - src/**/*.java - - samples/**/*.java - pom.xml - checkstyle.xml --- @@ -46,7 +45,6 @@ src/ generated/ # ← FERN-GENERATED, DO NOT EDIT test/java/com/skyflow/ ... # JUnit 4 tests mirroring main structure -samples/ # Standalone Maven project — vault / serviceaccount / detect / connection docs/ superpowers/specs/ # Design specs superpowers/plans/ # Implementation plans @@ -69,11 +67,6 @@ mvn test -Dtest=ClassName # single test class mvn package -DskipTests -q # build jar ``` -Samples (separate Maven project): -```bash -cd samples && mvn compile -q -``` - ## Credentials JSON Format The SDK reads a `credentials.json` file for service account authentication. The canonical field names (v3+) are: diff --git a/samples/CLAUDE.md b/samples/CLAUDE.md new file mode 100644 index 00000000..9ec2ea9a --- /dev/null +++ b/samples/CLAUDE.md @@ -0,0 +1,45 @@ +--- +name: skyflow-java-samples +description: Samples project context — file placement, structure, and rules for Skyflow Java SDK sample files. +paths: + - "**/*.java" + - pom.xml +--- + +# Skyflow Java SDK — Samples + +Standalone Maven project demonstrating SDK features. Compile with: +```bash +cd samples && mvn compile -q +``` + +## File Placement + +| Feature | Package | Directory | +|---|---|---| +| Vault ops (insert/get/update/delete/query/tokenize/detokenize) | `com.example.vault` | `samples/src/main/java/com/example/vault/` | +| Service account auth | `com.example.serviceaccount` | `samples/src/main/java/com/example/serviceaccount/` | +| Connection | `com.example.connection` | `samples/src/main/java/com/example/connection/` | +| Detect | `com.example.detect` | `samples/src/main/java/com/example/detect/` | +| Audit event operations | `com.example.audit` | `samples/src/main/java/com/example/audit/` | +| BIN lookup | `com.example.bin` | `samples/src/main/java/com/example/bin/` | + +File name: `Example.java` + +## Deprecated Samples + +Deprecated examples (v1-era or superseded APIs) live in: +``` +samples/src/main/java/com/example/vault/deprecated/ +``` +Do not update deprecated samples — they are kept for reference only. New samples go in the parent package, not in `deprecated/`. + +## Rules + +- Vault IDs / cluster IDs: `""`, `""` +- Credential values: `""`, `""` +- Credentials file path: `"credentials.json"` (relative, never absolute) +- Always catch `SkyflowException` and print `e.getMessage()` +- No separate `*Options` classes — use request builder methods directly +- Keep under 80 lines +- Imports from `com.skyflow.*` only — never from `com.skyflow.generated.*` From c8b52b0a060c74309fcb9346e48612236ccce536 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:03:48 +0530 Subject: [PATCH 04/54] SK-2832 add basic samples reference to root CLAUDE.md --- CLAUDE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CLAUDE.md b/CLAUDE.md index ad46df3f..e847ac8b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,6 +45,7 @@ src/ generated/ # ← FERN-GENERATED, DO NOT EDIT test/java/com/skyflow/ ... # JUnit 4 tests mirroring main structure +samples/ # Standalone Maven project — see samples/CLAUDE.md for placement rules docs/ superpowers/specs/ # Design specs superpowers/plans/ # Implementation plans From 50fe8bffa2ea9350468cad6d9ad2560345f01552 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:08:53 +0530 Subject: [PATCH 05/54] SK-2832 make commit guidelines directive for Claude --- CLAUDE.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index e847ac8b..3e175bca 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -112,6 +112,8 @@ See `docs/superpowers/specs/` for in-progress design specs and `docs/superpowers ## Commit & PR Guidelines ### Commit messages +**When making any `git commit`, always extract the Jira ticket ID from the current branch name and include it in the message.** Use `/commit ` to do this automatically. + Every commit on a PR branch **must** include a Jira ticket ID — enforced by the `check-commit-message` step in `.github/workflows/pr.yml`. Accepted formats: From c3ec30ffc9250c42c28b9dd1daee2e68038374fa Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:09:50 +0530 Subject: [PATCH 06/54] SK-2832 replace redundant commit rules with /commit reference --- CLAUDE.md | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3e175bca..6ab538dc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -112,24 +112,7 @@ See `docs/superpowers/specs/` for in-progress design specs and `docs/superpowers ## Commit & PR Guidelines ### Commit messages -**When making any `git commit`, always extract the Jira ticket ID from the current branch name and include it in the message.** Use `/commit ` to do this automatically. - -Every commit on a PR branch **must** include a Jira ticket ID — enforced by the `check-commit-message` step in `.github/workflows/pr.yml`. - -Accepted formats: -``` -SK-1234 short description -SK-1234: short description -feat: SK-1234 short description -fix(SK-1234): short description -``` - -Exempt patterns (no ticket needed): -- `[AUTOMATED]` — release version bumps only -- `Merge ...` — merge commits -- `Release ...` — release commits - -Conventional Commits prefixes (`feat:`, `fix:`, `chore:`, `docs:`) are encouraged but only valid alongside a Jira ID. +Always use `/commit ` when committing — it extracts the Jira ticket ID from the branch name and validates the format against the CI check in `.github/workflows/pr.yml`. ### Branch naming Branch name must include your GitHub username: From 9de9e92517991c3b5de80bceb04905452826ed93 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:10:38 +0530 Subject: [PATCH 07/54] fix: example --- CLAUDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6ab538dc..6edbafd1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -121,7 +121,7 @@ Branch name must include your GitHub username: /- ``` -Example: `karthik/GV-770-ext-auth-json-error` +Example: `devesh/SK-770-ext-auth-json-error` ### PR template The `.github/pull_request_template.md` requires: **Why**, **Goal**, **Testing** sections. Tech debt section is optional. From a9647cee1f2e41044865af2c12aa06f9fe3b79a8 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:16:36 +0530 Subject: [PATCH 08/54] SK-2832 rename /test to /quality, add 100% coverage requirement --- .claude/commands/{test.md => quality.md} | 40 +++++++++++++++++------- CLAUDE.md | 2 +- 2 files changed, 29 insertions(+), 13 deletions(-) rename .claude/commands/{test.md => quality.md} (50%) diff --git a/.claude/commands/test.md b/.claude/commands/quality.md similarity index 50% rename from .claude/commands/test.md rename to .claude/commands/quality.md index e690a355..77e363ed 100644 --- a/.claude/commands/test.md +++ b/.claude/commands/quality.md @@ -1,6 +1,6 @@ --- -name: test -description: Quality pipeline — compile, checkstyle, build, tests, coverage analysis. Pass a class name to target a single test class. +name: quality +description: Quality pipeline — compile, checkstyle, build, tests, coverage check. Pass a class name to target a single test class. paths: - src/**/*.java - pom.xml @@ -13,6 +13,19 @@ Use `$ARGUMENTS` to target a specific test class (e.g. `BearerTokenTests`). If e > Baseline failures are listed in CLAUDE.md under "Known Pre-existing Test Failures". > Do not investigate them unless specifically asked. Only report failures **beyond** that baseline. +## Coverage Requirements + +**All code written or modified by Claude must have 100% coverage — both instruction and branch.** + +**All public interfaces must have 100% coverage — both instruction and branch.** This applies to: +- All classes under `src/main/java/com/skyflow/vault/` (controllers, data, tokens, connection, audit, bin, detect) +- All classes under `src/main/java/com/skyflow/config/` +- All classes under `src/main/java/com/skyflow/serviceaccount/` + +Flag any gap as a blocker — **NEEDS FIXES** if coverage is below 100% on Claude-written or public interface code. + +--- + ## Pipeline ### Step 1 — Compile @@ -45,12 +58,15 @@ mvn test -q 2>&1 | tail -40 Report: tests run, failures, errors. Flag any pre-existing failures separately from new ones. ### Step 5 — Coverage analysis -Flag any public interface class (`src/main/java/com/skyflow/vault/`, `src/main/java/com/skyflow/config/`, `src/main/java/com/skyflow/serviceaccount/`) that has no corresponding test file under `src/test/`. +For every public interface class and every class touched by Claude in this session: +- Check for a corresponding test file under `src/test/` +- Check that every public method has at least one positive and one negative test case +- Check that every branch (if/else, switch, try/catch) is covered -For classes that do have tests, check whether each public method has at least one positive and one negative test case. List any gaps. +List all gaps. Any gap on Claude-written or public interface code is a **blocker**. ### Step 6 — Edge case identification -For any test class below complete coverage, identify missing scenarios: +For any class below 100% coverage, identify missing scenarios: - Null / empty inputs - Invalid types / wrong enum values - Concurrent / reuse scenarios @@ -61,13 +77,13 @@ Write concrete JUnit 4 test method stubs (not full implementations) for each gap ### Step 7 — Report ``` -| Step | Status | Notes | -|---|---|---| -| Compile | ✅ / ❌ | ... | -| Checkstyle | ✅ / ❌ | ... | -| Build | ✅ / ❌ | ... | -| Tests | ✅ / ❌ | N passed, M failed | -| Coverage gaps | ... | list classes | +| Step | Status | Notes | +|------------------|-----------|------------------------------| +| Compile | ✅ / ❌ | ... | +| Checkstyle | ✅ / ❌ | ... | +| Build | ✅ / ❌ | ... | +| Tests | ✅ / ❌ | N passed, M failed | +| Coverage (100%) | ✅ / ❌ | list classes with gaps | ``` Conclude with **READY TO MERGE** or **NEEDS FIXES** and a prioritised fix list. diff --git a/CLAUDE.md b/CLAUDE.md index 6edbafd1..5e4747cd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -106,7 +106,7 @@ See `docs/superpowers/specs/` for in-progress design specs and `docs/superpowers - `/code-smell` — standalone structural smell analysis only (long methods, dead code, misplaced logic) - `/code-security` — standalone security audit only (credentials, input validation, HTTP security) - `/sdk-sample ` — generate a sample file for a feature -- `/test [ClassName]` — run quality pipeline (compile → checkstyle → build → test → coverage) +- `/quality [ClassName]` — run quality pipeline (compile → checkstyle → build → test → 100% coverage check) - `/commit ` — stage check + Jira-aware commit (extracts ticket ID from branch name) ## Commit & PR Guidelines From 1b54cee9f64682cca6f36f0ead1e011485fda9ea Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:17:29 +0530 Subject: [PATCH 09/54] SK-2832 reference /quality in code-review final verdict --- .claude/commands/code-review.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 498fcfac..616c590c 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -105,3 +105,4 @@ Read the file `.claude/commands/code-security.md` and follow all of its instruct After all three steps, close with: 1. A tech-debt summary table grouped by category (SDK Patterns / Error Handling / Naming / Tests / Smells / Security) 2. A verdict: `APPROVE` / `APPROVE WITH FIXES` / `REQUEST CHANGES` +3. If verdict is not `APPROVE`, remind: run `/quality` to verify compile, tests, and 100% coverage before merging. From d3dc74b5f840fcc779758b519c8c5a3b81266d06 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:18:08 +0530 Subject: [PATCH 10/54] SK-2832 add /quality check step to /commit before committing --- .claude/commands/commit.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.claude/commands/commit.md b/.claude/commands/commit.md index 6aae4614..2a28b220 100644 --- a/.claude/commands/commit.md +++ b/.claude/commands/commit.md @@ -46,7 +46,11 @@ Validate against the `pr.yml` enforced pattern: `(\[?[A-Z]{1,10}-[1-9][0-9]*)|(\ - Must contain a Jira ID — a bare description without a ticket ID will fail CI. - If validation fails, report the exact requirement and stop. -## Step 4 — Commit +## Step 4 — Quality check + +Before committing, confirm `/quality` has been run and passed (compile, tests, 100% coverage on changed code). If it has not been run, ask the user whether to run it now before proceeding. + +## Step 5 — Commit ```bash git commit -m "" From 390b5133d56824085b334480f5589002a221846d Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:21:47 +0530 Subject: [PATCH 11/54] SK-2832 enforce /commit rule in CLAUDE.md, add /quality to review skill --- .claude/skills/requesting-code-review/SKILL.md | 2 ++ CLAUDE.md | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.claude/skills/requesting-code-review/SKILL.md b/.claude/skills/requesting-code-review/SKILL.md index 9f662842..46967910 100644 --- a/.claude/skills/requesting-code-review/SKILL.md +++ b/.claude/skills/requesting-code-review/SKILL.md @@ -31,6 +31,7 @@ paths: | SDK logic, patterns, naming, tests | `/code-review` — SDK checks + smell + security | | Structural debt only | `/code-smell` — standalone smell analysis | | Auth, credentials, tokens, HTTP | `/code-security` — standalone security audit | +| Compile + tests + 100% coverage | `/quality` — run after fixing review findings, before `/commit` | For security-sensitive changes, run both: ```bash @@ -74,3 +75,4 @@ HEAD_SHA=$(git rev-parse HEAD) - Fix Important issues before proceeding - Note Minor/Smell issues for later - Push back with reasoning if you disagree +- After fixing: run `/quality` to verify compile + tests + 100% coverage, then `/commit` diff --git a/CLAUDE.md b/CLAUDE.md index 5e4747cd..c6487c09 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -112,7 +112,7 @@ See `docs/superpowers/specs/` for in-progress design specs and `docs/superpowers ## Commit & PR Guidelines ### Commit messages -Always use `/commit ` when committing — it extracts the Jira ticket ID from the branch name and validates the format against the CI check in `.github/workflows/pr.yml`. +**Never run `git commit` directly. Always use `/commit `** — it extracts the Jira ticket ID from the branch name, confirms `/quality` has passed, and validates the format against the CI check in `.github/workflows/pr.yml`. ### Branch naming Branch name must include your GitHub username: From 2f1eac1f68bdc1400d7380dcd234797b18176b30 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:24:09 +0530 Subject: [PATCH 12/54] SK-2832 require /quality before code-review, not just after --- .claude/commands/code-review.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 616c590c..fae1ff9a 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -8,6 +8,10 @@ paths: You are a senior engineer performing a thorough code review on the Skyflow Java SDK. +## Pre-requisite + +Before starting the review, confirm `/quality` has been run and passed (compile, tests, 100% coverage). If it has not been run, run it now before proceeding with the review. + ## Scope Use `$ARGUMENTS` to determine scope: @@ -105,4 +109,4 @@ Read the file `.claude/commands/code-security.md` and follow all of its instruct After all three steps, close with: 1. A tech-debt summary table grouped by category (SDK Patterns / Error Handling / Naming / Tests / Smells / Security) 2. A verdict: `APPROVE` / `APPROVE WITH FIXES` / `REQUEST CHANGES` -3. If verdict is not `APPROVE`, remind: run `/quality` to verify compile, tests, and 100% coverage before merging. +3. Remind: run `/quality` again after any fixes before merging. From 80a5ebfc9e7c7c453673918d12eefb497887f21c Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:26:40 +0530 Subject: [PATCH 13/54] SK-2832 add descriptive header to checkstyle-on-edit.py --- .claude/hooks/checkstyle-on-edit.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/.claude/hooks/checkstyle-on-edit.py b/.claude/hooks/checkstyle-on-edit.py index 4fd0787d..689b2b74 100644 --- a/.claude/hooks/checkstyle-on-edit.py +++ b/.claude/hooks/checkstyle-on-edit.py @@ -1,3 +1,18 @@ +# checkstyle-on-edit.py — PostToolUse hook for the Skyflow Java SDK. +# +# Registered in .claude/settings.json under hooks.PostToolUse with matcher "Edit|Write". +# Fires automatically after every Edit or Write tool call on any file. +# +# What it does: +# - Ignores non-.java files immediately (no Maven overhead). +# - For files under src/main/java/, runs checkstyle scoped to that single file +# via -Dcheckstyle.includes= to keep it fast. +# - For files outside src/main/java/ (e.g. tests, samples), runs full-module checkstyle. +# - Prints the last 20 lines of any violations so Claude sees them in-turn +# without needing a separate /quality run. +# +# Config: checkstyle.xml — generated/ is excluded by pom.xml config so Fern +# auto-generated code is never flagged. import sys, json, subprocess, os d = json.load(sys.stdin) From fedad62c0373ff8e60b58e5a1c70e4677d039e26 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:52:41 +0530 Subject: [PATCH 14/54] SK-2832 remove paths from review skill, delegate all rules to code-review.md --- .claude/skills/requesting-code-review/SKILL.md | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/.claude/skills/requesting-code-review/SKILL.md b/.claude/skills/requesting-code-review/SKILL.md index 46967910..e4290c7c 100644 --- a/.claude/skills/requesting-code-review/SKILL.md +++ b/.claude/skills/requesting-code-review/SKILL.md @@ -1,9 +1,6 @@ --- name: requesting-code-review description: Use when completing tasks, implementing major features, or before merging to verify work meets requirements -paths: - - src/main/java/**/*.java - - src/test/java/**/*.java --- # Requesting Code Review @@ -50,7 +47,8 @@ Agent tool (general-purpose): You are a senior engineer reviewing the Skyflow Java SDK. Read CLAUDE.md for project conventions, then read and follow - .claude/commands/code-review.md for the full review process. + .claude/commands/code-review.md for the full review process + including all rules, output format, and act-on-feedback guidance. Git range to review: Base: {BASE_SHA} @@ -70,9 +68,4 @@ BASE_SHA=$(git merge-base main HEAD) # branch vs main HEAD_SHA=$(git rev-parse HEAD) ``` -**3. Act on feedback:** -- Fix Critical issues immediately -- Fix Important issues before proceeding -- Note Minor/Smell issues for later -- Push back with reasoning if you disagree -- After fixing: run `/quality` to verify compile + tests + 100% coverage, then `/commit` +All review rules, severity definitions, output format, and post-review steps are defined in `.claude/commands/code-review.md` — that file is the single source of truth. From cadbca9d196248cd74d8c5d8acdc2d08705f5d9d Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:55:20 +0530 Subject: [PATCH 15/54] SK-2832 restore paths to review skill for proactive review triggers --- .claude/skills/requesting-code-review/SKILL.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.claude/skills/requesting-code-review/SKILL.md b/.claude/skills/requesting-code-review/SKILL.md index e4290c7c..cdb1ddf2 100644 --- a/.claude/skills/requesting-code-review/SKILL.md +++ b/.claude/skills/requesting-code-review/SKILL.md @@ -1,6 +1,9 @@ --- name: requesting-code-review description: Use when completing tasks, implementing major features, or before merging to verify work meets requirements +paths: + - src/main/java/**/*.java + - src/test/java/**/*.java --- # Requesting Code Review From 967c3c99bab092277a2b40b1d901c72cfae3c87b Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 19:58:38 +0530 Subject: [PATCH 16/54] SK-2832 add devesh to cspell wordlist --- .cspell.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.cspell.json b/.cspell.json index a982f837..6f06b9ef 100644 --- a/.cspell.json +++ b/.cspell.json @@ -99,7 +99,8 @@ "detok", "qhdmceurtnlz", "ngrok", - "obac" + "obac", + "devesh" ], "languageSettings": [ { From ed287b107f8ef75277e31cf30fedb669ed2e85e7 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 29 May 2026 20:00:08 +0530 Subject: [PATCH 17/54] SK-2832 suppress cspell on branch name example, revert devesh from wordlist --- .cspell.json | 3 +-- CLAUDE.md | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.cspell.json b/.cspell.json index 6f06b9ef..a982f837 100644 --- a/.cspell.json +++ b/.cspell.json @@ -99,8 +99,7 @@ "detok", "qhdmceurtnlz", "ngrok", - "obac", - "devesh" + "obac" ], "languageSettings": [ { diff --git a/CLAUDE.md b/CLAUDE.md index c6487c09..a5d3e94c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -121,6 +121,7 @@ Branch name must include your GitHub username: /- ``` + Example: `devesh/SK-770-ext-auth-json-error` ### PR template From e437f4485d5a1c22f1d3e50a3c8394b471f0f28b Mon Sep 17 00:00:00 2001 From: skyflow-bharti Date: Mon, 1 Jun 2026 12:29:05 +0530 Subject: [PATCH 18/54] SK-2832 update the claude setup and remove duplicate things --- .claude/commands/code-patterns.md | 59 +++++++++++++ .../commands/{quality.md => code-quality.md} | 2 +- .claude/commands/code-review.md | 68 ++------------- .claude/commands/code-security.md | 2 + .claude/commands/code-smell.md | 2 + .claude/commands/commit.md | 2 +- .../skills/requesting-code-review/SKILL.md | 2 +- CLAUDE.md | 24 +++++- .../java/com/skyflow/ConnectionClient.java | 4 +- src/main/java/com/skyflow/VaultClient.java | 86 ++++++++++++++++--- .../java/com/skyflow/utils/HttpUtility.java | 5 +- 11 files changed, 172 insertions(+), 84 deletions(-) create mode 100644 .claude/commands/code-patterns.md rename .claude/commands/{quality.md => code-quality.md} (99%) diff --git a/.claude/commands/code-patterns.md b/.claude/commands/code-patterns.md new file mode 100644 index 00000000..dfacb1d8 --- /dev/null +++ b/.claude/commands/code-patterns.md @@ -0,0 +1,59 @@ +--- +name: code-patterns +description: SDK pattern review — request/response/options shape, error handling, naming, test coverage, code quality. +paths: + - src/main/java/**/*.java + - src/test/java/**/*.java +exclude: + - src/main/java/com/skyflow/generated/** +--- + +You are a senior engineer reviewing the Skyflow Java SDK against its established patterns. + +Coding rules (error handling, request/response shape, naming, no magic strings) are defined in CLAUDE.md — apply those during this review. + +## Scope + +Files are passed in by the caller (usually `/code-review`). Review only those files. + +--- + +## 1. 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 + +--- + +## 2. 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` + +--- + +## Output format + +Group findings by 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 | +``` + +**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 | diff --git a/.claude/commands/quality.md b/.claude/commands/code-quality.md similarity index 99% rename from .claude/commands/quality.md rename to .claude/commands/code-quality.md index 77e363ed..21ce6aa9 100644 --- a/.claude/commands/quality.md +++ b/.claude/commands/code-quality.md @@ -1,5 +1,5 @@ --- -name: quality +name: code-quality description: Quality pipeline — compile, checkstyle, build, tests, coverage check. Pass a class name to target a single test class. paths: - src/**/*.java diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index fae1ff9a..6fa74777 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -4,13 +4,15 @@ description: Full code review — SDK patterns, naming, test coverage, then runs paths: - src/main/java/**/*.java - src/test/java/**/*.java +exclude: + - src/main/java/com/skyflow/generated/** --- You are a senior engineer performing a thorough code review on the Skyflow Java SDK. ## Pre-requisite -Before starting the review, confirm `/quality` has been run and passed (compile, tests, 100% coverage). If it has not been run, run it now before proceeding with the review. +Before starting the review, confirm `/code-quality` has been run and passed (compile, tests, 100% coverage). If it has not been run, run it now before proceeding with the review. ## Scope @@ -28,67 +30,7 @@ Use `$ARGUMENTS` to determine scope: ## Step 1 — SDK Pattern Review -Check the files in scope against the rules below. - -### 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>` 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 and response field normalisation - -Follow the conventions in CLAUDE.md under "Naming Conventions". Key enforcement points: -- Acronyms as words: `skyflowId`, `tokenUri`, `clientId` — never uppercase abbreviations -- Builder setters: `setFooId()` not `setFooID()`; constants: `UPPER_SNAKE_CASE`; classes: `PascalCase` -- Response maps: `skyflowId` (camelCase) only — never `skyflow_id`; `getErrors()` must be present on every response class -- Deprecated methods: `@Deprecated(since = "x.x", forRemoval = true)` + `@deprecated` Javadoc with `{@link}` to replacement - -### 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` - -### Output for Step 1 - -Group findings by 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 | -``` - -**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 | +Read the file `.claude/commands/code-patterns.md` and follow all of its instructions for the same files in scope. Produce its full output (per-file findings table + severity key). --- @@ -109,4 +51,4 @@ Read the file `.claude/commands/code-security.md` and follow all of its instruct After all three steps, close with: 1. A tech-debt summary table grouped by category (SDK Patterns / Error Handling / Naming / Tests / Smells / Security) 2. A verdict: `APPROVE` / `APPROVE WITH FIXES` / `REQUEST CHANGES` -3. Remind: run `/quality` again after any fixes before merging. +3. Remind: run `/code-quality` again after any fixes before merging. diff --git a/.claude/commands/code-security.md b/.claude/commands/code-security.md index 0fa69923..7de061f6 100644 --- a/.claude/commands/code-security.md +++ b/.claude/commands/code-security.md @@ -7,6 +7,8 @@ paths: - src/main/java/com/skyflow/utils/**/*.java - src/main/java/com/skyflow/vault/controller/**/*.java - pom.xml +exclude: + - src/main/java/com/skyflow/generated/** --- You are a security engineer auditing the Skyflow Java SDK for vulnerabilities. diff --git a/.claude/commands/code-smell.md b/.claude/commands/code-smell.md index f456a8d8..4cc5953c 100644 --- a/.claude/commands/code-smell.md +++ b/.claude/commands/code-smell.md @@ -3,6 +3,8 @@ 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 +exclude: + - src/main/java/com/skyflow/generated/** --- You are a senior engineer performing a code smell analysis on the Skyflow Java SDK. diff --git a/.claude/commands/commit.md b/.claude/commands/commit.md index 2a28b220..651cc507 100644 --- a/.claude/commands/commit.md +++ b/.claude/commands/commit.md @@ -48,7 +48,7 @@ Validate against the `pr.yml` enforced pattern: `(\[?[A-Z]{1,10}-[1-9][0-9]*)|(\ ## Step 4 — Quality check -Before committing, confirm `/quality` has been run and passed (compile, tests, 100% coverage on changed code). If it has not been run, ask the user whether to run it now before proceeding. +Before committing, confirm `/code-quality` has been run and passed (compile, tests, 100% coverage on changed code). If it has not been run, ask the user whether to run it now before proceeding. ## Step 5 — Commit diff --git a/.claude/skills/requesting-code-review/SKILL.md b/.claude/skills/requesting-code-review/SKILL.md index cdb1ddf2..5d89b793 100644 --- a/.claude/skills/requesting-code-review/SKILL.md +++ b/.claude/skills/requesting-code-review/SKILL.md @@ -31,7 +31,7 @@ paths: | SDK logic, patterns, naming, tests | `/code-review` — SDK checks + smell + security | | Structural debt only | `/code-smell` — standalone smell analysis | | Auth, credentials, tokens, HTTP | `/code-security` — standalone security audit | -| Compile + tests + 100% coverage | `/quality` — run after fixing review findings, before `/commit` | +| Compile + tests + 100% coverage | `/code-quality` — run after fixing review findings, before `/commit` | For security-sensitive changes, run both: ```bash diff --git a/CLAUDE.md b/CLAUDE.md index a5d3e94c..6e83fcb2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -5,6 +5,8 @@ paths: - src/**/*.java - pom.xml - checkstyle.xml +exclude: + - src/main/java/com/skyflow/generated/** --- # Skyflow Java SDK — Claude Code Instructions @@ -58,6 +60,26 @@ docs/ - **Response maps:** always use `skyflowId` (camelCase) — the raw API returns `skyflow_id` (snake_case) which VaultController normalises before returning to callers - **Constants class:** use `com.skyflow.utils.Constants` for string literals; `ErrorMessage` enum for error message strings +## SDK Coding Rules + +These apply whenever writing or modifying code — not just during review. + +### Error handling +- All public methods must declare `throws SkyflowException` +- Never swallow exceptions — always re-throw as `SkyflowException` +- No `System.out.println` or `e.printStackTrace()` — use `LogUtil` +- `catch (Exception e)` without re-throw is always a bug + +### Request / Response patterns +- Request builders are data holders — validation belongs in `Validations.validateXxxRequest()`, not in `build()` +- No separate `*Options` classes — options are fields on the request builder itself +- All response classes must have `getErrors()` returning `null` when no errors + +### String literals +- Use `Constants` for string literals and `ErrorMessage` enum for error messages — no magic strings + +--- + ## Build and Test ```bash @@ -112,7 +134,7 @@ See `docs/superpowers/specs/` for in-progress design specs and `docs/superpowers ## Commit & PR Guidelines ### Commit messages -**Never run `git commit` directly. Always use `/commit `** — it extracts the Jira ticket ID from the branch name, confirms `/quality` has passed, and validates the format against the CI check in `.github/workflows/pr.yml`. +**Never run `git commit` directly. Always use `/commit `** — it extracts the Jira ticket ID from the branch name, confirms `/code-quality` has passed, and validates the format against the CI check in `.github/workflows/pr.yml`. ### Branch naming Branch name must include your GitHub username: diff --git a/src/main/java/com/skyflow/ConnectionClient.java b/src/main/java/com/skyflow/ConnectionClient.java index d67122ad..3715b509 100644 --- a/src/main/java/com/skyflow/ConnectionClient.java +++ b/src/main/java/com/skyflow/ConnectionClient.java @@ -40,7 +40,7 @@ protected void updateConnectionConfig(ConnectionConfig connectionConfig) throws prioritiseCredentials(); } - protected void setBearerToken() throws SkyflowException { + protected synchronized void setBearerToken() throws SkyflowException { prioritiseCredentials(); Validations.validateCredentials(this.finalCredentials); if (this.finalCredentials.getApiKey() != null) { @@ -89,7 +89,7 @@ private void prioritiseCredentials() throws SkyflowException { } catch (SkyflowException e) { throw e; } catch (Exception e) { - throw new RuntimeException(e); + throw new SkyflowException(ErrorCode.SERVER_ERROR.getCode(), ErrorMessage.EmptyCredentials.getMessage()); } } diff --git a/src/main/java/com/skyflow/VaultClient.java b/src/main/java/com/skyflow/VaultClient.java index 1d5e5d74..5060c48f 100644 --- a/src/main/java/com/skyflow/VaultClient.java +++ b/src/main/java/com/skyflow/VaultClient.java @@ -1,5 +1,18 @@ package com.skyflow; +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.util.ArrayList; +import java.util.Base64; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; + import com.skyflow.config.Credentials; import com.skyflow.config.VaultConfig; import com.skyflow.enums.DetectEntities; @@ -10,8 +23,24 @@ import com.skyflow.generated.rest.ApiClient; import com.skyflow.generated.rest.ApiClientBuilder; import com.skyflow.generated.rest.resources.files.FilesClient; -import com.skyflow.generated.rest.resources.files.requests.*; -import com.skyflow.generated.rest.resources.files.types.*; +import com.skyflow.generated.rest.resources.files.requests.DeidentifyFileAudioRequestDeidentifyAudio; +import com.skyflow.generated.rest.resources.files.requests.DeidentifyFileDocumentPdfRequestDeidentifyPdf; +import com.skyflow.generated.rest.resources.files.requests.DeidentifyFileImageRequestDeidentifyImage; +import com.skyflow.generated.rest.resources.files.requests.DeidentifyFileRequestDeidentifyDocument; +import com.skyflow.generated.rest.resources.files.requests.DeidentifyFileRequestDeidentifyPresentation; +import com.skyflow.generated.rest.resources.files.requests.DeidentifyFileRequestDeidentifySpreadsheet; +import com.skyflow.generated.rest.resources.files.requests.DeidentifyFileRequestDeidentifyStructuredText; +import com.skyflow.generated.rest.resources.files.types.DeidentifyFileAudioRequestDeidentifyAudioEntityTypesItem; +import com.skyflow.generated.rest.resources.files.types.DeidentifyFileAudioRequestDeidentifyAudioOutputTranscription; +import com.skyflow.generated.rest.resources.files.types.DeidentifyFileDocumentPdfRequestDeidentifyPdfEntityTypesItem; +import com.skyflow.generated.rest.resources.files.types.DeidentifyFileImageRequestDeidentifyImageEntityTypesItem; +import com.skyflow.generated.rest.resources.files.types.DeidentifyFileImageRequestDeidentifyImageMaskingMethod; +import com.skyflow.generated.rest.resources.files.types.DeidentifyFileRequestDeidentifyDocumentEntityTypesItem; +import com.skyflow.generated.rest.resources.files.types.DeidentifyFileRequestDeidentifyPresentationEntityTypesItem; +import com.skyflow.generated.rest.resources.files.types.DeidentifyFileRequestDeidentifySpreadsheetEntityTypesItem; +import com.skyflow.generated.rest.resources.files.types.DeidentifyFileRequestDeidentifyStructuredTextEntityTypesItem; +import com.skyflow.generated.rest.resources.files.types.DeidentifyFileRequestDeidentifyTextEntityTypesItem; +import com.skyflow.generated.rest.resources.files.types.DeidentifyFileRequestEntityTypesItem; import com.skyflow.generated.rest.resources.query.QueryClient; import com.skyflow.generated.rest.resources.records.RecordsClient; import com.skyflow.generated.rest.resources.records.requests.RecordServiceBatchOperationBody; @@ -24,8 +53,40 @@ import com.skyflow.generated.rest.resources.tokens.TokensClient; import com.skyflow.generated.rest.resources.tokens.requests.V1DetokenizePayload; import com.skyflow.generated.rest.resources.tokens.requests.V1TokenizePayload; +import com.skyflow.generated.rest.types.BatchRecordMethod; +import com.skyflow.generated.rest.types.DeidentifyStringResponse; +import com.skyflow.generated.rest.types.FileData; +import com.skyflow.generated.rest.types.FileDataDataFormat; +import com.skyflow.generated.rest.types.FileDataDeidentifyAudio; +import com.skyflow.generated.rest.types.FileDataDeidentifyAudioDataFormat; +import com.skyflow.generated.rest.types.FileDataDeidentifyDocument; +import com.skyflow.generated.rest.types.FileDataDeidentifyDocumentDataFormat; +import com.skyflow.generated.rest.types.FileDataDeidentifyImage; +import com.skyflow.generated.rest.types.FileDataDeidentifyImageDataFormat; +import com.skyflow.generated.rest.types.FileDataDeidentifyPdf; +import com.skyflow.generated.rest.types.FileDataDeidentifyPresentation; +import com.skyflow.generated.rest.types.FileDataDeidentifyPresentationDataFormat; +import com.skyflow.generated.rest.types.FileDataDeidentifySpreadsheet; +import com.skyflow.generated.rest.types.FileDataDeidentifySpreadsheetDataFormat; +import com.skyflow.generated.rest.types.FileDataDeidentifyStructuredText; +import com.skyflow.generated.rest.types.FileDataDeidentifyStructuredTextDataFormat; +import com.skyflow.generated.rest.types.FileDataDeidentifyText; +import com.skyflow.generated.rest.types.Format; +import com.skyflow.generated.rest.types.FormatMaskedItem; +import com.skyflow.generated.rest.types.FormatPlaintextItem; +import com.skyflow.generated.rest.types.FormatRedactedItem; +import com.skyflow.generated.rest.types.ShiftDates; +import com.skyflow.generated.rest.types.ShiftDatesEntityTypesItem; +import com.skyflow.generated.rest.types.StringResponseEntities; +import com.skyflow.generated.rest.types.TokenTypeMapping; +import com.skyflow.generated.rest.types.TokenTypeMappingEntityOnlyItem; +import com.skyflow.generated.rest.types.TokenTypeMappingEntityUnqCounterItem; +import com.skyflow.generated.rest.types.TokenTypeMappingVaultTokenItem; import com.skyflow.generated.rest.types.Transformations; -import com.skyflow.generated.rest.types.*; +import com.skyflow.generated.rest.types.V1BatchRecord; +import com.skyflow.generated.rest.types.V1DetokenizeRecordRequest; +import com.skyflow.generated.rest.types.V1FieldRecords; +import com.skyflow.generated.rest.types.V1TokenizeRecordRequest; import com.skyflow.logs.InfoLogs; import com.skyflow.serviceaccount.util.Token; import com.skyflow.utils.Constants; @@ -36,24 +97,23 @@ import com.skyflow.vault.data.InsertRequest; import com.skyflow.vault.data.UpdateRequest; import com.skyflow.vault.detect.DeidentifyFileRequest; -import com.skyflow.vault.detect.*; +import com.skyflow.vault.detect.DeidentifyTextRequest; +import com.skyflow.vault.detect.DeidentifyTextResponse; +import com.skyflow.vault.detect.EntityInfo; +import com.skyflow.vault.detect.ReidentifyTextRequest; +import com.skyflow.vault.detect.TextIndex; +import com.skyflow.vault.detect.TokenFormat; import com.skyflow.vault.tokens.ColumnValue; import com.skyflow.vault.tokens.DetokenizeData; import com.skyflow.vault.tokens.DetokenizeRequest; import com.skyflow.vault.tokens.TokenizeRequest; + import io.github.cdimascio.dotenv.Dotenv; import io.github.cdimascio.dotenv.DotenvException; import okhttp3.ConnectionPool; import okhttp3.OkHttpClient; import okhttp3.Request; -import java.io.File; -import java.io.IOException; -import java.nio.file.Files; -import java.util.*; -import java.util.concurrent.TimeUnit; -import java.util.stream.Collectors; - public class VaultClient { private final VaultConfig vaultConfig; @@ -232,7 +292,7 @@ protected File getFileForFileUpload(FileUploadRequest fileUploadRequest) throws return null; } - protected void setBearerToken() throws SkyflowException { + protected synchronized void setBearerToken() throws SkyflowException { prioritiseCredentials(); Validations.validateCredentials(this.finalCredentials); if (this.finalCredentials.getApiKey() != null) { @@ -879,7 +939,7 @@ private void prioritiseCredentials() throws SkyflowException { } catch (SkyflowException e) { throw e; } catch (Exception e) { - throw new RuntimeException(e); + throw new SkyflowException(ErrorCode.SERVER_ERROR.getCode(), ErrorMessage.EmptyCredentials.getMessage()); } } } diff --git a/src/main/java/com/skyflow/utils/HttpUtility.java b/src/main/java/com/skyflow/utils/HttpUtility.java index 671e2415..811bd553 100644 --- a/src/main/java/com/skyflow/utils/HttpUtility.java +++ b/src/main/java/com/skyflow/utils/HttpUtility.java @@ -2,6 +2,7 @@ import com.google.gson.JsonElement; import com.google.gson.JsonObject; +import com.skyflow.errors.ErrorMessage; import com.skyflow.errors.SkyflowException; import java.io.*; @@ -83,8 +84,8 @@ public static String sendRequest(String method, URL url, JsonObject params, Map< if (connection.getErrorStream() != null) streamReader = new InputStreamReader(connection.getErrorStream()); else { - String description = appendRequestId("replace with description", requestID); - throw new SkyflowException(description); + String description = appendRequestId(ErrorMessage.ErrorOccurred.getMessage(), requestID); + throw new SkyflowException(httpCode, new Throwable(description), responseHeaders, "{}"); } } else { streamReader = new InputStreamReader(connection.getInputStream()); From d571befedd8cff100b2a6fa793c0eaa3e7f77ddf Mon Sep 17 00:00:00 2001 From: skyflow-bharti Date: Mon, 1 Jun 2026 12:49:07 +0530 Subject: [PATCH 19/54] SK-2832 Rename the commit md file to git-commit --- .claude/commands/{commit.md => git-commit.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .claude/commands/{commit.md => git-commit.md} (100%) diff --git a/.claude/commands/commit.md b/.claude/commands/git-commit.md similarity index 100% rename from .claude/commands/commit.md rename to .claude/commands/git-commit.md From 5ad91d6eae5dfee4c91113b344a1cadc247713e3 Mon Sep 17 00:00:00 2001 From: skyflow-bharti Date: Mon, 1 Jun 2026 15:30:10 +0530 Subject: [PATCH 20/54] SK-2832 Removed the duplicate section from commands --- .claude/commands/code-patterns.md | 12 +++-------- .claude/commands/code-quality.md | 15 +++++++------- .claude/commands/code-review.md | 3 +-- .claude/commands/code-security.md | 8 ++------ .claude/commands/code-smell.md | 6 ++++-- .claude/commands/git-commit.md | 7 ++++--- .claude/commands/sdk-sample.md | 4 ++++ .../skills/requesting-code-review/SKILL.md | 3 +++ CLAUDE.md | 20 ++++++++++++++----- 9 files changed, 44 insertions(+), 34 deletions(-) diff --git a/.claude/commands/code-patterns.md b/.claude/commands/code-patterns.md index dfacb1d8..731eccd5 100644 --- a/.claude/commands/code-patterns.md +++ b/.claude/commands/code-patterns.md @@ -6,12 +6,11 @@ paths: - src/test/java/**/*.java exclude: - src/main/java/com/skyflow/generated/** +context: fork --- You are a senior engineer reviewing the Skyflow Java SDK against its established patterns. -Coding rules (error handling, request/response shape, naming, no magic strings) are defined in CLAUDE.md — apply those during this review. - ## Scope Files are passed in by the caller (usually `/code-review`). Review only those files. @@ -21,18 +20,13 @@ Files are passed in by the caller (usually `/code-review`). Review only those fi ## 1. 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 +- Follow the Tests coding rules (Assert conventions, no production class mocking, 100% coverage) --- ## 2. 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` +- Follow the Code quality coding rules (@SuppressWarnings, LogUtil for deprecation) --- diff --git a/.claude/commands/code-quality.md b/.claude/commands/code-quality.md index 21ce6aa9..423113bd 100644 --- a/.claude/commands/code-quality.md +++ b/.claude/commands/code-quality.md @@ -4,23 +4,24 @@ description: Quality pipeline — compile, checkstyle, build, tests, coverage ch paths: - src/**/*.java - pom.xml +exclude: + - src/main/java/com/skyflow/generated/** +context: fork --- Run the Skyflow Java SDK quality pipeline. Use `$ARGUMENTS` to target a specific test class (e.g. `BearerTokenTests`). If empty, run the full suite. -> Baseline failures are listed in CLAUDE.md under "Known Pre-existing Test Failures". +> Baseline failures are documented in the Known Pre-existing Test Failures table. > Do not investigate them unless specifically asked. Only report failures **beyond** that baseline. ## Coverage Requirements -**All code written or modified by Claude must have 100% coverage — both instruction and branch.** - -**All public interfaces must have 100% coverage — both instruction and branch.** This applies to: -- All classes under `src/main/java/com/skyflow/vault/` (controllers, data, tokens, connection, audit, bin, detect) -- All classes under `src/main/java/com/skyflow/config/` -- All classes under `src/main/java/com/skyflow/serviceaccount/` +Follow the Tests coding rules (100% instruction + branch coverage). Public interface packages: +- `src/main/java/com/skyflow/vault/` (controllers, data, tokens, connection, audit, bin, detect) +- `src/main/java/com/skyflow/config/` +- `src/main/java/com/skyflow/serviceaccount/` Flag any gap as a blocker — **NEEDS FIXES** if coverage is below 100% on Claude-written or public interface code. diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 6fa74777..1124549d 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -6,6 +6,7 @@ paths: - src/test/java/**/*.java exclude: - src/main/java/com/skyflow/generated/** +context: fork --- You are a senior engineer performing a thorough code review on the Skyflow Java SDK. @@ -24,8 +25,6 @@ Use `$ARGUMENTS` to determine scope: 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. - --- ## Step 1 — SDK Pattern Review diff --git a/.claude/commands/code-security.md b/.claude/commands/code-security.md index 7de061f6..f03660ff 100644 --- a/.claude/commands/code-security.md +++ b/.claude/commands/code-security.md @@ -2,13 +2,11 @@ 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 + - src/main/java/com/skyflow/**/*.java - pom.xml exclude: - src/main/java/com/skyflow/generated/** +context: fork --- You are a security engineer auditing the Skyflow Java SDK for vulnerabilities. @@ -20,8 +18,6 @@ Use `$ARGUMENTS` to determine target files. If none provided, run: 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) diff --git a/.claude/commands/code-smell.md b/.claude/commands/code-smell.md index 4cc5953c..ee7ecdda 100644 --- a/.claude/commands/code-smell.md +++ b/.claude/commands/code-smell.md @@ -3,8 +3,12 @@ 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 + - src/test/java/**/*.java + - .claude/**/*.md + - docs/**/*.md exclude: - src/main/java/com/skyflow/generated/** +context: fork --- You are a senior engineer performing a code smell analysis on the Skyflow Java SDK. @@ -18,8 +22,6 @@ Use `$ARGUMENTS` to determine scope: 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 diff --git a/.claude/commands/git-commit.md b/.claude/commands/git-commit.md index 651cc507..429ca746 100644 --- a/.claude/commands/git-commit.md +++ b/.claude/commands/git-commit.md @@ -1,6 +1,7 @@ --- -name: commit +name: git-commit description: Stage check + Jira-aware commit — extracts ticket ID from branch name and validates against pr.yml commit-message check. +context: fork --- Create a git commit for staged changes on the current branch. @@ -13,7 +14,7 @@ Use `$ARGUMENTS` as the commit message description. If empty, ask the user for a git rev-parse --abbrev-ref HEAD ``` -Extract the Jira ticket ID using the pattern `[A-Z]{1,10}-[0-9]+`: +Extract the Jira ticket ID using the pattern `[A-Z]{1,5}-[0-9]+`: - `devesh/SK-1234-fix-foo` → `SK-1234` - `karthik/GV-770-ext-auth-json-error` → `GV-770` - `username/SDK-2814-some-fix` → `SDK-2814` @@ -42,7 +43,7 @@ feat: SK-1234 add bulk insert support fix: GV-770 handle null bearer token on refresh ``` -Validate against the `pr.yml` enforced pattern: `(\[?[A-Z]{1,10}-[1-9][0-9]*)|(\[AUTOMATED\])|(Merge)|(Release)` +Validate against the `pr.yml` enforced pattern: `(\[?[A-Z]{1,5}-[1-9][0-9]*)|(\[AUTOMATED\])|(Merge)|(Release).+$` - Must contain a Jira ID — a bare description without a ticket ID will fail CI. - If validation fails, report the exact requirement and stop. diff --git a/.claude/commands/sdk-sample.md b/.claude/commands/sdk-sample.md index a7b84a65..97740142 100644 --- a/.claude/commands/sdk-sample.md +++ b/.claude/commands/sdk-sample.md @@ -1,9 +1,13 @@ --- name: sdk-sample description: Generate a Skyflow Java SDK sample file for a vault feature or service account operation. Compile-verified after creation. +context: fork paths: - samples/**/*.java - samples/pom.xml + - src/main/java/com/skyflow/**/*.java +exclude: + - src/main/java/com/skyflow/generated/** --- Create a Skyflow Java SDK sample file demonstrating: $ARGUMENTS diff --git a/.claude/skills/requesting-code-review/SKILL.md b/.claude/skills/requesting-code-review/SKILL.md index 5d89b793..7f5e9d55 100644 --- a/.claude/skills/requesting-code-review/SKILL.md +++ b/.claude/skills/requesting-code-review/SKILL.md @@ -4,6 +4,9 @@ description: Use when completing tasks, implementing major features, or before m paths: - src/main/java/**/*.java - src/test/java/**/*.java +exclude: + - src/main/java/com/skyflow/generated/** +context: fork --- # Requesting Code Review diff --git a/CLAUDE.md b/CLAUDE.md index 6e83fcb2..263165ca 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,8 +55,8 @@ docs/ ## Naming Conventions -- **Acronyms as words:** `skyflowId` (not `skyflowID`), `clientId` (not `clientID`), `tokenUri` (not `tokenURI`), `keyId` (not `keyID`) -- **Builder setters:** `setVaultId()`, `setClusterId()`, `setSkyflowId()` — never `setVaultID()` +- **Acronyms as words:** Examples: `skyflowId` (not `skyflowID`), `clientId` (not `clientID`), `tokenUri` (not `tokenURI`), `keyId` (not `keyID`) +- **Builder setters:** Examples: `setVaultId()`, `setClusterId()`, `setSkyflowId()` — never `setVaultID()` - **Response maps:** always use `skyflowId` (camelCase) — the raw API returns `skyflow_id` (snake_case) which VaultController normalises before returning to callers - **Constants class:** use `com.skyflow.utils.Constants` for string literals; `ErrorMessage` enum for error message strings @@ -78,6 +78,16 @@ These apply whenever writing or modifying code — not just during review. ### String literals - Use `Constants` for string literals and `ErrorMessage` enum for error messages — no magic strings +### Tests +- 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 +- All code written or modified by Claude must have 100% coverage — both instruction and branch + +### Code quality +- No `@SuppressWarnings` without a comment explaining why +- Use `LogUtil.printWarningLog` for deprecation warnings — not `System.err` + --- ## Build and Test @@ -128,13 +138,13 @@ See `docs/superpowers/specs/` for in-progress design specs and `docs/superpowers - `/code-smell` — standalone structural smell analysis only (long methods, dead code, misplaced logic) - `/code-security` — standalone security audit only (credentials, input validation, HTTP security) - `/sdk-sample ` — generate a sample file for a feature -- `/quality [ClassName]` — run quality pipeline (compile → checkstyle → build → test → 100% coverage check) -- `/commit ` — stage check + Jira-aware commit (extracts ticket ID from branch name) +- `/code-quality [ClassName]` — run quality pipeline (compile → checkstyle → build → test → 100% coverage check) +- `/git-commit ` — stage check + Jira-aware commit (extracts ticket ID from branch name) ## Commit & PR Guidelines ### Commit messages -**Never run `git commit` directly. Always use `/commit `** — it extracts the Jira ticket ID from the branch name, confirms `/code-quality` has passed, and validates the format against the CI check in `.github/workflows/pr.yml`. +**Never run `git commit` directly. Always use `/git-commit `** — it extracts the Jira ticket ID from the branch name, confirms `/code-quality` has passed, and validates the format against the CI check in `.github/workflows/pr.yml`. ### Branch naming Branch name must include your GitHub username: From 10b523e06c74c6cb4e99c385233a676d7070d678 Mon Sep 17 00:00:00 2001 From: skyflow-bharti Date: Mon, 1 Jun 2026 16:52:17 +0530 Subject: [PATCH 21/54] SK-2832 Removed the duplicate section from skill --- .../skills/requesting-code-review/SKILL.md | 36 ++----------------- 1 file changed, 2 insertions(+), 34 deletions(-) diff --git a/.claude/skills/requesting-code-review/SKILL.md b/.claude/skills/requesting-code-review/SKILL.md index 7f5e9d55..61e0e05d 100644 --- a/.claude/skills/requesting-code-review/SKILL.md +++ b/.claude/skills/requesting-code-review/SKILL.md @@ -38,40 +38,8 @@ context: fork For security-sensitive changes, run both: ```bash -/code-review src/main/java/com/skyflow/serviceaccount/ -/code-security src/main/java/com/skyflow/serviceaccount/ -``` - -**2. Fork context — dispatch a subagent reviewer:** - -The commands above run in the current session and share your context. For an independent second opinion (no confirmation bias, preserved main context window), dispatch a fresh subagent: - -``` -Agent tool (general-purpose): - description: "SDK code review" - prompt: | - You are a senior engineer reviewing the Skyflow Java SDK. - - Read CLAUDE.md for project conventions, then read and follow - .claude/commands/code-review.md for the full review process - including all rules, output format, and act-on-feedback guidance. - - Git range to review: - Base: {BASE_SHA} - Head: {HEAD_SHA} - - Run: - git diff --stat {BASE_SHA}..{HEAD_SHA} - git diff {BASE_SHA}..{HEAD_SHA} - - Description of what was implemented: - {DESCRIPTION} -``` - -Get the SHAs: -```bash -BASE_SHA=$(git merge-base main HEAD) # branch vs main -HEAD_SHA=$(git rev-parse HEAD) +/code-review src/main/java/com/skyflow/ +/code-security src/main/java/com/skyflow/ ``` All review rules, severity definitions, output format, and post-review steps are defined in `.claude/commands/code-review.md` — that file is the single source of truth. From 72e0853ca55a2eb7d0dd9113ae266b60b06f4d75 Mon Sep 17 00:00:00 2001 From: skyflow-bharti Date: Mon, 1 Jun 2026 16:55:10 +0530 Subject: [PATCH 22/54] SK-2832 Removed the duplicate claude setup file from samples --- samples/CLAUDE.md | 45 --------------------------------------------- 1 file changed, 45 deletions(-) delete mode 100644 samples/CLAUDE.md diff --git a/samples/CLAUDE.md b/samples/CLAUDE.md deleted file mode 100644 index 9ec2ea9a..00000000 --- a/samples/CLAUDE.md +++ /dev/null @@ -1,45 +0,0 @@ ---- -name: skyflow-java-samples -description: Samples project context — file placement, structure, and rules for Skyflow Java SDK sample files. -paths: - - "**/*.java" - - pom.xml ---- - -# Skyflow Java SDK — Samples - -Standalone Maven project demonstrating SDK features. Compile with: -```bash -cd samples && mvn compile -q -``` - -## File Placement - -| Feature | Package | Directory | -|---|---|---| -| Vault ops (insert/get/update/delete/query/tokenize/detokenize) | `com.example.vault` | `samples/src/main/java/com/example/vault/` | -| Service account auth | `com.example.serviceaccount` | `samples/src/main/java/com/example/serviceaccount/` | -| Connection | `com.example.connection` | `samples/src/main/java/com/example/connection/` | -| Detect | `com.example.detect` | `samples/src/main/java/com/example/detect/` | -| Audit event operations | `com.example.audit` | `samples/src/main/java/com/example/audit/` | -| BIN lookup | `com.example.bin` | `samples/src/main/java/com/example/bin/` | - -File name: `Example.java` - -## Deprecated Samples - -Deprecated examples (v1-era or superseded APIs) live in: -``` -samples/src/main/java/com/example/vault/deprecated/ -``` -Do not update deprecated samples — they are kept for reference only. New samples go in the parent package, not in `deprecated/`. - -## Rules - -- Vault IDs / cluster IDs: `""`, `""` -- Credential values: `""`, `""` -- Credentials file path: `"credentials.json"` (relative, never absolute) -- Always catch `SkyflowException` and print `e.getMessage()` -- No separate `*Options` classes — use request builder methods directly -- Keep under 80 lines -- Imports from `com.skyflow.*` only — never from `com.skyflow.generated.*` From b8fb38b69b55c89f16c622ea7e8627db8125f12d Mon Sep 17 00:00:00 2001 From: skyflow-bharti Date: Mon, 1 Jun 2026 19:48:35 +0530 Subject: [PATCH 23/54] SK-2832 Removed the unused code patterns --- .claude/commands/code-patterns.md | 53 ------------------------------- .claude/commands/code-review.md | 23 +++++++++++++- 2 files changed, 22 insertions(+), 54 deletions(-) delete mode 100644 .claude/commands/code-patterns.md diff --git a/.claude/commands/code-patterns.md b/.claude/commands/code-patterns.md deleted file mode 100644 index 731eccd5..00000000 --- a/.claude/commands/code-patterns.md +++ /dev/null @@ -1,53 +0,0 @@ ---- -name: code-patterns -description: SDK pattern review — request/response/options shape, error handling, naming, test coverage, code quality. -paths: - - src/main/java/**/*.java - - src/test/java/**/*.java -exclude: - - src/main/java/com/skyflow/generated/** -context: fork ---- - -You are a senior engineer reviewing the Skyflow Java SDK against its established patterns. - -## Scope - -Files are passed in by the caller (usually `/code-review`). Review only those files. - ---- - -## 1. Test coverage - -- Every public method must have at least one positive and one negative test -- Follow the Tests coding rules (Assert conventions, no production class mocking, 100% coverage) - ---- - -## 2. Code quality - -- Follow the Code quality coding rules (@SuppressWarnings, LogUtil for deprecation) - ---- - -## Output format - -Group findings by 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 | -``` - -**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 | diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 1124549d..399e260e 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -29,7 +29,28 @@ Use `$ARGUMENTS` to determine scope: ## Step 1 — SDK Pattern Review -Read the file `.claude/commands/code-patterns.md` and follow all of its instructions for the same files in scope. Produce its full output (per-file findings table + severity key). +Review all files in scope against the rules defined in `CLAUDE.md` (loaded automatically from the project root). Check every rule category: naming conventions, error handling, request/response patterns, string literals, tests, and code quality. + +Group findings by file and produce a table: + +``` +### 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 | +``` + +**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 | --- From 976079b2f0c6daf316373636e72ac58fe12dcb53 Mon Sep 17 00:00:00 2001 From: Aadarsh Date: Tue, 2 Jun 2026 18:35:31 +0530 Subject: [PATCH 24/54] SK-2832: Updated pr review workflow --- .claude/commands/code-review.md | 12 +- .claude/commands/code-security.md | 6 +- .github/workflows/claude-pr-review.yml | 151 ++++++++++++++----------- 3 files changed, 96 insertions(+), 73 deletions(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 399e260e..9662ad0d 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -13,16 +13,22 @@ You are a senior engineer performing a thorough code review on the Skyflow Java ## Pre-requisite -Before starting the review, confirm `/code-quality` has been run and passed (compile, tests, 100% coverage). If it has not been run, run it now before proceeding with the review. +If `GITHUB_ACTIONS` environment variable is set, skip this step (CI runs compile/test in a separate job). + +Otherwise, confirm `/code-quality` has been run and passed (compile, tests, 100% coverage). If it has not been run, run it now before proceeding with the review. ## Scope 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`: +- Empty / default — review files changed on current PR/branch vs base: ```bash - git diff main...HEAD --name-only | grep '\.java$' | grep -v 'generated' + # CI: GITHUB_BASE_REF is set (e.g. "main") — use origin/ prefix + # Local: unset — use main directly + BASE="${GITHUB_BASE_REF:+origin/$GITHUB_BASE_REF}" + BASE="${BASE:-main}" + git diff "$BASE"...HEAD --name-only | grep '\.java$' | grep -v 'generated' ``` --- diff --git a/.claude/commands/code-security.md b/.claude/commands/code-security.md index f03660ff..d2fd2983 100644 --- a/.claude/commands/code-security.md +++ b/.claude/commands/code-security.md @@ -15,7 +15,11 @@ You are a security engineer auditing the Skyflow Java SDK for vulnerabilities. Use `$ARGUMENTS` to determine target files. If none provided, run: ```bash -git diff main...HEAD --name-only | grep '\.java$' | grep -v 'generated' +# CI: GITHUB_BASE_REF is set (e.g. "main") — use origin/ prefix +# Local: unset — use main directly +BASE="${GITHUB_BASE_REF:+origin/$GITHUB_BASE_REF}" +BASE="${BASE:-main}" +git diff "$BASE"...HEAD --name-only | grep '\.java$' | grep -v 'generated' ``` ## Security Checks diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 821b2d50..f1bf772f 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -5,16 +5,21 @@ on: branches: [main] paths: - 'src/**/*.java' + workflow_dispatch: + inputs: + pr_number: + description: 'PR number to re-run review on' + required: false permissions: pull-requests: write contents: read jobs: - sdk-patterns-review: - name: SDK Patterns & Naming Review + sdk-review: + name: SDK Review (patterns + smell + security) runs-on: ubuntu-latest - timeout-minutes: 10 + timeout-minutes: 20 steps: - uses: actions/checkout@v4 with: @@ -23,66 +28,72 @@ jobs: - name: Install Claude CLI run: npm install -g @anthropic-ai/claude-code - - name: Get changed Java files - id: changed-files + - name: Check for non-generated Java changes + id: check run: | - FILES=$(git diff --name-only origin/${{ github.base_ref }}...${{ github.sha }} \ - | grep '\.java$' \ - | grep -v 'generated' \ - | tr '\n' ' ') - echo "files=$FILES" >> $GITHUB_OUTPUT - - - name: Run SDK patterns review - if: steps.changed-files.outputs.files != '' + CHANGED=$(git diff --name-only origin/${{ github.base_ref }}...HEAD \ + | grep '\.java$' | grep -v 'generated') + if [ -z "$CHANGED" ]; then + echo "has_changes=false" >> $GITHUB_OUTPUT + else + echo "has_changes=true" >> $GITHUB_OUTPUT + LINES=$(git diff origin/${{ github.base_ref }}...HEAD -- '*.java' \ + | grep -c '^[+-][^+-]' 2>/dev/null || echo 1) + echo "changed_lines=$LINES" >> $GITHUB_OUTPUT + fi + + - name: Run code review + if: steps.check.outputs.has_changes == 'true' id: review continue-on-error: true env: ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC: "1" + GITHUB_BASE_REF: ${{ github.base_ref }} + GITHUB_ACTIONS: "true" run: | - FILES="${{ steps.changed-files.outputs.files }}" - REVIEW=$(claude --print --model claude-sonnet-4-5 -p " - You are a senior engineer reviewing the Skyflow Java SDK. - - Review the following changed Java files for SDK pattern violations: - 1. Request/Response/Options patterns — builders are data holders, validation in Validations.java only - 2. Error handling — all public methods throw SkyflowException, no swallowed exceptions, no bare println/printStackTrace - 3. Naming — acronyms as words (skyflowId not skyflowID, tokenUri not tokenURI); UPPER_SNAKE constants; PascalCase classes - 4. Response normalisation — skyflowId not skyflow_id in response maps; getErrors() present on every response class - 5. Code quality — no magic strings (use Constants), no @SuppressWarnings without comment, deprecation via LogUtil.printWarningLog + RAW=$(claude --output-format json --model claude-sonnet-4-6 -p "/code-review" 2>/dev/null) - Skip src/main/java/com/skyflow/generated/ entirely. + RESULT=$(echo "$RAW" | jq -r '.result // "_Review failed to produce output._"') + INPUT_TOK=$(echo "$RAW" | jq -r '.usage.input_tokens // 0') + OUTPUT_TOK=$(echo "$RAW" | jq -r '.usage.output_tokens // 0') + CACHED_TOK=$(echo "$RAW" | jq -r '.usage.cache_read_input_tokens // 0') + COST=$(echo "$RAW" | jq -r '.total_cost_usd // 0') + LINES="${{ steps.check.outputs.changed_lines }}" - Files to review: $FILES - - For each file with findings, produce a markdown table: - | Severity | Line | Finding | - Severities: Critical (data loss/security), Bug (wrong behaviour), Quality (naming/patterns). - Skip Info-level observations. If no findings for a file, omit it. - If no findings at all, write: 'No issues found.' - - End with one of: APPROVE / APPROVE WITH FIXES / REQUEST CHANGES - ") echo "result<> $GITHUB_OUTPUT - echo "$REVIEW" >> $GITHUB_OUTPUT + echo "$RESULT" >> $GITHUB_OUTPUT echo "EOF" >> $GITHUB_OUTPUT + { + echo "## Token Usage — SDK Review" + echo "| Metric | Value |" + echo "|--------|-------|" + echo "| Input tokens | $INPUT_TOK |" + echo "| Output tokens | $OUTPUT_TOK |" + echo "| Cache hits | $CACHED_TOK |" + echo "| Total cost | \$$COST |" + echo "| Changed lines | $LINES |" + } >> $GITHUB_STEP_SUMMARY + - name: Post review comment - if: steps.changed-files.outputs.files != '' + if: steps.check.outputs.has_changes == 'true' uses: actions/github-script@v7 + env: + REVIEW_BODY: ${{ steps.review.outputs.result }} with: script: | - const review = `${{ steps.review.outputs.result }}`; - const files = `${{ steps.changed-files.outputs.files }}`; + const review = process.env.REVIEW_BODY || '_Review output unavailable._'; await github.rest.issues.createComment({ ...context.repo, issue_number: context.payload.pull_request.number, - body: `## Claude SDK Patterns Review\n\n${review}\n\n---\n_Files reviewed: \`${files}\`_` + body: `## Claude SDK Review\n\n${review}` }); security-review: name: Security Review (serviceaccount changes) runs-on: ubuntu-latest - timeout-minutes: 10 + timeout-minutes: 15 steps: - uses: actions/checkout@v4 with: @@ -104,10 +115,8 @@ jobs: if: steps.filter.outputs.serviceaccount == 'true' id: sa-files run: | - FILES=$(git diff --name-only origin/${{ github.base_ref }}...${{ github.sha }} \ - | grep 'serviceaccount' \ - | grep '\.java$' \ - | tr '\n' ' ') + FILES=$(git diff --name-only origin/${{ github.base_ref }}...HEAD \ + | grep 'serviceaccount' | grep '\.java$' | tr '\n' ' ') echo "files=$FILES" >> $GITHUB_OUTPUT - name: Run security audit @@ -116,40 +125,44 @@ jobs: continue-on-error: true env: ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC: "1" + GITHUB_BASE_REF: ${{ github.base_ref }} + GITHUB_ACTIONS: "true" + SA_FILES: ${{ steps.sa-files.outputs.files }} run: | - FILES="${{ steps.sa-files.outputs.files }}" - AUDIT=$(claude --print --model claude-sonnet-4-5 -p " - You are a security engineer auditing the Skyflow Java SDK serviceaccount module. - - Audit the following files for: - 1. Credential and token exposure — bearer tokens, API keys, private keys must never appear in logs, error messages, or toString() output - 2. Path traversal — file paths passed to new File(path) must not allow ../ - 3. JSON parsing — JsonParser calls must be wrapped in try/catch for JsonSyntaxException - 4. HTTP security — all API calls must be HTTPS; Authorization headers must not be logged at any level - 5. Token lifecycle — bearer token caching must check expiry before reuse; token refresh must be thread-safe - - Files: $FILES - - For each finding: - **Severity:** Critical / High / Medium / Low - **File:Line:** path:N - **Risk:** one sentence - **Fix:** one concrete sentence - - If no findings, write: 'No security issues found.' - End with overall risk rating: LOW / MEDIUM / HIGH / CRITICAL - ") + RAW=$(claude --output-format json --model claude-sonnet-4-6 \ + -p "/code-security $SA_FILES" 2>/dev/null) + + RESULT=$(echo "$RAW" | jq -r '.result // "_Audit failed to produce output._"') + INPUT_TOK=$(echo "$RAW" | jq -r '.usage.input_tokens // 0') + OUTPUT_TOK=$(echo "$RAW" | jq -r '.usage.output_tokens // 0') + CACHED_TOK=$(echo "$RAW" | jq -r '.usage.cache_read_input_tokens // 0') + COST=$(echo "$RAW" | jq -r '.total_cost_usd // 0') + echo "result<> $GITHUB_OUTPUT - echo "$AUDIT" >> $GITHUB_OUTPUT + echo "$RESULT" >> $GITHUB_OUTPUT echo "EOF" >> $GITHUB_OUTPUT + { + echo "## Token Usage — Security Audit" + echo "| Metric | Value |" + echo "|--------|-------|" + echo "| Input tokens | $INPUT_TOK |" + echo "| Output tokens | $OUTPUT_TOK |" + echo "| Cache hits | $CACHED_TOK |" + echo "| Total cost | \$$COST |" + } >> $GITHUB_STEP_SUMMARY + - name: Post security comment if: steps.filter.outputs.serviceaccount == 'true' && steps.sa-files.outputs.files != '' uses: actions/github-script@v7 + env: + AUDIT_BODY: ${{ steps.security.outputs.result }} + SA_FILES: ${{ steps.sa-files.outputs.files }} with: script: | - const audit = `${{ steps.security.outputs.result }}`; - const files = `${{ steps.sa-files.outputs.files }}`; + const audit = process.env.AUDIT_BODY || '_Audit output unavailable._'; + const files = process.env.SA_FILES; await github.rest.issues.createComment({ ...context.repo, issue_number: context.payload.pull_request.number, From 10e1c092e60b6a19096aeb8810cc1302bef5c17e Mon Sep 17 00:00:00 2001 From: Aadarsh Date: Tue, 2 Jun 2026 18:41:36 +0530 Subject: [PATCH 25/54] SK-2832: Fix claude issue --- .github/workflows/claude-pr-review.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index f1bf772f..b62584da 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -52,7 +52,7 @@ jobs: GITHUB_BASE_REF: ${{ github.base_ref }} GITHUB_ACTIONS: "true" run: | - RAW=$(claude --output-format json --model claude-sonnet-4-6 -p "/code-review" 2>/dev/null) + RAW=$(claude --output-format json --model claude-sonnet-4-6 -p "/code-review") RESULT=$(echo "$RAW" | jq -r '.result // "_Review failed to produce output._"') INPUT_TOK=$(echo "$RAW" | jq -r '.usage.input_tokens // 0') From 7953daa21797720f8587253b7ed4ab49e39c3f1d Mon Sep 17 00:00:00 2001 From: Aadarsh Date: Tue, 2 Jun 2026 18:46:28 +0530 Subject: [PATCH 26/54] SK-2832: fix --- .github/workflows/claude-pr-review.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index b62584da..987be2d6 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -52,7 +52,7 @@ jobs: GITHUB_BASE_REF: ${{ github.base_ref }} GITHUB_ACTIONS: "true" run: | - RAW=$(claude --output-format json --model claude-sonnet-4-6 -p "/code-review") + RAW=$(claude --dangerously-skip-permissions --output-format json --model claude-sonnet-4-6 -p "/code-review") RESULT=$(echo "$RAW" | jq -r '.result // "_Review failed to produce output._"') INPUT_TOK=$(echo "$RAW" | jq -r '.usage.input_tokens // 0') @@ -130,8 +130,8 @@ jobs: GITHUB_ACTIONS: "true" SA_FILES: ${{ steps.sa-files.outputs.files }} run: | - RAW=$(claude --output-format json --model claude-sonnet-4-6 \ - -p "/code-security $SA_FILES" 2>/dev/null) + RAW=$(claude --dangerously-skip-permissions --output-format json --model claude-sonnet-4-6 \ + -p "/code-security $SA_FILES") RESULT=$(echo "$RAW" | jq -r '.result // "_Audit failed to produce output._"') INPUT_TOK=$(echo "$RAW" | jq -r '.usage.input_tokens // 0') From ec3e4124a335c243123dc4ae8172412e541a137d Mon Sep 17 00:00:00 2001 From: Aadarsh Date: Tue, 2 Jun 2026 18:50:15 +0530 Subject: [PATCH 27/54] SK-2832: check api key valid or not --- .github/workflows/claude-pr-review.yml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 987be2d6..141b06ab 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -42,6 +42,14 @@ jobs: echo "changed_lines=$LINES" >> $GITHUB_OUTPUT fi + - name: Verify Claude CLI + env: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + run: | + echo "Claude version: $(claude --version)" + TEST=$(claude --dangerously-skip-permissions --output-format json -p "Reply with only the word: WORKING" 2>&1) + echo "Auth test: $TEST" + - name: Run code review if: steps.check.outputs.has_changes == 'true' id: review @@ -52,7 +60,7 @@ jobs: GITHUB_BASE_REF: ${{ github.base_ref }} GITHUB_ACTIONS: "true" run: | - RAW=$(claude --dangerously-skip-permissions --output-format json --model claude-sonnet-4-6 -p "/code-review") + RAW=$(claude --dangerously-skip-permissions --output-format json --model claude-sonnet-4-6 -p "/code-review" 2>&1) RESULT=$(echo "$RAW" | jq -r '.result // "_Review failed to produce output._"') INPUT_TOK=$(echo "$RAW" | jq -r '.usage.input_tokens // 0') From 93f8bd5dad9241c582a6dbd42edaa8c5c8f16790 Mon Sep 17 00:00:00 2001 From: Aadarsh Date: Tue, 2 Jun 2026 18:53:25 +0530 Subject: [PATCH 28/54] SK-2832: Debug --- .github/workflows/claude-pr-review.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 141b06ab..33a584aa 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -47,8 +47,12 @@ jobs: ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} run: | echo "Claude version: $(claude --version)" + set +e TEST=$(claude --dangerously-skip-permissions --output-format json -p "Reply with only the word: WORKING" 2>&1) - echo "Auth test: $TEST" + EXIT=$? + set -e + echo "Exit code: $EXIT" + echo "Output: $TEST" - name: Run code review if: steps.check.outputs.has_changes == 'true' From e409f0a6162a0b6e2082155f6803721c45774f90 Mon Sep 17 00:00:00 2001 From: Aadarsh Date: Tue, 2 Jun 2026 19:06:39 +0530 Subject: [PATCH 29/54] SK-2832: Updated the token count --- .github/workflows/claude-pr-review.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 33a584aa..d6a4cf93 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -67,9 +67,9 @@ jobs: RAW=$(claude --dangerously-skip-permissions --output-format json --model claude-sonnet-4-6 -p "/code-review" 2>&1) RESULT=$(echo "$RAW" | jq -r '.result // "_Review failed to produce output._"') - INPUT_TOK=$(echo "$RAW" | jq -r '.usage.input_tokens // 0') - OUTPUT_TOK=$(echo "$RAW" | jq -r '.usage.output_tokens // 0') - CACHED_TOK=$(echo "$RAW" | jq -r '.usage.cache_read_input_tokens // 0') + INPUT_TOK=$(echo "$RAW" | jq '[.modelUsage[].inputTokens // 0] | add // 0') + OUTPUT_TOK=$(echo "$RAW" | jq '[.modelUsage[].outputTokens // 0] | add // 0') + CACHED_TOK=$(echo "$RAW" | jq '[.modelUsage[].cacheReadInputTokens // 0] | add // 0') COST=$(echo "$RAW" | jq -r '.total_cost_usd // 0') LINES="${{ steps.check.outputs.changed_lines }}" @@ -146,9 +146,9 @@ jobs: -p "/code-security $SA_FILES") RESULT=$(echo "$RAW" | jq -r '.result // "_Audit failed to produce output._"') - INPUT_TOK=$(echo "$RAW" | jq -r '.usage.input_tokens // 0') - OUTPUT_TOK=$(echo "$RAW" | jq -r '.usage.output_tokens // 0') - CACHED_TOK=$(echo "$RAW" | jq -r '.usage.cache_read_input_tokens // 0') + INPUT_TOK=$(echo "$RAW" | jq '[.modelUsage[].inputTokens // 0] | add // 0') + OUTPUT_TOK=$(echo "$RAW" | jq '[.modelUsage[].outputTokens // 0] | add // 0') + CACHED_TOK=$(echo "$RAW" | jq '[.modelUsage[].cacheReadInputTokens // 0] | add // 0') COST=$(echo "$RAW" | jq -r '.total_cost_usd // 0') echo "result<> $GITHUB_OUTPUT From 376bdef3e5e8a5a285c8440f9690a57b35e71da1 Mon Sep 17 00:00:00 2001 From: Aadarsh Date: Tue, 2 Jun 2026 19:18:16 +0530 Subject: [PATCH 30/54] SK-2832: Updated condition for code-review --- .claude/commands/code-review.md | 7 +++++-- .github/workflows/claude-pr-review.yml | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 9662ad0d..c3a203b6 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -24,12 +24,15 @@ Use `$ARGUMENTS` to determine scope: - A file or directory path — review only that path - Empty / default — review files changed on current PR/branch vs base: ```bash - # CI: GITHUB_BASE_REF is set (e.g. "main") — use origin/ prefix - # Local: unset — use main directly BASE="${GITHUB_BASE_REF:+origin/$GITHUB_BASE_REF}" BASE="${BASE:-main}" git diff "$BASE"...HEAD --name-only | grep '\.java$' | grep -v 'generated' ``` + **If `GITHUB_ACTIONS` is set:** work from the diff output directly (changed lines only) instead of reading full files: + ```bash + git diff "$BASE"...HEAD -- '*.java' | grep -v 'src/main/java/com/skyflow/generated/' + ``` + Review only added lines (`+` prefix) from the diff. Do not comment on unchanged context lines or pre-existing code. --- diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index d6a4cf93..c3d71832 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -17,7 +17,7 @@ permissions: jobs: sdk-review: - name: SDK Review (patterns + smell + security) + name: SDK PR Review (changed lines only) runs-on: ubuntu-latest timeout-minutes: 20 steps: From e0736e500d8098b8c50051db8ecd96f9c39609e7 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 5 Jun 2026 14:50:35 +0530 Subject: [PATCH 31/54] SK-2832: scope Claude PR review to changed lines, consolidate output, inline comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: the PR review reviewed whole files (not just the diff), produced a large scattered comment, and flagged mostly pre-existing debt — so small PRs to large files (VaultClient, HttpUtility) surfaced many irrelevant issues. Fixes: - code-smell.md / code-security.md: add GITHUB_ACTIONS "PR / CI mode" — work from the diff and only report issues introduced by added (+) lines; never report whole-file metrics (Long class/method, large param list) or pre-existing debt unless the diff creates them. - code-review.md: propagate PR mode to the chained smell/security steps and define a single consolidated CI output (one verdict + one blocking-findings table + collapsed non-blocking section + a json:inline block). - claude-pr-review.yml: post one PR review with the summary as the body and inline comments anchored to changed lines; safe fallback to a summary comment if a line cannot be anchored. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/commands/code-review.md | 39 ++++++++++++++++++- .claude/commands/code-security.md | 6 +++ .claude/commands/code-smell.md | 22 ++++++++++- .github/workflows/claude-pr-review.yml | 52 +++++++++++++++++++++++--- 4 files changed, 110 insertions(+), 9 deletions(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index c3a203b6..4a9db064 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -67,17 +67,54 @@ Group findings by file and produce a table: Read the file `.claude/commands/code-smell.md` and follow all of its instructions for the same files in scope. Produce its full output (per-file smell table + smell summary + recommendation). +**If `GITHUB_ACTIONS` is set:** apply that command's **PR / CI mode** — report only smells introduced by added (`+`) lines; do not report whole-file metrics (Long class/method, large parameter list) or any pre-existing debt. Do **not** print code-smell's standalone tables, summary, or recommendation — collect its findings into the single consolidated report defined in **Output (PR / CI mode)** below. + --- ## Step 3 — Security Audit Read the file `.claude/commands/code-security.md` and follow all of its instructions for the same files in scope. Produce its full output (per-finding blocks + summary table + overall risk rating). +**If `GITHUB_ACTIONS` is set:** apply that command's **PR / CI mode** — report only issues introduced by added (`+`) lines; do not raise pre-existing vulnerabilities or whole-project checks the diff does not touch. Do **not** print code-security's standalone per-finding blocks, summary, or risk rating — collect its findings into the single consolidated report defined in **Output (PR / CI mode)** below. + --- -## Final Verdict +## Final Verdict (local mode only) + +> Skip this section when `GITHUB_ACTIONS` is set — use **Output (PR / CI mode)** instead. After all three steps, close with: 1. A tech-debt summary table grouped by category (SDK Patterns / Error Handling / Naming / Tests / Smells / Security) 2. A verdict: `APPROVE` / `APPROVE WITH FIXES` / `REQUEST CHANGES` 3. Remind: run `/code-quality` again after any fixes before merging. + +--- + +## Output (PR / CI mode) + +When `GITHUB_ACTIONS` is set, **do not** print the three steps' standalone tables/summaries/verdicts. Merge every finding from Steps 1–3 into a single de-duplicated report (if the same issue is flagged by more than one step, keep it once with the highest severity). Emit **exactly** the following, and nothing else: + +1. **One-line verdict** — `APPROVE` / `APPROVE WITH FIXES` / `REQUEST CHANGES`, followed by a one-sentence rationale. + +2. **One blocking-findings table** (Critical / Bug / Edge Case / High / Medium only). Omit the table entirely if there are none and say "No blocking findings on the changed lines." + ``` + | File:Line | Severity | Category | Finding | + |-----------|----------|----------|---------| + ``` + +3. **A collapsed section** for everything non-blocking (Quality / Smell / Low / Info): + ``` +
Non-blocking (Quality / Smell) — N items + + | File:Line | Severity | Finding | + |-----------|----------|---------| +
+ ``` + +4. **An inline-findings block** — a fenced ```` ```json:inline ```` block whose body is a JSON array of the findings that should be posted as inline review comments. Include **only blocking findings (step 2) whose line is an added (`+`) line in the diff** — never non-blocking items, never lines outside the diff. Each entry: + ```json:inline + [{ "path": "src/main/java/com/skyflow/Foo.java", "line": 42, "severity": "Bug", "comment": "skyflow_id not normalised to skyflowId" }] + ``` + Emit `[]` if there are none. The workflow parses this block to attach inline comments and renders items 1–3 as the review summary; keep it as the **last** thing in the output. + +Be concise: no preamble, no restating the diff, no per-step headers. diff --git a/.claude/commands/code-security.md b/.claude/commands/code-security.md index d2fd2983..a042b215 100644 --- a/.claude/commands/code-security.md +++ b/.claude/commands/code-security.md @@ -22,6 +22,12 @@ BASE="${BASE:-main}" git diff "$BASE"...HEAD --name-only | grep '\.java$' | grep -v 'generated' ``` +**If `GITHUB_ACTIONS` is set (PR review mode):** audit only the code this PR changed. Work from the diff: +```bash +git diff "$BASE"...HEAD -- '*.java' | grep -v 'src/main/java/com/skyflow/generated/' +``` +Report a finding **only if an added line (`+` prefix) introduces or directly exposes it.** Do not raise pre-existing vulnerabilities in unchanged code, and skip whole-project checks the diff does not touch (e.g. the dependency-CVE check in §6 unless `pom.xml` changed). If the added lines introduce no security issues, state that explicitly rather than listing pre-existing risks. (Local / non-CI runs and explicit file arguments keep full-file auditing.) + ## Security Checks ### 1. Credential and token exposure (Critical) diff --git a/.claude/commands/code-smell.md b/.claude/commands/code-smell.md index ee7ecdda..d405436f 100644 --- a/.claude/commands/code-smell.md +++ b/.claude/commands/code-smell.md @@ -17,10 +17,28 @@ You are a senior engineer performing a code smell analysis on the Skyflow Java S Use `$ARGUMENTS` to determine scope: - A file or directory path — analyse only that path -- Empty / default — analyse files changed on current branch vs `main`: +- Empty / default — analyse files changed on current branch vs base: ```bash - git diff main...HEAD --name-only | grep '\.java$' | grep -v 'generated' + BASE="${GITHUB_BASE_REF:+origin/$GITHUB_BASE_REF}" + BASE="${BASE:-main}" + git diff "$BASE"...HEAD --name-only | grep '\.java$' | grep -v 'generated' ``` + **If `GITHUB_ACTIONS` is set (PR review mode):** work from the diff, not whole files, and apply the **PR / CI mode** rules below: + ```bash + git diff "$BASE"...HEAD -- '*.java' | grep -v 'src/main/java/com/skyflow/generated/' + ``` + +--- + +## PR / CI mode (changed lines only) + +When `GITHUB_ACTIONS` is set, the analysis must reflect **only what this PR changed** — pre-existing debt must not be re-litigated on every PR: + +- Report a smell **only if an added line (`+` prefix) introduces it.** Never flag smells in unchanged/context lines or pre-existing code. +- **Do not report whole-file metrics** — *Long class, Long method, Large parameter list, pre-existing dead code, raw HashMap chains* — unless the diff itself *creates* the violation (e.g. the PR adds a brand-new method over 40 lines, or pushes a class past 300 lines for the first time). A small diff to a large legacy file must **not** trigger "Long class" or a pre-existing "Long method". +- Duplicated-code, deep-nesting, and magic-number smells: flag only when they appear in **added** lines. +- If the added lines introduce no smells, state **"No new smells introduced by this PR."** Do not enumerate pre-existing debt. +- This restriction applies to PR review only. Local / non-CI runs and explicit path arguments keep full-file analysis. --- diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index c3d71832..8ccca4f9 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -95,12 +95,52 @@ jobs: REVIEW_BODY: ${{ steps.review.outputs.result }} with: script: | - const review = process.env.REVIEW_BODY || '_Review output unavailable._'; - await github.rest.issues.createComment({ - ...context.repo, - issue_number: context.payload.pull_request.number, - body: `## Claude SDK Review\n\n${review}` - }); + const raw = process.env.REVIEW_BODY || '_Review output unavailable._'; + const pull_number = context.payload.pull_request.number; + + // Split the inline-findings JSON block (```json:inline ... ```) from the summary. + let summary = raw; + let inline = []; + const m = raw.match(/```json:inline\s*([\s\S]*?)```/); + if (m) { + summary = raw.replace(m[0], '').trim(); + try { inline = JSON.parse(m[1].trim()); } catch (e) { inline = []; } + } + + const comments = (Array.isArray(inline) ? inline : []) + .filter(f => f && f.path && Number.isInteger(f.line)) + .map(f => ({ + path: f.path, + line: f.line, + side: 'RIGHT', + body: `**${f.severity || 'Finding'}**: ${f.comment || ''}`.trim() + })); + + const body = `## Claude SDK Review\n\n${summary}`; + + // One PR review: summary as the body + inline comments anchored to changed lines. + try { + await github.rest.pulls.createReview({ + ...context.repo, + pull_number, + event: 'COMMENT', + body, + comments + }); + } catch (e) { + // Inline anchoring fails if a line is not in the diff — fall back to a single + // summary comment and list the would-be inline findings so nothing is lost. + const list = comments.length + ? '\n\n
Inline findings (could not attach to lines)\n\n' + + comments.map(c => `- \`${c.path}:${c.line}\` — ${c.body}`).join('\n') + + '\n
' + : ''; + await github.rest.issues.createComment({ + ...context.repo, + issue_number: pull_number, + body: body + list + }); + } security-review: name: Security Review (serviceaccount changes) From c1bf5fc5e7e28a315bae77313c7a1eb0e9da353c Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 5 Jun 2026 15:14:44 +0530 Subject: [PATCH 32/54] SK-2832: fail Claude PR review on API errors, drop redundant security job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - claude-pr-review.yml: the review check went green even when the Claude call failed (invalid key / no credits) because of `continue-on-error: true` and because the CLI exits 0 with the error in `.result`. Now the Verify step fails fast on missing/invalid key or zero credits, and the review step drops continue-on-error, inspects the CLI exit code + `.is_error`, and fails the job (no more error text posted as a "review"). - claude-pr-review.yml: remove the `security-review` job — it duplicated the security audit already run by /code-review Step 3, added cost + a second comment, and scanned whole files. - code-review.md: tag findings under serviceaccount/** with 🔐 and treat their security findings as >=High, so dropping the job loses no coverage. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/commands/code-review.md | 2 + .github/workflows/claude-pr-review.yml | 104 ++++++------------------- 2 files changed, 24 insertions(+), 82 deletions(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 4a9db064..9e7c98e2 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -77,6 +77,8 @@ Read the file `.claude/commands/code-security.md` and follow all of its instruct **If `GITHUB_ACTIONS` is set:** apply that command's **PR / CI mode** — report only issues introduced by added (`+`) lines; do not raise pre-existing vulnerabilities or whole-project checks the diff does not touch. Do **not** print code-security's standalone per-finding blocks, summary, or risk rating — collect its findings into the single consolidated report defined in **Output (PR / CI mode)** below. +**Serviceaccount emphasis:** when a changed file is under `serviceaccount/**` (auth, credentials, JWT, token lifecycle), prefix its findings with 🔐 and treat any security finding there as **at least High severity** (blocking). This replaces the need for a separate serviceaccount security job. + --- ## Final Verdict (local mode only) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 8ccca4f9..a7ec8d04 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -46,26 +46,38 @@ jobs: env: ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} run: | + if [ -z "$ANTHROPIC_API_KEY" ]; then + echo "::error::ANTHROPIC_API_KEY secret is not set." + exit 1 + fi echo "Claude version: $(claude --version)" set +e TEST=$(claude --dangerously-skip-permissions --output-format json -p "Reply with only the word: WORKING" 2>&1) EXIT=$? set -e - echo "Exit code: $EXIT" - echo "Output: $TEST" + IS_ERROR=$(echo "$TEST" | jq -r '.is_error // empty' 2>/dev/null) + if [ "$EXIT" -ne 0 ] || [ "$IS_ERROR" = "true" ]; then + MSG=$(echo "$TEST" | jq -r '.result // .error // .' 2>/dev/null | head -c 300) + echo "::error::Claude CLI check failed (invalid key, no API credits, or connectivity): $MSG" + exit 1 + fi + echo "Claude CLI OK" - name: Run code review if: steps.check.outputs.has_changes == 'true' id: review - continue-on-error: true env: ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC: "1" GITHUB_BASE_REF: ${{ github.base_ref }} GITHUB_ACTIONS: "true" run: | + set +e RAW=$(claude --dangerously-skip-permissions --output-format json --model claude-sonnet-4-6 -p "/code-review" 2>&1) + CLAUDE_EXIT=$? + set -e + IS_ERROR=$(echo "$RAW" | jq -r '.is_error // empty' 2>/dev/null) RESULT=$(echo "$RAW" | jq -r '.result // "_Review failed to produce output._"') INPUT_TOK=$(echo "$RAW" | jq '[.modelUsage[].inputTokens // 0] | add // 0') OUTPUT_TOK=$(echo "$RAW" | jq '[.modelUsage[].outputTokens // 0] | add // 0') @@ -88,6 +100,13 @@ jobs: echo "| Changed lines | $LINES |" } >> $GITHUB_STEP_SUMMARY + # Fail the check on a real API/runtime error (invalid key, no credits, etc.) + # instead of posting the error text as a "review". + if [ "$CLAUDE_EXIT" -ne 0 ] || [ "$IS_ERROR" = "true" ]; then + echo "::error::Claude review failed (invalid key, no API credits, or runtime error): $(echo "$RESULT" | head -c 300)" + exit 1 + fi + - name: Post review comment if: steps.check.outputs.has_changes == 'true' uses: actions/github-script@v7 @@ -141,82 +160,3 @@ jobs: body: body + list }); } - - security-review: - name: Security Review (serviceaccount changes) - runs-on: ubuntu-latest - timeout-minutes: 15 - steps: - - uses: actions/checkout@v4 - with: - fetch-depth: 0 - - - name: Check for serviceaccount changes - id: filter - uses: dorny/paths-filter@v3 - with: - filters: | - serviceaccount: - - 'src/**/serviceaccount/**/*.java' - - - name: Install Claude CLI - if: steps.filter.outputs.serviceaccount == 'true' - run: npm install -g @anthropic-ai/claude-code - - - name: Get changed serviceaccount files - if: steps.filter.outputs.serviceaccount == 'true' - id: sa-files - run: | - FILES=$(git diff --name-only origin/${{ github.base_ref }}...HEAD \ - | grep 'serviceaccount' | grep '\.java$' | tr '\n' ' ') - echo "files=$FILES" >> $GITHUB_OUTPUT - - - name: Run security audit - if: steps.filter.outputs.serviceaccount == 'true' && steps.sa-files.outputs.files != '' - id: security - continue-on-error: true - env: - ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} - CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC: "1" - GITHUB_BASE_REF: ${{ github.base_ref }} - GITHUB_ACTIONS: "true" - SA_FILES: ${{ steps.sa-files.outputs.files }} - run: | - RAW=$(claude --dangerously-skip-permissions --output-format json --model claude-sonnet-4-6 \ - -p "/code-security $SA_FILES") - - RESULT=$(echo "$RAW" | jq -r '.result // "_Audit failed to produce output._"') - INPUT_TOK=$(echo "$RAW" | jq '[.modelUsage[].inputTokens // 0] | add // 0') - OUTPUT_TOK=$(echo "$RAW" | jq '[.modelUsage[].outputTokens // 0] | add // 0') - CACHED_TOK=$(echo "$RAW" | jq '[.modelUsage[].cacheReadInputTokens // 0] | add // 0') - COST=$(echo "$RAW" | jq -r '.total_cost_usd // 0') - - echo "result<> $GITHUB_OUTPUT - echo "$RESULT" >> $GITHUB_OUTPUT - echo "EOF" >> $GITHUB_OUTPUT - - { - echo "## Token Usage — Security Audit" - echo "| Metric | Value |" - echo "|--------|-------|" - echo "| Input tokens | $INPUT_TOK |" - echo "| Output tokens | $OUTPUT_TOK |" - echo "| Cache hits | $CACHED_TOK |" - echo "| Total cost | \$$COST |" - } >> $GITHUB_STEP_SUMMARY - - - name: Post security comment - if: steps.filter.outputs.serviceaccount == 'true' && steps.sa-files.outputs.files != '' - uses: actions/github-script@v7 - env: - AUDIT_BODY: ${{ steps.security.outputs.result }} - SA_FILES: ${{ steps.sa-files.outputs.files }} - with: - script: | - const audit = process.env.AUDIT_BODY || '_Audit output unavailable._'; - const files = process.env.SA_FILES; - await github.rest.issues.createComment({ - ...context.repo, - issue_number: context.payload.pull_request.number, - body: `## Claude Security Audit (serviceaccount)\n\n${audit}\n\n---\n_Files audited: \`${files}\`_` - }); From fba0b3e62aa63301ac06617fe8c2e55aee21d553 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Fri, 5 Jun 2026 15:59:12 +0530 Subject: [PATCH 33/54] SK-2832: drop serviceaccount severity tag from code-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security coverage for serviceaccount/** is already provided by /code-review Step 3 with risk-appropriate severities, so the extra 🔐 tag + blocking severity-floor was unnecessary and could turn minor nits into blockers. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/commands/code-review.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 9e7c98e2..4a9db064 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -77,8 +77,6 @@ Read the file `.claude/commands/code-security.md` and follow all of its instruct **If `GITHUB_ACTIONS` is set:** apply that command's **PR / CI mode** — report only issues introduced by added (`+`) lines; do not raise pre-existing vulnerabilities or whole-project checks the diff does not touch. Do **not** print code-security's standalone per-finding blocks, summary, or risk rating — collect its findings into the single consolidated report defined in **Output (PR / CI mode)** below. -**Serviceaccount emphasis:** when a changed file is under `serviceaccount/**` (auth, credentials, JWT, token lifecycle), prefix its findings with 🔐 and treat any security finding there as **at least High severity** (blocking). This replaces the need for a separate serviceaccount security job. - --- ## Final Verdict (local mode only) From 97f1ea447103bfab8381a7680c756e7adf8e1cb6 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 13:49:43 +0530 Subject: [PATCH 34/54] SK-2832: fix severity taxonomy in PR review (Quality is advisory, not blocking) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Step 1 legend marked Quality as "fix before merge" (blocking) while the consolidated CI output bucketed Quality as non-blocking, so a Quality finding landed in the blocking table while the "Non-blocking (Quality/Smell)" section reported 0 — contradictory output. - Step 1 legend: Quality is now explicitly "No — advisory" (Critical/Bug/Edge remain blocking). - Output (PR/CI mode): add one authoritative severity split — Blocking = Critical/Bug/Edge/High/Medium; Advisory = Quality/Smell/Low/Info — drive the verdict from blocking only, forbid advisory rows in the blocking table, and rename the collapsed section to "Advisory (Quality / Smell / Low / Info)". Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/commands/code-review.md | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 4a9db064..d461fd06 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -54,12 +54,12 @@ Group findings by file and produce a table: **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 | +| Level | Meaning | Blocks merge? | +|---|---|---| +| **Critical** | Data loss, silent failure, security risk | Yes | +| **Bug** | Wrong behaviour, incorrect output | Yes | +| **Edge Case** | Unhandled input that will cause runtime failure | Yes | +| **Quality** | Maintainability issue, naming violation, missing pattern | No — advisory | --- @@ -92,19 +92,23 @@ After all three steps, close with: ## Output (PR / CI mode) -When `GITHUB_ACTIONS` is set, **do not** print the three steps' standalone tables/summaries/verdicts. Merge every finding from Steps 1–3 into a single de-duplicated report (if the same issue is flagged by more than one step, keep it once with the highest severity). Emit **exactly** the following, and nothing else: +When `GITHUB_ACTIONS` is set, **do not** print the three steps' standalone tables/summaries/verdicts. Merge every finding from Steps 1–3 into a single de-duplicated report (if the same issue is flagged by more than one step, keep it once with the highest severity). Emit **exactly** the following, and nothing else. -1. **One-line verdict** — `APPROVE` / `APPROVE WITH FIXES` / `REQUEST CHANGES`, followed by a one-sentence rationale. +**Severity buckets (single source of truth for this mode — every finding is exactly one):** +- **Blocking** (must fix before merge): `Critical`, `Bug`, `Edge Case`, `High`, `Medium`. +- **Advisory** (does not block merge): `Quality`, `Smell`, `Low`, `Info`. -2. **One blocking-findings table** (Critical / Bug / Edge Case / High / Medium only). Omit the table entirely if there are none and say "No blocking findings on the changed lines." +1. **One-line verdict** — `REQUEST CHANGES` if there is **≥1 blocking** finding; `APPROVE WITH FIXES` if there are only advisory findings; `APPROVE` if there are none. Add a one-sentence rationale. + +2. **Blocking-findings table** — blocking severities only. A `Quality` / `Smell` / `Low` / `Info` finding must **never** appear here. Omit the table and say "No blocking findings on the changed lines." if there are none. ``` | File:Line | Severity | Category | Finding | |-----------|----------|----------|---------| ``` -3. **A collapsed section** for everything non-blocking (Quality / Smell / Low / Info): +3. **Advisory section (collapsed)** — every advisory finding (and only advisory ones). The count `N` must match the row count. ``` -
Non-blocking (Quality / Smell) — N items +
Advisory (Quality / Smell / Low / Info) — N items | File:Line | Severity | Finding | |-----------|----------|---------| From 9238c56e1229228c2b204fdf74edb03dba057551 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 13:55:01 +0530 Subject: [PATCH 35/54] SK-2832: split review taxonomy into one Severity scale + separate Category MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The blocking set mixed severity levels (Critical/High/Medium) with issue types (Bug/Edge Case) in a single column, which reads oddly — a Critical/High finding *is* a bug. Now there are two independent axes: - Severity (how serious): Critical / High / Medium / Low / Info — one scale shared by SDK-pattern, smell, and security findings. - Category (what kind): Correctness / Edge case / Security / Pattern / Naming / Tests / Smell. Blocking = Critical / High / Medium; Advisory = Low / Info. "Bug" is now Severity High/Critical + Category Correctness; "Quality"/"Smell" map to Low/Info. Updated the Step 1 table/legend, the CI output buckets + tables, the inline json schema (adds "category"), and the workflow inline-comment body. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/commands/code-review.md | 43 +++++++++++++++----------- .github/workflows/claude-pr-review.yml | 2 +- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index d461fd06..818578ce 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -45,21 +45,28 @@ Group findings by file and produce a table: ``` ### 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 | +| Severity | Category | Line | Finding | +|----------|----------|------|---------| +| Critical | Security | 42 | SkyflowException swallowed in catch block | +| High | Correctness | 87 | skyflow_id not normalised to skyflowId | +| Low | Pattern | 103 | Magic string "records" — use Constants | ``` -**Severities:** +Every finding has **two independent axes** — don't conflate them: -| Level | Meaning | Blocks merge? | +**Severity** — *how serious* (one scale shared by all three steps): + +| Severity | Meaning | Blocks merge? | |---|---|---| -| **Critical** | Data loss, silent failure, security risk | Yes | -| **Bug** | Wrong behaviour, incorrect output | Yes | -| **Edge Case** | Unhandled input that will cause runtime failure | Yes | -| **Quality** | Maintainability issue, naming violation, missing pattern | No — advisory | +| **Critical** | Data loss, security breach, silent failure | Yes | +| **High** | Wrong behaviour / bug / guaranteed runtime failure | Yes | +| **Medium** | Likely problem, risky or unhandled input, missing safeguard | Yes | +| **Low** | Minor maintainability, naming, style, code smell | No — advisory | +| **Info** | Note / FYI | No — advisory | + +**Category** — *what kind*: `Correctness` (a bug), `Edge case`, `Security`, `Pattern`, `Naming`, `Tests`, `Smell`. + +A logic bug is **Severity `High`/`Critical` + Category `Correctness`** — never severity "Bug". A magic string is **Severity `Low` + Category `Pattern`** — never severity "Quality". Keep level in the Severity column and kind in the Category column. --- @@ -94,13 +101,13 @@ After all three steps, close with: When `GITHUB_ACTIONS` is set, **do not** print the three steps' standalone tables/summaries/verdicts. Merge every finding from Steps 1–3 into a single de-duplicated report (if the same issue is flagged by more than one step, keep it once with the highest severity). Emit **exactly** the following, and nothing else. -**Severity buckets (single source of truth for this mode — every finding is exactly one):** -- **Blocking** (must fix before merge): `Critical`, `Bug`, `Edge Case`, `High`, `Medium`. -- **Advisory** (does not block merge): `Quality`, `Smell`, `Low`, `Info`. +**Severity buckets (single source of truth for this mode — based on the one Severity scale; Category is a separate axis and never appears as a severity):** +- **Blocking** (must fix before merge): `Critical`, `High`, `Medium`. +- **Advisory** (does not block merge): `Low`, `Info`. 1. **One-line verdict** — `REQUEST CHANGES` if there is **≥1 blocking** finding; `APPROVE WITH FIXES` if there are only advisory findings; `APPROVE` if there are none. Add a one-sentence rationale. -2. **Blocking-findings table** — blocking severities only. A `Quality` / `Smell` / `Low` / `Info` finding must **never** appear here. Omit the table and say "No blocking findings on the changed lines." if there are none. +2. **Blocking-findings table** — `Critical` / `High` / `Medium` only. A `Low` / `Info` finding must **never** appear here. Omit the table and say "No blocking findings on the changed lines." if there are none. ``` | File:Line | Severity | Category | Finding | |-----------|----------|----------|---------| @@ -108,16 +115,16 @@ When `GITHUB_ACTIONS` is set, **do not** print the three steps' standalone table 3. **Advisory section (collapsed)** — every advisory finding (and only advisory ones). The count `N` must match the row count. ``` -
Advisory (Quality / Smell / Low / Info) — N items +
Advisory (Low / Info) — N items | File:Line | Severity | Finding | |-----------|----------|---------|
``` -4. **An inline-findings block** — a fenced ```` ```json:inline ```` block whose body is a JSON array of the findings that should be posted as inline review comments. Include **only blocking findings (step 2) whose line is an added (`+`) line in the diff** — never non-blocking items, never lines outside the diff. Each entry: +4. **An inline-findings block** — a fenced ```` ```json:inline ```` block whose body is a JSON array of the findings that should be posted as inline review comments. Include **only blocking findings (step 2) whose line is an added (`+`) line in the diff** — never advisory items, never lines outside the diff. Each entry: ```json:inline - [{ "path": "src/main/java/com/skyflow/Foo.java", "line": 42, "severity": "Bug", "comment": "skyflow_id not normalised to skyflowId" }] + [{ "path": "src/main/java/com/skyflow/Foo.java", "line": 42, "severity": "High", "category": "Correctness", "comment": "skyflow_id not normalised to skyflowId" }] ``` Emit `[]` if there are none. The workflow parses this block to attach inline comments and renders items 1–3 as the review summary; keep it as the **last** thing in the output. diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index a7ec8d04..6be0c95d 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -132,7 +132,7 @@ jobs: path: f.path, line: f.line, side: 'RIGHT', - body: `**${f.severity || 'Finding'}**: ${f.comment || ''}`.trim() + body: `**${f.severity || 'Finding'}${f.category ? ' · ' + f.category : ''}**: ${f.comment || ''}`.trim() })); const body = `## Claude SDK Review\n\n${summary}`; From 87ef55fc3a73c53caff2fa3b0e0e974e3310e1c8 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 14:10:45 +0530 Subject: [PATCH 36/54] SK-2832: tidy AI review output (rename, crisp summary, no preamble, wider cols) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename the posted comment header "Claude SDK Review" -> "AI Code Review". - Strip model preamble/narration ("Now I have… / Let me…") before posting: instruct the command that its entire output is the comment body starting at the verdict, and defensively slice anything before the verdict/heading in the workflow. - Crisp summary: blocking findings carry their full explanation in the inline comment; the summary table keeps the Finding cell to one line (no duplication). - Merge Severity + Category into one "Severity · Category" column so the columns aren't cramped. - Frame the summary as the review outcome, not a restatement of the PR. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/commands/code-review.md | 29 +++++++++++++------------- .github/workflows/claude-pr-review.yml | 6 +++++- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 818578ce..5b6acfb5 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -99,33 +99,34 @@ After all three steps, close with: ## Output (PR / CI mode) -When `GITHUB_ACTIONS` is set, **do not** print the three steps' standalone tables/summaries/verdicts. Merge every finding from Steps 1–3 into a single de-duplicated report (if the same issue is flagged by more than one step, keep it once with the highest severity). Emit **exactly** the following, and nothing else. +When `GITHUB_ACTIONS` is set, your **entire output is the body of a code-review comment** — not a chat reply. The **first character must be the verdict**. Never emit any preamble, planning, narration, acknowledgement, "Now I have…/Let me…" line, restatement of the diff, or per-step headers. Describe the **review outcome**, not the PR. -**Severity buckets (single source of truth for this mode — based on the one Severity scale; Category is a separate axis and never appears as a severity):** +Merge every finding from Steps 1–3 into one de-duplicated report (same issue flagged by multiple steps → keep once at the highest severity). Emit **exactly** the following, and nothing else. + +**Severity buckets (single source of truth; Category is a separate axis, never a severity):** - **Blocking** (must fix before merge): `Critical`, `High`, `Medium`. - **Advisory** (does not block merge): `Low`, `Info`. -1. **One-line verdict** — `REQUEST CHANGES` if there is **≥1 blocking** finding; `APPROVE WITH FIXES` if there are only advisory findings; `APPROVE` if there are none. Add a one-sentence rationale. +1. **Verdict** (first line, nothing above it) — `REQUEST CHANGES` if ≥1 blocking finding; `APPROVE WITH FIXES` if only advisory; `APPROVE` if none — plus a one-sentence rationale. -2. **Blocking-findings table** — `Critical` / `High` / `Medium` only. A `Low` / `Info` finding must **never** appear here. Omit the table and say "No blocking findings on the changed lines." if there are none. +2. **Blocking-findings table** — `Critical` / `High` / `Medium` only (never `Low` / `Info`). Keep the `Finding` cell to **one crisp line**: each blocking finding is also posted as an inline comment carrying the full explanation, so do **not** duplicate the detail here. Omit the table and write "No blocking findings on the changed lines." if there are none. ``` - | File:Line | Severity | Category | Finding | - |-----------|----------|----------|---------| + | File:Line | Severity · Category | Finding | + |-----------|---------------------|---------| + | HttpUtility.java:88 | High · Correctness | getMessage() returns null on the no-body error path | ``` -3. **Advisory section (collapsed)** — every advisory finding (and only advisory ones). The count `N` must match the row count. +3. **Advisory section (collapsed)** — every advisory finding, one crisp line each; `N` must equal the row count. ```
Advisory (Low / Info) — N items - | File:Line | Severity | Finding | - |-----------|----------|---------| + | File:Line | Severity · Category | Finding | + |-----------|---------------------|---------|
``` -4. **An inline-findings block** — a fenced ```` ```json:inline ```` block whose body is a JSON array of the findings that should be posted as inline review comments. Include **only blocking findings (step 2) whose line is an added (`+`) line in the diff** — never advisory items, never lines outside the diff. Each entry: +4. **Inline-findings block** — a fenced ```` ```json:inline ```` block: a JSON array of **only blocking findings whose line is an added (`+`) line**. Put the **full explanation in `comment`** (this is what renders inline on the code); keep the table above terse. Never include advisory items or lines outside the diff. ```json:inline - [{ "path": "src/main/java/com/skyflow/Foo.java", "line": 42, "severity": "High", "category": "Correctness", "comment": "skyflow_id not normalised to skyflowId" }] + [{ "path": "src/main/java/com/skyflow/Foo.java", "line": 42, "severity": "High", "category": "Correctness", "comment": "Full explanation of the issue and the fix goes here." }] ``` - Emit `[]` if there are none. The workflow parses this block to attach inline comments and renders items 1–3 as the review summary; keep it as the **last** thing in the output. - -Be concise: no preamble, no restating the diff, no per-step headers. + Emit `[]` if there are none. Keep this block **last**; the workflow strips it from the visible summary and renders items 1–3 as the review body. diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 6be0c95d..46136ff3 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -126,6 +126,10 @@ jobs: try { inline = JSON.parse(m[1].trim()); } catch (e) { inline = []; } } + // Strip any leading model preamble/narration before the verdict or first heading. + const start = summary.search(/^(#|REQUEST CHANGES|APPROVE WITH FIXES|APPROVE\b)/m); + if (start > 0) summary = summary.slice(start).trim(); + const comments = (Array.isArray(inline) ? inline : []) .filter(f => f && f.path && Number.isInteger(f.line)) .map(f => ({ @@ -135,7 +139,7 @@ jobs: body: `**${f.severity || 'Finding'}${f.category ? ' · ' + f.category : ''}**: ${f.comment || ''}`.trim() })); - const body = `## Claude SDK Review\n\n${summary}`; + const body = `## AI Code Review\n\n${summary}`; // One PR review: summary as the body + inline comments anchored to changed lines. try { From 09c4cce172fd4eee0f8861529c8df1314ad6d5a6 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 14:17:53 +0530 Subject: [PATCH 37/54] SK-2832: fix AI review markdown rendering (flat blocks, robust preamble strip) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Output spec: require each part to be a top-level block at the left margin with blank lines around tables/
; forbid emitting the parts as a markdown numbered list (tables and
nested in list items do not render on GitHub) — likely cause of the broken formatting. - Workflow: make the preamble strip line-based and tolerant of a decorated verdict (e.g. **REQUEST CHANGES**) instead of only a bare line-start match. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/commands/code-review.md | 2 ++ .github/workflows/claude-pr-review.yml | 9 ++++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 5b6acfb5..09a617a4 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -103,6 +103,8 @@ When `GITHUB_ACTIONS` is set, your **entire output is the body of a code-review Merge every finding from Steps 1–3 into one de-duplicated report (same issue flagged by multiple steps → keep once at the highest severity). Emit **exactly** the following, and nothing else. +**Rendering rules (GitHub markdown):** emit each part below as a **top-level block at the left margin**, separated by a blank line. The numbers are labels for you — do **not** reproduce them as a markdown numbered list, and do **not** indent the tables or `
` (tables/`
` nested inside list items do not render on GitHub). Every table needs a blank line before and after it, and `
` needs a blank line after the `` tag. + **Severity buckets (single source of truth; Category is a separate axis, never a severity):** - **Blocking** (must fix before merge): `Critical`, `High`, `Medium`. - **Advisory** (does not block merge): `Low`, `Info`. diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 46136ff3..e1e4f31f 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -126,9 +126,12 @@ jobs: try { inline = JSON.parse(m[1].trim()); } catch (e) { inline = []; } } - // Strip any leading model preamble/narration before the verdict or first heading. - const start = summary.search(/^(#|REQUEST CHANGES|APPROVE WITH FIXES|APPROVE\b)/m); - if (start > 0) summary = summary.slice(start).trim(); + // Strip any leading model preamble/narration before the verdict line + // (tolerates markdown decoration like **REQUEST CHANGES** or a heading). + const lines = summary.split('\n'); + const idx = lines.findIndex(l => + /\b(REQUEST CHANGES|APPROVE WITH FIXES|APPROVE)\b/.test(l) || /^#{1,6}\s/.test(l)); + if (idx > 0) summary = lines.slice(idx).join('\n').trim(); const comments = (Array.isArray(inline) ? inline : []) .filter(f => f && f.path && Number.isInteger(f.line)) From 1975a3507904fd66f4b9a62c8f5547c40ebe0092 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 14:25:14 +0530 Subject: [PATCH 38/54] SK-2832: make inline-findings block leak-proof (HTML sentinel, no backtick fence) The model put ```java code fences inside the JSON "comment" values, which broke the non-greedy ```json:inline``` extraction regex (it stopped at the first inner ``` ). Result: the JSON dumped into the visible comment AND inline comments stopped (the truncated JSON failed to parse -> empty array). - code-review.md: emit the inline block inside an HTML comment sentinel () which never renders even if parsing fails; forbid triple backticks / code fences inside the "comment" field. - claude-pr-review.yml: extract from the HTML sentinel (backtick-proof), keep a legacy ```json:inline fallback, and defensively strip any stray block to end-of-text so a malformed block can never leak into the summary. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/commands/code-review.md | 16 ++++++++++++---- .github/workflows/claude-pr-review.yml | 19 +++++++++++++------ 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 09a617a4..cff1f4d7 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -127,8 +127,16 @@ Merge every finding from Steps 1–3 into one de-duplicated report (same issue f
``` -4. **Inline-findings block** — a fenced ```` ```json:inline ```` block: a JSON array of **only blocking findings whose line is an added (`+`) line**. Put the **full explanation in `comment`** (this is what renders inline on the code); keep the table above terse. Never include advisory items or lines outside the diff. - ```json:inline - [{ "path": "src/main/java/com/skyflow/Foo.java", "line": 42, "severity": "High", "category": "Correctness", "comment": "Full explanation of the issue and the fix goes here." }] +4. **Inline-findings block** — the **very last thing** in your output, wrapped in an **HTML comment** so it never renders even if parsing fails. Its body is a JSON array of **only blocking findings whose line is an added (`+`) line** (never advisory items, never lines outside the diff). Put the **full explanation in `comment`** (this is what renders inline on the code); keep the summary table above terse. + + **Critical:** `comment` must be **plain text** — you may use single backticks for short identifiers, but **never triple backticks / code fences / `\`\`\`` anywhere inside the JSON** (they corrupt extraction). Describe the fix in prose, not a code block. + + Use exactly these sentinels (emit `[]` for the array if there are no inline findings): + ``` - Emit `[]` if there are none. Keep this block **last**; the workflow strips it from the visible summary and renders items 1–3 as the review body. + + ``` + + The workflow extracts this block, strips it from the visible summary, and renders items 1–3 as the review body. diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index e1e4f31f..88d23e70 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -117,14 +117,21 @@ jobs: const raw = process.env.REVIEW_BODY || '_Review output unavailable._'; const pull_number = context.payload.pull_request.number; - // Split the inline-findings JSON block (```json:inline ... ```) from the summary. + // Extract inline findings, then strip the block so it never leaks into the comment. + // Primary format: HTML-comment sentinel (never renders, + // and backticks inside the JSON can't break extraction). Legacy fallback: ```json:inline fence. let summary = raw; let inline = []; - const m = raw.match(/```json:inline\s*([\s\S]*?)```/); - if (m) { - summary = raw.replace(m[0], '').trim(); - try { inline = JSON.parse(m[1].trim()); } catch (e) { inline = []; } - } + const html = raw.match(//); + const fenced = raw.match(/```+\s*json:inline\s*([\s\S]*?)```/); + const block = html || fenced; + if (block) { try { inline = JSON.parse(block[1].trim()); } catch (e) { inline = []; } } + // Strip the sentinel block and any stray json:inline fence (to end-of-text, since it must + // be last) so a malformed block can never appear in the visible summary. + summary = summary + .replace(//g, '') + .replace(/```+\s*json:inline[\s\S]*$/i, '') + .trim(); // Strip any leading model preamble/narration before the verdict line // (tolerates markdown decoration like **REQUEST CHANGES** or a heading). From 1b07efb7edeeaed03db4ab8a7d613b156ab73523 Mon Sep 17 00:00:00 2001 From: Aadarsh Date: Mon, 8 Jun 2026 14:27:17 +0530 Subject: [PATCH 39/54] SK-2832: Updated cache write --- .github/workflows/claude-pr-review.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 6be0c95d..5994581b 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -82,6 +82,7 @@ jobs: INPUT_TOK=$(echo "$RAW" | jq '[.modelUsage[].inputTokens // 0] | add // 0') OUTPUT_TOK=$(echo "$RAW" | jq '[.modelUsage[].outputTokens // 0] | add // 0') CACHED_TOK=$(echo "$RAW" | jq '[.modelUsage[].cacheReadInputTokens // 0] | add // 0') + CACHE_WRITE_TOK=$(echo "$RAW" | jq '[.modelUsage[].cacheCreationInputTokens // 0] | add // 0') COST=$(echo "$RAW" | jq -r '.total_cost_usd // 0') LINES="${{ steps.check.outputs.changed_lines }}" @@ -95,7 +96,8 @@ jobs: echo "|--------|-------|" echo "| Input tokens | $INPUT_TOK |" echo "| Output tokens | $OUTPUT_TOK |" - echo "| Cache hits | $CACHED_TOK |" + echo "| Cache hits (read) | $CACHED_TOK |" + echo "| Cache writes | $CACHE_WRITE_TOK |" echo "| Total cost | \$$COST |" echo "| Changed lines | $LINES |" } >> $GITHUB_STEP_SUMMARY From 667c4e0f12033da3f2f53c865a66f4dff9ad3dde Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 14:31:47 +0530 Subject: [PATCH 40/54] SK-2832: incremental + debounced Claude PR review Cancel superseded review runs via concurrency so only the latest commit is reviewed. Diff from the bot's last-reviewed commit (commit_id) instead of the PR base, falling back to a full review on first run, rebase, or manual dispatch. Stay silent when the incremental verdict is a bare APPROVE, so clean follow-up commits don't repost comments. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/commands/code-review.md | 5 +- .github/workflows/claude-pr-review.yml | 65 +++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index cff1f4d7..68c7c1dd 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -24,7 +24,10 @@ Use `$ARGUMENTS` to determine scope: - A file or directory path — review only that path - Empty / default — review files changed on current PR/branch vs base: ```bash - BASE="${GITHUB_BASE_REF:+origin/$GITHUB_BASE_REF}" + # REVIEW_BASE_SHA, when set (CI incremental review), is the last commit already + # reviewed by the bot — diff only lines added since then. Otherwise fall back to + # the PR base branch. + BASE="${REVIEW_BASE_SHA:-${GITHUB_BASE_REF:+origin/$GITHUB_BASE_REF}}" BASE="${BASE:-main}" git diff "$BASE"...HEAD --name-only | grep '\.java$' | grep -v 'generated' ``` diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 5b094eae..eafb35ff 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -15,6 +15,12 @@ permissions: pull-requests: write contents: read +# Debounce: when several pushes land on the same PR in quick succession, cancel +# superseded runs so only the latest commit (current HEAD) gets reviewed. +concurrency: + group: claude-pr-review-${{ github.event.pull_request.number || github.event.inputs.pr_number }} + cancel-in-progress: true + jobs: sdk-review: name: SDK PR Review (changed lines only) @@ -28,16 +34,60 @@ jobs: - name: Install Claude CLI run: npm install -g @anthropic-ai/claude-code + # Incremental review: find the commit the bot last reviewed (the commit_id of + # its most recent review on this PR). Subsequent runs diff only lines added + # since then, so unchanged commits are not re-reviewed and not re-commented. + - name: Determine last reviewed commit + id: lastrev + uses: actions/github-script@v7 + with: + script: | + const prNumber = context.payload.pull_request?.number + || '${{ github.event.inputs.pr_number }}'; + if (!prNumber) { core.setOutput('sha', ''); return; } + let sha = ''; + try { + const reviews = await github.paginate(github.rest.pulls.listReviews, { + ...context.repo, pull_number: Number(prNumber), per_page: 100, + }); + const mine = reviews + .filter(r => r.user && r.user.login === 'github-actions[bot]' && r.commit_id) + .sort((a, b) => new Date(b.submitted_at) - new Date(a.submitted_at)); + if (mine.length) sha = mine[0].commit_id; + } catch (e) { + core.warning(`listReviews failed (${e.message}); falling back to full review.`); + } + core.setOutput('sha', sha); + core.info(`last reviewed commit: ${sha || '(none — full review)'}`); + - name: Check for non-generated Java changes id: check + env: + LAST_SHA: ${{ steps.lastrev.outputs.sha }} + EVENT_NAME: ${{ github.event_name }} + BASE_BRANCH: ${{ github.base_ref }} run: | - CHANGED=$(git diff --name-only origin/${{ github.base_ref }}...HEAD \ + BASE_REF="origin/${BASE_BRANCH:-main}" + # Default to a full review against the PR base. Switch to incremental only + # for a real push event when the last reviewed commit is still an ancestor + # of HEAD (a rebase/force-push or manual re-run forces a full re-review). + RANGE_BASE="$BASE_REF" + if [ "$EVENT_NAME" != "workflow_dispatch" ] && [ -n "$LAST_SHA" ] \ + && git merge-base --is-ancestor "$LAST_SHA" HEAD 2>/dev/null; then + RANGE_BASE="$LAST_SHA" + echo "Incremental review: ${LAST_SHA}...HEAD" + else + echo "Full review: ${BASE_REF}...HEAD" + fi + echo "range_base=$RANGE_BASE" >> $GITHUB_OUTPUT + + CHANGED=$(git diff --name-only "$RANGE_BASE"...HEAD \ | grep '\.java$' | grep -v 'generated') if [ -z "$CHANGED" ]; then echo "has_changes=false" >> $GITHUB_OUTPUT else echo "has_changes=true" >> $GITHUB_OUTPUT - LINES=$(git diff origin/${{ github.base_ref }}...HEAD -- '*.java' \ + LINES=$(git diff "$RANGE_BASE"...HEAD -- '*.java' \ | grep -c '^[+-][^+-]' 2>/dev/null || echo 1) echo "changed_lines=$LINES" >> $GITHUB_OUTPUT fi @@ -71,6 +121,8 @@ jobs: CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC: "1" GITHUB_BASE_REF: ${{ github.base_ref }} GITHUB_ACTIONS: "true" + # Incremental base: last reviewed commit (or PR base for a full review). + REVIEW_BASE_SHA: ${{ steps.check.outputs.range_base }} run: | set +e RAW=$(claude --dangerously-skip-permissions --output-format json --model claude-sonnet-4-6 -p "/code-review" 2>&1) @@ -142,6 +194,15 @@ jobs: /\b(REQUEST CHANGES|APPROVE WITH FIXES|APPROVE)\b/.test(l) || /^#{1,6}\s/.test(l)); if (idx > 0) summary = lines.slice(idx).join('\n').trim(); + // Total silence: the diff is incremental, so a bare APPROVE means the new + // lines introduced no blocking or advisory finding — post nothing at all. + // (Alternation order matters: APPROVE WITH FIXES must be tried before APPROVE.) + const verdict = (summary.match(/\b(REQUEST CHANGES|APPROVE WITH FIXES|APPROVE)\b/) || [])[1]; + if (verdict === 'APPROVE') { + core.info('Verdict APPROVE — no new issues on the changed lines; posting nothing.'); + return; + } + const comments = (Array.isArray(inline) ? inline : []) .filter(f => f && f.path && Number.isInteger(f.line)) .map(f => ({ From 24338f8b2e0d422b56dec51c9b82517141380f73 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 14:42:27 +0530 Subject: [PATCH 41/54] SK-2832: remove redundancy in AI review output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Verdict: state theme/count only; never enumerate/restate findings (the table already lists them) — was duplicating the whole table in the rationale. - Blocking table: Finding cell must be a terse identifier (<=~12 words); full detail lives only in the inline comment, never repeated in the table. - Collapse the same issue across multiple files into one row (locations comma-separated) instead of near-duplicate rows; inline block still emits one entry per location. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/commands/code-review.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 68c7c1dd..121dfe0c 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -112,13 +112,14 @@ Merge every finding from Steps 1–3 into one de-duplicated report (same issue f - **Blocking** (must fix before merge): `Critical`, `High`, `Medium`. - **Advisory** (does not block merge): `Low`, `Info`. -1. **Verdict** (first line, nothing above it) — `REQUEST CHANGES` if ≥1 blocking finding; `APPROVE WITH FIXES` if only advisory; `APPROVE` if none — plus a one-sentence rationale. +1. **Verdict** (first line, nothing above it) — `REQUEST CHANGES` if ≥1 blocking finding; `APPROVE WITH FIXES` if only advisory; `APPROVE` if none. Follow it with **one short clause stating the theme/count only** — e.g. "— 2 error-handling issues on the changed lines." **Never enumerate or restate the individual findings**; the table already lists them. -2. **Blocking-findings table** — `Critical` / `High` / `Medium` only (never `Low` / `Info`). Keep the `Finding` cell to **one crisp line**: each blocking finding is also posted as an inline comment carrying the full explanation, so do **not** duplicate the detail here. Omit the table and write "No blocking findings on the changed lines." if there are none. +2. **Blocking-findings table** — `Critical` / `High` / `Medium` only (never `Low` / `Info`). The `Finding` cell is a **terse identifier (≤ ~12 words, a noun phrase)** — no mechanism, no "because…", no fix; the full explanation lives in the inline comment, so never repeat it here. If the **same issue appears at multiple locations**, emit **one row** with the locations comma-separated in `File:Line` (the inline block still gets one entry per location) — do not create near-duplicate rows. Omit the table and write "No blocking findings on the changed lines." if there are none. ``` | File:Line | Severity · Category | Finding | |-----------|---------------------|---------| | HttpUtility.java:88 | High · Correctness | getMessage() returns null on the no-body error path | + | VaultClient.java:942, ConnectionClient.java:92 | Medium · Pattern | misleading EmptyCredentials message in generic catch | ``` 3. **Advisory section (collapsed)** — every advisory finding, one crisp line each; `N` must equal the row count. From 8bbdc6c300539dfa7cff7e27b8dd55f480360a5e Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 14:48:32 +0530 Subject: [PATCH 42/54] SK-2832: add ReviewProbe to test PR review (intentional bugs) Temporary probe with deliberate SDK-rule violations (swallowed exception, System.out.println, magic string, missing SkyflowException) to verify the Claude PR review flags them. Revert after testing. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../java/com/skyflow/utils/ReviewProbe.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 src/main/java/com/skyflow/utils/ReviewProbe.java diff --git a/src/main/java/com/skyflow/utils/ReviewProbe.java b/src/main/java/com/skyflow/utils/ReviewProbe.java new file mode 100644 index 00000000..1e5fdcf6 --- /dev/null +++ b/src/main/java/com/skyflow/utils/ReviewProbe.java @@ -0,0 +1,19 @@ +package com.skyflow.utils; + +/** + * Temporary probe to validate the Claude PR review workflow. + * Intentionally contains SDK-rule violations — delete after testing. + */ +public class ReviewProbe { + + public String fetchRecord(String id) { + try { + // Magic string instead of a Constants entry. + return "records/" + id; + } catch (Exception e) { + // Swallowed exception, no re-throw as SkyflowException. + System.out.println("failed: " + e.getMessage()); + } + return null; + } +} From 25ecaf29ffd61ab6d3c03d7a266d9cf59c3985c6 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 14:52:03 +0530 Subject: [PATCH 43/54] SK-2832: add dummy comment block to ReviewProbe (incremental silence test) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment-only change with no new issue — incremental review should yield a bare APPROVE and post nothing. Revert after testing. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/main/java/com/skyflow/utils/ReviewProbe.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/main/java/com/skyflow/utils/ReviewProbe.java b/src/main/java/com/skyflow/utils/ReviewProbe.java index 1e5fdcf6..ad72af1d 100644 --- a/src/main/java/com/skyflow/utils/ReviewProbe.java +++ b/src/main/java/com/skyflow/utils/ReviewProbe.java @@ -6,6 +6,19 @@ */ public class ReviewProbe { + // Dummy comment block to test incremental review (no new issue expected). + // Line 1 of 10 + // Line 2 of 10 + // Line 3 of 10 + // Line 4 of 10 + // Line 5 of 10 + // Line 6 of 10 + // Line 7 of 10 + // Line 8 of 10 + // Line 9 of 10 + // Line 10 of 10 + + public String fetchRecord(String id) { try { // Magic string instead of a Constants entry. From d94438203db8b42cff40bf5b4c0075909471edd4 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 17:25:58 +0530 Subject: [PATCH 44/54] SK-2832: dedupe the verdict line in AI review output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The model emitted two near-identical verdict lines. Fixes: - code-review.md: require exactly one verdict line (first line, nowhere else); it carries count + theme(s); never a second/rephrased verdict. - claude-pr-review.yml: safety net — keep the first verdict line and drop any later standalone line that restates the verdict. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/commands/code-review.md | 2 +- .github/workflows/claude-pr-review.yml | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 121dfe0c..06b09579 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -112,7 +112,7 @@ Merge every finding from Steps 1–3 into one de-duplicated report (same issue f - **Blocking** (must fix before merge): `Critical`, `High`, `Medium`. - **Advisory** (does not block merge): `Low`, `Info`. -1. **Verdict** (first line, nothing above it) — `REQUEST CHANGES` if ≥1 blocking finding; `APPROVE WITH FIXES` if only advisory; `APPROVE` if none. Follow it with **one short clause stating the theme/count only** — e.g. "— 2 error-handling issues on the changed lines." **Never enumerate or restate the individual findings**; the table already lists them. +1. **Verdict** — emit **exactly one** verdict line, as the very first line and nowhere else: `REQUEST CHANGES` if ≥1 blocking finding; `APPROVE WITH FIXES` if only advisory; `APPROVE` if none. Follow it with **one short clause naming the count + theme(s)** — e.g. `REQUEST CHANGES — 5 blocking findings in ReviewProbe.java (error handling, naming, magic string).` **Never** write a second verdict line, repeat/rephrase the verdict, or enumerate the individual findings (the table lists them). 2. **Blocking-findings table** — `Critical` / `High` / `Medium` only (never `Low` / `Info`). The `Finding` cell is a **terse identifier (≤ ~12 words, a noun phrase)** — no mechanism, no "because…", no fix; the full explanation lives in the inline comment, so never repeat it here. If the **same issue appears at multiple locations**, emit **one row** with the locations comma-separated in `File:Line` (the inline block still gets one entry per location) — do not create near-duplicate rows. Omit the table and write "No blocking findings on the changed lines." if there are none. ``` diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index eafb35ff..c1b12728 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -194,6 +194,18 @@ jobs: /\b(REQUEST CHANGES|APPROVE WITH FIXES|APPROVE)\b/.test(l) || /^#{1,6}\s/.test(l)); if (idx > 0) summary = lines.slice(idx).join('\n').trim(); + // Collapse duplicate verdict lines: keep the first, drop any later standalone + // line that just restates the verdict. + const verdictRe = /^\**\s*(REQUEST CHANGES|APPROVE WITH FIXES|APPROVE)\b/; + let seenVerdict = false; + summary = summary.split('\n').filter(l => { + if (verdictRe.test(l)) { + if (seenVerdict) return false; + seenVerdict = true; + } + return true; + }).join('\n').trim(); + // Total silence: the diff is incremental, so a bare APPROVE means the new // lines introduced no blocking or advisory finding — post nothing at all. // (Alternation order matters: APPROVE WITH FIXES must be tried before APPROVE.) From c80e811336972c2fba1004f705e9ea5fc41dbcce Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 17:38:43 +0530 Subject: [PATCH 45/54] SK-2832: exclude generated/ from PR review changed-lines metric The changed-lines count diffed all *.java over the review range but, unlike the file-existence check, did not filter generated/. Add an exclude pathspec so the metric matches what is actually reviewed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/claude-pr-review.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index c1b12728..42e60fe4 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -87,7 +87,9 @@ jobs: echo "has_changes=false" >> $GITHUB_OUTPUT else echo "has_changes=true" >> $GITHUB_OUTPUT - LINES=$(git diff "$RANGE_BASE"...HEAD -- '*.java' \ + # Count added/removed lines over the same range the review uses, excluding + # generated/ so the metric matches what is actually reviewed. + LINES=$(git diff "$RANGE_BASE"...HEAD -- '*.java' ':(exclude)**/generated/**' \ | grep -c '^[+-][^+-]' 2>/dev/null || echo 1) echo "changed_lines=$LINES" >> $GITHUB_OUTPUT fi @@ -221,7 +223,7 @@ jobs: path: f.path, line: f.line, side: 'RIGHT', - body: `**${f.severity || 'Finding'}${f.category ? ' · ' + f.category : ''}**: ${f.comment || ''}`.trim() + body: `**${f.severity || 'Finding'}${f.category ? ' · ' + f.category : ''}${f.cwe ? ' · ' + f.cwe : ''}**: ${f.comment || ''}`.trim() })); const body = `## AI Code Review\n\n${summary}`; From 205011d7dfb63ca66999d156eb2d0c3fa9014386 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 17:59:17 +0530 Subject: [PATCH 46/54] SK-2832: stream Claude review output live to the Actions log Switch the review step to --output-format stream-json --verbose piped through tee, so each turn/tool event is visible live in the Actions log while the final result event is still parsed for the comment and token metrics. PIPESTATUS[0] preserves claude's exit code through the pipe. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/claude-pr-review.yml | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 42e60fe4..ad3ed2de 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -127,10 +127,24 @@ jobs: REVIEW_BASE_SHA: ${{ steps.check.outputs.range_base }} run: | set +e - RAW=$(claude --dangerously-skip-permissions --output-format json --model claude-sonnet-4-6 -p "/code-review" 2>&1) - CLAUDE_EXIT=$? + # stream-json + --verbose emits one NDJSON event per line (init, each + # assistant turn, every tool call/result, final result) as the review + # runs. tee shows them live in the Actions log AND saves them for parsing. + # PIPESTATUS[0] is claude's exit (not tee's). The final {"type":"result"} + # event has the same shape as --output-format json, so the jq paths below + # still work — we just extract that last line. + claude --dangerously-skip-permissions \ + --output-format stream-json --verbose \ + --model claude-sonnet-4-6 -p "/code-review" 2>&1 \ + | tee /tmp/claude-review.jsonl + CLAUDE_EXIT=${PIPESTATUS[0]} set -e + RAW=$(grep '"type":"result"' /tmp/claude-review.jsonl | tail -n 1) + if [ -z "$RAW" ]; then + RAW='{"is_error":true,"result":"_Review produced no result event._"}' + fi + IS_ERROR=$(echo "$RAW" | jq -r '.is_error // empty' 2>/dev/null) RESULT=$(echo "$RAW" | jq -r '.result // "_Review failed to produce output._"') INPUT_TOK=$(echo "$RAW" | jq '[.modelUsage[].inputTokens // 0] | add // 0') From 49ea99452196a722dab928ffc817ebb4d396f960 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 18:00:02 +0530 Subject: [PATCH 47/54] SK-2832: swap probe bugs to test incremental new-issue detection Remove the original violations (swallowed exception, System.out.println, magic string) and introduce a fresh set (hardcoded credential, uncommented @SuppressWarnings, printStackTrace, RuntimeException re-throw) so the incremental review should flag only the new issues. Revert after testing. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../java/com/skyflow/utils/ReviewProbe.java | 39 ++++++++++--------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/src/main/java/com/skyflow/utils/ReviewProbe.java b/src/main/java/com/skyflow/utils/ReviewProbe.java index ad72af1d..7eb7247e 100644 --- a/src/main/java/com/skyflow/utils/ReviewProbe.java +++ b/src/main/java/com/skyflow/utils/ReviewProbe.java @@ -1,32 +1,35 @@ package com.skyflow.utils; +import java.util.Map; + /** * Temporary probe to validate the Claude PR review workflow. * Intentionally contains SDK-rule violations — delete after testing. */ public class ReviewProbe { - // Dummy comment block to test incremental review (no new issue expected). - // Line 1 of 10 - // Line 2 of 10 - // Line 3 of 10 - // Line 4 of 10 - // Line 5 of 10 - // Line 6 of 10 - // Line 7 of 10 - // Line 8 of 10 - // Line 9 of 10 - // Line 10 of 10 - + // NEW BUG 1 (security): hardcoded credential embedded in source. + private static final String API_TOKEN = "hardcoded-admin-password-123"; public String fetchRecord(String id) { + // Original bugs removed: swallowed exception, System.out.println, magic string. + return id; + } + + // NEW BUG 2 (code quality): @SuppressWarnings with no explanatory comment. + @SuppressWarnings("unchecked") + public Map castPayload(Object raw) { + return (Map) raw; + } + + public void process(String value) { try { - // Magic string instead of a Constants entry. - return "records/" + id; - } catch (Exception e) { - // Swallowed exception, no re-throw as SkyflowException. - System.out.println("failed: " + e.getMessage()); + Integer.parseInt(value); + } catch (NumberFormatException e) { + // NEW BUG 3 (error handling): printStackTrace instead of LogUtil. + e.printStackTrace(); + // NEW BUG 4 (error handling): re-thrown as RuntimeException, not SkyflowException. + throw new RuntimeException("bad value: " + API_TOKEN); } - return null; } } From 1cf756fc5ae8837b309d575d16cd075674dfcce8 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 18:13:47 +0530 Subject: [PATCH 48/54] SK-2832: add intentional advisory smells to probe (magic number, commented-out code) Add two structural smells on new lines so code-smell's PR/CI mode surfaces them as Category=Smell advisory rows: magic numbers (3600, 24) and a commented-out code block. Revert after testing. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/main/java/com/skyflow/utils/ReviewProbe.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/main/java/com/skyflow/utils/ReviewProbe.java b/src/main/java/com/skyflow/utils/ReviewProbe.java index 7eb7247e..3227b065 100644 --- a/src/main/java/com/skyflow/utils/ReviewProbe.java +++ b/src/main/java/com/skyflow/utils/ReviewProbe.java @@ -32,4 +32,15 @@ public void process(String value) { throw new RuntimeException("bad value: " + API_TOKEN); } } + + // NEW SMELL 1 (advisory): magic numbers — 3600 and 24 have no named constant. + public long sessionTtlSeconds() { + return 3600 * 24; + } + + // NEW SMELL 2 (advisory): commented-out code block with no explanation. + // public void legacyFlush() { + // cache.clear(); + // reset(); + // } } From fb8901dd3f3ab8acbb28affbdf20bd9dc9104cd7 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 18:27:42 +0530 Subject: [PATCH 49/54] SK-2832 Segregate code-review PR/CI output spec into .claude/includes/code-review-ci.md Move the PR/CI consolidated-output spec out of code-review.md into a plain include file, fetched via a conditional Read only when GITHUB_ACTIONS is set. Local runs use the Final Verdict section and never load the include. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/commands/code-review.md | 43 ++------------------------ .claude/includes/code-review-ci.md | 49 ++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 41 deletions(-) create mode 100644 .claude/includes/code-review-ci.md diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 06b09579..67c8ff8f 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -102,45 +102,6 @@ After all three steps, close with: ## Output (PR / CI mode) -When `GITHUB_ACTIONS` is set, your **entire output is the body of a code-review comment** — not a chat reply. The **first character must be the verdict**. Never emit any preamble, planning, narration, acknowledgement, "Now I have…/Let me…" line, restatement of the diff, or per-step headers. Describe the **review outcome**, not the PR. +**If `GITHUB_ACTIONS` is set:** read `.claude/includes/code-review-ci.md` and produce **exactly** the consolidated code-review-comment report it specifies — nothing else. Do **not** emit the local-mode **Final Verdict** above. (This file is fetched only in CI; local runs never read it.) -Merge every finding from Steps 1–3 into one de-duplicated report (same issue flagged by multiple steps → keep once at the highest severity). Emit **exactly** the following, and nothing else. - -**Rendering rules (GitHub markdown):** emit each part below as a **top-level block at the left margin**, separated by a blank line. The numbers are labels for you — do **not** reproduce them as a markdown numbered list, and do **not** indent the tables or `
` (tables/`
` nested inside list items do not render on GitHub). Every table needs a blank line before and after it, and `
` needs a blank line after the `` tag. - -**Severity buckets (single source of truth; Category is a separate axis, never a severity):** -- **Blocking** (must fix before merge): `Critical`, `High`, `Medium`. -- **Advisory** (does not block merge): `Low`, `Info`. - -1. **Verdict** — emit **exactly one** verdict line, as the very first line and nowhere else: `REQUEST CHANGES` if ≥1 blocking finding; `APPROVE WITH FIXES` if only advisory; `APPROVE` if none. Follow it with **one short clause naming the count + theme(s)** — e.g. `REQUEST CHANGES — 5 blocking findings in ReviewProbe.java (error handling, naming, magic string).` **Never** write a second verdict line, repeat/rephrase the verdict, or enumerate the individual findings (the table lists them). - -2. **Blocking-findings table** — `Critical` / `High` / `Medium` only (never `Low` / `Info`). The `Finding` cell is a **terse identifier (≤ ~12 words, a noun phrase)** — no mechanism, no "because…", no fix; the full explanation lives in the inline comment, so never repeat it here. If the **same issue appears at multiple locations**, emit **one row** with the locations comma-separated in `File:Line` (the inline block still gets one entry per location) — do not create near-duplicate rows. Omit the table and write "No blocking findings on the changed lines." if there are none. - ``` - | File:Line | Severity · Category | Finding | - |-----------|---------------------|---------| - | HttpUtility.java:88 | High · Correctness | getMessage() returns null on the no-body error path | - | VaultClient.java:942, ConnectionClient.java:92 | Medium · Pattern | misleading EmptyCredentials message in generic catch | - ``` - -3. **Advisory section (collapsed)** — every advisory finding, one crisp line each; `N` must equal the row count. - ``` -
Advisory (Low / Info) — N items - - | File:Line | Severity · Category | Finding | - |-----------|---------------------|---------| -
- ``` - -4. **Inline-findings block** — the **very last thing** in your output, wrapped in an **HTML comment** so it never renders even if parsing fails. Its body is a JSON array of **only blocking findings whose line is an added (`+`) line** (never advisory items, never lines outside the diff). Put the **full explanation in `comment`** (this is what renders inline on the code); keep the summary table above terse. - - **Critical:** `comment` must be **plain text** — you may use single backticks for short identifiers, but **never triple backticks / code fences / `\`\`\`` anywhere inside the JSON** (they corrupt extraction). Describe the fix in prose, not a code block. - - Use exactly these sentinels (emit `[]` for the array if there are no inline findings): - - ``` - - ``` - - The workflow extracts this block, strips it from the visible summary, and renders items 1–3 as the review body. +Otherwise (local mode), this section does not apply — use the **Final Verdict (local mode)** section above. diff --git a/.claude/includes/code-review-ci.md b/.claude/includes/code-review-ci.md new file mode 100644 index 00000000..d8b0fa0d --- /dev/null +++ b/.claude/includes/code-review-ci.md @@ -0,0 +1,49 @@ +# Code Review — PR / CI Output Spec + +> Loaded by `/code-review` (`.claude/commands/code-review.md`) **only when `GITHUB_ACTIONS` is set**. +> This is a plain include, not a slash command — it has no frontmatter and is not in `commands/`, +> so it cannot be invoked directly. It defines the consolidated review-comment format the CI +> workflow extracts and posts. + +When `GITHUB_ACTIONS` is set, your **entire output is the body of a code-review comment** — not a chat reply. The **first character must be the verdict**. Never emit any preamble, planning, narration, acknowledgement, "Now I have…/Let me…" line, restatement of the diff, or per-step headers. Describe the **review outcome**, not the PR. + +Merge every finding from Steps 1–3 into one de-duplicated report (same issue flagged by multiple steps → keep once at the highest severity). Emit **exactly** the following, and nothing else. + +**Rendering rules (GitHub markdown):** emit each part below as a **top-level block at the left margin**, separated by a blank line. The numbers are labels for you — do **not** reproduce them as a markdown numbered list, and do **not** indent the tables or `
` (tables/`
` nested inside list items do not render on GitHub). Every table needs a blank line before and after it, and `
` needs a blank line after the `` tag. + +**Severity buckets (single source of truth; Category is a separate axis, never a severity):** +- **Blocking** (must fix before merge): `Critical`, `High`, `Medium`. +- **Advisory** (does not block merge): `Low`, `Info`. + +1. **Verdict** — emit **exactly one** verdict line, as the very first line and nowhere else: `REQUEST CHANGES` if ≥1 blocking finding; `APPROVE WITH FIXES` if only advisory; `APPROVE` if none. Follow it with **one short clause naming the count + theme(s)** — e.g. `REQUEST CHANGES — 5 blocking findings in ReviewProbe.java (error handling, naming, magic string).` **Never** write a second verdict line, repeat/rephrase the verdict, or enumerate the individual findings (the table lists them). + +2. **Blocking-findings table** — `Critical` / `High` / `Medium` only (never `Low` / `Info`). The `Finding` cell is a **terse identifier (≤ ~12 words, a noun phrase)** — no mechanism, no "because…", no fix; the full explanation lives in the inline comment, so never repeat it here. If the **same issue appears at multiple locations**, emit **one row** with the locations comma-separated in `File:Line` (the inline block still gets one entry per location) — do not create near-duplicate rows. Omit the table and write "No blocking findings on the changed lines." if there are none. + ``` + | File:Line | Severity · Category | Finding | + |-----------|---------------------|---------| + | HttpUtility.java:88 | High · Correctness | getMessage() returns null on the no-body error path | + | VaultClient.java:942, ConnectionClient.java:92 | Medium · Pattern | misleading EmptyCredentials message in generic catch | + ``` + +3. **Advisory section (collapsed)** — every advisory finding, one crisp line each; `N` must equal the row count. + ``` +
Advisory (Low / Info) — N items + + | File:Line | Severity · Category | Finding | + |-----------|---------------------|---------| +
+ ``` + +4. **Inline-findings block** — the **very last thing** in your output, wrapped in an **HTML comment** so it never renders even if parsing fails. Its body is a JSON array of **only blocking findings whose line is an added (`+`) line** (never advisory items, never lines outside the diff). Put the **full explanation in `comment`** (this is what renders inline on the code); keep the summary table above terse. + + **Critical:** `comment` must be **plain text** — you may use single backticks for short identifiers, but **never triple backticks / code fences / `\`\`\`` anywhere inside the JSON** (they corrupt extraction). Describe the fix in prose, not a code block. + + Use exactly these sentinels (emit `[]` for the array if there are no inline findings): + + ``` + + ``` + + The workflow extracts this block, strips it from the visible summary, and renders items 1–3 as the review body. From c6129225e4d6be5b610c3b96d32e6d0396f59e79 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 18:38:34 +0530 Subject: [PATCH 50/54] SK-2832: swap probe to a fresh set of bugs and smells Resolve the previous violations and add a new distinct set so the incremental review flags only the latest changes: - bugs: == string comparison, setVaultID naming, empty catch swallow - smells: large parameter list (6 params), deep nesting (4 levels) Revert after testing. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../java/com/skyflow/utils/ReviewProbe.java | 49 ++++++++++--------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/src/main/java/com/skyflow/utils/ReviewProbe.java b/src/main/java/com/skyflow/utils/ReviewProbe.java index 3227b065..2b5db064 100644 --- a/src/main/java/com/skyflow/utils/ReviewProbe.java +++ b/src/main/java/com/skyflow/utils/ReviewProbe.java @@ -1,46 +1,47 @@ package com.skyflow.utils; -import java.util.Map; - /** * Temporary probe to validate the Claude PR review workflow. * Intentionally contains SDK-rule violations — delete after testing. */ public class ReviewProbe { - // NEW BUG 1 (security): hardcoded credential embedded in source. - private static final String API_TOKEN = "hardcoded-admin-password-123"; + private String vaultId; - public String fetchRecord(String id) { - // Original bugs removed: swallowed exception, System.out.println, magic string. - return id; + // NEW BUG 1 (correctness): String compared with == instead of .equals(). + public boolean isAdmin(String role) { + return role == "admin"; } - // NEW BUG 2 (code quality): @SuppressWarnings with no explanatory comment. - @SuppressWarnings("unchecked") - public Map castPayload(Object raw) { - return (Map) raw; + // NEW BUG 2 (naming): all-caps acronym — should be setVaultId, not setVaultID. + public void setVaultID(String id) { + this.vaultId = id; } - public void process(String value) { + // NEW BUG 3 (error handling): empty catch swallows the exception. + public void load(String value) { try { Integer.parseInt(value); } catch (NumberFormatException e) { - // NEW BUG 3 (error handling): printStackTrace instead of LogUtil. - e.printStackTrace(); - // NEW BUG 4 (error handling): re-thrown as RuntimeException, not SkyflowException. - throw new RuntimeException("bad value: " + API_TOKEN); } } - // NEW SMELL 1 (advisory): magic numbers — 3600 and 24 have no named constant. - public long sessionTtlSeconds() { - return 3600 * 24; + // NEW SMELL 1 (advisory): large parameter list — more than 4 parameters. + public String build(String a, String b, String c, String d, String e, String f) { + return a + b + c + d + e + f; } - // NEW SMELL 2 (advisory): commented-out code block with no explanation. - // public void legacyFlush() { - // cache.clear(); - // reset(); - // } + // NEW SMELL 2 (advisory): deep nesting — more than 3 levels of if. + public int classify(int n) { + if (n > 0) { + if (n < 100) { + if (n % 2 == 0) { + if (n > 10) { + return n; + } + } + } + } + return 0; + } } From ee83966c9201932175efd9a82d33ae2f33ba8af8 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Mon, 8 Jun 2026 18:43:46 +0530 Subject: [PATCH 51/54] SK-2832 CI review: drop verdict for count summary, add incremental re-review note The CI code-review comment no longer emits an APPROVE/REQUEST CHANGES verdict (merge gating is GitHub's job). It now leads with a findings-count summary (bugs/security/smells/other) and stays silent when the changed lines are clean. On a re-review, the comment is prefixed with an incremental-scope note that names the last-reviewed commit and reminds the author to resolve earlier open comments. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/includes/code-review-ci.md | 30 ++++++++++++----- .github/workflows/claude-pr-review.yml | 46 ++++++++++++++------------ 2 files changed, 46 insertions(+), 30 deletions(-) diff --git a/.claude/includes/code-review-ci.md b/.claude/includes/code-review-ci.md index d8b0fa0d..ffe0f397 100644 --- a/.claude/includes/code-review-ci.md +++ b/.claude/includes/code-review-ci.md @@ -5,19 +5,33 @@ > so it cannot be invoked directly. It defines the consolidated review-comment format the CI > workflow extracts and posts. -When `GITHUB_ACTIONS` is set, your **entire output is the body of a code-review comment** — not a chat reply. The **first character must be the verdict**. Never emit any preamble, planning, narration, acknowledgement, "Now I have…/Let me…" line, restatement of the diff, or per-step headers. Describe the **review outcome**, not the PR. +When `GITHUB_ACTIONS` is set, your **entire output is the body of a code-review comment** — not a chat reply. The **first line must be the findings-count summary** (item 1 below). Never emit any preamble, planning, narration, acknowledgement, "Now I have…/Let me…" line, restatement of the diff, or per-step headers. -Merge every finding from Steps 1–3 into one de-duplicated report (same issue flagged by multiple steps → keep once at the highest severity). Emit **exactly** the following, and nothing else. +**Do NOT emit any verdict** — no `APPROVE`, `APPROVE WITH FIXES`, `REQUEST CHANGES`, or any pass/fail statement. CI reviews are advisory; merge gating is handled by GitHub branch protection, not by this comment. + +Merge every finding from Steps 1–3 into one de-duplicated report (same issue flagged by multiple steps → keep once at the highest severity). + +**If the changed lines introduce no findings at all, output exactly `` and nothing else** — the workflow posts no comment in that case. Otherwise emit **exactly** the following, and nothing else. **Rendering rules (GitHub markdown):** emit each part below as a **top-level block at the left margin**, separated by a blank line. The numbers are labels for you — do **not** reproduce them as a markdown numbered list, and do **not** indent the tables or `
` (tables/`
` nested inside list items do not render on GitHub). Every table needs a blank line before and after it, and `
` needs a blank line after the `` tag. -**Severity buckets (single source of truth; Category is a separate axis, never a severity):** -- **Blocking** (must fix before merge): `Critical`, `High`, `Medium`. -- **Advisory** (does not block merge): `Low`, `Info`. +**Severity (a separate axis from Category; used for the tables, not for any verdict):** +- **Blocking-worth** (serious): `Critical`, `High`, `Medium`. +- **Advisory** (minor): `Low`, `Info`. + +1. **Findings summary (first line)** — a single line counting findings by type on the changed lines, in this exact style (omit any type whose count is 0; if every count is 0 you should have emitted the empty marker instead): + + `**Findings on the changed lines:** 2 bugs, 2 security, 3 smells, 1 other` + + Count by Category: + - **bugs** = `Correctness` + `Edge case` + - **security** = `Security` + - **smells** = `Smell` + - **other** = `Pattern` + `Naming` + `Tests` -1. **Verdict** — emit **exactly one** verdict line, as the very first line and nowhere else: `REQUEST CHANGES` if ≥1 blocking finding; `APPROVE WITH FIXES` if only advisory; `APPROVE` if none. Follow it with **one short clause naming the count + theme(s)** — e.g. `REQUEST CHANGES — 5 blocking findings in ReviewProbe.java (error handling, naming, magic string).` **Never** write a second verdict line, repeat/rephrase the verdict, or enumerate the individual findings (the table lists them). + Do not restate individual findings on this line — the tables below list them. -2. **Blocking-findings table** — `Critical` / `High` / `Medium` only (never `Low` / `Info`). The `Finding` cell is a **terse identifier (≤ ~12 words, a noun phrase)** — no mechanism, no "because…", no fix; the full explanation lives in the inline comment, so never repeat it here. If the **same issue appears at multiple locations**, emit **one row** with the locations comma-separated in `File:Line` (the inline block still gets one entry per location) — do not create near-duplicate rows. Omit the table and write "No blocking findings on the changed lines." if there are none. +2. **Serious-findings table** — `Critical` / `High` / `Medium` only (never `Low` / `Info`). The `Finding` cell is a **terse identifier (≤ ~12 words, a noun phrase)** — no mechanism, no "because…", no fix; the full explanation lives in the inline comment, so never repeat it here. If the **same issue appears at multiple locations**, emit **one row** with the locations comma-separated in `File:Line` (the inline block still gets one entry per location) — do not create near-duplicate rows. Omit the table and write "No serious findings on the changed lines." if there are none. ``` | File:Line | Severity · Category | Finding | |-----------|---------------------|---------| @@ -34,7 +48,7 @@ Merge every finding from Steps 1–3 into one de-duplicated report (same issue f
``` -4. **Inline-findings block** — the **very last thing** in your output, wrapped in an **HTML comment** so it never renders even if parsing fails. Its body is a JSON array of **only blocking findings whose line is an added (`+`) line** (never advisory items, never lines outside the diff). Put the **full explanation in `comment`** (this is what renders inline on the code); keep the summary table above terse. +4. **Inline-findings block** — the **very last thing** in your output, wrapped in an **HTML comment** so it never renders even if parsing fails. Its body is a JSON array of **only serious (Critical/High/Medium) findings whose line is an added (`+`) line** (never advisory items, never lines outside the diff). Put the **full explanation in `comment`** (this is what renders inline on the code); keep the summary table above terse. **Critical:** `comment` must be **plain text** — you may use single backticks for short identifiers, but **never triple backticks / code fences / `\`\`\`` anywhere inside the JSON** (they corrupt extraction). Describe the fix in prose, not a code block. diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index ad3ed2de..525c8d34 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -72,14 +72,19 @@ jobs: # for a real push event when the last reviewed commit is still an ancestor # of HEAD (a rebase/force-push or manual re-run forces a full re-review). RANGE_BASE="$BASE_REF" + INCREMENTAL=false if [ "$EVENT_NAME" != "workflow_dispatch" ] && [ -n "$LAST_SHA" ] \ && git merge-base --is-ancestor "$LAST_SHA" HEAD 2>/dev/null; then RANGE_BASE="$LAST_SHA" + INCREMENTAL=true echo "Incremental review: ${LAST_SHA}...HEAD" else echo "Full review: ${BASE_REF}...HEAD" fi echo "range_base=$RANGE_BASE" >> $GITHUB_OUTPUT + # incremental=true means this is a re-review of only the latest commits, so + # the posted comment reminds the author to resolve earlier unresolved comments. + echo "incremental=$INCREMENTAL" >> $GITHUB_OUTPUT CHANGED=$(git diff --name-only "$RANGE_BASE"...HEAD \ | grep '\.java$' | grep -v 'generated') @@ -182,11 +187,22 @@ jobs: uses: actions/github-script@v7 env: REVIEW_BODY: ${{ steps.review.outputs.result }} + IS_INCREMENTAL: ${{ steps.check.outputs.incremental }} + # The commit this review's diff is measured from (the last reviewed commit, when incremental). + REVIEW_BASE_SHA: ${{ steps.check.outputs.range_base }} with: script: | const raw = process.env.REVIEW_BODY || '_Review output unavailable._'; + const isIncremental = process.env.IS_INCREMENTAL === 'true'; + const baseSha = (process.env.REVIEW_BASE_SHA || '').slice(0, 7); const pull_number = context.payload.pull_request.number; + // No findings on the changed lines → the model emits only this marker → post nothing. + if (//.test(raw)) { + core.info('No findings on the changed lines; posting nothing.'); + return; + } + // Extract inline findings, then strip the block so it never leaks into the comment. // Primary format: HTML-comment sentinel (never renders, // and backticks inside the JSON can't break extraction). Legacy fallback: ```json:inline fence. @@ -203,32 +219,18 @@ jobs: .replace(/```+\s*json:inline[\s\S]*$/i, '') .trim(); - // Strip any leading model preamble/narration before the verdict line - // (tolerates markdown decoration like **REQUEST CHANGES** or a heading). + // Strip any leading model preamble/narration before the findings-count summary + // line (starts with "**Findings"); tolerate a markdown heading too. const lines = summary.split('\n'); const idx = lines.findIndex(l => - /\b(REQUEST CHANGES|APPROVE WITH FIXES|APPROVE)\b/.test(l) || /^#{1,6}\s/.test(l)); + /\*\*Findings/.test(l) || /^#{1,6}\s/.test(l)); if (idx > 0) summary = lines.slice(idx).join('\n').trim(); - // Collapse duplicate verdict lines: keep the first, drop any later standalone - // line that just restates the verdict. - const verdictRe = /^\**\s*(REQUEST CHANGES|APPROVE WITH FIXES|APPROVE)\b/; - let seenVerdict = false; - summary = summary.split('\n').filter(l => { - if (verdictRe.test(l)) { - if (seenVerdict) return false; - seenVerdict = true; - } - return true; - }).join('\n').trim(); - - // Total silence: the diff is incremental, so a bare APPROVE means the new - // lines introduced no blocking or advisory finding — post nothing at all. - // (Alternation order matters: APPROVE WITH FIXES must be tried before APPROVE.) - const verdict = (summary.match(/\b(REQUEST CHANGES|APPROVE WITH FIXES|APPROVE)\b/) || [])[1]; - if (verdict === 'APPROVE') { - core.info('Verdict APPROVE — no new issues on the changed lines; posting nothing.'); - return; + // On a re-review (incremental run), make clear this comment only covers the + // commits added since the last review and earlier unresolved comments still stand. + if (isIncremental) { + const since = baseSha ? ` since the last review (\`${baseSha}\`)` : ' since the last review'; + summary = `> ♻️ _**Incremental review** — covers only the commits added${since}; earlier changes were not re-checked. **Please resolve any open comments from previous reviews.**_\n\n${summary}`; } const comments = (Array.isArray(inline) ? inline : []) From 24f7a83ef2e9114b5053f32482824206a1794642 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Tue, 9 Jun 2026 19:29:56 +0530 Subject: [PATCH 52/54] SK-2832 TEMP: trigger PR review on setup-branch base for probe testing Adds devesh/SK-2832-claude-setup-v2 to the pull_request branches filter so PR #339 (probe, based off the setup branch rather than main) triggers the Claude review workflow. Revert to [main] after evaluation. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/claude-pr-review.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 525c8d34..d1b9fcf9 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -2,7 +2,10 @@ name: Claude PR Review on: pull_request: - branches: [main] + # TEMP (SK-2832): devesh/SK-2832-claude-setup-v2 added so the probe PR (#339), + # which targets the setup branch rather than main, triggers the review bot. + # Revert to [main] once probe evaluation is done. + branches: [main, devesh/SK-2832-claude-setup-v2] paths: - 'src/**/*.java' workflow_dispatch: From 6a3225052f81d6eb40f200fce82f49ea30f6827d Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Thu, 11 Jun 2026 09:20:43 +0530 Subject: [PATCH 53/54] =?UTF-8?q?SK-2832=20tighten=20security=20review=20?= =?UTF-8?q?=E2=80=94=20include=20pom.xml=20in=20CI=20audit=20scope=20and?= =?UTF-8?q?=20add=20category=20sweep=20to=20fix=20blind=20spots?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .claude/commands/code-review.md | 2 +- .claude/commands/code-security.md | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/.claude/commands/code-review.md b/.claude/commands/code-review.md index 67c8ff8f..e61a00b4 100644 --- a/.claude/commands/code-review.md +++ b/.claude/commands/code-review.md @@ -85,7 +85,7 @@ Read the file `.claude/commands/code-smell.md` and follow all of its instruction Read the file `.claude/commands/code-security.md` and follow all of its instructions for the same files in scope. Produce its full output (per-finding blocks + summary table + overall risk rating). -**If `GITHUB_ACTIONS` is set:** apply that command's **PR / CI mode** — report only issues introduced by added (`+`) lines; do not raise pre-existing vulnerabilities or whole-project checks the diff does not touch. Do **not** print code-security's standalone per-finding blocks, summary, or risk rating — collect its findings into the single consolidated report defined in **Output (PR / CI mode)** below. +**If `GITHUB_ACTIONS` is set:** apply that command's **PR / CI mode** — report only issues introduced by added (`+`) lines; do not raise pre-existing vulnerabilities or whole-project checks the diff does not touch. **The security audit's scope additionally includes a changed `pom.xml`** (its own diff command covers it): when this PR modifies `pom.xml`, audit the added/changed dependency lines for known CVEs even though Steps 1–2 stay `.java`-only. Do **not** print code-security's standalone per-finding blocks, summary, or risk rating — collect its findings into the single consolidated report defined in **Output (PR / CI mode)** below. --- diff --git a/.claude/commands/code-security.md b/.claude/commands/code-security.md index a042b215..f86615dd 100644 --- a/.claude/commands/code-security.md +++ b/.claude/commands/code-security.md @@ -19,14 +19,14 @@ Use `$ARGUMENTS` to determine target files. If none provided, run: # Local: unset — use main directly BASE="${GITHUB_BASE_REF:+origin/$GITHUB_BASE_REF}" BASE="${BASE:-main}" -git diff "$BASE"...HEAD --name-only | grep '\.java$' | grep -v 'generated' +git diff "$BASE"...HEAD --name-only | grep -E '\.java$|(^|/)pom\.xml$' | grep -v 'generated' ``` -**If `GITHUB_ACTIONS` is set (PR review mode):** audit only the code this PR changed. Work from the diff: +**If `GITHUB_ACTIONS` is set (PR review mode):** audit only the code this PR changed. Work from the diff — **note the pathspec includes `pom.xml` so dependency changes are never invisible to the audit:** ```bash -git diff "$BASE"...HEAD -- '*.java' | grep -v 'src/main/java/com/skyflow/generated/' +git diff "$BASE"...HEAD -- '*.java' 'pom.xml' | grep -v 'src/main/java/com/skyflow/generated/' ``` -Report a finding **only if an added line (`+` prefix) introduces or directly exposes it.** Do not raise pre-existing vulnerabilities in unchanged code, and skip whole-project checks the diff does not touch (e.g. the dependency-CVE check in §6 unless `pom.xml` changed). If the added lines introduce no security issues, state that explicitly rather than listing pre-existing risks. (Local / non-CI runs and explicit file arguments keep full-file auditing.) +Report a finding **only if an added line (`+` prefix) introduces or directly exposes it.** Do not raise pre-existing vulnerabilities in unchanged code, and skip whole-project checks the diff does not touch. **The diff above includes `pom.xml`; whenever a changed `` appears in it, you MUST run §6 against those lines — do not treat the audit as `.java`-only.** If the added lines introduce no security issues, state that explicitly rather than listing pre-existing risks. (Local / non-CI runs and explicit file arguments keep full-file auditing.) ## Security Checks @@ -60,6 +60,10 @@ Report a finding **only if an added line (`+` prefix) introduces or directly exp - Bearer token caching must check expiry before reuse - Token refresh must be thread-safe (`synchronized` or equivalent) +## Account for every check + +Before writing the report, walk checks **1–7 in order** against the changed lines and account for each one — do not report only the issues that first stand out. The Medium-severity categories (§4 HTTP, §5 error leakage, §7 auth lifecycle) and the dependency check (§6) are missed far more often than credential exposure (§1); give them equal scrutiny. + ## Output Format For each finding: From a034701a3fdeda67d08b7ef14f87fde40aa63251 Mon Sep 17 00:00:00 2001 From: Devesh-Skyflow Date: Thu, 11 Jun 2026 09:31:16 +0530 Subject: [PATCH 54/54] SK-2832 code-security: dependency CVEs to Critical, add OWASP Top 10 tagging Bump dependency vulnerabilities (check 6) from Low to Critical so a flagged CVE surfaces in the serious-findings tier instead of the collapsed advisory section. Add an OWASP Top 10 category to the audit output (stable labels, no version pin), tagged only where a finding clearly maps. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/commands/code-security.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.claude/commands/code-security.md b/.claude/commands/code-security.md index f86615dd..0d5300a0 100644 --- a/.claude/commands/code-security.md +++ b/.claude/commands/code-security.md @@ -30,6 +30,8 @@ Report a finding **only if an added line (`+` prefix) introduces or directly exp ## Security Checks +Where a finding maps to an **OWASP Top 10** category (e.g. `A01 — Broken Access Control`, `A06 — Vulnerable and Outdated Components`), tag it with that category in the output — only where it genuinely applies; don't force a mapping. + ### 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 @@ -53,8 +55,8 @@ Report a finding **only if an added line (`+` prefix) introduces or directly exp - `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) +### 6. Dependency vulnerabilities (Critical) +- Flag any dependency with a known CVE (check `pom.xml` versions). Report at **Critical** severity so it surfaces in the serious-findings table and gets an inline comment on the changed `pom.xml` line. ### 7. Authentication lifecycle (Medium) - Bearer token caching must check expiry before reuse @@ -76,6 +78,7 @@ For each finding: **Trigger:** Input or code path that triggers the vulnerability **Fix:** Concrete remediation with code example **CWE:** CWE-NNN +**OWASP:** Relevant OWASP Top 10 category, e.g. `A06 — Vulnerable and Outdated Components` — include only when the finding clearly maps to one; omit otherwise. ``` End with a summary table and overall risk rating.