Add @tiny-fish/mcp: local MCP proxy to agent.tinyfish.ai/mcp - #2
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughImplemented Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant HTTPServer
participant MCPAdapter
participant ProxyCore
participant Upstream
MCPClient->>HTTPServer: POST /mcp
HTTPServer->>MCPAdapter: validate and parse request
MCPAdapter->>ProxyCore: forward JSON-RPC message
ProxyCore->>Upstream: authenticated request with session headers
Upstream-->>ProxyCore: JSON or SSE response
ProxyCore-->>MCPAdapter: response and stream events
MCPAdapter-->>MCPClient: JSON response or relayed SSE
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (13)
.github/workflows/release.yml (1)
23-28: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRemove the npm cache from the publishing job.
cache: npmrestores a cache whose key can be written by less privileged workflow runs. This job produces the published tarball and signs it with provenance, so a poisoned cache would be attested as trusted. Dependency install time is not critical for a tag release. Drop the cache here and keep it inci.yml.🔒 Proposed change
- name: Setup Node.js uses: actions/setup-node@v4 with: node-version: 22 - cache: npm registry-url: https://registry.npmjs.org🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 23 - 28, Remove the cache: npm setting from the Setup Node.js step in the publishing workflow, while retaining the node-version and registry-url configuration. Leave npm caching enabled in ci.yml.Source: Linters/SAST tools
.github/workflows/ci.yml (1)
8-20: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRestrict the
GITHUB_TOKENpermissions and stop credential persistence.Neither job declares a
permissions:block, so both jobs receive the default token scope. Both jobs only read the repository, build, and test. Add a workflow-levelpermissions: contents: readblock. Also setpersist-credentials: falseon eachactions/checkoutstep, because the checked-out.git/configotherwise keeps the token for the rest of the job.🔒 Proposed hardening
on: push: branches: [main] pull_request: +permissions: + contents: read + jobs: ci: runs-on: ubuntu-latest steps: - name: Checkout code uses: actions/checkout@v4 + with: + persist-credentials: falseApply the same
persist-credentials: falseaddition to the checkout step at Lines 54-56.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 8 - 20, Add a workflow-level permissions block granting only contents: read, and update every actions/checkout step in the workflow—including the checkout near Lines 54-56—with persist-credentials set to false. Keep the existing job steps and checkout behavior otherwise unchanged.Source: Linters/SAST tools
tests/errors.test.ts (1)
455-464: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClose the
ProxyCoreafter the test.The test creates a
ProxyCoreand closes the mock upstream, but never callscore.closeAll(). The proxy keeps its session state and abort controllers for the aborted stream. Add the cleanup so this suite does not leave live handles for later files in the same worker.♻️ Proposed cleanup
} finally { vi.restoreAllMocks(); + core.closeAll(); await mock.close(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/errors.test.ts` around lines 455 - 464, Update the test using the `core` instance in the `logs the LocalWriteError and destroys the socket` case to call `core.closeAll()` during cleanup, alongside the existing mock upstream and spy cleanup, ensuring it runs even when the test assertion or handler fails.tests/origin.test.ts (1)
19-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add two more denied/allowed cases.
Two behaviors that
checkOriginrelies on are untested: uppercase host normalization and Node's comma-joined duplicateOriginheader.💚 Proposed additions
const denied: string[] = [ "https://evil.example.com", + // Node joins duplicate Origin headers with ", " — must not parse as loopback + "http://127.0.0.1:3711, http://evil.example.com", "http://evil.example.com:3711","https://127.0.0.1:3711", "https://localhost:3711", + "http://LOCALHOST:3711", // URL parsing lowercases the hostname ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/origin.test.ts` around lines 19 - 33, Add test coverage in the origin-checking cases around checkOrigin for uppercase host normalization and duplicate Origin headers represented as Node’s comma-joined value, adding the expected denied or allowed outcomes consistent with the existing origin policy.tests/upstream.test.ts (1)
30-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNormalize recorded headers through
Headers.
recordingFetchcastsinit.headerstoRecord<string, string>and reads it withObject.entries.UpstreamClientpasses a plain object today, so the assertions work. If the client later passes aHeadersinstance or an array of pairs,Object.entriesreturns no entries and every header assertion in this suite passes without checking anything.♻️ Proposed normalization
- headers: Object.fromEntries( - Object.entries((init?.headers ?? {}) as Record<string, string>).map(([k, v]) => [ - k.toLowerCase(), - v, - ]) - ), + headers: Object.fromEntries(new Headers(init?.headers ?? {}).entries()),
Headerslowercases names already, so the downstream lookups stay unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/upstream.test.ts` around lines 30 - 44, Update the fetch mock’s header recording in recordingFetch to normalize init.headers through the Headers API before building call.headers. Preserve lowercase header names and ensure plain objects, Headers instances, and arrays of pairs are all captured for downstream assertions.src/core/sse.ts (1)
96-108: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueOptional: bound the line buffer.
buffergrows without a limit until a newline arrives. An upstream that streams bytes without any newline makes the proxy accumulate the whole stream in memory. The current upstream is trusted, so this is defensive only. A cap that throwsUpstreamProtocolErrorpast a maximum line length would keep memory bounded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/sse.ts` around lines 96 - 108, Bound the accumulated line data in the SSE parsing loop around buffer and processLine: enforce a maximum line length while no newline is received, and throw UpstreamProtocolError when the limit is exceeded. Keep normal newline processing and CRLF handling unchanged, using the existing error type and configuration conventions if available.src/http/origin.ts (1)
1-25: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional: add a
Hostheader allowlist as a second rebinding layer.The
Origincheck blocks browser-driven DNS rebinding, because a rebound page sends its own origin. It does not cover a request that carries noOriginand a reboundHostsuch asattacker.example.com:3711. Such a request is allowed today. AHostallowlist limited to127.0.0.1andlocalhoston the bound port closes that path. The caller insrc/http/index.ts(lines 34-58) is the natural place to apply both checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http/origin.ts` around lines 1 - 25, Add a Host-header allowlist alongside checkOrigin and apply both validations in the request handling flow in the caller around the existing Origin check. Accept only localhost or 127.0.0.1 with the configured bound port, reject rebound or malformed Host values, and preserve the current Origin behavior for requests without an Origin header.src/core/errors.ts (1)
222-232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: exclude
LocalWriteErrorfrom the upstream condition text.
LocalWriteErrorextendsProxyCoreError, so it matches this branch and produces "Upstream stream ended unexpectedly". Todaysrc/http/adapter.ts(lines 162-268) returns early forLocalWriteErrorbefore callingtoStreamErrorFrame, so the mislabel is unreachable. An explicit guard keeps the label correct if the adapter ordering changes.♻️ Proposed defensive branch
const condition = failure instanceof UpstreamAbortedError ? "The proxy aborted the upstream request mid-stream (local session closed or shutting down); " - : "Upstream stream ended unexpectedly before the final response; "; + : failure instanceof LocalWriteError + ? "Delivering the relayed stream to the local client failed; " + : "Upstream stream ended unexpectedly before the final response; ";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/errors.ts` around lines 222 - 232, The condition text in the ProxyCoreError handling branch should exclude LocalWriteError from the upstream-stream-ended label. Update the condition near runHint and condition so LocalWriteError receives an appropriate local-write classification, while preserving the existing UpstreamAbortedError and other upstream failure messages.src/core/upstream.ts (1)
101-111: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a pre-response upstream timeout.
Do not pass a live
AbortSignal.timeout(...)to the returned SSE stream. Its later abort also terminates response-body consumption. Use a cancellable timer and clear it whenfetchresolves. Keepoptions.signalactive for the stream.
AbortSignal.timeout(...)producesTimeoutError;toTransportErrorcurrently maps it toUpstreamUnreachableError, notUpstreamAbortedError. Preserve that distinction from client cancellation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/upstream.ts` around lines 101 - 111, Update the fetch flow around the upstream request in the relevant method to use a cancellable pre-response timer rather than passing AbortSignal.timeout directly to the returned SSE stream. Combine the timer’s abort signal with options.signal for fetch, clear the timer immediately after fetch resolves, and keep options.signal active for response-body consumption. Preserve TimeoutError as the timeout-specific transport error while continuing to map client cancellation distinctly through toTransportError.src/http/adapter.ts (1)
323-330: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a maximum request body size to
readBody.
readBodybuffers the whole request body with no limit. Any local process that can reach127.0.0.1:3711/mcpcan stream an unbounded body and exhaust proxy memory. The Origin allowlist does not stop this, becausecheckOriginaccepts a missingOriginheader (src/http/origin.ts:16).Cap the buffered size and answer HTTP 413 when the cap is exceeded.
🛡️ Proposed limit
+const MAX_BODY_BYTES = 4 * 1024 * 1024; + function readBody(req: IncomingMessage): Promise<Buffer> { return new Promise((resolve, reject) => { const chunks: Buffer[] = []; - req.on("data", (chunk: Buffer) => chunks.push(chunk)); + let size = 0; + req.on("data", (chunk: Buffer) => { + size += chunk.length; + if (size > MAX_BODY_BYTES) { + reject(new BodyTooLargeError(MAX_BODY_BYTES)); + req.destroy(); + return; + } + chunks.push(chunk); + }); req.on("end", () => resolve(Buffer.concat(chunks))); req.on("error", reject); }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http/adapter.ts` around lines 323 - 330, Update readBody to enforce a maximum accumulated request-body size while buffering chunks, immediately stop processing and reject when the limit is exceeded, and ensure the HTTP request handler converts that condition into a 413 response. Preserve normal resolution for bodies within the limit and avoid continuing to accumulate data after rejection.tests/helpers/mock-upstream.ts (1)
371-375: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the fallback error write with
res.headersSent.
streamAutomationwrites the SSE headers before it streams frames. If anything throws after that point, this.catchcallsjsonRpcError, which callsres.writeHeada second time. Node then throwsERR_HTTP_HEADERS_SENTinside the.catchcallback, which surfaces as an unhandled rejection and can fail an unrelated test.🛡️ Proposed guard
const server = createServer((req, res) => { void handle(req, res).catch((err: unknown) => { + if (res.headersSent) { + res.destroy(); + return; + } jsonRpcError(res, ErrorCodes.InternalError, `Internal server error: ${String(err)}`, -1); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/helpers/mock-upstream.ts` around lines 371 - 375, Update the catch handler around handle in the createServer callback to call jsonRpcError only when res.headersSent is false; otherwise avoid writing a second response after SSE headers have been sent.src/http/index.ts (1)
36-41: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider validating the
Hostheader in addition toOrigin.
checkOriginreturnstruewhen theOriginheader is absent (src/http/origin.ts:16). The loopback bind plus theOriginallowlist blocks browser-driven DNS rebinding, because browsers always sendOriginon cross-origin requests. AHostallowlist (127.0.0.1:<port>,localhost:<port>) closes the remaining rebinding surface for clients that omitOrigin.This is a defense-in-depth suggestion, not an exploitable defect in the current design.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http/index.ts` around lines 36 - 41, Validate the request Host header alongside checkOrigin in the HTTP request handling flow before deriving pathname. Allow only the configured loopback hosts (127.0.0.1 and localhost with the server port), reject other or missing Host values with the existing forbidden response, and preserve the current Origin validation behavior.tests/session.test.ts (1)
78-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHarden
hangingResponderagainst an already-aborted signal.
hangingResponderonly rejects from theabortlistener. If the signal is already aborted when the responder runs, the returned promise never settles and the test hangs instead of failing. Add an immediate check.♻️ Proposed hardening
function hangingResponder(): (call: RecordedCall) => Promise<Response> { return (call) => new Promise<Response>((_resolve, reject) => { + const fail = (): void => + reject(new DOMException("This operation was aborted", "AbortError")); + if (call.signal?.aborted === true) { + fail(); + return; + } - call.signal?.addEventListener("abort", () => - reject(new DOMException("This operation was aborted", "AbortError")) - ); + call.signal?.addEventListener("abort", fail, { once: true }); }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/session.test.ts` around lines 78 - 85, Update hangingResponder to check call.signal?.aborted immediately when creating the Promise and reject with the existing AbortError behavior if already aborted, while retaining the abort event listener for signals that abort later.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 9-20: Gate the release and usage documentation on PF-3214: in
CHANGELOG.md lines 9-20, keep version 0.1.0 under Unreleased or otherwise mark
the npm release as gated; in README.md lines 24-62, clearly label installation
and API-key usage as blocked or pre-release until the required upstream
authentication and surface-filtering dependency is available.
In `@README.md`:
- Around line 224-228: Before publishing 0.1.0, replace the TBD Owner and
Support policy entries in the README with the designated maintainer and public
support details. Remove the internal release-runbook reference and document the
publicly available issue-triage and release-support process instead.
- Around line 20-22: Add the text language identifier to the opening fenced code
blocks containing the agent.tinyfish.ai MCP URL and the corresponding block at
the second reported location, preserving their existing contents.
In `@src/config.ts`:
- Around line 41-59: Update the URL validation around the raw value and
isLoopback checks to reject URLs containing username or password credentials,
and replace both validation messages with generic errors that never include raw,
query, or credential data. Add a regression test covering a URL with embedded
secrets and assert the emitted configuration error excludes those secrets.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 8-20: Add a workflow-level permissions block granting only
contents: read, and update every actions/checkout step in the workflow—including
the checkout near Lines 54-56—with persist-credentials set to false. Keep the
existing job steps and checkout behavior otherwise unchanged.
In @.github/workflows/release.yml:
- Around line 23-28: Remove the cache: npm setting from the Setup Node.js step
in the publishing workflow, while retaining the node-version and registry-url
configuration. Leave npm caching enabled in ci.yml.
In `@src/core/errors.ts`:
- Around line 222-232: The condition text in the ProxyCoreError handling branch
should exclude LocalWriteError from the upstream-stream-ended label. Update the
condition near runHint and condition so LocalWriteError receives an appropriate
local-write classification, while preserving the existing UpstreamAbortedError
and other upstream failure messages.
In `@src/core/sse.ts`:
- Around line 96-108: Bound the accumulated line data in the SSE parsing loop
around buffer and processLine: enforce a maximum line length while no newline is
received, and throw UpstreamProtocolError when the limit is exceeded. Keep
normal newline processing and CRLF handling unchanged, using the existing error
type and configuration conventions if available.
In `@src/core/upstream.ts`:
- Around line 101-111: Update the fetch flow around the upstream request in the
relevant method to use a cancellable pre-response timer rather than passing
AbortSignal.timeout directly to the returned SSE stream. Combine the timer’s
abort signal with options.signal for fetch, clear the timer immediately after
fetch resolves, and keep options.signal active for response-body consumption.
Preserve TimeoutError as the timeout-specific transport error while continuing
to map client cancellation distinctly through toTransportError.
In `@src/http/adapter.ts`:
- Around line 323-330: Update readBody to enforce a maximum accumulated
request-body size while buffering chunks, immediately stop processing and reject
when the limit is exceeded, and ensure the HTTP request handler converts that
condition into a 413 response. Preserve normal resolution for bodies within the
limit and avoid continuing to accumulate data after rejection.
In `@src/http/index.ts`:
- Around line 36-41: Validate the request Host header alongside checkOrigin in
the HTTP request handling flow before deriving pathname. Allow only the
configured loopback hosts (127.0.0.1 and localhost with the server port), reject
other or missing Host values with the existing forbidden response, and preserve
the current Origin validation behavior.
In `@src/http/origin.ts`:
- Around line 1-25: Add a Host-header allowlist alongside checkOrigin and apply
both validations in the request handling flow in the caller around the existing
Origin check. Accept only localhost or 127.0.0.1 with the configured bound port,
reject rebound or malformed Host values, and preserve the current Origin
behavior for requests without an Origin header.
In `@tests/errors.test.ts`:
- Around line 455-464: Update the test using the `core` instance in the `logs
the LocalWriteError and destroys the socket` case to call `core.closeAll()`
during cleanup, alongside the existing mock upstream and spy cleanup, ensuring
it runs even when the test assertion or handler fails.
In `@tests/helpers/mock-upstream.ts`:
- Around line 371-375: Update the catch handler around handle in the
createServer callback to call jsonRpcError only when res.headersSent is false;
otherwise avoid writing a second response after SSE headers have been sent.
In `@tests/origin.test.ts`:
- Around line 19-33: Add test coverage in the origin-checking cases around
checkOrigin for uppercase host normalization and duplicate Origin headers
represented as Node’s comma-joined value, adding the expected denied or allowed
outcomes consistent with the existing origin policy.
In `@tests/session.test.ts`:
- Around line 78-85: Update hangingResponder to check call.signal?.aborted
immediately when creating the Promise and reject with the existing AbortError
behavior if already aborted, while retaining the abort event listener for
signals that abort later.
In `@tests/upstream.test.ts`:
- Around line 30-44: Update the fetch mock’s header recording in recordingFetch
to normalize init.headers through the Headers API before building call.headers.
Preserve lowercase header names and ensure plain objects, Headers instances, and
arrays of pairs are all captured for downstream assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9269f1b-5408-4764-9b9c-fc296907dcc5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (38)
.editorconfig.github/workflows/ci.yml.github/workflows/release.yml.gitignore.prettierrcCHANGELOG.mdLICENSEREADME.mdeslint.config.jspackage.jsonsrc/config.tssrc/core/errors.tssrc/core/proxy-core.tssrc/core/session.tssrc/core/sse.tssrc/core/upstream.tssrc/http/adapter.tssrc/http/index.tssrc/http/origin.tssrc/index.tssrc/log.tssrc/shutdown.tssrc/version.tstests/adapter.test.tstests/config.test.tstests/errors.test.tstests/helpers/http.tstests/helpers/mock-upstream.tstests/origin.test.tstests/proxy.integration.test.tstests/relay.test.tstests/session.test.tstests/sse.test.tstests/upstream.test.tstsconfig.all.jsontsconfig.jsonvitest.config.tsvitest.integration.config.ts
- Never echo TINYFISH_UPSTREAM_URL values in config errors; reject URLs with embedded credentials - Remove internal doc/process references from code comments; trim comments - Name maintainer in README; drop TBD support-policy section - Keep 0.1.0 changes under Unreleased until first publish; mark README pre-release - Add language identifiers to README fenced blocks Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fetch_content requires urls/format/links/image_links; run_web_automation requires a client-minted session_id argument. Verified green against sandbox now that /mcp accepts X-API-Key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Summary
Initial implementation of
@tiny-fish/mcp— a locally-run MCP server distributed on npm that transparently reverse-proxies a local Streamable-HTTP endpoint (http://127.0.0.1:3711/mcp) to the hostedagent.tinyfish.ai/mcp, authenticating withTINYFISH_API_KEY.tools/list/tools/callare forwarded live, so parity with the hosted server is automatic (raw byte-pipe architecture; the MCP SDK's server transport was evaluated and rejected because it can't adopt upstream-issued session ids and diverges on framing/status codes).Authorizationheaders are never forwarded; the key never appears in logs or responses.run_web_automationSSE progress is relayed byte-verbatim with backpressure; client disconnect aborts the upstream fetch./mcpTest plan
npm run lint && npm run type-check && npm run build && npm test(156/156)env -u TINYFISH_API_KEY npm run test:integrationskips with noticenpm packsmoke test: tarball contains only dist/ + README + LICENSE + package.json; installed bin verified against the mock upstream🤖 Generated with Claude Code