Add file de-identification and re-identification tools

[jstjoe]

Add file de-identification and re-identification tools

Summary

Adds three MCP tools that expose Skyflow's Detect file endpoints, with first-class support for passing files by signed/public URL and a job-handle pattern for Skyflow's asynchronous file processing:

Tool	What it does
de-identify-file	Detects and redacts sensitive data in images (bmp/jpg/jpeg/png/tif/tiff), PDFs, doc/docx, xls/xlsx/csv, ppt/pptx, txt, json/xml, dcm, and audio (mp3/wav).
get-file-run-status	Polls an asynchronous de-identification run by runId, with optional bounded server-side long-polling (waitSeconds, 0–55s).
re-identify-file	Restores original values in previously de-identified csv/doc/docx/json/txt/xls/xlsx/xml files (synchronous endpoint).

File input: URL or base64

Skyflow's file endpoints only accept base64 content — they cannot fetch URLs. So the file tools accept either:

• fileUrl — a signed or public URL (S3/GCS presigned, etc.). The server downloads it and converts it to base64 before forwarding to Skyflow. The download is hardened: https/http only; a 25 MB cap enforced while streaming (so a missing/spoofed Content-Length can't force an unbounded buffer); a 30 s timeout that covers the body read, not just the headers; and an SSRF guard that blocks loopback/private/link-local/CGNAT/metadata hosts across encodings (dotted-quad, 32-bit decimal, hex, IPv4-mapped IPv6) and re-validates the host on every redirect hop. DNS-name resolution to an internal IP is out of scope for the guard (documented) — rely on network egress controls for that.
• fileDataBase64 + fileName — inline base64 for hosts that can pass file content directly. The JSON body limit was raised 5 MB → 25 MB to accommodate this.

The file name (explicit arg → Content-Disposition → URL path → Content-Type fallback) determines the extension, which selects Skyflow's type-specific endpoint and data_format.

Async handling

File de-identification at Skyflow is asynchronous (POST returns a run_id; results come from GET /v1/detect/runs/{run_id}). Because this server is stateless (new transport + Skyflow client per request, serverless-friendly), the Skyflow runId itself is used as the durable job handle — the standard MCP polling-tool pattern:

1. de-identify-file starts the run and waits up to waitTimeSeconds (default 25 s, SDK max 64 s). Small files complete inline and return the processed file directly.
2. If the run is still processing, the response carries runId, status: "IN_PROGRESS", and a note telling the agent to call get-file-run-status with that runId.
3. get-file-run-status returns the full result (processed file base64, detected-entity artifacts, counts) on SUCCESS, an isError response with Skyflow's failure message on FAILED, or the same polling note if still in progress. waitSeconds long-polls server-side with backoff to reduce round-trips.

MCP's experimental tasks API (SEP-1686) was considered and deliberately not used: it requires server-side task state, which this per-request/serverless architecture doesn't have, and host support is still minimal. The runId-based polling tool works with every MCP host today.

Common Skyflow file options supported

entities, allowRegexList/restrictRegexList, tokenType (entity_unique_counter | entity_only), maskingMethod (BLACKBOX/BLUR for images), outputProcessedFile (images/audio), outputOcrText (images), outputTranscription (audio, plaintext/diarized), pixelDensity/maxResolution (PDFs), dateShift (min/max days + entities), and audio bleep (gain/frequency/padding). Re-identification supports redactedEntities/maskedEntities/plainTextEntities routing.

Implementation notes

• skyflow-node upgraded ^2.0.0 → ^2.1.2. 2.0.0's deidentifyFile had a broken timeout path (resolved {runId} then destructured it as an array → TypeError), which made the async flow unusable. 2.1.2 returns {runId, status: "IN_PROGRESS"} correctly.
• src/lib/detect/detectRest.ts — small fetch-based client for the two Detect endpoints the SDK doesn't expose at a usable level: GET /v1/detect/runs/{run_id} (the SDK's getDetectRun drops the failure message) and POST /v1/detect/reidentify/file (no high-level SDK method exists). It sends the same Authorization: Bearer <credential> the SDK sends, and defensively parses both camelCase and snake_case response fields — the live API returns camelCase for several fields even though the OpenAPI-generated types say snake_case (Skyflow's own SDKs read both).
• src/lib/files/fileSource.ts — URL download + input resolution. ResolvedFile carries the decoded Buffer so the ~25 MB payload isn't needlessly round-tripped base64→Buffer→base64 on the request path.
• src/lib/mappings/fileFormats.ts — format allowlists mirroring the generated REST enums, extension↔MIME maps, and the set of formats that honor transformations, with clear upfront validation errors.
• Real MIME types — the SDK/API report a category label (e.g. redacted_image) in the file type field, not a MIME type. Processed-file outputs derive a real MIME from the file extension so UIs render/download correctly.
• Fewer misleading fields — a warning is emitted when dateShift is requested for a format Skyflow silently ignores it on (image/PDF/doc/spreadsheet/presentation); zero-valued count fields the SDK defaults to 0 are omitted.
• Response size — file-tool results keep the full payload in structuredContent but replace the large base64 blobs with placeholders in the text content channel, so a multi-MB file isn't serialized and shipped twice per response.
• Request context now carries vaultUrl + the raw credential value (never logged) so REST-based tools can authenticate; exposed via getDetectRestContext().
• UI apps: de-identify-file UI updated (IN_PROGRESS banner with polling guidance, URL-aware loading state, warnings display) and shared with get-file-run-status; new re-identify-file UI with inline text preview + download link.
• Anonymous mode: all three file tools return setup-instruction errors (consistent with re-identify).

Testing

• 76 new unit tests across the new/updated modules (deIdentifyFile, getFileRunStatus, reIdentifyFile, fileSource) covering URL/base64 input, async polling, REST error mapping, SSRF blocking (incl. alternate IP encodings and redirect re-validation), streaming size cap, and MIME derivation. 244 tests total, all passing.
• Full build (pnpm build) green.
• Live smoke test: tools/list shows all 5 tools with correct UI resource URIs; validation paths (missing input, both inputs, unsupported formats, SSRF-blocked URLs incl. decimal-encoded loopback) return clean isError outputs; URL download pipeline verified end-to-end against a public URL; REST error mapping verified against an unreachable vault.
• Reviewed with a multi-angle adversarial pass; all surfaced findings (SSRF hardening, download timeout/memory bounds, MIME correctness, async status handling, redundant base64 work) were fixed and re-verified.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Nv6ppGHwgQFFZSd32FBiZA

---

Generated by Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
sky-mcp-streamable	Ready	Preview, Comment	Jul 4, 2026 11:16pm

[github-actions]

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

[claude]

Code Review — File de-identification & re-identification tools

Thorough, well-documented PR. The async job-handle pattern is the right call for a stateless/serverless server, the SSRF hardening is genuinely careful (streaming size cap, per-hop redirect re-validation, alternate IP encodings), and toFileToolResult avoiding double-shipping multi-MB base64 is a nice touch. CLAUDE.md, README, CHANGELOG, and the modify-a-tool checklist are all updated. Comments below, ordered by importance.

Security

1. SSRF guard misses shorthand / octal IPv4 forms (medium).
ipLiteralFromHostname handles dotted-quad, 32-bit decimal (2130706433), hex (0x7f000001), and IPv4-mapped IPv6 — nicely covered by tests. But it doesn't normalize:

• dotted-shorthand like 127.1 or 10.1 (isIP("127.1") === 0, and it contains a . so the all-digits branch is skipped), and
• octal forms like 0177.0.0.1.

Node's resolver / inet_aton-style parsing commonly expands http://127.1/ to 127.0.0.1, so these can reach loopback/metadata despite the guard. Consider normalizing these forms (or, better, see #2).

2. Residual SSRF via DNS is the real exposure — worth more than a doc note.
The guard only inspects IP literals; a hostname whose A record points at 169.254.169.254 (cloud metadata) or an internal address sails through. This is documented as out-of-scope, but since this feature's whole purpose is to have the server fetch arbitrary user-supplied URLs, that's the primary risk, not an edge case. Strongest mitigation is to resolve the hostname yourself and run each resolved address through isBlockedIp before connecting (and pin the connection to the validated IP to avoid a rebind between check and fetch). At minimum, please make the deployment docs' "rely on network egress controls" guidance prominent rather than a parenthetical — a default deploy on a cloud VM with a reachable metadata endpoint is exploitable.

3. Verify raw API keys work as Authorization: Bearer for the REST calls.
detectRest.ts forwards credentialKey directly as Authorization: Bearer <value>. For JWT bearer tokens that matches the SDK. But when a client authenticates with a Skyflow API key, the SDK normally exchanges credentials for a bearer token internally — here we bypass the SDK. If the Detect REST endpoints don't accept a raw API key as a bearer token, then get-file-run-status and re-identify-file will 401 for API-key users while de-identify/de-identify-file (which go through the SDK) keep working — a confusing partial failure. Please confirm against the live API for both credential types (the PR notes only mention verifying against an unreachable vault), or document the limitation.

Correctness / bugs

4. detectRest metadata fields only read a single key. sizeInKb: pick(data, "size"), durationInSeconds: pick(data, "duration"), pageCount: pick(data, "pages"), slideCount: pick(data, "slides") don't apply the same camelCase/snake_case tolerance used everywhere else in this file, and the key names look guessed. If the live API names them differently (or nests them like wordCharacterCount), these silently drop to undefined. Worth confirming the actual field names, or extending pick to cover both casings for consistency.

Performance / operational

5. 25 MB JSON body limit is now global. express.json({ limit: "25mb" }) applies to every request, including the text-only de-identify/re-identify tools, raising the memory/DoS surface for all endpoints. Consider scoping the large limit to the file paths, or at least note the per-request memory cost (25 MB raw + ~33 MB base64 + JSON overhead) against serverless memory limits and concurrency.

6. Long-poll vs. serverless timeouts. waitSeconds up to 55s and a 65s per-fetch timeout can exceed common serverless function limits (e.g. Vercel's default). It's flagged in CLAUDE.md pitfalls — good — just make sure the deployed function timeout is configured accordingly.

Nits

• get-file-run-status returns a success-shaped output when status is SUCCESS but no processed file is present (e.g. OCR/transcription-only runs) — correct here — while re-identify-file treats SUCCESS-without-file as an error. Both are right for their endpoints; just calling out the intentional asymmetry.
• fileNameFromContentDisposition only matches the literal UTF-8'...' charset prefix in filename*; other RFC 5987 charsets would fall through to the plain filename param. Fine in practice.

Tests

Good coverage — 76 new tests spanning URL/base64 input, async polling with backoff, REST error mapping, redirect re-validation, streaming size cap, and several IP-encoding SSRF blocks. Suggest adding cases for the shorthand/octal forms in #1 (127.1, 0177.0.0.1) so the guard's boundary is pinned down.

Overall this is solid, defensive work. #1–#3 are the items I'd want resolved (or explicitly confirmed) before merge; the rest are polish.

🤖 Generated with Claude Code

[github-actions]

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

[jstjoe]

Thanks for the careful review — pushed 7eff41c addressing the security items.

#2 (DNS SSRF) — fixed. This was the real gap. assertHostAllowed now resolves the hostname (dns.lookup, all: true) and rejects if any resolved address is private/loopback/link-local/CGNAT/metadata, and the redirect loop re-validates each hop. Residual DNS-rebinding (check-time vs fetch-time TOCTOU) can't be fully closed without pinning the connection to the validated IP via a custom undici dispatcher; I've left that out for now but documented it and made the "enforce network egress controls (block 169.254.169.254)" guidance prominent in both the README (an [!IMPORTANT] callout) and CLAUDE.md rather than a parenthetical. New tests cover a hostname resolving to metadata and a resolution failure.

#1 (shorthand/octal IPv4) — already covered, now pinned by tests. downloadFileFromUrl parses through new URL() before validating, and Node's WHATWG parser normalizes these to canonical dotted-quad first (http://127.1/ → 127.0.0.1, http://0177.0.0.1/ → 127.0.0.1, http://2852039166/ → 169.254.169.254), so isBlockedIp catches them. ipLiteralFromHostname looks permissive read in isolation, but it only ever sees the already-normalized url.hostname. Added explicit test cases for 127.1 / 0177.0.0.1 / 2852039166 to lock this down.

#3 (raw API key as Bearer) — verified consistent, no change. The SDK does the same thing: for API-key creds, getBearerToken returns { key: apiKey }, VaultClient.initConfig sets currentToken = authInfo.key, and the generated Files client sends Authorization: Bearer <that>. So the SDK's deidentify-file path and detectRest.ts's runs/reidentify calls send the identical header — API-key auth works the same on both. (JWTs are likewise forwarded verbatim.)

#4 (metadata field keys) — confirmed against the SDK. size/duration/pages/slides are the exact keys skyflow-node's own parseDeidentifyFileResponse reads from this endpoint (data.size, data.duration, …); they're single-word so there's no camel/snake variant to fall back to. Counts do use the dual-casing wordCharacterCount/word_count handling.

#5 / #6 (global body limit, long-poll vs serverless timeout) — acknowledged, doc-level. The 25 MB limit can't be scoped per-tool since everything is one /mcp endpoint (the tool name is inside the yet-to-be-parsed body); the memory/timeout tradeoffs are noted in CLAUDE.md pitfalls. Callers that don't need URL input can use fileDataBase64.

Nits — the get-file-run-status vs re-identify-file SUCCESS-without-file asymmetry is intentional (a de-identify run can legitimately produce OCR/transcription-only output; re-identify must return a file), as you noted.

All 249 unit tests pass and the build is green.

---

Generated by Claude Code

[claude]

Code Review — Add file de-identification and re-identification tools

Thorough, well-documented PR. The SSRF hardening (encoding-normalized IP checks, per-hop redirect re-validation, DNS resolution of hostnames), the streaming size cap that ignores a spoofed Content-Length, the "keep base64 out of the text channel" response optimization, and the runId-as-job-handle async pattern are all solid design choices. The PR description and inline comments make the tradeoffs easy to follow. Below are findings, roughly by impact.

Potential bugs / correctness

1. getDetectRunRest reads count fields with single names while everything else reads both casings (src/lib/detect/detectRest.ts:187-190).
sizeInKb: pick(data, "size"), durationInSeconds: pick(data, "duration"), pageCount: pick(data, "pages"), slideCount: pick(data, "slides") use a single guessed field name each, whereas the rest of the parser defensively reads camelCase and snake_case (per the file's own header note). If the live API returns e.g. sizeInKb/pageCount/durationInSeconds (matching what the SDK path in deIdentifyFile.ts reads) or the snake_case equivalents, the polled result silently drops these counts while the inline result keeps them — an inconsistency between the two paths for the same run. The unit tests assert size/pages but they're authored against the same assumption, so they don't independently confirm the wire shape. Recommend applying the same dual/normalized pick(...) here, or verifying the field names against a live poll response.

2. IN_PROGRESS responses carry a misleading extension/mimeType (src/lib/tools/deIdentifyFile.ts:269-273).
processedExtension = response.extension || resolved.extension and output.mimeType = ... are set unconditionally, so an IN_PROGRESS run (no processedFileData yet) still reports the input file's extension and a real MIME type. A UI keying a download/preview affordance off mimeType/extension could render a control for a file that isn't there. Consider only setting these when response.fileBase64 is present.

Security

3. 25 MB JSON body limit now applies to every request, before auth (src/server.ts:506 vs :527).
express.json({ limit: "25mb" }) is a global middleware that runs before the /mcp route's authenticateBearer and anonymousRateLimiter. So an unauthenticated client can force the server to buffer up to 25 MB per request (5× the previous 5 MB) before credentials are ever checked — a cheap memory-pressure vector, and the rate limiter (also route-scoped) can't gate it. Options: scope the large limit to the file endpoints only, keep /mcp at a smaller limit and rely on fileUrl for large files, or document the expected upstream/proxy body limit for deployments.

4. SSRF residual: validated IP isn't pinned for the actual connection (src/lib/files/fileSource.ts:129-159).
The DNS-rebinding TOCTOU is already documented — good. Two things worth adding to that note: (a) fetch re-resolves independently and may connect to a different address than the one assertHostAllowed validated (round-robin / dual A+AAAA records where only one is internal); (b) closing this properly means pinning the connection to the validated IP (custom lookup/agent) rather than re-resolving. Acceptable to defer to egress controls as documented, but pinning would be the real fix if untrusted URLs are in scope.

Design / operational

5. Long server-side waits may exceed common gateway timeouts.
de-identify-file waits up to MAX_WAIT_TIME_SECONDS = 64, get-file-run-status long-polls up to MAX_STATUS_WAIT_SECONDS = 55, and detectRest uses a 65 s request timeout. Many serverless/proxy front-ends cap at 30–60 s. MAX_STATUS_WAIT_SECONDS is commented as staying "under gateway timeouts," but the 64 s de-identify wait and 65 s REST timeout can blow past a 60 s edge. Worth documenting a recommended deployment timeout, or lowering the defaults to a safer ceiling.

Nits

• isBlockedIp IPv6 uses startsWith("fc")/startsWith("fd") which correctly covers fc00::/7; fine. It doesn't cover a few IPv4 reserved test ranges (192.0.2.0/24, 198.18.0.0/15) — not security-relevant, just noting completeness.
• toFileToolResult replaces detectedEntities (an array in the schema) with a string in the text channel only; structuredContent keeps the real array, so schema validation is unaffected. Correct, just easy to misread.

Test coverage

Good breadth (76 new tests) covering SSRF encodings, redirect re-validation, streaming cap, MIME derivation, async polling, and REST error mapping. The main gap is that the REST response field names (finding #1) are asserted against assumed shapes rather than a captured live response — a recorded fixture from a real run would harden the highest-risk area.

Nice work overall — the async handling and download hardening are genuinely careful.

🤖 Generated with Claude Code

[github-actions]

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

[jstjoe]

Thanks for the second pass — pushed 5cf9d01.

#2 (IN_PROGRESS leaking extension/mimeType) — fixed. Good catch. deIdentifyFile now sets processedFileData/extension/mimeType only inside the if (response.fileBase64) block, so an IN_PROGRESS run reports just runId/status/note and never implies a downloadable file. (The de-identify-file UI already returned early for IN_PROGRESS, but structuredContent consumers saw the stale values.) Added assertions to the IN_PROGRESS test.

#3 (25 MB body buffered pre-auth) — fixed. Moved express.json({ limit: "25mb" }) off the global app and onto the /mcp route, after authenticateBearer and anonymousRateLimiter. authenticateBearer only reads the Authorization header + apiKey query param and the limiter only reads the IP, so an unauthenticated or rate-limited client is now rejected before the body is buffered. Verified: an unauthenticated POST returns 401 (not a 413 after buffering), and authenticated calls still work.

#1 (count field names) — confirmed correct, no change. I checked this against skyflow-node's source: its own parseDeidentifyFileResponse reads exactly data.size, data.duration, data.pages, data.slides (and data.wordCharacterCount.wordCount) from this same runs endpoint. So getDetectRunRest and the SDK-backed inline path both derive from the identical wire keys — the two paths are consistent, and these are single-word keys with no camel/snake variant. You're right that a recorded live fixture would be stronger than asserting an assumed shape; I don't have live Skyflow credentials in this environment to capture one, so the SDK source (the authoritative consumer of the same endpoint) is the cross-check I relied on.

#4 (pin validated IP) — deferred, documented. Agreed the DNS check is check-time only and fetch re-resolves independently (incl. dual A/AAAA where only one is internal). Full closure needs a custom undici dispatcher pinning the connection to the validated address; I've left that out to avoid interfering with deployment-level HTTP(S) proxy setups, and the README/CLAUDE.md call out enforcing egress controls (block 169.254.169.254) as the required mitigation when untrusted URLs are in scope.

#5 (long waits vs gateway timeouts) — documented, defaults are safe. The defaults stay under a typical 30 s edge (de-identify-file waits 25 s; get-file-run-status defaults to a single check, waitSeconds 0). The higher ceilings (64 s / 55 s / 65 s) are opt-in, and because the runId persists, a gateway cutting off a long wait is recoverable by polling rather than a lost result. Noted in CLAUDE.md pitfalls.

Nits — the IPv4 doc/benchmark ranges (192.0.2.0/24, 198.18.0.0/15) aren't routable internal targets so I left them out; the toFileToolResult text-channel string vs structured array is intentional as you noted.

249 tests pass, build green.

---

Generated by Claude Code

[claude]

Code Review — File de-identification & re-identification tools

Reviewed the full diff statically (build/test execution was sandbox-blocked in my environment, so I'm trusting the PR's reported pnpm build + 244-test green run). Overall this is a high-quality, carefully-scoped PR — the async job-handle pattern, per-hop SSRF re-validation, and response-size handling are all thoughtfully done, and the CLAUDE.md tool checklist was followed end to end. Below are observations, roughly by severity.

Strengths

• SSRF hardening is genuinely thorough (fileSource.ts): scheme allowlist, IP-literal decoding across dotted-quad/decimal/hex/IPv4-mapped-IPv6, DNS resolution with "block if any resolved address is internal," and re-validation on every redirect hop with redirect: "manual". The residual DNS-rebind TOCTOU is correctly identified and documented.
• Streaming size cap (readBodyWithCap) defends against a missing/spoofed Content-Length — the buffer can't grow unbounded before the check.
• Body-parser relocation (25 MB express.json moved onto /mcp after authenticateBearer + rate limiter) closes a real pre-auth memory-exhaustion vector. Nice catch in the follow-up commit.
• Response-size optimization (toFileToolResult) keeps full data in structuredContent but strips base64 from the text channel — avoids double-shipping multi-MB payloads.
• Gating processed-file metadata on actual output so an IN_PROGRESS run doesn't advertise a phantom downloadable file.
• Test coverage is strong — 80 cases across the four new modules, including both camelCase and snake_case REST parsing, SSRF encodings, redirect re-validation, and the streaming cap. detectRest.ts has no dedicated test file but is well-exercised indirectly via fetch-mocked tool tests.

Suggestions / things to consider

1. Serverless timeout tension (medium). The PR is framed as serverless-friendly, yet de-identify-file holds a request up to 64 s (SDK waitTime) and get-file-run-status up to 55 s server-side. Vercel/Lambda function timeouts (10–60 s depending on tier) can sever these long-held requests before the poll returns. Worth documenting a recommended waitTimeSeconds/waitSeconds for constrained platforms, or noting the minimum function-timeout requirement, so users don't hit truncated responses.

2. SUCCESS-without-processed-file reads as an empty win (low). In getFileRunStatus.toOutput, processedFileData is only set when an output item has processedFile && type !== "entities", and the polling note is attached only when status !== "SUCCESS". If a run reports SUCCESS but yields no non-entities processed file, the caller gets a success-shaped response with no file and no note. reIdentifyFile.ts already guards this (treats SUCCESS-without-file as an error, lines 87–96) — consider mirroring that defensiveness here.

3. de-identify-file non-terminal status without a note (low). In deIdentifyFile.ts the polling note is attached only when status === "IN_PROGRESS" (line 316). If the SDK ever returns a runId with a different non-complete status and no fileBase64, the agent gets runId+status but no guidance to poll. Attaching the note for any "has runId but no processed file" case (as getFileRunStatus does with status !== "SUCCESS") would be more robust.

4. detectRest sends the bearer to an unvalidated vaultUrl (informational). detectFetch forwards Authorization: Bearer <credentialKey> to the raw context.vaultUrl with no host allowlist. This isn't an escalation — the credential is the caller's own and vaultUrl was already client-controlled for the SDK — but note the difference from the SDK path, which derives clusterId and builds its own URL rather than trusting the full URL. A one-line comment that the credential is only ever sent to the caller-specified vault would preempt future confusion.

5. IPv6 blocklist minor gaps (low). isBlockedIp uses prefix-string checks (startsWith("fc")/"fd"/"fe80") and doesn't cover NAT64 (64:ff9b::/96) or some special-purpose ranges. Given the documented reliance on network egress controls this is acceptable, but it's the least robust part of an otherwise strong guard.

None of the above are blockers. Items 2 and 3 are the ones I'd most encourage addressing since they affect the agent-facing contract. Nice work overall.

Reviewed by Claude Opus 4.8 — static review; build/tests not re-run locally.

[jstjoe]

Thanks — pushed e138067 addressing the two contract items (#2, #3) you flagged.

#3 (de-identify-file non-terminal status without a note) — fixed. It now attaches the polling note whenever there's a runId and the status isn't SUCCESS (was checking the exact "IN_PROGRESS" string), mirroring get-file-run-status. A completed run that returned no file inline (entities-only) reports SUCCESS and correctly gets no note.

#2 (get-file-run-status SUCCESS-without-file) — fixed, as a note rather than an error. When a run reports SUCCESS but yields no processed file and no entities, it now attaches an explanatory note so the response doesn't read as a lost result. I kept it a note rather than mirroring re-identify-file's error, because (as noted in the earlier round) a de-identify run can legitimately produce no file artifact — whereas re-identify must always return one, so erroring there is correct.

#4 (bearer to vaultUrl) — added the clarifying comment. detectFetch now documents that the credential is the caller's own and only ever goes to the caller-specified, per-request-validated vault URL — never a host derived from user file input.

#1 (serverless timeouts) — documented. The defaults are safe (de-identify-file 25 s; get-file-run-status single-check by default); the higher opt-in ceilings and the "configure the function timeout / prefer smaller waitSeconds on constrained platforms; the runId persists so a cut-off wait is recoverable by polling" guidance are in the CLAUDE.md pitfalls.

#5 (NAT64 / special-purpose IPv6) — left as-is. Acceptable given the documented egress-control requirement; NAT64 embeds an arbitrary IPv4 and blocking the whole 64:ff9b::/96 risks over-blocking legitimate deployments, so I'd rather not guess there.

252 tests pass, build green. Appreciate the three thorough passes.

---

Generated by Claude Code

[github-actions]

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

[claude]

Review: Add file de-identification and re-identification tools

Thorough, well-documented PR. The async job-handle pattern, the streaming SSRF-hardened download path, and the per-route body parser (moved after auth/rate-limiting) are all solid engineering, and the 76 new unit tests cover the tricky paths (alternate IP encodings, redirect re-validation, streaming size cap, MIME derivation). Findings ordered by importance.

🔴 Highest priority — REST path forwards the caller's bearer to an unvalidated host

detectRest.ts sends Authorization: Bearer <credentialKey> to context.vaultUrl verbatim (src/lib/detect/detectRest.ts:89,96-99), where vaultUrl is the normalized, client-supplied query parameter. But the SDK path never uses that host — it derives the host from clusterId and pins it to *.vault.skyflowapis.com. validateVaultConfig/extractClusterId only require the substring X.vault anywhere in the string (src/lib/validation/vaultConfig.ts:60), so:

vaultUrl=https://abc.vault.attacker.com   ->  clusterId="abc"  (passes validation)
  SDK  (de-identify)      -> https://abc.vault.skyflowapis.com   [pinned]
  REST (status/re-id)     -> https://abc.vault.attacker.com      [bearer sent here]

Two consequences:

1. Credential forwarding to an arbitrary host. Blast radius is limited because the credential is the caller's own, but a crafted vaultUrl makes the server ship that token to an attacker-controlled domain. This is a different trust boundary than the fileUrl SSRF guard you (correctly) hardened: there the concern is where we fetch from; here it's where we send credentials to.
2. Silent host-divergence bug. The de-identify (SDK) and status/re-identify (REST) calls in the same flow can target different hosts, so a runId from the SDK path may not resolve on the REST path if the domains differ.

Recommendation: validate that vaultUrl's host ends with an allowlisted Skyflow suffix (e.g. .vault.skyflowapis.com / .dev) in validateVaultConfig, and/or build the REST base URL from clusterId exactly as the SDK does so both paths provably hit the same host. Also consider rejecting non-https vaultUrl on the REST path — normalizedVaultUrl currently preserves a caller-supplied http://, which would send the bearer in plaintext.

🟡 Serverless timeout budget

The app is exported "for serverless environments (like Vercel)". de-identify-file can block the request up to MAX_WAIT_TIME_SECONDS (64s) plus URL download (up to 30s), get-file-run-status up to 55s, and detectRest uses a 65s per-call timeout. These exceed the default function duration on several serverless tiers (often 10–60s), surfacing as opaque gateway timeouts rather than the clean IN_PROGRESS/polling contract. Worth documenting that the bounded-wait defaults may need lowering behind short-timeout gateways.

🟢 Minor / nits

• In-progress label coupling (getFileRunStatus.ts:106): the long-poll loop only continues while run.status === "IN_PROGRESS". A different non-terminal label (e.g. PENDING/RUNNING) would exit the loop immediately — handled gracefully via the still-processing note, but no server-side waiting occurs. Consider a poll condition of "not in {SUCCESS, FAILED}".
• IPv6 blocklist is prefix-based (fileSource.ts:65): fc/fd/fe80 cover ULA + link-local, but other reserved ranges (e.g. 2001:db8::) aren't covered. Low risk given the DNS-resolution guard.
• pick<T> casts without runtime checks (detectRest.ts:71): a numeric field returned as a string would flow through unchecked into a number field. Not observed, just fragile.
• UI rendering escapes interpolated fields and confines base64 to data: URL bodies — no XSS concerns spotted. 👍

Test coverage

Strong — the SSRF matrix and streaming size cap are well exercised. If you act on finding #1, add a test asserting a non-Skyflow vaultUrl host is rejected (or that the REST base is derived from clusterId).

---

🤖 Reviewed with Claude Code

[github-actions]

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

[jstjoe]

Good catch on #1 — this was a real gap the earlier passes and I missed. Fixed in 637e941.

🔴 #1 (bearer forwarded to an unvalidated host) — fixed. You're right: extractClusterId only requires <id>.vault to appear anywhere, so https://abc.vault.attacker.com validated, and the REST path sent the bearer there while the SDK pinned to abc.vault.skyflowapis.com. Rather than trust the client host at all, the REST base URL is now derived from clusterId via a new getVaultBaseUrl(clusterId) that mirrors the SDK's getVaultURL(clusterId, Env.PROD) → https://<clusterId>.vault.skyflowapis.com. Consequences:

• The credential can only ever be sent to the real Skyflow host — a crafted vaultUrl host is ignored entirely.
• The SDK and REST paths provably target the same vault, so a runId minted by de-identify-file always resolves on get-file-run-status.
• It's always https, which also closes the plaintext-bearer concern for a client-supplied http:// vaultUrl.

I went with "build from clusterId" over "allowlist the vaultUrl suffix" because it's the stronger guarantee (zero trust in the client host) and it can't over-reject regional Skyflow domains. Added a test asserting a crafted abc.vault.attacker.com still yields a skyflowapis.com REST base.

🟢 in-progress label coupling — fixed too. The get-file-run-status long-poll now continues while status is not in {SUCCESS, FAILED} (was keyed on the exact IN_PROGRESS string), so waitSeconds waits through a PENDING/RUNNING-style label. Added a test polling through a PENDING status to SUCCESS.

🟡 serverless timeout budget — documented (as in the prior rounds): defaults are safe (de-identify-file 25 s; get-file-run-status single-check by default), the higher ceilings are opt-in, and CLAUDE.md notes to lower the waits / raise the function timeout behind short-timeout gateways; the persisted runId makes a cut-off wait recoverable by polling.

Remaining nits — acknowledged, no change. IPv6 doc/reserved ranges (2001:db8::) aren't internal targets; the pick<T> casts trust the documented Skyflow response contract (and the two-casing tolerance already covers the real variance). Both are noted as low-risk in your review.

255 tests pass, build green. Thanks again — the credential-host issue was the most valuable find across all four passes.

---

Generated by Claude Code

[claude]

Code Review — Add file de-identification and re-identification tools

Overall this is a high-quality, carefully-considered PR. The security hardening around the fileUrl download path (streaming size cap, per-hop redirect re-validation, IP-encoding-aware SSRF guard, deriving the Detect REST base URL from clusterId instead of the client-supplied vaultUrl) is genuinely well done, and the stateless runId-as-job-handle pattern is a sensible fit for the serverless architecture. The 76 new tests and the multi-round self-review show. Comments below are mostly minor polish — nothing blocking.

Strengths

• SSRF defense is layered and correct: isBlockedIp covers loopback/private/link-local/CGNAT/multicast plus IPv4-mapped IPv6, ipLiteralFromHostname catches decimal/hex encodings (defense-in-depth on top of new URL() normalization), hostnames are DNS-resolved and each redirect hop is re-validated. The residual DNS-rebinding TOCTOU is explicitly documented with an egress-control recommendation — the right call.
• getVaultBaseUrl(clusterId) closes a real credential-forwarding vector — good catch that extractClusterId only requires the <id>.vault substring, so https://abc.vault.attacker.com would otherwise have passed.
• Body parser moved off the global app onto /mcp after auth + rate limiting — prevents an unauthenticated client from forcing a 25 MB buffer. Nice.
• toFileToolResult stripping base64 blobs from the text channel avoids double-shipping multi-MB payloads while keeping them in structuredContent.

Suggestions (non-blocking)

1. Inline base64 size ceiling is inconsistent with the download cap. MAX_DOWNLOAD_BYTES is 25 MB of decoded bytes, but base64 inflates ~33%, and the express.json({ limit: "25mb" }) limit applies to the encoded JSON body. So an inline fileDataBase64 file above ~18 MB raw is rejected by the body parser with a generic PayloadTooLargeError before the clean FileSourceError in resolveFileInput can fire. Consider raising the JSON limit to ~34 MB (or documenting that inline files are effectively capped lower than URL downloads) so the two paths agree and the error is friendly.

2. A body-read timeout surfaces a raw AbortError message. In downloadFileFromUrl, the try/catch that maps aborts to "download timed out after 30s" wraps only the fetch() call. readBodyWithCap() runs outside that try, so if the 30s timeout fires during the streamed body read, the for await throws a raw AbortError that reaches the tool's generic catch as "The operation was aborted." rather than the intended timeout message. The size-cap abort correctly throws a FileSourceError, but the timeout-during-body case doesn't. Wrapping the readBodyWithCap call in the same abort→FileSourceError mapping would make the "timeout covers the body read" behavior match its error message.

3. Entity/format validation runs after the file download. In both handleReIdentifyFile and handleDeIdentifyFile, resolveFileInput (which may download from a URL) executes before getEntityEnum(...) validates the entity names. An invalid entity string therefore pays for a full download before failing. Cheap to validate the entity args first. Minor.

4. getVaultBaseUrl hardcodes the PROD host (.skyflowapis.com). This matches the SDK's default getVaultURL(clusterId, Env.PROD) used elsewhere, so the REST and SDK paths stay consistent today. Just flagging it as a latent coupling: if a deployment is ever pointed at a non-prod Skyflow environment, the two REST-based tools (get-file-run-status, re-identify-file) would silently target prod. A short comment noting the env assumption (or threading env from the SDK config) would help future maintainers.

5. getEntityEnum(e) as string for REST format routing. reidentifyFileRest sends DetectEntities enum values as the redacted/masked/plaintext arrays, relying on those values being exactly the strings the /reidentify/file API expects. The PR notes a live smoke test was done, so this is likely fine — worth a brief inline note that the enum value equals the API contract, since a future enum-value change would break routing silently rather than at compile time.

Tests

Coverage is strong — URL/base64 input, async polling, REST error mapping, SSRF (incl. alternate IP encodings + redirect re-validation), streaming cap, and MIME derivation are all exercised. Two gaps worth a quick add: (a) inline fileDataBase64 that exceeds the body limit (item 1), and (b) a body-read-phase timeout producing the timeout message (item 2).

Nice work overall — the async/stateless design and the SSRF/credential-host hardening are the standout parts.

🤖 Generated with Claude Code

[github-actions]

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

[jstjoe]

Thanks — pushed 7c9cfee with all five polish items.

#2 (body-read timeout → raw AbortError) — fixed. You're right: the abort→message mapping only wrapped fetch(), so a timeout during the streamed body read escaped as "The operation was aborted." Extracted a downloadFailure helper and wrapped the readBodyWithCap call too, so a body-phase timeout now reports "download timed out after 30s" (the size-cap FileSourceError still passes through unchanged). Added a test with a body stream that aborts mid-read.

#1 (inline base64 ceiling vs download cap) — fixed. Raised the /mcp JSON limit to 34 MB so an inline fileDataBase64 up to the 25 MB decoded cap (base64 +~33% plus the JSON envelope) fits and fails with the clean FileSourceError from resolveFileInput rather than a generic PayloadTooLargeError. Inline and URL paths now share the same effective 25 MB raw ceiling. (This stays behind the post-auth/rate-limit body parser, so it doesn't reopen the pre-auth buffering vector.)

#3 (entity validation after download) — fixed. Both handlers now run getEntityEnum(...) on the entity args (and dateShift.entities) before resolveFileInput, so an invalid entity name fails fast instead of paying for a full download first.

#4 (getVaultBaseUrl PROD coupling) — documented. Added a comment noting it hardcodes the PROD host to match the SDK instance (which this server creates with clusterId only → Env.PROD), and that a non-prod deployment must update the SDK config and this helper together.

#5 (enum value == API contract) — noted inline. Added a comment where the reidentify format arrays are built that the DetectEntities enum value is exactly the string the /reidentify/file API expects.

256 tests pass, build green. Appreciate the five rounds — the credential-host derivation and this body-read timeout fix were the most substantive.

---

Generated by Claude Code

[claude]

Code Review — File de-identify / re-identify tools

Thorough, well-structured PR. The handler/schema/test discipline from CLAUDE.md is followed throughout, docs (README, CHANGELOG, CLAUDE.md) are updated to five tools, and test coverage (URL/base64 resolution, async polling, REST casing, SSRF, streaming cap) is genuinely good. Notes below are mostly hardening/robustness — nothing blocking.

Strengths

• Credential-forwarding safety is the standout. Deriving the REST base from getVaultBaseUrl(clusterId) rather than the loosely-validated client vaultUrl (src/lib/validation/vaultConfig.ts, wired in src/server.ts:597-601) closes a real exfiltration path — extractClusterId only requires the <id>.vault substring, so https://abc.vault.attacker.com would otherwise have received the bearer token. Good catch, well documented.
• SSRF guard (fileSource.ts) is comprehensive: multi-encoding IP literals, DNS resolution with per-address blocking, per-redirect re-validation, and a streaming size cap that survives a spoofed/absent Content-Length. The TOCTOU limitation is honestly documented.
• Response de-duplication in toFileToolResult (base64 replaced by a placeholder in the text channel, full payload only in structuredContent) avoids shipping multi-MB blobs twice.

Suggestions

1. Default synchronous wait may exceed serverless limits (medium). The PR positions the runId pattern as serverless-friendly, but DEFAULT_WAIT_TIME_SECONDS = 25 (deIdentifyFile.ts:39), MAX_WAIT_TIME_SECONDS = 64, and get-file-run-status waitSeconds up to MAX_STATUS_WAIT_SECONDS = 55 all hold the HTTP request open. On platforms like Vercel (Hobby 10s, Pro 60s default) a 25s default wait can hit a platform 504 before the graceful runId response is ever produced — defeating the purpose of the async handle. Consider a smaller default (e.g. 8–10s) and/or documenting the interaction with the deployment function timeout.

2. DNS lookup in assertHostAllowed is not covered by the download timeout (minor). downloadFileFromUrl sets a 30s AbortController, but that signal only aborts fetch. The pre-fetch lookup() (fileSource.ts:151) has no timeout, so a slow/hostile resolver can block past DOWNLOAD_TIMEOUT_MS. Wrapping the lookup in a Promise.race with a timeout would make the 30s bound actually total.

3. Undrained response bodies on redirect / error paths (minor). With redirect: "manual", 3xx responses (fileSource.ts:293-318) and non-ok responses are neither read nor cancelled before continue/throw. Under undici this can keep the socket alive until GC. A response.body?.cancel() before continuing/throwing releases it promptly.

4. Possible self-contradictory response for an inline-completed run (minor robustness). In handleDeIdentifyFile, if the SDK ever returns fileBase64 and a runId with an absent/non-SUCCESS status, the response carries both a processed file and a still-processing polling note (deIdentifyFile.ts:272-330). Tests suggest completed runs report SUCCESS, so this is latent, but guarding the note with if (!output.processedFileData && ...) would make it robust to a missing status field.

5. Inconsistent zero-count handling between the two file tools (nit). deIdentifyFile.ts:295-317 intentionally omits 0 counts, while getFileRunStatus.ts:58-63 includes any value the API sends explicitly (!== undefined, so an explicit 0 passes through). Not a bug, but the two tools can surface the same run counts differently.

6. Doc nit. CLAUDE.md says 25MB JSON payload limit but src/server.ts:523 uses 34mb (sized for a 25MB decoded file + ~33% base64 inflation + envelope). Worth clarifying that 25MB is the decoded-file cap, not the JSON body limit.

Nice work overall — the security reasoning in the download path and the credential-host derivation are the highlights.

🤖 Generated with Claude Code

[github-actions]

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

[jstjoe]

Thanks — pushed 04098f8 with the concrete items.

#2 (DNS lookup not covered by the download timeout) — fixed. The lookup() in assertHostAllowed is now raced against the download's AbortSignal, so a slow/hostile resolver can't block past DOWNLOAD_TIMEOUT_MS — the 30s bound is now genuinely total, not fetch-only.

#3 (undrained response bodies) — fixed. Added drainBody(response) (response.body?.cancel()) on the redirect-continue and non-ok paths so redirect: "manual" responses don't hold the socket until GC.

#4 (self-contradictory inline-completed response) — fixed. The polling note is now suppressed when processedFileData is present, so a run can never return both a processed file and a "still processing" note, even if the SDK ever pairs fileBase64 with a non-SUCCESS/absent status. Added a test.

#5 (inconsistent zero-count handling) — fixed. get-file-run-status now omits zero counts too (was !== undefined), matching de-identify-file, so the same run's counts surface identically from both tools.

#6 (doc nit: 25MB vs 34mb) — fixed. CLAUDE.md now states the 34MB JSON body limit explicitly and clarifies that 25MB decoded is the user-facing file cap for both inline and URL inputs.

#1 (default synchronous wait vs serverless limits) — documented, default kept at 25s. I kept the 25s default (it suits the 60s Pro-tier this realistically targets and lets more small files complete inline without a poll round-trip), but strengthened the CLAUDE.md pitfall: the bounded waits hold the HTTP request open, so the function timeout must exceed the chosen wait + download, and on short-timeout tiers (e.g. Vercel Hobby 10s) callers should pass a smaller waitTimeSeconds/waitSeconds or raise maxDuration — with the runId making any cut-off wait recoverable by polling. Lowering the global default to 8–10s would penalize the common Pro deployment for the constrained-tier case, so I made it tunable-and-documented rather than changing the default; happy to revisit if you'd prefer a lower default.

257 tests pass, build green.

---

Generated by Claude Code

[claude]

Code Review — File de-identification & re-identification tools

Thorough, well-structured PR. The async job-handle pattern is the right call for a stateless/serverless server, the SSRF hardening is genuinely careful, and the docs (CLAUDE.md pitfalls, README SSRF callout) are excellent. Findings are grouped by severity — nothing here is blocking; most are nits and hardening suggestions.

🟢 Strengths worth calling out

• Credential-host derivation is correct and important. Routing REST calls to getVaultBaseUrl(clusterId) instead of the loosely-validated client vaultUrl (which only requires the substring <id>.vault to appear anywhere) prevents forwarding the bearer credential to https://abc.vault.attacker.com. The dedicated test locks this in — the highest-value security decision in the PR.
• SSRF guard is layered: scheme allowlist, IP-literal decoding across encodings (decimal/hex/IPv4-mapped IPv6), DNS resolution with per-address checks, per-hop redirect re-validation, and a streaming size cap that does not trust Content-Length. The DNS-rebinding TOCTOU gap is explicitly documented rather than hidden.
• Body limit moved after auth + rate limiting (parseMcpBody per-route) so an unauthenticated client cannot force the server to buffer a 34MB body before credentials are checked — a real improvement over the old global express.json.
• Response de-duplication (toFileToolResult stripping base64 blobs from the text channel) avoids shipping multi-MB payloads twice.
• Test coverage is strong: alternate IP encodings, redirect SSRF, streaming cap without Content-Length, camelCase/snake_case parsing, and fake-timer long-poll tests.

🟡 Minor issues / suggestions

1. get-file-run-status: the initial status fetch bypasses the wait deadline, and a single hung call can exceed the intended waitSeconds. REQUEST_TIMEOUT_MS in detectRest.ts is 65s while MAX_STATUS_WAIT_SECONDS is 55s. With waitSeconds: 0, a stalled first request still blocks up to 65s, and a hung poll inside the loop can overshoot the deadline by up to a full request timeout. Bounded and documented in pitfall 9, but consider passing the remaining budget as the per-call timeout so wall-clock tracks waitSeconds.

2. de-identify-file does not explain a SUCCESS run that returned no artifact. get-file-run-status helpfully adds a "completed but returned no processed file" note; the inline path in deIdentifyFile.ts only attaches a note when status is not SUCCESS AND there is no file. A SUCCESS with no fileBase64 yields a silent, empty-looking success. Mirroring the getFileRunStatus note here would make the two paths consistent.

3. 34MB body limit headroom is tight. 25MB decoded is ~33.3MB base64, leaving ~680KB for the JSON envelope plus other args. A max-size inline file combined with large allowRegexList/restrictRegexList could push past 34MB and surface a generic PayloadTooLargeError instead of the intended clean FileSourceError. Worth a comment or a slightly larger margin.

4. fileNameFromContentDisposition only strips the UTF-8 charset prefix. A filename*=iso-8859-1 value would leave the charset/lang prefix in the decoded name. Low impact (extension inference still works), just noting it.

🔵 Nits

• README/CLAUDE.md describe the SSRF host blocklist as covering .internal/.local; the code also blocks bare localhost and .localhost — minor doc completeness.
• re-identify-file UI builds the download href with unescaped processedFileData. Safe today because it is base64 (charset cannot break out of the double-quoted attribute), but a brief comment would prevent a future "why is this not escaped?" edit.
• In detectRest.ts, pick() returns 0 (correctly non-null) and callers filter falsy counts downstream — consistent, but the two-layer "keep 0 here, drop 0 there" logic is subtle; a one-line note at the pick site could help.

Checklist verification

• inputSchema/outputSchema match handler signatures; shared fileRunOutputSchema reused for both file-run tools.
• UI build wiring updated in all four places (package.json, generate-ui-imports.ts, ui-html.d.ts, registerAppResource).
• CLAUDE.md tool count, Anonymous Mode table, Tool Implementations, and Common Pitfalls all updated.
• Anonymous mode returns setup errors for all three new tools.

Overall: solid, security-conscious work. The findings above are polish, not correctness blockers.

🤖 Generated with Claude Code

[github-actions]

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

[jstjoe]

Thanks — pushed e16826c with the consistency item and clarity nits.

#2 (de-identify-file empty-success) — fixed. Both file tools now share a completedWithoutArtifactNote() helper, so an inline de-identify-file run that completes with no processed file or entities gets the same explanatory note get-file-run-status already produced, instead of a silent empty success. Added a test.

Nits — addressed: comment noting the re-identify-file UI's base64 href is safe unescaped; comment at pick() that a numeric 0 is "present" and the tools omit zero counts downstream.

Deliberately not changing (with rationale):

• Jstjoe/2025 10 09 #1 (per-call timeout budget tracking waitSeconds): the overshoot is bounded (one request timeout) and documented in pitfall 9; threading a per-call remaining-budget through detectRest adds real complexity for a bounded, recoverable edge (the runId persists). Not worth the churn.
• add simple authentication and a sample .env #3 (34MB headroom): ~680 KB of envelope headroom is ample for normal args — allowRegexList/restrictRegexList are short strings, not megabytes — so a realistic max-size file won't exceed it.
• rename tools #4 (Content-Disposition non-UTF-8 charset): extension inference still works, and non-UTF-8 filename* is rare; low impact as you noted.

258 tests pass, build green.

This is my final polish pass on the automated-review feedback — the substantive findings across all seven rounds (credential-host derivation, body-read timeout, SSRF hardening, DNS-lookup bounding) are all in, and the remaining suggestions are documented tradeoffs. To avoid an endless review→commit loop I'll hold further changes here unless a genuine correctness issue or human-reviewer request comes up. The PR is ready for a maintainer's review/merge. I'll keep watching CI and merge-state until it's merged or closed.

---

Generated by Claude Code

[claude]

Code Review — File de-identification & re-identification tools

Overall this is a strong, carefully-built PR. The async job-handle pattern (returning Skyflow's runId as the durable handle) is the right call for a stateless/serverless server, the SSRF hardening is unusually thorough, and the response-channel handling (toFileToolResult keeping base64 out of the text channel) is a nice touch. The commit history shows the security-sensitive paths (credential forwarding, SSRF, body-parse-before-auth) were already tightened over several rounds. Test coverage (76 new tests hitting URL/base64 input, redirect re-validation, alternate IP encodings, streaming size cap, REST error mapping) is genuinely good.

A few observations, all non-blocking:

Security

• SSRF guard is solid but inherently advisory (DNS-rebinding TOCTOU). assertHostAllowed() resolves the host and rejects internal IPs, but fetch() then does its own independent DNS resolution — a hostile resolver can return a public A record to the guard and a private one to fetch. This is correctly documented in fileSource.ts and the README with the "rely on network egress controls" recommendation. Reiterating: for any deployment accepting untrusted fileUrls, egress firewalling to the metadata endpoint (169.254.169.254) is the real control — the app-layer guard is defense-in-depth, not a substitute. Fully pinning would require connecting to the validated IP with a Host/SNI override (e.g. a custom undici dispatcher).
• Deriving the Detect REST base URL from clusterId via getVaultBaseUrl() rather than the loosely-validated client vaultUrl (commit 637e941) is the correct fix — it closes the credential-forwarding vector and guarantees the SDK and REST paths hit the same host. Good catch during review.

Correctness / robustness

• fileNameFromContentDisposition only handles the UTF-8 filename* charset literally; a non-UTF-8 charset would fall through to the plain filename= branch (or capture the raw charset prefix if only filename* is present). Minor and unlikely for signed S3/GCS URLs, but worth a note.
• The empty-vs-still-processing note logic in deIdentifyFile.ts (stillProcessing then completedWithoutArtifact fallthrough) is correct for the cases I traced; it is subtle enough that the inline comments earn their keep — good that they are there.

Performance / memory

• The de-identify path avoids the redundant base64/Buffer/base64 round-trip by carrying ResolvedFile.buffer — nice. The re-identify path cannot (resolved.buffer.toString("base64") for the REST body), so for a ~25 MB inline file the peak per-request footprint is roughly: 34 MB request string + parsed body + 25 MB decoded buffer + ~33 MB re-encoded base64, and detectFetch reads the full response via response.text() (another ~33 MB for a large processed file) with no cap. Several multiples of the file size resident per in-flight request. Inherent to the feature, but combined with the bumped 34 MB body limit it is worth sizing serverless memory / concurrency limits accordingly.
• The bounded synchronous waits (de-identify-file up to 64 s, get-file-run-status up to 55 s) hold the HTTP connection open — correctly documented as needing to fit under the deploy function timeout, with the runId making a cut-off wait recoverable. Good.

Nits

• waitTimeSeconds is clamped in the handler (Math.min(Math.max(...,1),64)) and also constrained by the Zod schema (.min(1).max(64)) — belt-and-suspenders, harmless.
• de-identify-file's outputProcessedFile warning for non-image/audio formats, and the dateShift format warning, are good UX touches — surfacing what Skyflow silently ignores.

Nothing here blocks merge. The residual SSRF TOCTOU is the one item to make sure is understood operationally (egress controls), and it is already documented.

Reviewed with Claude Code