Skip to content

fix: emit a real X-Wherobots-Client hop instead of the legacy format (WBC-829) - #48

Merged
ClayMav merged 2 commits into
devfrom
fix/client-attribution-hop-grammar
Aug 7, 2026
Merged

fix: emit a real X-Wherobots-Client hop instead of the legacy format (WBC-829)#48
ClayMav merged 2 commits into
devfrom
fix/client-attribution-hop-grammar

Conversation

@ClayMav

@ClayMav ClayMav commented Jul 28, 2026

Copy link
Copy Markdown
Member

Summary

The SDK sends X-Wherobots-Client: wherobots-sql-driver/0.11.1. The backend parser requires each hop's first segment to be client=<token>; anything else degrades to the unknown sentinel. So every request from this SDK has been recorded as unattributed rather than as a TypeScript SDK request.

How it happened: the header predates the grammar. 083cdde (2026-06-03, "refactor: slim the platform layer and unify client identification") added it for an unrelated reason — browsers drop a JS-set User-Agent, so the SDK needed some way to identify itself in the browser — and reasonably picked the User-Agent shape. Four weeks later the attribution work (WBC-176..187) defined X-Wherobots-Client as an ordered chain of client=<token>;ver=;plat= hops and taught the backend to parse it. This SDK was not in that issue set: it already had the header, so it did not read as a gap.

What changed:

  • New src/clientHeader.ts, mirroring wherobots-python-sdk's client_header.py and the JDBC driver's ClientHeader.java. Emits client=typescript-sdk;ver=<v>;plat=<p>, sanitizes to ASCII, neutralizes the ,/; delimiters and control characters, and bounds the value to 512 bytes by dropping the leftmost upstream hops rather than emitting something the server discards wholesale.
  • New clientChain connection option, so an app embedding this SDK can pass its own origin hop and have it kept to the left of ours. Matches the JDBC driver's clientChain property and the Python SDK's client_chain.
  • plat resolves through the Platform interface rather than a typeof process guard. process.platform in shared code leaks into the browser bundle, which bundle.smoke.test.ts forbids — that test caught it. Node reports its OS; the browser reports browser, since it has no OS it can name honestly.

The header stays advisory: client-asserted, never gates auth, and a hostile clientChain costs provenance rather than the request.

Token choice: typescript-sdk, matching the repo name and the shape of the existing python-sdk token. Reserved in the vocabulary table in studio-backend docs/client-attribution.md (that doc is the contract) — riding on wherobots/studio-backend#2409, which already edits the same table.

Heads-up, not in this PR: this repo has no publish workflow and no release tags, so this fix does not reach production until someone runs npm publish manually.

Related Issues

Fixes WBC-829. Relates to WBC-176 through WBC-187.


Requester Checklist

  • I have self-reviewed my own code
  • I have added/updated tests that prove my fix/feature works
  • I have included visual proof (screenshot, video, or test output) if applicable
  • All CI checks are passing
  • PR size is S/M, OR I have justified the size and added a walkthrough
  • I have updated documentation if needed

Visual Proof

15 new tests in clientHeader.test.ts plus 2 in connection.test.ts that pin the wiring end to end (the builder being right is not the same as the connection sending it).

 ✓ src/bundle.smoke.test.ts (8 tests) 22ms
 ✓ src/clientHeader.test.ts (15 tests) 4ms
 ✓ src/platform/decompress.test.ts (5 tests) 11ms
 ✓ src/connection.test.ts (38 tests) 39ms
 ✓ src/acceptance.node.test.ts (4 tests) 54ms

 Test Files  5 passed (5)
      Tests  70 passed (70)

npm run lint clean (3 warnings, all pre-existing in files this PR does not touch), tsc --noEmit clean, Prettier clean.

Before / after on the wire:

- X-Wherobots-Client: wherobots-sql-driver/0.11.1        → source_client=unknown
+ X-Wherobots-Client: client=typescript-sdk;ver=0.11.1;plat=darwin
                                                          → source_client=typescript-sdk

Reviewer Checklist

  • Requester checklist above is complete
  • All CI checks are passing
  • Tests adequately cover the changes

- from Claude with ❤️

The SDK has sent `X-Wherobots-Client: wherobots-sql-driver/<version>` since
083cdde, which predates the hop grammar by four weeks. That commit added the
header for a different reason (browsers drop a JS-set User-Agent, so the SDK
needed some way to identify itself) and reasonably picked the User-Agent shape.
The attribution work then defined the header as an ordered chain of
`client=<token>;ver=;plat=` hops and taught the backend to parse it, but nobody
revisited this SDK: it already had the header, so it did not read as a gap.

The backend's parser treats a hop whose first segment is not `client=<token>`
as `unknown`, so every request from this SDK has been recorded as an
unattributed request rather than as a TypeScript SDK request.

- Add src/clientHeader.ts, mirroring the Python SDK and JDBC driver: builds
  `client=typescript-sdk;ver=<v>;plat=<p>`, sanitizes to ASCII, neutralizes the
  `,`/`;` delimiters and control characters, and bounds the value to 512 bytes
  by dropping the leftmost upstream hops rather than emitting something the
  server will discard wholesale.
- Add a `clientChain` connection option so an app embedding this SDK can pass
  its own origin hop, kept to the left of ours (matches the JDBC driver's
  `clientChain` property and the Python SDK's `client_chain`).
- Resolve `plat` through the Platform interface rather than a `typeof process`
  guard: `process.platform` in shared code would leak into the browser bundle,
  which bundle.smoke.test.ts forbids. Node reports its OS; the browser reports
  `browser`, since it has no OS it can name honestly.

The header stays advisory: it is client-asserted, never gates auth, and a
hostile `clientChain` costs provenance rather than the request. The token
`typescript-sdk` is reserved in the vocabulary table in studio-backend
docs/client-attribution.md, which is the contract.
@ClayMav
ClayMav marked this pull request as ready for review July 28, 2026 02:26
@ClayMav
ClayMav requested a review from a team as a code owner July 28, 2026 02:26

@salty-hambot salty-hambot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed by Salty Hambot 🤖🧂 — rubric mode

Verdict: ⚠️ concerns

Dimension Verdict Notes
correctness ✅ pass Byte-bounding loop, chain splitting, and truncation-strip logic are correct and thoroughly tested; only a cosmetic char-vs-byte cap on clientChain that downstream truncation neutralizes.
security ✅ pass CRLF/header-injection primitives are stripped via allowlist regexes and a test proves the value is always a legal fetch header; header is advisory-only and never touches auth.
privacy ✅ pass clientChain is caller-controlled free text logged at debug level, but it's advisory attribution data and apiKey/token are correctly excluded from the logged options.
reliability ✅ pass Oversized chains degrade gracefully by dropping leftmost hops while always preserving the SDK's own hop; never returns empty.
scalability ✅ pass Bounded 512-byte header with a simple shift loop; no scaling concerns in the changed paths.
observability ✅ pass Debug logging of connection options is consistent with existing behavior; clientChain surfacing is low-risk and documented as advisory.
clarity/maintainability ⚠️ concerns Two minor comment/naming traps: MAX_VALUE_CHARS comment misnames the client token, and MAX_HEADER_BYTES used as a UTF-16 char cap in schemas.ts.
test quality ✅ pass Excellent coverage of the sneaky failure modes — trailing separators post-truncation, CRLF injection, non-ASCII byte inflation, hop starvation — with tests that document the why.

Genuinely tight, paranoid-in-a-good-way header code with tests that hunt the exact edge cases you'd worry about — only real feedback is two comments/naming quirks (char-vs-byte, token-vs-param) that'll confuse the next reader, not the machine.

4 finding(s) posted.
💰 Review cost: $0.8258 · 207.1k in / 14.7k out tokens · ⏱️ 3m3.7s
💬 To request a re-review, comment @salty-hambot review

Comment thread src/clientHeader.ts
Comment thread src/clientHeader.ts Outdated
Comment thread src/schemas.ts
Comment thread src/connection.ts
MAX_VALUE_CHARS justified itself by citing the server's 64-character
client-token bound, but it only ever bounds ver/plat parameter values,
which that bound does not govern; CLIENT_TOKEN never passes through
sanitizeValue at all. Also note that the clientChain schema cap counts
UTF-16 code units, while MAX_HEADER_BYTES is a UTF-8 byte budget
everywhere else.
@ClayMav

ClayMav commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

@salty-hambot rubric

@salty-hambot salty-hambot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed by Salty Hambot 🤖🧂 — rubric mode

Verdict: ⚠️ concerns

Dimension Verdict Notes
correctness ✅ pass Byte-bounding loop, hop-dropping, and truncation-strip logic remain correct and thoroughly tested; only a stale comment noun (thread 2), not a behavior bug.
security ✅ pass CRLF/header-injection chars stripped via allowlist regexes; a test proves the value is always a legal fetch header, and the header is advisory-only, never touching auth.
privacy ✅ pass clientChain is caller-controlled advisory text logged at debug; apiKey/token are correctly destructured out of the logged options.
reliability ✅ pass Oversized chains degrade gracefully by dropping leftmost hops while always preserving the SDK's own hop; never returns empty.
scalability ✅ pass Bounded 512-byte header with a simple shift loop; no scaling concerns in the changed paths.
observability ✅ pass Debug logging of connection options is consistent with existing behavior; clientChain surfacing is low-risk and documented as advisory.
clarity/maintainability ⚠️ concerns MAX_VALUE_CHARS comment still aims at the wrong noun (client token vs ver/plat params) — a next-reader trap, unresolved from review 1.
test quality ✅ pass Excellent coverage of the sneaky failure modes — post-truncation trailing separators, CRLF injection, non-ASCII byte inflation, hop starvation — with tests documenting the why.

No new issues this round — the header code is still tight and paranoid-in-a-good-way. Only lingering nit is the MAX_VALUE_CHARS comment pointing at the wrong noun (thread 2), which is cosmetic and won't confuse the machine. Ship it.

Prior findings: ✅ 2 resolved · ⚠️ 2 still open

0 finding(s) posted.
💰 Review cost: $0.6812 · 152.1k in / 5.5k out tokens · ⏱️ 1m14.9s
💬 To request a re-review, comment @salty-hambot review

@salty-hambot salty-hambot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed by Salty Hambot 🤖🧂 — rubric mode

Verdict: ✅ pass

Dimension Verdict Notes
correctness ✅ pass Byte-bounding loop, chain splitting, and post-truncation separator strip are correct and thoroughly tested; the prior char-vs-byte cap is now documented as intentional.
security ✅ pass CRLF/header-injection primitives are stripped via allowlist regexes and a test proves the value is always a legal fetch header; header is advisory-only and never touches auth.
privacy ✅ pass clientChain is caller-controlled advisory attribution text logged at debug level; apiKey/token are correctly excluded from the logged options.
reliability ✅ pass Oversized chains degrade gracefully by dropping leftmost hops while always preserving the SDK's own hop; never returns empty.
scalability ✅ pass Bounded 512-byte header with a simple shift loop; no scaling concerns in the changed paths.
observability ✅ pass Debug logging of connection options is consistent with existing behavior; clientChain surfacing is low-risk and documented as advisory.
clarity/maintainability ✅ pass The two comment/naming traps from round one (token-vs-param, char-vs-byte) are now explicitly documented; comments are clear.
test quality ✅ pass Excellent coverage of the sneaky failure modes — trailing separators post-truncation, CRLF injection, non-ASCII byte inflation, hop starvation — with tests that document the why.

Re-review of a tight, well-tested attribution-header PR — no new commits, and the two comment/naming nits from round one are now fixed. Only lingering item is caller-supplied clientChain landing in debug logs verbatim, which was a non-blocking thought and stays out of scope. Ship it.

Prior findings: ✅ 3 resolved · ⚠️ 1 still open

0 finding(s) posted.
💰 Review cost: $0.8086 · 410.4k in / 6.9k out tokens · ⏱️ 1m35.9s
💬 To request a re-review, comment @salty-hambot review

@ClayMav
ClayMav merged commit 08384b0 into dev Aug 7, 2026
2 checks passed
@zongsizhang zongsizhang mentioned this pull request Aug 12, 2026
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants