Devesh/sk 2832 claude setup v2

[Devesh-Skyflow]

Start with a concise summary of the PR. The first three sections are required. The questions present in each section is there to help you guide you what to add. They are meant to be overwritten by your comments.

Why

• Why are you making the change?
• What is the underlying issue that you are trying to case, in case of fix?
• Why is it needed by the feature you are working on?
• What is the intent behind making the change?

Goal

• What is the intended outcome?
• What part of the feature should start working?
• What are the non-goals or will be covered in future PR?

Testing

• How was the code tested?
• If you haven't written unit tests, why?
• What more testing is needed? Do you intend to manually test it after deployment?
• Do you have any concerns if this changed is released to prod?

Tech debt

• Is the PR adding to tech debt in any way?
• Are you addressing some Tech debt in this PR?
• If both the above are false, feel free to remove this section.

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[github-actions]

Semgrep Findings: Issues with Error level severity are found (Error is Highest severity in Semgrep), Please resolve the issues before merging.

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[github-actions]

Semgrep Findings: Issues with Error level severity are found (Error is Highest severity in Semgrep), Please resolve the issues before merging.

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[github-actions]

Semgrep Findings: Issues with Error level severity are found (Error is Highest severity in Semgrep), Please resolve the issues before merging.

[github-actions]

AI Code Review

REQUEST CHANGES — 4 blocking findings in ReviewProbe.java (hardcoded credential in source and exception message, printStackTrace, RuntimeException instead of SkyflowException, @SuppressWarnings without comment).

File:Line	Severity · Category	Finding
ReviewProbe.java:9	Critical · Security	Hardcoded credential literal in source
ReviewProbe.java:22	Critical · Security	Hardcoded credential value leaked into exception message
ReviewProbe.java:20	High · Pattern	e.printStackTrace() instead of LogUtil
ReviewProbe.java:22	High · Correctness	RuntimeException thrown instead of SkyflowException; method lacks throws SkyflowException
ReviewProbe.java:19	Medium · Pattern	@SuppressWarnings("unchecked") with no explanatory comment

Advisory (Low / Info) — 0 items

File:Line	Severity · Category	Finding

[github-actions]

Critical · Security: Hardcoded credential literal API_TOKEN = "hardcoded-admin-password-123" is committed to source. Even in a test probe, embedding credentials in source risks accidental promotion to production code and exposure in git history (CWE-798). Remove the literal and inject any needed test value via an environment variable or test fixture — never a static final String constant.

[github-actions]

Critical · Security: The hardcoded credential API_TOKEN is concatenated directly into the RuntimeException message: "bad value: " + API_TOKEN. This surfaces the credential value in stack traces, logs, and any caller that catches the exception. Never include a credential (even a placeholder) in an exception message. Strip the token from the message; if context is needed, log it at DEBUG via LogUtil before throwing — do not put it in the exception payload.

[github-actions]

High · Pattern: e.printStackTrace() writes directly to stderr, bypassing the SDK logging framework. Per CLAUDE.md, all logging must go through LogUtil (e.g. LogUtil.printErrorLog(...)). Replace e.printStackTrace() with LogUtil.printErrorLog(e.getMessage()) or equivalent.

[github-actions]

High · Correctness: process() throws RuntimeException and is not declared throws SkyflowException. SDK rules require all public methods to declare throws SkyflowException and to wrap/re-throw all exceptions as SkyflowException, never as bare RuntimeException. Change the throw to throw new SkyflowException(ErrorCode.INVALID_INPUT, ...) and add throws SkyflowException to the method signature.

[github-actions]

Medium · Pattern: @SuppressWarnings("unchecked") is present with no explanatory comment. Per CLAUDE.md, every @SuppressWarnings must have a comment explaining why suppression is safe. Add a comment above the annotation, e.g. // safe: caller guarantees the object is Map<String, Object>.

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[github-actions]

Semgrep Findings: Issues with Error level severity are found (Error is Highest severity in Semgrep), Please resolve the issues before merging.

[github-actions]

AI Code Review

APPROVE WITH FIXES — 0 blocking findings; 2 advisory smells introduced in ReviewProbe.java (magic numbers, commented-out code).

No blocking findings on the changed lines.

Advisory (Low / Info) — 2 items

File:Line	Severity · Category	Finding
src/main/java/com/skyflow/utils/ReviewProbe.java:35	Low · Smell	Magic numbers 3600 and 24 in sessionTtlSeconds() — extract to named constants
src/main/java/com/skyflow/utils/ReviewProbe.java:39	Low · Smell	Commented-out legacyFlush() block with no TODO/ticket — remove or annotate

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[github-actions]

Semgrep Findings: Issues with Error level severity are found (Error is Highest severity in Semgrep), Please resolve the issues before merging.

[github-actions]

AI Code Review

♻️ Incremental review — covers only the commits added since the last review (fb8901d); earlier changes were not re-checked. Please resolve any open comments from previous reviews.

Findings on the changed lines: 2 bugs, 0 security, 2 smells, 1 other

File:Line	Severity · Category	Finding
src/main/java/com/skyflow/utils/ReviewProbe.java:9	High · Correctness	String identity comparison with == instead of .equals()
src/main/java/com/skyflow/utils/ReviewProbe.java:14	Medium · Naming	Method named setVaultID — SDK convention requires setVaultId

Advisory (Low / Info) — 3 items

File:Line	Severity · Category	Finding
src/main/java/com/skyflow/utils/ReviewProbe.java:19	Low · Smell	Empty catch block silently swallows NumberFormatException
src/main/java/com/skyflow/utils/ReviewProbe.java:24	Low · Smell	Method build has 6 parameters — exceeds 4-parameter limit; consider a builder or config object
src/main/java/com/skyflow/utils/ReviewProbe.java:28	Low · Smell	Deep nesting — 4 levels of if in classify(); extract inner blocks to named helpers

[github-actions]

High · Correctness: String comparison uses == (reference equality) instead of .equals(). In Java, two String objects with the same content are not guaranteed to be the same object. role == "admin" will almost always return false even when role is "admin". Fix: return "admin".equals(role);

[github-actions]

Medium · Naming: Method is named setVaultID but the SDK naming convention requires acronyms to be treated as words: setVaultId (camelCase). This is inconsistent with setVaultId(), setClusterId(), setSkyflowId() used throughout the SDK. Rename to setVaultId.

[saketh-skyflow]

🤖 Claude Code Review

Summary: The PR introduces Claude AI tooling configuration (commands, hooks, CI workflows) for automated code review and quality checks. One critical issue stands out: a test probe file with intentional bugs is being committed to the main source tree, plus a hardcoded absolute path in a hook script. The CI workflow also has a potential command injection vector.

Risk: high

Approved: no

Findings

src/main/java/com/skyflow/utils/ReviewProbe.java:1 — ReviewProbe
This file contains intentional bugs (string == comparison, swallowed exceptions) and is explicitly marked "delete after testing" — it must not be merged into main as it ships broken production code to SDK consumers.

// NEW BUG 1 (correctness): String compared with == instead of .equals().
public boolean isAdmin(String role) {
    return role == "admin";
}

---

.claude/hooks/checkstyle-on-edit.py:22 — (module level)
The root path is hardcoded to an absolute path on the developer's local machine (/home/devb/SDK/skyflow-java), which means this hook will silently do nothing (or error) on any other machine, including CI runners and other contributors' environments.

root = '/home/devb/SDK/skyflow-java'

---

.github/workflows/claude-pr-review.yml:175 — generate-review step
The $COMMITS variable (raw git log output containing commit messages from untrusted PR authors) is interpolated directly into a shell run: block, creating a command injection risk — a commit message with shell metacharacters could alter the Claude CLI invocation or exfiltrate the ANTHROPIC_API_KEY.

COMMITS="${{ steps.commits.outputs.log }}"
NOTES=$(claude --print ... -p "...
$COMMITS
...")

Fix: pass the commits via a temporary file or an environment variable rather than inline shell expansion.

---

.github/workflows/claude-changelog.yml:44 — generate-release-notes step
Same injection pattern: ${{ steps.commits.outputs.log }} (raw git log of commit messages) is interpolated directly into the shell run: block before being passed to the claude CLI, allowing a malicious commit message to inject shell commands in a workflow that has contents: write permission.

COMMITS="${{ steps.commits.outputs.log }}"
NOTES=$(claude ... -p "...
$COMMITS
...")

---

.github/workflows/claude-pr-review.yml:155 — sdk-review job
The REVIEW_BASE_SHA and BASE_BRANCH environment variables derived from step outputs (which themselves come from GitHub API data) are used unquoted in a git diff command, which could allow whitespace or glob injection; they should always be double-quoted in the shell.

git diff "${RANGE_BASE}...HEAD" --name-only | grep '\.java$' | grep -v 'generated'

---

.github/workflows/claude-pr-review.yml:1 — workflow trigger
The workflow posts PR comments (write permission) using output from claude CLI driven by untrusted PR diff content; if a PR author can influence the Claude output (prompt injection via code comments), they could craft content that looks like a malicious review comment, though impact here is limited to misleading review text rather than code execution.

---

src/main/java/com/skyflow/VaultClient.java — setBearerToken / file handling
New file-reading logic added in this PR (Files.readAllBytes, new File(path)) does not appear to validate the path against path traversal (e.g. ../), which contradicts the security rule stated in .claude/commands/code-security.md §2.

---

src/main/java/com/skyflow/ConnectionClient.java:42 — prioritiseCredentials
The catch (Exception e) block now throws a SkyflowException with ErrorMessage.EmptyCredentials for any non-SkyflowException exception, which will misreport the root cause (e.g. a NullPointerException or IOException would be reported as "empty credentials"), making debugging very difficult; the original exception should be chained or its message preserved.

} catch (Exception e) {
    throw new SkyflowException(ErrorCode.SERVER_ERROR.getCode(), ErrorMessage.EmptyCredentials.getMessage());
}

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[github-actions]

Semgrep Findings: Issues with Error level severity are found (Error is Highest severity in Semgrep), Please resolve the issues before merging.

[github-actions]

AI Code Review

♻️ Incremental review — covers only the commits added since the last review (ee83966); earlier changes were not re-checked. Please resolve any open comments from previous reviews.

Findings summary:

• DetokenizeExample.java line 11: DetokenizeRecordResponse is imported but used only in comments (dead import) — Low · Smell
• GetExample.java line 60: downloadURL(true) in commented-out code references deprecated method without noting it — Low · Smell
• ReidentifyTextExample.java line 92: e.printStackTrace() — sample pattern consistent with existing detect samples, not a new violation introduced by this PR
• No security issues in added lines (hardcoded tokens in the sample text are placeholder documentation values, not real credentials)
• No serious (Critical/High/Medium) findings

All findings are advisory (Low/Info). Emitting the CI format:

---

Findings on the changed lines: 2 smells

No serious findings on the changed lines.

Advisory (Low / Info) — 2 items

File:Line	Severity · Category	Finding
samples/src/main/java/com/example/vault/DetokenizeExample.java:11	Low · Smell	Unused import — DetokenizeRecordResponse is referenced only in comments
samples/src/main/java/com/example/vault/GetExample.java:60	Low · Smell	Commented example uses deprecated downloadURL(Boolean) — the current method is downloadUrl(Boolean)

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[github-actions]

Semgrep Findings: Issues with Error level severity are found (Error is Highest severity in Semgrep), Please resolve the issues before merging.