feat(hermes): identify plugin requests with X-TF-Client-Name/Version - #35
feat(hermes): identify plugin requests with X-TF-Client-Name/Version#35londondavila wants to merge 7 commits into
Conversation
Every call from this plugin lands in TinyFish telemetry as untagged request_origin=api, indistinguishable from a hand-written curl. The server already reads X-TF-Client-Name and X-TF-Client-Version; CloudFront forwards them. Sending them names the plugin and its version on every request. - rest_client._headers adds X-TF-Client-Name: tinyfish-hermes and X-TF-Client-Version: __version__; every call site already goes through it - X-TF-Request-Origin deliberately not set: `api` stays the transport, identity lives in client_name, same split MCP clients use - bump 0.1.0 → 0.1.1 so the version header distinguishes upgraded installs Attributes new traffic only; existing installs keep landing as untagged until users update. The external SHA pin needs to move to this commit. Testing: pytest 183 passed; ruff check/format and mypy clean. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0138Vn8E8TfZkUWd3StqDgjp
Review fixes. uv.lock was generated by `uv run` and swept in by `git add`; nothing reads it (CI uses pip, no [tool.uv]) and it would drift silently. - remove hermes/uv.lock, ignore it - tests: one _AUTH_HEADERS constant instead of four pasted dicts; drop the contract test that duplicated what the four exact-dict asserts already prove - comment: request_origin stays `api` with these headers; what changes is client_name, so say that Testing: pytest 182 passed; ruff check/format, mypy clean. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0138Vn8E8TfZkUWd3StqDgjp
httpx encodes header values as ASCII, and UnicodeEncodeError is a ValueError, so a hand-edited plugin.yaml with a stray byte would turn every request into "TinyFish Search returned invalid JSON" — blaming the server. Only reachable on a directory install with no dist metadata, but the error text is wrong enough to cost someone an hour. - _resolve_version falls back to 0+unknown for non-ASCII values - test covers it Testing: pytest 183 passed; ruff, mypy clean. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0138Vn8E8TfZkUWd3StqDgjp
…data Both adversarial reviewers (Claude, Codex) reproduced the same failure: a directory install alongside any older `pip install tinyfish-hermes` (or an editable install) reported the stale distribution version, and this branch is what first sends that value on the wire as X-TF-Client-Version. The wheel omits plugin.yaml, so an adjacent manifest reliably means "directory install" and should win; metadata stays as the fallback for pip installs. - _resolve_version: manifest first, metadata second, then 0+unknown - tests flipped to match, plus the no-manifest metadata case Testing: pytest 184 passed; ruff, mypy clean. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0138Vn8E8TfZkUWd3StqDgjp
Review found this repo already has two identity vocabularies for the same integrations — api_integration uses bare tokens (dify, n8n, langchain) and the CLI sends X-TF-Client-Name: hermes when running under Hermes. A third (tinyfish-hermes) would make coalesce(client_name, api_integration) split one integration into two rows. request_origin (cli vs api) already separates plugin from harness, so the bare token is right. - X-TF-Client-Name: hermes - README: one section fixing the header contract for every integration, so the fourth one doesn't drift - tests: last pasted header dict hoisted to _AUTH_HEADERS Testing: pytest 184 passed; ruff, mypy clean. Co-Authored-By: Claude Code <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0138Vn8E8TfZkUWd3StqDgjp
|
Warning Review limit reached
On-demand reviews are free for the next 16 days. After that, they cost $0.25 per reviewed file. Or wait 3 minutes for your next included review. View limit detailsLimit details: You’ve used all 3 included reviews currently available. Your 41 included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughHermes now sends Suggested reviewers: Merge Risk: 🟡 Moderate · up to Hermes requests now identify the client and version, but the version-alignment safeguard can be bypassed when both fields are unreadable, and the SDK guidance does not generate the required client headers. These documentation and release-integrity issues should be corrected before merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Zechereh
left a comment
There was a problem hiding this comment.
Header plumbing looks right. Verified all six httpx call sites in hermes/tinyfish_hermes/rest_client.py (91, 132, 163, 202, 226, 263) route through _headers/_json_headers, so no request path bypasses the new headers. Suite is green — 184 passed.
Checked this against the merged PF-3830 work in ux-labs (#4796, #4797) and it lines up: 0+unknown is a documented accepted value there, the route's versionSchema (max(32), printable ASCII) accepts it, and setup_cli.py:277's plugin_version matches the hermes-plugin.ts parser. The .isascii() guard is doing more work than the comment suggests — #4796 returns 400 and drops the whole doctor event on a non-printable version, so this guard is what keeps a garbled plugin.yaml from silently darkening doctor telemetry.
Three things worth a look, none blocking.
README.md:36 — the contract is described in the present tense, but only hermes meets it.
dify/tools/base.py:13, n8n/nodes/Tinyfish/GenericFunctions.ts:142 and n8n/credentials/TinyfishApi.credentials.ts:34 still send X-API-Key alone; langchain/langchain_tinyfish/_api_wrapper.py:84 and google-adk/src/tinyfish_adk/tools.py:30 set only TF_API_INTEGRATION, not TF_CLIENT_NAME/TF_CLIENT_VERSION (README.md:43).
"Every integration … sends" reads as already-done, so someone will look at dify/n8n traffic in telemetry, see it undifferentiated from raw curl, and go hunting for a bug. Suggest making it normative: "integrations must send …; today: hermes."
Nit: the section lands below ## License, while the other contributor-facing section sits above it.
hermes/plugin.yaml:2 — this bump will invert PF-3830's drift metric at the next CLI release.
ux-labs/sdk/cli/src/lib/hermes-plugin.ts:24-26 pins:
export const HERMES_PLUGIN_SHA = '496cd63fefd982bbaa8a85ce78ef2270d700984f';
// Bump with the SHA; the source is `hermes/plugin.yaml` at that commit.
export const HERMES_PLUGIN_VERSION = '0.1.0';496cd63 is #33, where plugin.yaml is still 0.1.0 — so the pair is consistent and nothing breaks on merge, since the CLI installs at the pinned SHA.
The trap is the next CLI release. #4797 defines drift as hermes_plugin_version != hermes_plugin_expected_version, where expected is that constant. Move HERMES_PLUGIN_SHA past this commit without moving HERMES_PLUGIN_VERSION to 0.1.1 and every clean install reports as drifted. Deriving the constant automatically is tracked separately; in the meantime this is worth a heads-up to whoever cuts the next CLI release.
hermes/tests/test_rest_client.py:15 — the version assertion is self-referential.
_AUTH_HEADERS derives the expected X-TF-Client-Version from __version__, so it holds for whatever _resolve_version() returns, 0+unknown included. The client name is pinned to the literal "hermes" one line above at :14 — worth pinning the version to the plugin.yaml value the same way.
Related, and the bigger half: this PR makes plugin.yaml authoritative for directory installs and the numerator of the drift metric above, but nothing guards it against pyproject.toml. A future bump touching only pyproject.toml ships a stale X-TF-Client-Version silently. .github/workflows/plugin-manifests-ci.yml already does exactly this check for the claude plugin, so a one-line hermes-ci step would fit house convention.
Two nits, take or leave:
hermes/tinyfish_hermes/__init__.py:29— a garbled manifest shadows a good installed version: manifest0.1.1é+ dist metadata0.1.1returns0+unknown(ran it). Taking the first candidate that is non-empty and.isascii()would degrade to the next source instead of straight to the fallback.hermes/tinyfish_hermes/__init__.py:10— on a wheelparents[1]issite-packages/, and hatchling ships onlytinyfish_hermes, so an unrelated package with a top-levelplugin.yamlwould report its version as ours. Not reachable via the supported install path (hermes plugins install …/hermes --ref <sha>is a directory install), so genuinely low. Cheap guard if you want it: requirename: tinyfishin the file first —tests/test_version.py:36already writes it into the fixture.
One question: does the tinyfish SDK consume TF_CLIENT_NAME/TF_CLIENT_VERSION today? README.md:43 instructs SDK wrappers to set them, but nothing in this repo does, so that half of the contract can't be verified from here.
Zechereh
left a comment
There was a problem hiding this comment.
Approving — nothing here blocks. Header plumbing is correct, all six httpx call sites route through _headers/_json_headers, and the suite is green at 184.
One narrowing on README.md:36 from my earlier review: #37 landed, so n8n complies via nodes/Tinyfish/clientHeaders.ts (both the credential headers and the raw SSE fetch). Remaining gaps are dify/tools/base.py:13 and the SDK-wrapper sentence at :43 — TF_CLIENT_NAME appears nowhere in langchain or google-adk. Smaller edit than I first implied: dropping dify from the table until its PR lands may beat rewording the section.
The rest of my earlier comments stand as non-blocking follow-ups. The one worth not losing is the plugin.yaml bump's interaction with HERMES_PLUGIN_VERSION on the CLI side — handled separately, not a condition of this merge.
Non-blocking items from PR #35 review. The version guard degraded a garbled manifest straight to `0+unknown` even when installed metadata was good, and nothing kept `plugin.yaml` — now authoritative for the header and the numerator of the PF-3830 drift metric — in step with `pyproject.toml`. - `_resolve_version()` walks candidates and takes the first non-empty ASCII one, so a bad manifest falls through to dist metadata - hermes-ci: fail when `plugin.yaml` and `pyproject.toml` versions disagree - pin `X-TF-Client-Version` in the rest_client test to the literal `0.1.1` instead of deriving it from `__version__` - README: header contract stated as normative, with the integrations that do not meet it yet named (dify; langchain/google-adk on the SDK side) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nw7GKwVBvSXErUW8wkxaYD
… claim Both version parsers are regex-based, so a file the regex misses yields "" on both sides and compares equal — the check passes exactly when it should fail. Separately, `tinyfish==0.2.5` builds a fixed header set with no passthrough and never reads `TF_CLIENT_NAME`/`TF_CLIENT_VERSION`, so the README told SDK wrappers to do something that produces no headers. - hermes-ci: error out when either version is empty - README: SDK wrappers identify with `TF_API_INTEGRATION` alone until the SDK exposes the client headers Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nw7GKwVBvSXErUW8wkxaYD
Identify the plugin to the TinyFish API so its requests are attributable in usage analytics.
X-TF-Client-Name: hermesandX-TF-Client-Version: <plugin version>on every request viarest_client._headers, which every call already goes throughplugin.yamlover installed distribution metadata, so a directory install can't report a stalepip-installed version; non-ASCII values fall back to0+unknown