Skip to content

docs: guide for wrapping MCP tools with Skyflow de/re-identify - #27

Merged
jstjoe merged 8 commits into
mainfrom
claude/skyflow-deidentify-integration-docs-gizffz
Jul 20, 2026
Merged

jstjoe merged 8 commits into
mainfrom
claude/skyflow-deidentify-integration-docs-gizffz

Conversation

@jstjoe

@jstjoe jstjoe commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a new developer guide, docs/wrapping-mcp-tools-with-skyflow.md, explaining in simple terms how an outside developer can integrate Skyflow's de-identification and re-identification into their own remote HTTP MCP server.

Today this repo exposes de-identify / re-identify as standalone MCP tools. There was no guidance for the more common ask: folding the same Skyflow round-trip directly into a developer's existing tools — de-identify on the request, re-identify on the response, using reversible vault tokens so PII never leaves their server in the clear.

What's in the guide

  • The pattern — the three-step round-trip (de-identify args → run logic on tokenized text → re-identify results), with a Mermaid flow diagram and the common inverse variant (de-identify a tool's output).
  • Vault-token semanticsVAULT_TOKEN (reversible, persisted) vs ENTITY_UNIQUE_COUNTER (one-way), and the "same authenticated vault" requirement for re-identify.
  • Approach A — skyflow-node SDK (recommended) — client setup, two small deidentify/reidentify helpers, and a generic tool wrapper. Snippets mirror the live reference implementation in src/lib/tools/deIdentify.ts and src/lib/tools/reIdentify.ts.
  • Approach B — Detect REST API — a curl example for non-Node stacks, clearly flagged as illustrative with a pointer to the official Detect API reference.
  • Credentials & config, prerequisites, and gotchas.

Changes

  • New: docs/wrapping-mcp-tools-with-skyflow.md
  • Edit: README.md — link the guide from Learn More
  • Edit: CLAUDE.md — pointer to the guide from the Tool Implementations section

No source-code changes. Uses current tool names (de-identify / re-identify) throughout.

Notes for reviewers

  • The SDK snippets were verified against the in-repo handlers, so they match the exact call shape (skyflow.detect().deidentifyText(...) / reidentifyText(...), TokenFormatVAULT_TOKEN, response fields).
  • Skyflow's public Detect docs return 403 through this environment's proxy, so the REST section leads with the well-corroborated deidentify endpoint and links out to the official API reference for the authoritative/complete spec (including the reidentify body) rather than inventing fields. Please sanity-check the REST example against current docs.
  • Per steering, the guide uses a generic "your MCP server" example (no changes to the brave-search server), documents the SDK and REST approaches (not MCP-to-MCP), and stays intentionally focused; operational guidance (where-to-wrap strategies, error handling, latency/cost) is noted as a future revision.

🤖 Generated with Claude Code

https://claude.ai/code/session_01L1HGToAv1T8agSECYUA39F


Generated by Claude Code

Add docs/wrapping-mcp-tools-with-skyflow.md, a developer guide showing
how outside teams can embed Skyflow's de-identify (on request) and
re-identify (on response) round-trip inside their own remote HTTP MCP
server's tools using reversible vault tokens.

Covers the skyflow-node SDK approach (mirroring src/lib/tools/*) and a
Detect REST API alternative for other stacks, plus vault-token
semantics, prerequisites, credentials, and common gotchas. Links the
guide from README (Learn More) and CLAUDE.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1HGToAv1T8agSECYUA39F
@vercel

vercel Bot commented Jul 15, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
sky-mcp-streamable Ready Ready Preview, Comment Jul 16, 2026 7:44am

Request Review

@github-actions

Copy link
Copy Markdown

Gitleaks Findings: No secrets detected. Safe to proceed!

@claude

claude Bot commented Jul 15, 2026

Copy link
Copy Markdown

Review — docs: guide for wrapping MCP tools with Skyflow de/re-identify

Docs-only PR. I checked the new guide against the actual codebase it claims to mirror. Overall this is accurate, well-structured, and safe to merge — the technical claims hold up. A few notes below, one substantive.

✅ Verified against the code

  • SDK call shapes match the handlers exactly. The deidentify/reidentify helpers mirror src/lib/tools/deIdentify.ts and src/lib/tools/reIdentify.tsskyflow.detect().deidentifyText(new DeidentifyTextRequest(...), options), reidentifyText(new ReidentifyTextRequest(...)), TokenFormat.setDefault(TokenType.VAULT_TOKEN), and the res.entities[] shape { token, value, entity, textIndex, processedIndex, scores } all line up.
  • The clusterId regex is identical to the real one in src/lib/validation/vaultConfig.ts:60 (/(?:https?:\/\/)?([^.]+)\.vault/) — nice that the guide uses the current form rather than the older https://([^.]+).vault snippet still shown in CLAUDE.md.
  • Every referenced file exists: src/lib/validation/vaultConfig.ts, src/lib/mappings/entityMaps.ts, src/lib/middleware/authenticateBearer.ts.
  • All cited entity keys exist in ENTITY_MAP (email_address, ssn, credit_card, name, phone_number, ip_address, location, bank_account), and the VAULT_TOKEN vs ENTITY_UNIQUE_COUNTER / anonymous-mode reversibility framing matches the handlers.
  • Markdown anchor links (#approach-a--skyflow-node-sdk-recommended, #approach-b--detect-rest-api-any-language, #credentials--configuration) resolve correctly under GitHub's slug rules.

🟡 Main substantive point — the flagship search example undersells (or slightly misrepresents) the pattern

In the "Wrap a tool call" example, the query is de-identified before hitting the external API, then the results are re-identified:
```ts
const safeQuery = await deidentify(query);
const results = await callExternalApi(safeQuery);
const restored = await reidentify(JSON.stringify(results));
```
For a real search provider, the response generally won't contain the vault tokens that were in the query — a search engine returns matched documents, not an echo of your tokenized query terms. So `reidentify(results)` is frequently a no-op there, which can leave a reader thinking the round-trip "just works" for any tool. The genuinely clean demonstration is the inverse pattern the guide already mentions (de-identify your own data/output, re-identify for the authorized human) or an LLM/summarize tool where the tokenized input flows through into the output text. Consider making the primary worked example one where the tokens demonstrably survive into the response, and keep search as a caveat. At minimum, a one-line note that re-identify only restores tokens that actually appear in the downstream response would prevent a wrong mental model.

🟡 Minor

  • Approach B (REST) is explicitly speculative — the endpoint path (`/v1/detect/deidentify/string`), the `entity_types`/`token_type` body fields, and the `X-SKYFLOW-ACCOUNT-ID` requirement are unverified (the PR description and an inline `[!WARNING]` both flag this). Good that it's flagged; just calling it out so a reviewer with docs access confirms before readers copy/paste. The `reidentify` REST body is described only in prose, not shown — fine given it's illustrative.
  • `credentials: { apiKey }` vs `{ token }` is described correctly, matching `authenticateBearer.ts`'s JWT-vs-API-key detection.
  • Trailing-whitespace / line-length: not worth blocking, but if the repo wires up a markdown linter in CI it's worth a quick check (didn't see one).

Test coverage

N/A — documentation only, no source changes. Nothing to test.

Verdict: Approve pending consideration of the `search`-example nuance above. Accurate and genuinely useful.

🤖 Generated with Claude Code

Address review feedback: the previous `search` example re-identified the
provider's results, but a search response does not echo the tokenized
query, so re-identify there is usually a no-op and gives a misleading
mental model. Switch the flagship example to a summarize/LLM tool whose
output is derived from the tokenized input (tokens flow through and are
restored), and add a note clarifying that re-identify only restores
tokens that actually appear in the response — keeping search as the
request-side-protection caveat.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1HGToAv1T8agSECYUA39F
@github-actions

Copy link
Copy Markdown

Gitleaks Findings: No secrets detected. Safe to proceed!

@claude

claude Bot commented Jul 15, 2026

Copy link
Copy Markdown

Review — docs PR (de/re-identify wrapping guide)

I verified the guide's technical claims against the current source. This is a well-crafted, accurate doc. Approving in spirit; a few suggestions below, none blocking.

✅ Verified accurate

  • SDK call shapes match src/lib/tools/deIdentify.ts / reIdentify.ts exactly: skyflow.detect().deidentifyText(new DeidentifyTextRequest(text), options) and .reidentifyText(new ReidentifyTextRequest(text)), TokenFormat().setDefault(TokenType.VAULT_TOKEN), and the res.entities[] shape { token, value, entity, textIndex, processedIndex, scores }.
  • Cluster-ID regex /(?:https?:\/\/)?([^.]+)\.vault/ matches extractClusterId in src/lib/validation/vaultConfig.ts:60 verbatim (nice — this is more correct than the simplified https://([^.]+).vault shown in CLAUDE.md).
  • JWT detection description (3 dot-separated base64url parts) matches authenticateBearer.ts; credential formats { token } / { apiKey } are correct.
  • Entity-map keys (email_address, credit_card, bank_account, …) exist in src/lib/mappings/entityMaps.ts; ENTITY_MAP/getEntityEnum references are valid.
  • skyflow-node ^2.0.0 matches package.json. All cross-referenced file paths exist.

Suggestions

  1. Call out that an LLM may not preserve tokens verbatim. The summarize example relies on [EMAIL_ADDRESS_a1b2] flowing untouched through callThirdPartyLlm so reidentify can swap it back. In practice a summarizer/LLM can paraphrase, reformat, truncate, or drop bracketed tokens — which silently breaks re-identification (the token just won't be found). The existing Note covers the search no-op case but not this failure mode. A one-line warning ("re-identify only restores tokens that survive the downstream call byte-for-byte; models that rewrite text may mangle them") would save readers a confusing debugging session.

  2. The inline ?.[1]! in the client-setup snippet swallows the null case the real repo handles. The prerequisites snippet uses .match(...)?.[1] (returns string | undefined), but the client-setup snippet asserts non-null with !. The live code (extractClusterIdvalidateVaultConfig) returns string | null and emits a clear error on malformed URLs. Worth a note that production code should validate rather than assert, so a bad VAULT_URL fails loudly instead of passing undefined as the cluster ID.

  3. REST section (Approach B) — well-flagged, still guesswork. You've honestly disclosed that the endpoint path (/v1/detect/deidentify/string), body fields (vault_id, entity_types, token_type), and the X-SKYFLOW-ACCOUNT-ID header are illustrative and unverified. Good call putting the WARNING callout right there. No change needed, but consider trimming the concrete curl body to only fields you're confident about, or explicitly labeling the body # unverified — confirm against API reference inline, so a reader copy-pasting doesn't miss the caveat above it.

Nits

  • Prereqs table lists Account ID as "REST only"; consistent with CLAUDE.md's note that the SDK never consumes it. 👍

No source-code changes, no test impact. Docs-only, and the parts that touch real APIs are either verified or clearly flagged.

Address second review pass:
- Clarify that re-identify matches tokens verbatim, and that a
  paraphrasing/reformatting LLM may alter or drop bracketed tokens and
  silently break the round-trip (not just the search no-op case).
- Note that production code should validate the vault URL rather than
  assert with `!`, pointing to extractClusterId / validateVaultConfig.
- Add an inline "illustrative — confirm against the API reference"
  marker to the REST curl body so copy-pasters can't miss the caveat.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1HGToAv1T8agSECYUA39F
@github-actions

Copy link
Copy Markdown

Gitleaks Findings: No secrets detected. Safe to proceed!

@claude

claude Bot commented Jul 15, 2026

Copy link
Copy Markdown

PR Review — docs: guide for wrapping MCP tools with Skyflow

Thanks for this — it's a genuinely useful, well-written guide that fills a real gap (folding the Skyflow round-trip into a developer's own tools rather than calling the standalone tools). This is docs-only with no source changes, so the review focused on technical accuracy against the code it references. I verified the claims against the repo, and they hold up well.

✅ Verified accurate

  • SDK snippets match the live handlers. The deidentify/reidentify helpers mirror src/lib/tools/deIdentify.ts and src/lib/tools/reIdentify.ts exactly — skyflow.detect().deidentifyText(new DeidentifyTextRequest(...), options), TokenFormat.setDefault(TokenType.VAULT_TOKEN), and res.processedText. The entities[] field shape ({ token, value, entity, textIndex, processedIndex, scores }) matches the handler's mapping.
  • Cluster-ID regex is identical to extractClusterId in src/lib/validation/vaultConfig.ts (/(?:https?:\/\/)?([^.]+)\.vault/), and the referenced functions (extractClusterId, validateVaultConfig) exist.
  • Referenced paths all exist: src/lib/validation/vaultConfig.ts, src/lib/mappings/entityMaps.ts (ENTITY_MAP), src/lib/middleware/authenticateBearer.ts.
  • Entity string examples are real ENTITY_MAP keys (email_address, ssn, credit_card, name, phone_number, ip_address, location, bank_account).
  • Token-type semantics are correctVAULT_TOKEN (reversible/persisted) vs ENTITY_UNIQUE_COUNTER (one-way), consistent with the anonymous-mode behavior in CLAUDE.md.
  • Account-ID nuance is right — the guide correctly states the SDK does not use it (matches CLAUDE.md's note that ACCOUNT_ID was removed as never consumed by the SDK) and scopes it to the REST approach only.
  • Internal anchor links resolve (#approach-a--…, #approach-b--…, #credentials--configuration).
  • README + CLAUDE.md cross-links are well placed and correctly targeted.

Quality notes

  • The "tokens must round-trip verbatim" caveat is the most valuable part of the guide — flagging that a paraphrasing LLM can drop/mangle a [TOKEN] so it silently won't re-identify is exactly the footgun a naive integrator would hit. Nicely done.
  • Honest hedging on the REST section. Since that section is explicitly illustrative and unverified, worth calling out the specific unverified pieces so a reader knows what to double-check: the endpoint path /v1/detect/deidentify/string, the entity_types: ["all"] and token_type: { "default": "vault_token" } body fields, and whether X-SKYFLOW-ACCOUNT-ID is actually required. The warning box covers this, but naming them would sharpen it.

Minor / optional suggestions

  1. ?.[1]! double-assertion in Approach A step 1. The guide already calls this out and points to the validation helpers — good. Since the repo exports extractClusterId, consider having the snippet import and use it directly (or show the if (!clusterId) throw guard inline) so the copy-paste path is the safe one rather than the one the surrounding prose warns against.
  2. Statelessness gotcha vs. VAULT_TOKEN persistence. "Keep the round-trip stateless per call" is good advice, but a reader might wonder how re-identify works across separate tool invocations. A half-sentence clarifying that the token↔value mapping lives in the vault (so re-identify works in a later call), while you shouldn't hold tokens in local server memory, would remove the apparent tension.
  3. Terminology: the intro and body consistently use the current tool names (de-identify / re-identify) — confirmed, no stale names.

Test coverage

N/A — docs-only, no runtime surface. No tests needed.

Overall: Accurate, appropriately hedged where it can't verify (REST), and mirrors the real code. Recommend merge after (optionally) tightening the REST caveats and the cluster-ID snippet. 👍

🤖 Generated with Claude Code

Address third review pass:
- Replace the `?.[1]!` cluster-ID assertion with an inline guard that
  throws on a missing/malformed vault URL, so the copy-paste path is the
  safe one rather than the pattern the surrounding prose warns against.
- Resolve the apparent tension in the "stateless per call" gotcha: note
  the token<->value mapping lives in the vault, so re-identify still
  resolves in a later call or separate process.
- Name the specific unverified REST pieces (endpoint path, entity_types
  / token_type body fields, X-SKYFLOW-ACCOUNT-ID) in the warning box.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1HGToAv1T8agSECYUA39F
@github-actions

Copy link
Copy Markdown

Gitleaks Findings: No secrets detected. Safe to proceed!

@claude

claude Bot commented Jul 15, 2026

Copy link
Copy Markdown

Review — docs: guide for wrapping MCP tools with Skyflow de/re-identify

Nice addition. This is a docs-only PR (docs/wrapping-mcp-tools-with-skyflow.md + two link edits), and I verified the technical claims against the live source rather than just skimming. Overall it's accurate, well-structured, and refreshingly honest about what it hasn't verified. Recommending approval with a few minor suggestions.

Accuracy — verified against source ✅

  • SDK call shapes (deidentify/reidentify helpers) match src/lib/tools/deIdentify.ts and reIdentify.ts exactly — skyflow.detect().deidentifyText(new DeidentifyTextRequest(...), options), TokenFormat().setDefault(TokenType.VAULT_TOKEN), reidentifyText(new ReidentifyTextRequest(...)), and the res.processedText / res.entities[] fields.
  • Cluster-ID regex matches extractClusterId in src/lib/validation/vaultConfig.ts:60 character-for-character. Good call replacing the ?.[1]! assertion with a throwing guard — that matches the repo's own validateVaultConfig behavior and the safe-copy-paste intent.
  • Entity strings (email_address, ssn, name, credit_card, phone_number, ip_address, location, bank_account) all exist as keys in ENTITY_MAP (src/lib/mappings/entityMaps.ts).
  • JWT vs API-key description matches authenticateBearer.ts (3 dot-separated base64url parts -> { token }, else { apiKey }).
  • Account ID / X-SKYFLOW-ACCOUNT-ID is correctly scoped to REST only, consistent with CLAUDE.md noting the SDK never consumed it.

The verbatim-token / paraphrasing-LLM caveat and the same-authenticated-vault requirement for re-identify are both correct and are exactly the traps a first-time integrator would hit — good that they're called out prominently.

Minor suggestions (non-blocking)

  1. Inconsistent non-null assertions. The guide adds a throwing guard for clusterId and warns against silently passing undefined — but then uses process.env.SKYFLOW_VAULT_ID! and process.env.SKYFLOW_API_KEY! a few lines later. Since the point of the third review pass was making the copy-paste path the safe one, consider validating those env vars too (or a one-line note that the same fail-loudly treatment applies).
  2. Prerequisites table example values. Vault ID ebfc9bee4242abcd has the Vault URL's cluster ID ebfc9bee4242 as a literal prefix, which could suggest the cluster ID is a substring of the vault ID rather than derived from the URL. Visibly unrelated placeholder values would remove the ambiguity.
  3. REST section is appropriately flagged as illustrative/unverified (endpoint path, entity_types/token_type body, X-SKYFLOW-ACCOUNT-ID). No objection given the honesty — but confirming the reidentify body shape against current docs would raise the section's value.

Test coverage

N/A — documentation only, no source changes. The doc's snippets mirror code already unit-tested in tests/unit/tools/.

Great work keeping the snippets tied to the real handlers and being explicit about the unverified REST pieces rather than inventing fields. 👍

Address fourth review pass:
- Change the example Vault ID so it no longer shares the cluster ID as a
  literal prefix, which could imply the cluster ID is a substring of the
  vault ID rather than derived from the vault URL.
- Note that the other required env vars deserve the same fail-loudly
  validation as the cluster ID guard.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1HGToAv1T8agSECYUA39F
@github-actions

Copy link
Copy Markdown

Gitleaks Findings: No secrets detected. Safe to proceed!

@claude

claude Bot commented Jul 15, 2026

Copy link
Copy Markdown

Review — docs: guide for wrapping MCP tools with Skyflow de/re-identify

Thanks for this — it's a genuinely useful, well-structured guide. I verified the technical claims against the repo source rather than just reading for prose, and the accuracy is high. Summary: approve, with a few minor suggestions.

What I verified ✅

  • SDK call shapes match the live handlers. skyflow.detect().deidentifyText(new DeidentifyTextRequest(text), options), TokenFormat + setDefault(TokenType.VAULT_TOKEN), options.setTokenFormat(...), options.setEntities(...), and reidentifyText(new ReidentifyTextRequest(text)) all match src/lib/tools/deIdentify.ts and src/lib/tools/reIdentify.ts exactly.
  • Response fields are correct. res.processedText and the entities[] entry shape { token, value, entity, textIndex, processedIndex, scores } match deIdentify.ts precisely.
  • Cross-references resolve. src/lib/validation/vaultConfig.ts exports extractClusterId / validateVaultConfig; src/lib/mappings/entityMaps.ts exports ENTITY_MAP; src/lib/middleware/authenticateBearer.ts implements the "3 dot-separated base64url parts → JWT, else API key" logic the guide describes.
  • The cluster-ID regex is identical to extractClusterId's (/(?:https?:\/\/)?([^.]+)\.vault/), and the anonymous-mode / ENTITY_UNIQUE_COUNTER non-reversibility claim matches reIdentify.ts's anonymousModeRestricted behavior.
  • Anchor links are valid (#approach-a--skyflow-node-sdk-recommended, #approach-b--detect-rest-api-any-language, #credentials--configuration all resolve under GitHub's slug rules), and the README/CLAUDE.md links point at the new file.
  • Every entity string listed as a "common value" (email_address, ssn, credit_card, name, phone_number, ip_address, location, bank_account) exists as a key in ENTITY_MAP.

The iterative self-review across the 5 commits paid off — the summarize-vs-search reframing (tokens must survive verbatim through a paraphrasing LLM) and the fail-loudly cluster-ID guard are exactly the right things to call out, and the guard now makes the copy-paste path the safe one.

Suggestions (all minor / non-blocking)

  1. REST section (Approach B) is the main residual risk. You've flagged it thoroughly as illustrative/unverified, which is the right call given the docs proxy returned 403. But since a reader can still copy-paste it, consider either (a) trimming it to just the endpoint + a link, or (b) having a maintainer with dashboard access confirm the /v1/detect/deidentify/string path, the token_type/entity_types body fields, and whether X-SKYFLOW-ACCOUNT-ID is actually required before merge. Note: CLAUDE.md records that ACCOUNT_ID is "never consumed by the Skyflow SDK," so the guide's claim that it's needed only for REST is consistent — but the REST-side requirement itself is the unverified part.

  2. entities[] breakdown note. The guide says the SDK returns res.entities[] with the field shape shown — true, but the repo's handler re-maps those fields into its own output object rather than passing the SDK object straight through. A one-line "(the repo maps these into its tool output)" would prevent a reader assuming the raw SDK shape is guaranteed stable.

  3. Latency/cost of the double round-trip is deferred to a future revision, which is reasonable — but since it's the first practical question anyone wrapping a hot-path tool will hit, even one sentence ("each wrapped call adds two Detect API round-trips") in the Gotchas would set expectations.

  4. Purely cosmetic: the guide references skyflow-node@^2.0.0 matching the repo — good — but node_modules isn't installed in this checkout, so I couldn't statically confirm the SDK's type exports (DeidentifyTextOptions, setEntities, etc.) against the published .d.ts. They match the repo's usage, which imports them the same way, so this is almost certainly fine.

Test coverage

Docs-only change with no runtime surface — no tests needed or expected. ✅

Nice work. The one thing I'd want a human to eyeball before merge is the REST example (#1).

🤖 Generated with Claude Code

Using Skyflow's own Detect OpenAPI spec (from the call-rest-apis skill) as
the source of truth, replace the previously-hedged REST section with a
verified one:
- Remove the bogus X-SKYFLOW-ACCOUNT-ID header (and the Account ID
  prerequisite / env var) — the Detect API authenticates with the Bearer
  token alone.
- Keep token_type as a TokenTypeMapping object ({ "default": "vault_token" }),
  now confirmed against the spec.
- Add a verified re-identify example (POST /v1/detect/reidentify/string
  takes { text, vault_id } and returns { text }) instead of prose.
- Show real response shapes (processed_text / entities / word_count /
  character_count) and note the v1 vs v2 (beta) split and where the REST
  bearer token comes from.
- Drop the "illustrative / unverified" warnings now that the shapes are
  confirmed.

Note: the entity names for REST entity_types are the lowercase ENTITY_MAP
keys; token_type.default accepts vault_token / entity_unq_counter /
entity_only.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1HGToAv1T8agSECYUA39F
@github-actions

Copy link
Copy Markdown

Gitleaks Findings: No secrets detected. Safe to proceed!

@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review — docs: guide for wrapping MCP tools with Skyflow

Docs-only PR (new guide + two link pointers, no source changes). I verified the technical claims against the current source. Overall this is a high-quality, accurate guide — nicely scoped and honest about what's verified vs. illustrative.

✅ Verified accurate against the codebase

  • SDK call shapes match the live handlers exactly: skyflow.detect().deidentifyText(new DeidentifyTextRequest(text), options) and .reidentifyText(new ReidentifyTextRequest(text)) (src/lib/tools/deIdentify.ts, reIdentify.ts).
  • Response fieldsres.processedText, res.wordCount, res.charCount, and res.entities[] with { token, value, entity, textIndex, processedIndex, scores } — all match the handler mappings.
  • Cluster-ID regex /(?:https?:\/\/)?([^.]+)\.vault/ is copied verbatim from extractClusterId in src/lib/validation/vaultConfig.ts. 👍
  • Token-type semantics (VAULT_TOKEN reversible/persisted vs ENTITY_UNIQUE_COUNTER one-way) match handleDeIdentify, and the "anonymous mode can't re-identify" warning matches handleReIdentify.
  • Credential detection (JWT = 3 base64url parts → { token }, else { apiKey }) matches looksLikeJwt in authenticateBearer.ts.
  • ENTITY_MAP location and sample entity names all exist in src/lib/mappings/entityMaps.ts; ^2.0.0 SDK version matches package.json. Internal anchor links resolve correctly.
  • The verbatim-token-matching caveat (a paraphrasing LLM can drop/alter a bracketed token, and search results are often a re-identify no-op) is an excellent, frequently-missed point.

🔎 Suggestions

  1. REST re-identify response field is unverified but stated as fact. The line "Note the re-identify response field is text, while de-identify returns processed_text" reads as definitive, but the PR notes the live Detect docs 403 through this environment. A reader will copy that and read response.text; if it's wrong the round-trip silently breaks. Consider tagging the REST response bodies as illustrative (request examples are lower-risk), consistent with how you already hedge the section intro.

  2. The deidentify/reidentify helpers swallow errors. Unlike the repo handlers (which catch SkyflowError and surface http_code/details), the snippet helpers have no error handling, so a copy-paste user gets unhandled rejections on a bad token/credential. Deferring full error handling to "a later revision" is fine, but a one-line note at the helpers (not only in the closing section) would set expectations where the code actually gets copied.

  3. Minor: the summarize example calls an undefined callThirdPartyLlm(...). Clearly a placeholder, but a // your existing call comment would remove any ambiguity.

Not applicable

No perf/security-code surface and no tests to add (docs only). The prose correctly steers users toward reversible vault tokens and authenticated credentials, which is the safe default.

Nice work — the accuracy of the SDK snippets against the live handlers is exactly right, and points 1–2 are the only things I'd tighten before merge.

Flag at the point of copy — not only in the closing section — that the
deidentify/reidentify helper snippets skip error handling, and point to
how the repo handlers catch SkyflowError and surface http_code/details.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1HGToAv1T8agSECYUA39F
@github-actions

Copy link
Copy Markdown

Gitleaks Findings: No secrets detected. Safe to proceed!

@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review — docs: guide for wrapping MCP tools with Skyflow

Nice, well-crafted developer guide. I verified the technical claims against the current codebase and they hold up well. Docs-only PR, no source changes.

Verified accurate against the repo

  • File references all exist and match: src/lib/tools/deIdentify.ts, reIdentify.ts, src/lib/validation/vaultConfig.ts, src/lib/mappings/entityMaps.ts, src/lib/middleware/authenticateBearer.ts, src/server.ts.
  • SDK call shapes are exact. skyflow.detect().deidentifyText(new DeidentifyTextRequest(text), options) and .reidentifyText(new ReidentifyTextRequest(text)), TokenFormat().setDefault(TokenType.VAULT_TOKEN), and res.processedText all match deIdentify.ts/reIdentify.ts verbatim.
  • Entity metadata shape ({ token, value, entity, textIndex, processedIndex, scores }) matches the mapping in deIdentify.ts:48-55.
  • Cluster-ID regex /(?:https?:\/\/)?([^.]+)\.vault/ is copied faithfully from extractClusterId (vaultConfig.ts:60).
  • JWT-vs-API-key description matches authenticateBearer.ts (3 dot-separated base64url parts -> token, else apiKey).
  • DetectEntities.EMAIL_ADDRESS / SSN / NAME and the lowercase string list all exist in ENTITY_MAP.
  • Token-type semantics (VAULT_TOKEN reversible/persisted vs ENTITY_UNIQUE_COUNTER one-way, anonymous-mode restriction) match the handlers and CLAUDE.md.
  • All internal anchor links resolve to their headings; README and CLAUDE.md links point at the real file path.

Suggestions

  1. REST section (Approach B) — flag the unverified fields more visibly. You already note in the PR description that Skyflow docs 403 through this environment and that only the SDK section was verified. The invented-looking REST JSON fields are the one place a reader could be misled. In particular the claim that the re-identify response field is text while de-identify returns processed_text is a specific asymmetry worth double-checking against live docs — the SDK surfaces both as processedText, so a REST layer returning text for one and processed_text for the other would be surprising. Consider an inline caveat right on the REST example (e.g. "field names below are illustrative — confirm against the Detect API reference") rather than only in the PR description, since PR descriptions do not travel with the doc. Same applies to entity_type / entity_scores / location / word_count / character_count.

  2. Minor consistency nit: the SDK example shows res.entities[].scores while the REST example shows entity_scores/location/entity_type. Thats a legitimate SDK-vs-wire difference, but a one-line note that the SDK renames/camelCases the raw REST fields would preempt confusion.

  3. Nice touch that the generic example uses the official SDK registerTool (not this repos registerAppTool) — correct for an outside developers own server and avoids implying they need the ext-apps layer.

Test coverage: N/A — documentation only, no runtime surface to test.

Overall this is accurate, clearly written, and honest about its own uncertainty. The only real risk is the hand-authored REST field names; tightening the inline caveat there would make it merge-ready.

The v1 wire format is asymmetric (de-identify returns processed_text,
re-identify returns text) and uses entity_type/entity_scores/location;
note that the skyflow-node SDK surfaces both responses as processedText
and exposes entity/scores/textIndex/processedIndex, so readers moving
between Approach A and B aren't surprised.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1HGToAv1T8agSECYUA39F
@github-actions

Copy link
Copy Markdown

Gitleaks Findings: No secrets detected. Safe to proceed!

@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review — PR #27: docs guide for wrapping MCP tools with Skyflow

Reviewed as a documentation-only change (3 files, +339/-0: new docs/wrapping-mcp-tools-with-skyflow.md, plus link edits in README.md and CLAUDE.md). No source code changes, so the review focused on technical accuracy against the live implementation rather than runtime behavior.

Verified accurate ✅

I cross-checked every code claim in the guide against the repo:

  • SDK call shapes match src/lib/tools/deIdentify.ts / reIdentify.ts exactly — skyflow.detect().deidentifyText(new DeidentifyTextRequest(...), options), TokenFormat().setDefault(TokenType.VAULT_TOKEN), reidentifyText(new ReidentifyTextRequest(...)), and res.processedText.
  • Entity struct { token, value, entity, textIndex, processedIndex, scores } matches the mapping in deIdentify.ts:48-55.
  • Cluster-ID regex /(?:https?://)?([^.]+).vault/ is identical to extractClusterId in src/lib/validation/vaultConfig.ts:60, and the fail-loud guard mirrors validateVaultConfig.
  • JWT vs API-key distinction matches authenticateBearer.ts (3 dot-separated base64url parts to { token }, else { apiKey }).
  • Entity names — ENTITY_MAP keys are indeed lowercase (email_address, ssn, name, …) and all is a real key (DetectEntities.ALL), so the REST entity_types note is correct.
  • Token semantics — VAULT_TOKEN (reversible) vs ENTITY_UNIQUE_COUNTER (anonymous mode, one-way) matches the handler logic and CLAUDE.md.

The commit history shows unusually careful iteration: the re-identify verbatim-match caveat, the cluster-ID guard, and the REST-section correction (dropping the bogus X-SKYFLOW-ACCOUNT-ID header after verifying against the Detect OpenAPI spec) all address real correctness traps. Nicely done.

Minor suggestions (non-blocking)

  1. Illustrative snippet completeness — the registerTool("summarize", …) example uses z.string() and callThirdPartyLlm(...) without an import of z or a definition. Clearly pseudocode, but a one-line comment (z from zod; callThirdPartyLlm is your existing logic) would stop a copy-paster from hitting a confusing error.

  2. REST response field verification — word_count / character_count / location.{start,end} in the REST examples are the one area I could not independently verify from this repo (the SDK normalizes them to wordCount/charCount/textIndex). The PR notes these were checked against the Detect OpenAPI spec — worth one more human sanity-check before publishing, since the guide now presents them without the earlier illustrative hedge.

  3. Re-identify round-trip framing — the caveats about paraphrasing LLMs dropping bracketed tokens are excellent and honest. Consider making the request-side-protection-is-the-guaranteed-win line more prominent (e.g. a callout), since it is the most important takeaway for someone reaching for re-identify on free-form LLM output.

Test coverage

N/A — docs only. No handler/schema changes, so the CLAUDE.md Modifying Tools checklist does not apply. The CLAUDE.md pointer was added appropriately.

Overall: High-quality, accurate documentation that fills a real gap. Approve after the reviewer confirms the REST wire-format field names. 🤖

@jstjoe
jstjoe merged commit f7a863f into main Jul 20, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants