SK: 2832 Added Claude Code setup for Java

[Devesh-Skyflow]

incremental claude setup changes

[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]

Claude SDK Patterns Review

---

Files reviewed: src/main/java/com/skyflow/ConnectionClient.java src/main/java/com/skyflow/VaultClient.java src/main/java/com/skyflow/utils/HttpUtility.java

[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]

Claude SDK Patterns Review

---

Files reviewed: src/main/java/com/skyflow/ConnectionClient.java src/main/java/com/skyflow/VaultClient.java src/main/java/com/skyflow/utils/HttpUtility.java

[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]

Claude SDK Review

Invalid API key · Fix external API key

[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]

Claude SDK Review

Invalid API key · Fix external API key

[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]

Claude SDK Review

Invalid API key · Fix external API key

[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]

Claude SDK Review

Invalid API key · Fix external API key

[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]

Claude SDK Review

Now I have all the information needed for a comprehensive review. Let me compile the full report.

---

Code Review Report

Scope: 3 files changed vs origin/main

• src/main/java/com/skyflow/ConnectionClient.java
• src/main/java/com/skyflow/VaultClient.java
• src/main/java/com/skyflow/utils/HttpUtility.java

Summary of changes: synchronized added to setBearerToken() in both client classes; RuntimeException replaced with SkyflowException in both prioritiseCredentials() catch blocks; HttpUtility error handling improved to use ErrorMessage.ErrorOccurred and SkyflowException(httpCode, …) constructor with structured error information; wildcard imports replaced with explicit imports in VaultClient.

---

Step 1 — SDK Pattern Review

src/main/java/com/skyflow/ConnectionClient.java

Severity	Line	Finding
Quality	43	setBearerToken() is synchronized but prioritiseCredentials() and setApiKey() (which mutate finalCredentials and apiKey) are not — other methods that call prioritiseCredentials() directly (setCommonCredentials, updateConnectionConfig) are unsynchronised, creating partial protection
Quality	92	Catch block uses ErrorMessage.EmptyCredentials for a generic Exception that is not a credentials-empty scenario — misleads callers with wrong error semantics. A more appropriate message (e.g., ErrorMessage.UnexpectedError) should be used

src/main/java/com/skyflow/VaultClient.java

Severity	Line	Finding
Bug	299	LogUtil.printInfoLog(InfoLogs.REUSE_API_KEY.getLog()) is logged on every call when apiKey credentials are configured — including the first call. ConnectionClient.setApiKey() (line 56–61) correctly distinguishes first-set from reuse; VaultClient.setBearerToken() must mirror that logic
Bug	299–300	VaultClient stores the API key value into the token field (token = this.finalCredentials.getApiKey()), but the apiKey field declared at line 127 is never assigned in VaultClient — it is only reset to null in prioritiseCredentials. The field is dead code in this class and the credential-change detection on line 934 resets a field that was never populated
Edge Case	589	Optional<DeidentifyFileAudioRequestDeidentifyAudioOutputTranscription> outputTranscriptionType = null — initialised to null instead of Optional.empty(). When request.getOutputTranscription() is null, this null Optional is passed to the generated builder's outputTranscription(Optional) setter which stores null directly into the field, causing NullPointerException in equals() / hashCode() later
Edge Case	401–403	Optional<List<FormatMaskedItem>> maskEntities = null (and redactedEntities, plaintextEntities) — same pattern: null Optional is passed to Format.Builder.masked(Optional) which stores it directly, causing NPE in Format.equalTo(). Should be initialised to Optional.empty()
Edge Case	472	Optional<List<ShiftDatesEntityTypesItem>> entityTypes = null in getTransformations() — the if/else guarantees it is always assigned before use on this specific path, but the pattern is dangerous and inconsistent with the rest of the file which uses Optional.empty()
Quality	563–567	Unsafe raw-type cross-cast: Optional.of(list of AudioEntityTypesItem).map(list -> (List<TokenTypeMappingEntityOnlyItem>)(List<?>)list) — this fabricates an Optional<List<TokenTypeMappingEntityOnlyItem>> backed by objects of an unrelated type. Any attempt to use the elements via the typed interface will throw ClassCastException
Quality	571–577	Same unsafe cross-cast pattern for entityUniqueCounter in getDeidentifyAudioRequest — List<DeidentifyFileAudioRequestDeidentifyAudioEntityTypesItem> cast to List<TokenTypeMappingEntityUnqCounterItem>
Quality	295	setBearerToken() is synchronized but prioritiseCredentials(), setCommonCredentials(), and updateVaultConfig() are unsynchronised, giving inconsistent thread-safety coverage
Quality	941–943	Same semantic mismatch as ConnectionClient line 92 — generic Exception mapped to ErrorMessage.EmptyCredentials

src/main/java/com/skyflow/utils/HttpUtility.java

Severity	Line	Finding
Quality	87–88	Improved from previous "replace with description" placeholder — correct fix. No SDK pattern violation
Quality	75	Local variable requestID shadows static field HttpUtility.requestID — confusing and potentially error-prone if the intent changes
Quality	164–168	appendRequestId uses string concatenation with " - requestId: " magic string rather than a Constants entry

---

Step 2 — Code Smell Analysis

src/main/java/com/skyflow/ConnectionClient.java

Smell	Line	Detail
Long class (borderline)	1–96	Class is compact at 96 lines — no violation
Null checks scattered	64–93	prioritiseCredentials has 3 levels of nesting (try/if-else/if) plus 3 catch clauses — extracting the dotenv lookup to a helper would flatten the nesting

src/main/java/com/skyflow/VaultClient.java

Smell	Line	Detail
Long class	1–945	945 lines — well over the 300-line threshold. Class handles token management, 10+ request-builder methods, audio/image/PDF/document file type mapping, and HTTP client wiring. Decomposition by concern (e.g., FileRequestMapper, CredentialManager) is warranted
Long method	334–398	getDeidentifyStringRequest() is 64 lines — candidate for decomposition
Long method	550–611	getDeidentifyAudioRequest() is 61 lines
Long method	499–547	getDeidentifyTextFileRequest() is 48 lines
Dead code	127	private String apiKey — declared and reset but never assigned a value; unreachable assignment path
Dead code	490–496	getEntityTypes() private helper exists but is not called by getDeidentifyStringRequest or getReidentifyStringRequest — its callers do inline getEntityTypes(...) via the public helper, while the body at 490 does the same thing; worth reviewing whether all file-type methods actually use it
Temporary field	125–126	finalCredentials is null at construction and only populated on first setBearerToken() call — acts as a temporary field
Magic strings	534, 534	"// Build the final request" comment restates what the code does — adds no value (explains what, not why)
Duplicated code	617–646, 700–719, 722–746	getDeidentifyPdfRequest, getDeidentifyPresentationRequest, getDeidentifySpreadsheetRequest, getDeidentifyDocumentRequest share an identical pattern of entityTypes/entityUniqueCounter/allowRegex/restrictRegex block followed by buildTokenType() call — could be consolidated
Null checks scattered	401–428	Three consecutive null guards for Optional fields that could use Optional.ofNullable(list).map(...) for clarity

src/main/java/com/skyflow/utils/HttpUtility.java

Smell	Line	Detail
Magic string	164	Literal " - requestId: " — extract to Constants
Long method	28–114	sendRequest() is 86 lines — handles connection setup, content-type dispatch, request/response streaming, error handling, and cleanup; candidate for decomposition
Deep nesting	38–70	4 levels of nesting in sendRequest (try → if → if → try-with-resources)
Variable shadowing	75	Local requestID shadows static HttpUtility.requestID — confusing, prefer String xRequestId for the local

Spell check: No unknown words found by cspell.

---

Smell Summary

Category	Count	Files affected
Long methods	4	VaultClient.java, HttpUtility.java
Long class	1	VaultClient.java
Dead code	1	VaultClient.java (apiKey field)
Duplicated code	1	VaultClient.java (file-type builder methods)
Null checks / Optional misuse	3	VaultClient.java
Magic strings	2	VaultClient.java, HttpUtility.java
Deep nesting	2	HttpUtility.java, ConnectionClient.java
Variable shadowing	1	HttpUtility.java
Stale/trivial comment	1	VaultClient.java

Recommendation: SIGNIFICANT DEBT — VaultClient.java at 945 lines accumulates multiple responsibilities, contains dead code, duplicated builder patterns, and three null Optional initialisation bugs; these need to be addressed before the class grows further.

---

Step 3 — Security Audit

src/main/java/com/skyflow/VaultClient.java : line 905

Severity: Medium
Risk: The OkHttpClient interceptor captures this.token by reference to VaultClient — the Authorization header will always reflect the latest value of this.token. However, because sharedHttpClient is created once and reused across all credential rotations (even after token is reset to null in prioritiseCredentials), a window exists between token = null and the next setBearerToken() call where the interceptor injects "Bearer null" as the header into any concurrent in-flight request.
Trigger: Thread A calls setCommonCredentials() → prioritiseCredentials() resets token = null. Thread B (in-flight request) → interceptor reads this.token = null → sends Authorization: Bearer null.
Fix: Add a null-guard in the interceptor: "Bearer " + (VaultClient.this.token != null ? VaultClient.this.token : ""), and consider making token volatile.
CWE: CWE-362 (Race Condition)

---

src/main/java/com/skyflow/VaultClient.java : line 281–293 (getFileForFileUpload)

Severity: High
Risk: Path traversal — new File(fileUploadRequest.getFilePath()) and new File(fileUploadRequest.getFileName()) accept caller-supplied paths without sanitisation. A crafted path such as ../../etc/passwd would write to or read from arbitrary filesystem locations.
Trigger: Caller passes filePath = "../../sensitive/file" or fileName = "../../evil" to FileUploadRequest.
Fix: Validate that the resolved canonical path starts with an expected base directory, or reject paths containing ..:

File f = new File(path).getCanonicalFile();
if (!f.toPath().startsWith(allowedBaseDir)) throw new SkyflowException(...);

CWE: CWE-22 (Path Traversal)

---

src/main/java/com/skyflow/VaultClient.java : line 285–287

Severity: Medium
Risk: Base64-decoded bytes from a caller-supplied string are written to a new File(fileUploadRequest.getFileName()) resolved relative to the current working directory of the JVM process. If fileName contains path components (e.g., subdir/secret.key), files may be silently created outside the expected output location.
Trigger: FileUploadRequest.getBase64() is non-null and getFileName() contains a relative path.
Fix: Validate getFileName() contains no path separators, or write to a controlled temp directory.
CWE: CWE-22 (Path Traversal / Improper Path Handling)

---

src/main/java/com/skyflow/VaultClient.java : line 299

Severity: Medium
Risk: LogUtil.printInfoLog(InfoLogs.REUSE_API_KEY.getLog()) — confirm that InfoLogs.REUSE_API_KEY does not include the API key value in its log message. If the log message were to include it (e.g., via a format argument later), this would expose credentials to log files.
Trigger: Any call to setBearerToken() with API key credentials.
Fix: InfoLogs.REUSE_API_KEY is confirmed as "Reusing api key." — no credential in the log message. No action needed, but keep this invariant as the enum grows.
CWE: CWE-312 (Cleartext Storage of Sensitive Information) — informational only

---

src/main/java/com/skyflow/VaultClient.java : lines 914–943 / src/main/java/com/skyflow/ConnectionClient.java : lines 64–94

Severity: Medium
Risk: DotenvException (and unknown Exception) in prioritiseCredentials() are caught and re-thrown as SkyflowException(ErrorCode.INVALID_INPUT, ErrorMessage.EmptyCredentials). The original exception's message is discarded but the exception message of EmptyCredentials exposes that credentials are empty — this is expected and acceptable. However, the generic catch (Exception e) silently discards e.getMessage(), which may hinder debugging of unexpected errors (e.g., a SecurityException from the JVM).
Trigger: Any unexpected Exception in prioritiseCredentials().
Fix: Log the unexpected exception class at WARN before rethrowing: LogUtil.printWarningLog("Unexpected error in prioritiseCredentials: " + e.getClass().getSimpleName()).
CWE: CWE-390 (Detection of Error Condition Without Action)

---

src/main/java/com/skyflow/utils/HttpUtility.java : lines 83–89

Severity: Low
Risk: When connection.getErrorStream() returns null (HTTP error but no body), a SkyflowException is thrown with responseBody = "{}". The response headers are included via responseHeaders. This is correct and the fix is sound. No new security issue introduced.
Trigger: HTTP status > 299 with empty error stream.
Fix: No action required — the change is an improvement over the previous placeholder string.
CWE: N/A

---

src/main/java/com/skyflow/utils/HttpUtility.java : line 22 (static mutable field)

Severity: Medium
Risk: private static String requestID is a shared mutable static field written on every request (line 77) without synchronisation. Under concurrent requests, one thread's requestID will overwrite another's. The public getRequestID() method may return the wrong request ID to a caller, potentially exposing request IDs to the wrong error context and leaking transactional correlation to wrong log entries.
Trigger: Two concurrent calls to sendRequest().
Fix: Make requestID a ThreadLocal<String> or remove the static field and pass requestID as a return value/parameter.
CWE: CWE-362 (Race Condition on Shared Resource)

---

Authentication Lifecycle (both client classes)

Severity: Medium
Risk: setBearerToken() is now synchronized, which is correct. However, setCommonCredentials() and updateVaultConfig()/updateConnectionConfig() call prioritiseCredentials() without synchronisation. A concurrent setBearerToken() call and a setCommonCredentials() call can interleave, causing finalCredentials to be read by setBearerToken() while it is being written by setCommonCredentials().
Trigger: Concurrent credential update and token refresh.
Fix: Synchronise setCommonCredentials() and the updateXxxConfig() methods on the same lock, or use a ReadWriteLock.
CWE: CWE-362 (Race Condition)

---

Security Summary Table

File	Line	Severity	Issue
VaultClient.java	281–293	High	Path traversal via unsanitised filePath / fileName
VaultClient.java	285–287	Medium	Base64 decode written to CWD-relative path
VaultClient.java	905	Medium	Interceptor injects "Bearer null" during credential rotation race
VaultClient.java, ConnectionClient.java	Multiple	Medium	setCommonCredentials/updateConfig unsynchronised — race with setBearerToken
HttpUtility.java	22	Medium	Static mutable requestID field — not thread-safe
VaultClient.java, ConnectionClient.java	941/92	Low	Generic exception discarded silently — hinders debugging

Overall Risk Rating: MEDIUM — the path traversal finding (High) and the concurrent requestID static mutation are the most impactful issues and should be addressed before GA release.

---

Final Verdict

Tech-Debt Summary

Category	Issues	Severity
Error Handling	Generic Exception mapped to misleading EmptyCredentials error code in both clients	Quality
SDK Patterns	apiKey field in VaultClient is dead code; REUSE_API_KEY logged on first use	Bug
Null Safety	Three Optional fields initialised to null instead of Optional.empty() — latent NPE in equals/hashCode	Edge Case
Type Safety	Unsafe cross-cast List<AudioEntityTypesItem> to List<TokenTypeMappingEntityOnlyItem> via erasure	Quality
Thread Safety	Incomplete synchronisation: setCommonCredentials, updateVaultConfig, updateConnectionConfig are unsynchronised	Medium Security
Smells	VaultClient at 945 lines; 4 long methods; duplicated file-type builder pattern; dead apiKey field	Significant Debt
Security	Path traversal in getFileForFileUpload; static requestID race; interceptor "Bearer null" race	High + 2×Medium

Verdict: REQUEST CHANGES

The three changes in this PR (synchronized, SkyflowException instead of RuntimeException, and the HttpUtility error-message fix) are individually correct and move the SDK in the right direction. However, the PR uncovers pre-existing issues in the surrounding code — particularly the three null Optional initialisations that are latent NPEs, the dead apiKey field in VaultClient, the misleading REUSE_API_KEY log on first use, the path traversal in getFileForFileUpload, and the unsynchronised requestID static field — that must be addressed before merge, and the incomplete synchronisation coverage that partly defeats the synchronized fix this PR introduces.

Required before merge:

1. Fix null Optional initialisations (lines 401–403, 472, 589 in VaultClient.java) → use Optional.empty()
2. Remove dead apiKey field from VaultClient, or assign it correctly to mirror ConnectionClient
3. Fix REUSE_API_KEY log — only log on second+ use, not the first assignment
4. Fix HttpUtility.requestID static field → ThreadLocal<String>
5. Sanitise file paths in getFileForFileUpload to prevent path traversal
6. Add test coverage for the null error stream + HTTP error path added in HttpUtility
7. Synchronise setCommonCredentials / updateVaultConfig / updateConnectionConfig — or the synchronized setBearerToken provides only cosmetic protection

Run /code-quality again after fixes 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]

Claude SDK Review

Now I have all the information needed to produce the full review. Let me compile the findings.

---

Step 1 — SDK Pattern Review

src/main/java/com/skyflow/ConnectionClient.java

Severity	Line	Finding
Bug	91–93	catch (Exception e) re-throws SkyflowException(ErrorCode.SERVER_ERROR.getCode(), ErrorMessage.EmptyCredentials.getMessage()). The error code is 500 (SERVER_ERROR) but the message says "Validation error. Invalid credentials. Credentials must not be empty." — mismatched code/message pairing. A generic unexpected exception is not a credentials validation failure. Should use a distinct error message (e.g., ErrorMessage.ErrorOccurred).
Quality	56–62	setApiKey() logs REUSE_API_KEY only when reusing, but does not log when setting the key for the first time — asymmetric logging compared to VaultClient.setBearerToken() which logs REUSE_API_KEY unconditionally on every apiKey != null call.
Quality	43	setBearerToken() is now synchronized (the change in this PR). This is correct but the method also calls prioritiseCredentials() inside the lock, which loads a .env file (Dotenv.load()) — a potentially slow I/O operation on every token check. I/O under a broad lock may cause throughput issues under concurrent use.

---

src/main/java/com/skyflow/VaultClient.java

Severity	Line	Finding
Bug	127, 934	apiKey field is declared but never read — only written (apiKey = null on line 934). In setBearerToken() (lines 298–300), when an API key is present the value is stored in token, not apiKey. The field is dead storage. This is inconsistent with ConnectionClient which does use apiKey distinctly. Either the field should be removed, or the assignment on line 300 should also set apiKey (for symmetry with ConnectionClient).
Bug	233	getBatchInsertRequestBody() always passes continueOnError(true) to the API, silently ignoring request.getContinueOnError(). Line 184 in getDetokenizePayload correctly forwards request.getContinueOnError(). This is a hardcoded behavioural override that masks the caller's intent.
Bug	939–942	Same SERVER_ERROR + EmptyCredentials message mismatch as ConnectionClient — the catch-all for any unexpected exception reports a credentials validation message with a server error code.
Quality	150	Method getDetectFileAPi() has a capitalisation typo: APi should be Api. While not introduced in this PR, the method name is exposed in the diff context. Callers in DetectController.java propagate the same typo.
Quality	299	LogUtil.printInfoLog(InfoLogs.REUSE_API_KEY.getLog()) is logged even on the first use of an API key (i.e., when token is null). The log message "Reusing api key" is misleading on first call — it should only fire when the key was already set.
Quality	315	Method is named getDeIdentifyTextResponse (capital I) while all other deidentify methods use getDeidentify* (lower i). This breaks the naming convention.
Quality	401–403, 472, 589	Optional<...> = null — several Optional variables are initialised to null instead of Optional.empty(). An Optional holding null defeats its purpose: if the variable is passed to a builder without a null-check, the builder receives null rather than an absent Optional. Lines: maskEntities, redactedEntities, plaintextEntities (401–403), entityTypes in getTransformations (472), outputTranscriptionType (589). The last case (line 607) passes this raw null directly into .outputTranscription(outputTranscriptionType).
Quality	905	"Authorization" / "Bearer " are magic string literals. Constants.SDK_AUTH_HEADER_KEY already exists for x-skyflow-authorization; a Constants.AUTHORIZATION_HEADER_KEY and Constants.BEARER_PREFIX should be added.
Quality	880–886	mapAudioDataFormat() uses magic string literals "mp3" and "wav" that are not in Constants.

---

src/main/java/com/skyflow/utils/HttpUtility.java

Severity	Line	Finding
Quality	39, 41, 49–51, 59, 63	Magic string literals for HTTP content types: "content-type", "application/json", "multipart/form-data", "application/x-www-form-urlencoded", "Accept", "*/*" — none are backed by Constants. Constants.REQUEST_ID_HEADER_KEY already exists for "x-request-id" but is not used on line 75 which repeats the literal.
Quality	75	Local variable requestID shadows the static field HttpUtility.requestID. While disambiguated by the HttpUtility.requestID = assignment, this is a naming hazard. The local should be named e.g. responseRequestId.
Quality	94	StringBuffer is used instead of StringBuilder. StringBuffer is synchronised and has no advantage in a local, single-thread context.

---

Step 2 — Code Smell Analysis

src/main/java/com/skyflow/ConnectionClient.java

Smell	Line	Detail
Deep nesting	65–93	prioritiseCredentials(): try → if/else-if/else → inner if/else — 3 levels; manageable but the outer catch chain is verbose
Null checks scattered	67–80	Three consecutive null checks (connectionConfig, commonCredentials, dotenv result) could use early-return guard clauses for readability

---

src/main/java/com/skyflow/VaultClient.java

Smell	Line	Detail
Long class	1–945	945 lines — well over the 300-line threshold. Class carries request-body builders, token management, detect adapters, HTTP client construction, and credentials prioritisation — multiple distinct responsibilities.
Long method	334–398	getDeidentifyStringRequest() is 65 lines — candidate for extracting buildTokenTypeForString() and buildFormatOptionals() helpers
Long method	550–611	getDeidentifyAudioRequest() is 62 lines — the bleep parameter block (lines 600–605) and token-format block could be private helpers
Dead code	127	private String apiKey field — declared, reset to null, but never read (see Bug above)
Temporary field	127	apiKey is set to null in one code path but never populated in VaultClient — textbook temporary/phantom field
Optional assigned null	401–403, 472, 589	Initialising Optional<T> to null instead of Optional.empty() — misuse of Optional semantics
Magic strings	881, 884	"mp3", "wav" literals in mapAudioDataFormat() without Constants backing
Magic strings	905	"Authorization", "Bearer " literals in lambda without Constants
Magic number	902	10 and 1 in new ConnectionPool(10, 1, TimeUnit.MINUTES) — extract to Constants.HTTP_MAX_IDLE_CONNECTIONS / Constants.HTTP_KEEP_ALIVE_MINUTES
Magic literal	233	Hardcoded true for continueOnError in getBatchInsertRequestBody() instead of request.getContinueOnError()
Duplicate logic	618–641, 671, 700, 729, 755, 783	buildTokenType() was extracted (line 853) but getDeidentifyPdfRequest() and getDeidentifyDocumentRequest() still inline the if-chain manually instead of calling buildTokenType(). Inconsistent use of the helper.
Large parameter list	550, 662, 696, 722, 748, 776	Several getDeidentify*Request() methods take 4 parameters; the dataFormat/format string parameter is ambiguously named and could be an enum
Comment explains what	534	// Build the final request restates what the next 10 lines obviously do — remove it

---

src/main/java/com/skyflow/utils/HttpUtility.java

Smell	Line	Detail
Long method	28–114	sendRequest() is 87 lines — covers connection setup, header injection, body serialisation, error reading, and response assembly. Should be split into writeRequestBody(), readResponse(), etc.
Magic strings	38–63	"content-type", "application/json", "multipart/form-data", "Accept", "*/*" — no Constants backing
Magic string	75	"x-request-id" repeated when Constants.REQUEST_ID_HEADER_KEY already exists
Variable shadowing	75	Local requestID shadows static field HttpUtility.requestID
StringBuffer vs StringBuilder	94	new StringBuffer() — use StringBuilder in single-threaded context
Null check scattered	84–89	Nested if (httpCode > 299) → if (connection.getErrorStream() != null) → else throw repeats a block at line 101 — consolidate into a handleResponse() method

---

Smell Summary

Category	Count	Files Affected
Long methods	3	VaultClient.java, HttpUtility.java
Long class	1	VaultClient.java
Magic strings/numbers	9	VaultClient.java, HttpUtility.java
Dead / phantom field	1	VaultClient.java
Optional misuse	4	VaultClient.java
Duplicate inline logic	1	VaultClient.java
StringBuffer vs StringBuilder	1	HttpUtility.java
Variable shadowing	1	HttpUtility.java
Trivial comment	1	VaultClient.java

Recommendation: SIGNIFICANT DEBT — VaultClient.java is a 945-line class with multiple responsibilities, duplicate inline logic despite a helper being present, dead fields, and systematic Optional = null misuse that risks NPEs.

---

Step 3 — Security Audit

src/main/java/com/skyflow/VaultClient.java : line 905

Severity: Medium
Risk: The Authorization header value ("Bearer " + this.token) is constructed inside an OkHttp interceptor lambda. If any logging interceptor or network layer logs request headers (e.g., during debugging), the bearer token is exposed in logs. The token itself is not sanitised before header injection.
Trigger: Any OkHttp debug interceptor added downstream, or if the lambda is wrapped in a logger.
Fix: This is an inherent risk of header-based auth. Mitigate by ensuring no HttpLoggingInterceptor is added to the builder chain, and document that callers must not add logging interceptors.
CWE: CWE-532 (Insertion of Sensitive Information into Log File)

---

src/main/java/com/skyflow/VaultClient.java : line 589

Severity: Medium
Risk: Optional<DeidentifyFileAudioRequestDeidentifyAudioOutputTranscription> outputTranscriptionType = null. When transcription == null, outputTranscriptionType remains null (not Optional.empty()). It is then passed directly on line 607 as .outputTranscription(outputTranscriptionType). If the generated builder's outputTranscription() method does not null-guard its Optional parameter, this will throw a NullPointerException at runtime, which would be surfaced to callers unchecked (not wrapped in SkyflowException).
Trigger: Any DeidentifyFileRequest where getOutputTranscription() returns null.
Fix: Change initialisation to Optional.empty():

Optional<DeidentifyFileAudioRequestDeidentifyAudioOutputTranscription> outputTranscriptionType = Optional.empty();

CWE: CWE-476 (NULL Pointer Dereference)

---

src/main/java/com/skyflow/VaultClient.java : line 401–403, 472

Severity: Medium
Risk: maskEntities, redactedEntities, plaintextEntities (lines 401–403) and entityTypes (line 472) are all Optional<List<T>> initialised to null. If the enclosing logic path leaves them null and passes them to a builder setter that expects Optional but does not null-check, the result is an unguarded NPE — not wrapped in SkyflowException, so it escapes to the caller as a raw RuntimeException.
Trigger: ReidentifyTextRequest with no masked/redacted/plaintext entities, or a Transformations object with an empty entity list.
Fix: Initialise all as Optional.empty().
CWE: CWE-476 (NULL Pointer Dereference)

---

src/main/java/com/skyflow/VaultClient.java : line 281–293 (getFileForFileUpload)

Severity: High
Risk: new File(fileUploadRequest.getFilePath()) creates a File object from a caller-supplied path with no path-traversal check (../). While Validations.validateFileUploadRequest() checks f.exists() and f.isFile(), it does not prevent traversal to files outside the intended directory (e.g., /etc/passwd or ../../../sensitive.json). The Base64 branch writes to new File(fileUploadRequest.getFileName()) — a filename-only path — in the current working directory, which may be unintended.
Trigger: Caller passes filePath = "../../credentials.json" to a FileUploadRequest.
Fix: Canonicalise the path before use:

File resolved = new File(fileUploadRequest.getFilePath()).getCanonicalFile();
// optionally: verify it's under an allowed base directory

CWE: CWE-22 (Path Traversal)

---

src/main/java/com/skyflow/utils/HttpUtility.java : line 22, 25, 75–79 (static requestID)

Severity: Medium
Risk: requestID is a non-volatile static field written by multiple threads (one per concurrent HTTP call) and read by callers via getRequestID(). Under concurrent use, a thread reading getRequestID() may get the request ID of a different thread's in-flight request. This leaks one caller's request ID into another caller's error context.
Trigger: Two concurrent API calls that both fail; SkyflowException thrown by one thread may contain the request ID generated by the other.
Fix: Remove the static field. Pass the request ID through the return value or a dedicated response wrapper, or use a ThreadLocal<String>. Example:

private static final ThreadLocal<String> requestId = new ThreadLocal<>();
public static String getRequestID() { return requestId.get(); }
// In sendRequest: requestId.set(...)

CWE: CWE-362 (Race Condition), CWE-200 (Exposure of Sensitive Information to Wrong Party)

---

src/main/java/com/skyflow/ConnectionClient.java : line 91–93 and VaultClient.java : line 939–942

Severity: Low
Risk: The catch (Exception e) block swallows the original exception e completely — the cause is not chained into the new SkyflowException. Any diagnostic information (stack trace, message) from the unexpected exception is permanently lost, making incidents very difficult to debug.
Trigger: Any unexpected runtime exception during prioritiseCredentials().
Fix: Chain the cause:

throw new SkyflowException(ErrorCode.SERVER_ERROR.getCode(), e.getMessage(), e);

Or use an appropriate SkyflowException constructor that accepts a Throwable cause.
CWE: CWE-390 (Detection of Error Condition Without Action)

---

src/main/java/com/skyflow/utils/HttpUtility.java : line 900–910 (no HTTP timeouts)

Severity: Medium
Risk: The OkHttpClient built in updateExecutorInHTTP() has no connectTimeout, readTimeout, or callTimeout configured. OkHttp defaults are 10 seconds for connect and read, but these defaults can be changed globally. Without explicit configuration, a hung server or slow network will block the calling thread indefinitely (or for an unpredictably long time), causing thread exhaustion under load.
Trigger: Vault endpoint hangs or network is slow.
Fix:

sharedHttpClient = new OkHttpClient.Builder()
    .connectTimeout(30, TimeUnit.SECONDS)
    .readTimeout(30, TimeUnit.SECONDS)
    .callTimeout(60, TimeUnit.SECONDS)
    .connectionPool(new ConnectionPool(10, 1, TimeUnit.MINUTES))
    ...

CWE: CWE-400 (Uncontrolled Resource Consumption)

---

Security Summary Table

File	Line	Severity	Issue	CWE
VaultClient.java	905	Medium	Bearer token potentially logged via OkHttp interceptor	CWE-532
VaultClient.java	589, 401–403, 472	Medium	Optional = null — unchecked NPE escapes as RuntimeException	CWE-476
VaultClient.java	281–293	High	Path traversal — no canonical path check on user-supplied file paths	CWE-22
HttpUtility.java	22, 75–79	Medium	Static non-volatile requestID leaks request IDs across threads	CWE-362, CWE-200
ConnectionClient.java / VaultClient.java	91, 942	Low	Original exception cause swallowed — diagnostic info lost	CWE-390
VaultClient.java	899–912	Medium	No connect/read/call timeout on OkHttpClient	CWE-400

Overall Risk Rating: MEDIUM — No credential exposure or token leakage in logs, HTTPS is enforced via Constants.SECURE_PROTOCOL, and token refresh is now thread-safe (synchronized — the PR's main change). The main risks are path traversal on file upload, the thread-unsafe static requestID, Optional = null NPEs under specific caller inputs, and missing HTTP timeouts.

---

Final Verdict

Tech Debt Summary

Category	Issues	Details
SDK Patterns — Error Handling	2 Critical	SERVER_ERROR code paired with EmptyCredentials message (misleading); original cause swallowed in catch-all
SDK Patterns — Naming	3	getDetectFileAPi typo, getDeIdentifyTextResponse capital-I inconsistency, apiKey field never read
SDK Patterns — Request/Response	1	getBatchInsertRequestBody ignores request.getContinueOnError() — hardcodes true
Tests	1	New synchronized keyword and behaviour changes in setBearerToken() are not covered by new unit tests
Smells	9	945-line class, 3 long methods, 4 Optional = null misuse, magic strings/numbers, dead field, duplicate inline logic
Security	5	Path traversal (High), thread-unsafe static requestID (Medium), Optional = null NPE (Medium), no HTTP timeout (Medium), cause swallowed (Low)

Verdict: REQUEST CHANGES

Must fix before merge:

1. Bug (VaultClient.java:233) — getBatchInsertRequestBody hardcodes continueOnError(true); forward request.getContinueOnError().
2. Bug (VaultClient.java:127/934, ConnectionClient.java symmetry) — Dead apiKey field in VaultClient; clarify whether API key should be stored in token or apiKey and make both classes consistent.
3. Security/Bug (VaultClient.java:589, 401–403, 472) — Replace all Optional<T> = null with Optional.empty() to prevent NPE escaping as unchecked RuntimeException.
4. Security (VaultClient.java:281–293) — Add getCanonicalFile() or path-traversal guard before using caller-supplied file paths.
5. Security (HttpUtility.java:22) — Replace static requestID field with ThreadLocal<String> to prevent cross-thread request ID leakage.
6. Error Handling (ConnectionClient.java:92, VaultClient.java:942) — Use a suitable error message for the generic exception catch (not EmptyCredentials), and chain the original cause into SkyflowException.

Should fix before merge:
7. Optional<...> outputTranscriptionType = null → Optional.empty() (line 589).
8. Magic strings "mp3", "wav", "Authorization", "Bearer " → Constants.
9. Magic string "x-request-id" in HttpUtility.java:75 → Constants.REQUEST_ID_HEADER_KEY.
10. OkHttpClient missing explicit timeouts (connectTimeout, readTimeout).

Run /code-quality again after any fixes 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]

Claude SDK Review

Now I have all the data needed for a thorough review. Here is the full output.

---

Step 1 — SDK Pattern Review

src/main/java/com/skyflow/VaultClient.java

Severity	Line	Finding
Bug	941–942	catch (Exception e) in prioritiseCredentials() maps to ErrorCode.SERVER_ERROR (500) with ErrorMessage.EmptyCredentials. A generic unexpected Exception is semantically a server/internal error (500 is reasonable), but the error message EmptyCredentials ("Invalid credentials. Credentials must not be empty.") is a validation-error message and is factually wrong for an unexpected exception. The caller gets a misleading "credentials must not be empty" for a non-credentials failure. Should use a more appropriate message (e.g. ErrorMessage.ErrorOccurred) or re-throw with the original cause attached.
Bug	941–942	The catch (Exception e) silently discards the original exception e. No cause is forwarded to SkyflowException(int, String) which takes (code, String) — the root cause is permanently lost and unloggable. CLAUDE.md rule: "catch (Exception e) without re-throw is always a bug."
Quality	941–942	SkyflowException(ErrorCode.SERVER_ERROR.getCode(), ErrorMessage.EmptyCredentials.getMessage()) — EmptyCredentials is a credentials-validation error message re-used for generic exceptions. Misleading for callers who get a 500 code but a 400-flavoured message.

src/main/java/com/skyflow/ConnectionClient.java

Severity	Line	Finding
Bug	91–92	Same root-cause loss as VaultClient: catch (Exception e) in prioritiseCredentials() constructs SkyflowException(SERVER_ERROR.getCode(), EmptyCredentials.getMessage()) without forwarding e. Original exception is silently swallowed.
Bug	91–92	Same wrong-message issue: EmptyCredentials is a validation message, not suitable for a generic server-side exception.

src/main/java/com/skyflow/utils/HttpUtility.java

Severity	Line	Finding
Quality	87	appendRequestId(ErrorMessage.ErrorOccurred.getMessage(), requestID) — requestID here is the local variable shadowing the static field. At line 75 a local String requestID is declared; the static field is HttpUtility.requestID. Using the local variable here is correct (it was just assigned), but the naming collision between the local variable requestID and the static field HttpUtility.requestID is a pre-existing confusion the new line perpetuates.
Edge Case	87–88	The new SkyflowException(httpCode, new Throwable(description), responseHeaders, "{}") call passes responseHeaders to the four-arg constructor. SkyflowException's four-arg constructor calls setRequestId(responseHeaders) which does responseHeaders.get(Constants.REQUEST_ID_HEADER_KEY). HttpURLConnection.getHeaderFields() can return a map where the null key maps to the status line — passing it into SkyflowException is fine, but if responseHeaders is null (which getHeaderFields() never returns but worth noting), the four-arg constructor would NPE inside setRequestId. This is a pre-existing risk made newly relevant by this change — the original code never hit the four-arg path in this branch.
Quality	87–88	The fallback body "{}" passed as the response body means SkyflowException will try to parse it as JSON, find no "error" key, and silently fall through — resulting in message = null, grpcCode = null, httpStatus = null, and details = null. The actual description message from appendRequestId(...) is put into the Throwable cause, not into message. Callers calling e.getMessage() will get null. Consider constructing the exception so getMessage() returns the description string.

---

Step 2 — Code Smell Analysis

src/main/java/com/skyflow/VaultClient.java

Smell	Line	Detail
Dead code	87–88 (import block)	Import reorganisation (wildcard → explicit) is non-functional but the diff introduces a blank extra line between okhttp3.Request and io.github.cdimascio imports. Minor but signals the file was touched for style reasons; no smell risk.
Stale comment	n/a (pre-existing)	No new stale comments introduced in the diff.
Long method (pre-existing)	295	setBearerToken() is unchanged aside from synchronized; no new smell introduced here.

src/main/java/com/skyflow/ConnectionClient.java

Smell	Line	Detail
No new smells introduced	—	The synchronized keyword addition and exception-replacement change are minimal; no structural issues added.

src/main/java/com/skyflow/utils/HttpUtility.java

Smell	Line	Detail
Magic string	88	Literal "{}" as a placeholder response body for the four-arg SkyflowException. Should be a named constant (e.g. Constants.EMPTY_JSON_BODY) or JsonObject#toString() so the intent is explicit.
Null check scattered	83–88	The if (connection.getErrorStream() != null) / else chain at line 83-89 introduces a new branch. The new else path calls appendRequestId then throws — but the description local variable and the message in new Throwable(description) are only reachable via this one branch. Not decomposed, but acceptable given the small scope.

Spell check: No unknown words found in any of the three changed files.

Smell Summary

Category	Count	Files affected
Magic string	1	HttpUtility.java
Dead code	0	—
Long methods	0 (pre-existing, not introduced)	—

Recommendation: MINOR DEBT — one magic string literal in the new branch; no structural smells introduced by the changes themselves.

---

Step 3 — Security Audit

src/main/java/com/skyflow/VaultClient.java : line 941–942

Severity: Medium
Risk: Information leakage and misleading error attribution. The generic catch (Exception e) silently discards the original exception (including its message and stack trace) and replaces it with a validation-style message ("Invalid credentials"). If an unexpected runtime exception occurs (e.g., a NullPointerException during credential lookup), the caller receives a 500 code with a "credentials must not be empty" message — making debugging impossible without server-side tracing, and potentially misleading operators into thinking the issue is a configuration problem.
Trigger: Any unexpected RuntimeException thrown from inside the try block in prioritiseCredentials().
Fix: Forward the original cause: throw new SkyflowException(ErrorCode.SERVER_ERROR.getCode(), ErrorMessage.ErrorOccurred.getMessage(), e) (using the three-arg constructor), or at minimum log it: LogUtil.printErrorLog(e.getMessage()) before throwing.
CWE: CWE-209 (Generation of Error Message Containing Sensitive Information — inverse: error message hides critical information)

---

src/main/java/com/skyflow/ConnectionClient.java : line 91–92

Severity: Medium
Risk: Same as VaultClient above — silent exception swallowing in catch (Exception e) with misleading error message. Original cause is permanently discarded.
Trigger: Any unexpected RuntimeException in the prioritiseCredentials() try block.
Fix: Same as VaultClient: forward cause or log before throwing.
CWE: CWE-209 / CWE-390

---

src/main/java/com/skyflow/utils/HttpUtility.java : line 87–88

Severity: Medium
Risk: Silent null message. The new SkyflowException(httpCode, new Throwable(description), responseHeaders, "{}") puts the human-readable error description inside a Throwable cause, not in the SkyflowException's own message field. Because the response body "{}" has no "error" key, setResponseBody() falls through without setting message. The exception's getMessage() returns null. Any downstream code calling e.getMessage() (e.g., a logging framework, a caller's catch block) will see null rather than the helpful description string.
Trigger: Any HTTP response with code > 299 and a null/empty error stream.
Fix: Use a constructor that sets message directly, or populate the fallback JSON body with the description:

// Option A — use the (int, String) constructor directly:
throw new SkyflowException(httpCode, description);

// Option B — keep four-arg but put description in body:
String fakeBody = "{\"error\":{\"message\":\"" + description + "\"}}";
throw new SkyflowException(httpCode, new Throwable(description), responseHeaders, fakeBody);

CWE: CWE-755 (Improper Handling of Exceptional Conditions)

---

src/main/java/com/skyflow/VaultClient.java and ConnectionClient.java : setBearerToken() — Thread Safety

Severity: Low (improvement verified correct)
Risk: The synchronized keyword addition is the right fix for the token refresh race condition under concurrent vault/connection operations. No new risk introduced.
Trigger: Concurrent calls to setBearerToken().
Fix: Already applied correctly — no action needed.
CWE: CWE-362 (Race Condition) — mitigated by this change.

---

Security Summary Table

File	Line	Severity	Issue	CWE
VaultClient.java	941–942	Medium	Silent exception swallow + misleading error message	CWE-209/390
ConnectionClient.java	91–92	Medium	Silent exception swallow + misleading error message	CWE-209/390
HttpUtility.java	87–88	Medium	SkyflowException.getMessage() returns null in null-errorStream branch	CWE-755

Overall Risk Rating: LOW-MEDIUM — No credential exposure, no path traversal, no token leakage. All three Medium findings are error-quality issues that impair debuggability rather than create exploitable vulnerabilities.

---

Final Verdict

Tech Debt Summary

Category	Count	Files	Notes
SDK Patterns / Error Handling	4	VaultClient.java, ConnectionClient.java	catch (Exception e) silently discards root cause; wrong ErrorMessage.EmptyCredentials reused for generic exceptions
SDK Patterns / Naming	0	—	No naming violations in changed lines
Tests	1	HttpUtilityTests.java	No test exercises the null-errorStream branch introduced in HttpUtility (the else at line 86–89). That branch throws a SkyflowException with getMessage() == null — an uncovered and incorrect behaviour
Smells	1	HttpUtility.java	Magic string "{}"
Security	3	All three files	Two silent-swallow instances; one null-message exception

Must-Fix Before Merge

1. VaultClient.java line 941–942 and ConnectionClient.java line 91–92: Replace throw new SkyflowException(ErrorCode.SERVER_ERROR.getCode(), ErrorMessage.EmptyCredentials.getMessage()) with a constructor that forwards the original exception as a cause (e.g. SkyflowException(int, String, Throwable)) and uses ErrorMessage.ErrorOccurred as the message. EmptyCredentials is a 400-class validation message and must not be used here.

2. HttpUtility.java line 87–88: The SkyflowException thrown on the null-errorStream path will have getMessage() == null. Use the two-arg SkyflowException(int, String) constructor so getMessage() returns the description string, or restructure the four-arg call so the body JSON contains the message. Additionally, add a test covering this branch.

Verdict: REQUEST CHANGES

Two must-fix bugs (wrong error message semantics + root-cause loss), one missing test branch, and one null-message exception on a newly introduced code path. After fixes, run /code-quality again before merging.

[github-actions]

Claude SDK Review

Now I have all the context I need for a complete review. Let me compile the full review.

---

Code Review — SK-2832

Scope: 3 changed files (CI diff mode — reviewing added lines only)

• src/main/java/com/skyflow/ConnectionClient.java
• src/main/java/com/skyflow/VaultClient.java
• src/main/java/com/skyflow/utils/HttpUtility.java

Pre-requisite: GITHUB_ACTIONS=true — skipping local /code-quality run per instructions.

---

Step 1 — SDK Pattern Review

src/main/java/com/skyflow/ConnectionClient.java

Severity	Line	Finding
Bug	92	catch (Exception e) now throws ErrorMessage.EmptyCredentials for every possible unexpected exception (e.g. a NullPointerException inside Dotenv.load()). The message "Invalid credentials" is actively misleading for non-credential-related failures. The original RuntimeException(e) preserved the cause; the fix loses it entirely and gives users a false diagnosis. Use a distinct ErrorMessage (e.g. ErrorMessage.ErrorOccurred) with SERVER_ERROR, or at minimum preserve e as the cause: new SkyflowException(ErrorCode.SERVER_ERROR.getCode(), e.getMessage()).
Quality	43	setBearerToken() is now synchronized. The lock object is the ConnectionClient instance, which is correct. However the Javadoc (if it exists) should note this is thread-safe. No Javadoc present — add a brief comment explaining the concurrency intent.

---

src/main/java/com/skyflow/VaultClient.java

Severity	Line	Finding
Bug	942	Same root issue as ConnectionClient.java:92 — the catch (Exception e) block for prioritiseCredentials() discards the actual cause and reports EmptyCredentials for every unexpected failure. The exception cause (e) is swallowed, violating the "never swallow exceptions" rule in CLAUDE.md.
Bug	295–313	setBearerToken() is now synchronized, but updateExecutorInHTTP() at line 309 mutates sharedHttpClient and apiClient without the same lock. If setBearerToken() is called concurrently from two threads, the first thread enters the synchronized block, calls updateExecutorInHTTP() which checks if (sharedHttpClient == null) — but sharedHttpClient is a regular instance field written outside the synchronized method. The OkHttpClient creation is not atomic and apiClient is set at line 311 after updateExecutorInHTTP(). This is a TOCTOU race: partial visibility of the apiClient rebuild is not prevented. The correct fix is to also synchronized the updateExecutorInHTTP() call or use a dedicated lock object / double-checked locking with volatile.
Quality	295	Same Javadoc/comment note as ConnectionClient — the concurrency rationale is undocumented.
Quality	150	Pre-existing: getDetectFileAPi() — typo "APi" (not introduced in this PR but unchanged). Flag for awareness — naming convention requires getDetectFileApi().

---

src/main/java/com/skyflow/utils/HttpUtility.java

Severity	Line	Finding
Quality	87	The new SkyflowException(httpCode, new Throwable(description), responseHeaders, "{}") passes "{}" as the response body. SkyflowException(int, Throwable, Map, String) will attempt to parse that JSON and, finding no "error" key, fall silently into the catch block where cause.getLocalizedMessage() returns the description string — so the message does propagate. However, the httpStatus will be set to HttpStatus.BAD_REQUEST.getHttpStatus() ("Bad Request") regardless of the actual HTTP error code (e.g. 502, 503). Consider using the 4-arg constructor more accurately or building a minimal error body.
Quality	87	new Throwable(description) is used purely as a carrier for a message string — a Throwable is semantically misleading here (it implies a stack trace / exception cause). A cleaner pattern would use the SkyflowException(int, String) constructor directly, which is precisely what the 2-arg constructor was designed for.

---

Step 2 — Code Smell Analysis

Spell check: cspell produced no violations on the three changed files.

src/main/java/com/skyflow/ConnectionClient.java

Smell	Line	Detail
Misleading error message	92	Generic Exception e → EmptyCredentials message masks non-credential failures (see Bug above)

src/main/java/com/skyflow/VaultClient.java

Smell	Line	Detail
Long class	1–946	File is 946 lines — well over the 300-line threshold. Pre-existing, not introduced here, but the new import block (+35 lines) makes it longer. Flag for awareness.
Optional fields initialized to null	401–403, 472, 589	Optional<List<FormatMaskedItem>> maskEntities = null and similar patterns. An Optional reference set to null defeats the purpose of Optional — it should be Optional.empty(). These are pre-existing but were present in the same file context reviewed.
Null-assigned Optional passed downstream	589–607	outputTranscriptionType is null when no transcription is set, then passed directly to .outputTranscription(outputTranscriptionType). The builder likely accepts null but it's semantically wrong — use Optional.empty() as the initial value.
Deep nesting	510–580	getDeidentifyAudioRequest() has 4 levels of nesting inside if (tokenFormat != null) → if (...) {} → Optional.of(...).map(...).
Repeated tokenFormat null-check block	Multiple	The tokenFormat != null → entityOnly check → entityUniqueCounter check pattern is repeated verbatim in at least 8 methods (getDeidentifyTextFileRequest, getDeidentifyAudioRequest, getDeidentifyPdfRequest, getDeidentifyGenericFileRequest, getDeidentifyStringRequest, and the 3 that call buildTokenType). The 3 methods calling buildTokenType correctly delegate; the other 5 should do the same — this is a DRY violation at the boundary of what was changed.
Misleading error message	942	Generic Exception e → EmptyCredentials (see Bug above)

src/main/java/com/skyflow/utils/HttpUtility.java

Smell	Line	Detail
Misuse of Throwable as message carrier	88	new Throwable(description) is used solely to pass a string — semantically incorrect, prefer the 2-arg SkyflowException(int, String)

Smell Summary

Category	Count	Files affected
Misleading error message (swallowed cause)	2	ConnectionClient.java, VaultClient.java
Optional initialized to null	4+	VaultClient.java
Repeated tokenFormat-null-check blocks (DRY)	5	VaultClient.java
Deep nesting	1	VaultClient.java
Long class	1	VaultClient.java
Misuse of Throwable as carrier	1	HttpUtility.java

Recommendation: MINOR DEBT — The import cleanup and synchronized additions are good hygiene. The repeated tokenFormat pattern and Optional-as-null smells are pre-existing but not improved by this PR.

---

Step 3 — Security Audit

src/main/java/com/skyflow/ConnectionClient.java : line 92

Severity: Medium
Risk: The catch (Exception e) now wraps the original exception in a SkyflowException with a fixed message but discards the cause entirely (throw new SkyflowException(ErrorCode.SERVER_ERROR.getCode(), ErrorMessage.EmptyCredentials.getMessage())). This is actually a security improvement over the old throw new RuntimeException(e) because RuntimeException(e) would surface the full exception chain (potentially including stack frames with sensitive file paths or credentials-file content) to unchecked exception handlers. The new form is controlled. However, callers that catch RuntimeException (not SkyflowException) will now silently miss these errors — this is a behavioral contract change.
Trigger: Any unexpected error during Dotenv.load() or Credentials comparison in prioritiseCredentials().
Fix: Preserve the fix but add the original as a suppressed exception for debuggability: SkyflowException se = new SkyflowException(ErrorCode.SERVER_ERROR.getCode(), ErrorMessage.ErrorOccurred.getMessage()); se.addSuppressed(e); throw se;
CWE: CWE-390 (Detection of Error Condition Without Action)

---

src/main/java/com/skyflow/VaultClient.java : line 942

Severity: Medium
Risk: Identical to ConnectionClient above — the full cause is swallowed. Additionally, ErrorMessage.EmptyCredentials in a SERVER_ERROR context creates a semantic contradiction: a 500 error with a "Credentials must not be empty" validation message can mislead callers into retrying with different credentials when the real issue was a server-side or environment error.
Trigger: Any unexpected Exception thrown inside prioritiseCredentials() that is not a DotenvException or SkyflowException.
Fix: Same as above — use ErrorMessage.ErrorOccurred and preserve the cause.
CWE: CWE-390, CWE-209 (Generation of Error Message Containing Sensitive Information — the wrong message tells the caller something incorrect about the failure mode)

---

src/main/java/com/skyflow/VaultClient.java : lines 295–313 (setBearerToken / updateExecutorInHTTP)

Severity: Medium
Risk: Thread-safety was partially addressed (the synchronized keyword on setBearerToken()) but updateExecutorInHTTP() reads and writes sharedHttpClient (line 900–912) outside synchronization. Two threads calling setBearerToken() concurrently on the same VaultClient instance cannot race on setBearerToken itself (the synchronized prevents that), but the sharedHttpClient field is not volatile and the apiClient rebuild inside setBearerToken is the critical section — which is in fact all within the synchronized block. On re-inspection: updateExecutorInHTTP() is a private method called within setBearerToken(), so it is protected by the synchronized lock. This is safe. No finding — false alarm corrected.
Note: The synchronized on setBearerToken() is correct and the protection covers updateExecutorInHTTP().

---

src/main/java/com/skyflow/utils/HttpUtility.java : line 87–88

Severity: Low
Risk: The previous code threw new SkyflowException(description) using the single-string constructor (which sets no httpCode, hence defaults to null/0). The new code correctly passes httpCode to the 4-arg constructor — this is an improvement. The responseHeaders are forwarded so x-request-id extraction will work. The "{}" response body is correct (no body available). The new Throwable(description) is benign but non-idiomatic.
Trigger: HTTP error response with a null errorStream (rare but possible with some proxies/firewalls).
Fix: Replace with new SkyflowException(httpCode, description) using the 2-arg constructor for cleaner semantics — but this is low priority.
CWE: CWE-392 (Missing Report of Error Condition) — the original code used a non-standard constructor that lost the HTTP code; the fix resolves this.

---

Security Summary

File	Line	Severity	Issue
ConnectionClient.java	92	Medium	catch (Exception e) discards cause; wrong error message for non-credential failures
VaultClient.java	942	Medium	Same as above — EmptyCredentials message on SERVER_ERROR is semantically wrong
HttpUtility.java	87–88	Low	new Throwable(description) misuse; 2-arg SkyflowException constructor would be cleaner

Overall Risk Rating: LOW — The three changed lines are improvements over the previous code (RuntimeException → SkyflowException; magic string → ErrorMessage; 0-code exception → http-code exception). The residual issues are semantic/clarity concerns, not exploitable vulnerabilities.

---

Final Verdict

Tech Debt Summary

Category	Issue	Files
Error Handling	catch (Exception e) discards the original cause; throws EmptyCredentials for non-credential failures (misleading error code + message on 500)	ConnectionClient.java:92, VaultClient.java:942
Error Handling	HttpUtility: new Throwable(message) used as a string carrier — semantically wrong, prefer SkyflowException(httpCode, description)	HttpUtility.java:88
SDK Patterns	synchronized added but no comment/Javadoc explaining concurrency contract	ConnectionClient.java:43, VaultClient.java:295
Naming	Pre-existing: getDetectFileAPi() typo (not introduced here)	VaultClient.java:150
Smells	Optional<T> = null anti-pattern in several methods (pre-existing)	VaultClient.java
Smells	tokenFormat null-check block copy-pasted across 5 methods — buildTokenType() helper exists but under-used	VaultClient.java

---

Verdict: REQUEST CHANGES

Must fix before merge:

1. ConnectionClient.java:92 and VaultClient.java:942 — Replace ErrorMessage.EmptyCredentials with ErrorMessage.ErrorOccurred (or a more appropriate message) in the catch (Exception e) block, and preserve the original exception as a suppressed or cause. The current code gives callers a 500 + "Invalid credentials" message for failures that have nothing to do with credentials — this will cause production support nightmares. Example fix:

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

   (At minimum — ideally also preserve the cause for debugging.)

2. HttpUtility.java:88 — The fix is functionally correct and a clear improvement over the "replace with description" placeholder. If the team prefers the cleaner 2-arg form: throw new SkyflowException(httpCode, appendRequestId(ErrorMessage.ErrorOccurred.getMessage(), requestID)); — but this is low priority and could be addressed separately.

Reminder: Run /code-quality after applying fixes before merging.