feat: emit the X-Wherobots-Client attribution header (WBC-820) - #27
feat: emit the X-Wherobots-Client attribution header (WBC-820)#27ClayMav wants to merge 5 commits into
Conversation
Java and BI-tool traffic reaching Wherobots was unattributable: this driver never sent the shared X-Wherobots-Client header, so those requests were recorded as `unknown`. Add ClientHeader, which renders this driver's hop — `client=jdbc;ver=<driver version>;plat=<os name>` — and appends it to the right of any upstream chain, per the append-only chain convention in studio-backend's docs/client-attribution.md. The version comes from the JAR manifest and degrades to `unknown` when there is no manifest to read (IDE and test runs), so the hop is always well formed. Wire it in at WherobotsSessionSupplier.create(), the one place every outgoing request originates, so the session creation POST, the session polling GETs and the WebSocket upgrade all carry it, rather than at individual call sites. An upstream chain is sanitized before it is sent: characters that would corrupt the hop grammar or inject a header are replaced, and a chain that would push the value past the 512-byte bound is dropped rather than truncated, since a truncated chain is unparseable and an oversized header is discarded wholesale by the server-side parser. The header is advisory telemetry and never affects authentication. Refs WBC-820
A caller that embeds the driver inside another Wherobots client (an MCP server, a BI integration) already knows the chain the request came from. Without a way to pass it, that provenance is lost and the request is attributed to the JDBC driver as if it were the origin. Add a `clientChain` connection property that stages the caller's chain in the outgoing headers; the session supplier then appends this driver's own hop to the right of it, keeping the origin leftmost. The value is sanitized and size-bounded where the header is rendered, so a malformed or hostile chain degrades to a plain single-hop header instead of failing the connection. Refs WBC-820
There was a problem hiding this comment.
Reviewed by Salty Hambot 🤖🧂 — rubric mode
Verdict: ✅ pass
| Dimension | Verdict | Notes |
|---|---|---|
| correctness | ✅ pass | Advisory header rendering is sound; append-once refactor avoids the double-hop bug and the sole grammar concern is advisory-only, not a defect. |
| security | ✅ pass | CRLF/control-char stripping plus java.net.http's own validation block header injection, and the value never touches auth. |
| privacy | ✅ pass | Header carries only client token, version, and OS name — no user or credential data. |
| reliability | ✅ pass | 512-byte bound drops oversized chains rather than truncating, so a bad clientChain can never break a connection. |
| scalability | ✅ pass | Pure string work on a small bounded header; no scaling concern. |
| observability | No debug log captures the final rendered attribution value, so a misattribution in prod needs a packet capture to diagnose. | |
| clarity/maintainability | ✅ pass | Well-documented helper; only a cosmetic dangling-hyphen edge in param truncation. |
| test quality | ✅ pass | Thorough coverage of injection, size-bound, casing, and HttpRequest acceptance; one test leaks os.name without teardown. |
Solid advisory-header plumbing with genuinely good test coverage — the append-once refactor quietly kills a double-hop bug. Reviewer B's 'validate the grammar' blocker over-reads advisory telemetry as a contract, so it's dropped; what's left is a dangling-hyphen nit, a leaky os.name test, and a wish for a debug log.
4 finding(s) posted · 1 filtered as false positives.
💰 Review cost: $0.8297 · 271.1k in / 17.2k out tokens · ⏱️ 3m19.9s
💬 To request a re-review, comment @salty-hambot review
…WBC-820) Three non-blocking review findings. `sanitizeParam` stripped leading/trailing `-` before applying the 63-character cap, so the cut itself could expose a new trailing one: a version of `<62 chars>,rc1` sanitizes to `<62 chars>-rc1` and truncates to `<62 chars>-`. Stripping now happens after the cut as well. `WherobotsJdbcDriverTest` overwrote `os.name` and `java.version` with no teardown, leaving them set for whatever ran next in the same JVM. It now uses the save/restore pattern `ClientHeaderTest` already establishes. Nothing depends on the leak today only because every test that reads those properties sets them first; test ordering or parallel execution would end that. `WherobotsSessionSupplier.create` now debug-logs the rendered chain. Without it the only way to see what attribution actually went out is a packet capture. The chain is advisory, client-asserted metadata with no credentials in it, so logging it is safe.
There was a problem hiding this comment.
Reviewed by Salty Hambot 🤖🧂 — rubric mode
Verdict:
| Dimension | Verdict | Notes |
|---|---|---|
| correctness | Only a theoretical duplicate-case-key collapse in withHop; no current caller can hit it, but the Map contract permits it. |
|
| security | ✅ pass | CRLF/control-char stripping plus java.net.http's own validation block header injection; the advisory value never touches auth. |
| privacy | ✅ pass | Header carries only client token, version, and OS name — no credentials, confirmed safe-to-log in the new debug line. |
| reliability | ✅ pass | 512-byte bound drops oversized chains rather than truncating mid-grammar, so a bad clientChain can never break a connection. |
| scalability | ✅ pass | Pure bounded string work on a small header; no scaling concern. |
| observability | ✅ pass | Prior gap closed — the rendered attribution value now logs at debug right after withHop(). |
| clarity/maintainability | ✅ pass | Well-documented helper; dangling-hyphen edge is now handled and commented. |
| test quality | ✅ pass | Thorough coverage plus the new boundary and property-restore tests; prior os.name leak fixed. |
The fixes all landed clean — dangling hyphen, leaky os.name, and the missing debug log are done. Only leftover is a theoretical duplicate-case-key collapse in withHop that no real caller can trigger; worth an airtight merge, not worth blocking.
Prior findings: ✅ 4 resolved
1 finding(s) posted.
💰 Review cost: $0.7313 · 241.1k in / 7.4k out tokens · ⏱️ 1m45.4s
💬 To request a re-review, comment @salty-hambot review
`withHop` matched the header case-insensitively but kept only the last match, so a map holding both `x-wherobots-client` and `X-Wherobots-Client` would emit the second and silently drop the first hop's provenance. No caller can produce that today -- `getClientChainHeader()` returns a single canonical key -- but the parameter type is a plain `Map<String, String>`, which permits it, and the method's contract promises to collapse to one canonical header. Joining in encounter order makes that promise hold for any map, not just the one we happen to pass.
There was a problem hiding this comment.
Reviewed by Salty Hambot 🤖🧂 — rubric mode
Verdict: ✅ pass
| Dimension | Verdict | Notes |
|---|---|---|
| correctness | ✅ pass | The last theoretical case-key collapse in withHop is now closed by StringJoiner; append-once flow renders exactly one canonical hop. |
| security | ✅ pass | CRLF/control-char stripping plus java.net.http validation block header injection; advisory value never touches auth. |
| privacy | ✅ pass | Header carries only client token, version, and OS name — no credentials in the debug-logged value. |
| reliability | ✅ pass | 512-byte bound drops oversized chains rather than truncating mid-grammar, so a bad clientChain can never break a connection. |
| scalability | ✅ pass | Pure bounded string work on a small header; no scaling concern. |
| observability | ✅ pass | Rendered attribution value now logs at debug right after withHop() in the standard connection path. |
| clarity/maintainability | ✅ pass | Well-documented helper; dangling-hyphen edge handled and commented, case-variant join explained inline. |
| test quality | ✅ pass | Thorough coverage of injection, size-bound, casing, truncation boundary, case-variant join, and property restoration. |
Third time's the charm — the last theoretical case-key collapse is now airtight via StringJoiner, all five prior threads resolved, tests locked it in. Ship it.
Prior findings: ✅ 5 resolved
0 finding(s) posted.
💰 Review cost: $0.6788 · 142.9k in / 9.2k out tokens · ⏱️ 1m55.1s
💬 To request a re-review, comment @salty-hambot review
Summary
The JDBC driver had zero references to
X-Wherobots-Client, so every Java and BI-tool request (DataGrip, DBeaver, JDBC-backed services) landed in prod attributed asunknown. This adds the header.Every outgoing request now carries
client=jdbc;ver=<driver version>;plat=<os name>, e.g.Follows the contract in studio-backend's
docs/client-attribution.md: comma-separated hops, leftmost is the origin, append on the right, 512-byte bound, advisory only and never an input to auth.Changes
ClientHeader(new) renders the driver's hop and appends it to the right of any upstream chain. The version comes from the JAR manifest (Implementation-Version) and degrades tover=unknownwhen there is no manifest, so the hop is always well formed;platis dropped rather than faked whenos.nameis unavailable.WherobotsSessionSupplier.create(), the single place every request originates, so the session-creation POST, the session-polling GETs and the WebSocket upgrade are all covered instead of individual call sites. Both publiccreate()overloads go through it, so library callers that bypass theDriverare covered too.clientChainconnection property lets a caller embedding the driver supply an upstream chain to prepend (client=claude_web, client=mcp;ver=0.9, client=jdbc;ver=0.4.0;plat=linux). It is sanitized before it goes out: characters that would corrupt the hop grammar or inject a header (CR/LF, tabs, quotes) are replaced, empty hops are dropped, and a chain that would push the value past 512 UTF-8 bytes is dropped whole rather than truncated, since a truncated chain is unparseable and an oversized header is discarded wholesale by the backend parser. A badclientChaindegrades to a plain single-hop header, never a failed connection.Matches the already-merged implementations in
wherobots-cli(Go),vs-code-extension(TS) andwherobots-python-dbapi(Python).Testing
./gradlew clean build shadowJar --rerun-taskson the JDK 17 toolchain: BUILD SUCCESSFUL, 44 tests, 0 failures (the 6 skipped are the pre-existingWherobotsResultSetTestcases that needWHEROBOTS_API_KEY)../gradlew javadocis clean for the new file.21 new/updated JUnit tests covering:
HttpRequest.Builderaccepting the rendered value (java.net.httprejects illegal header values outright, so this also proves a hostile chain can't break session creation)ver=unknown; noos.name→platomittedclientChainappend case, including case-insensitive collapse of an existing header keyVerified against the real built JAR (not just the class directory) so the manifest-version path is exercised end to end:
No live session was created against staging or prod as part of this change.
Follow-up
jdbcis not yet in the canonical token vocabulary table in studio-backend'sdocs/client-attribution.md. No backend change is needed to record it (the parser is value-agnostic), but the table should get a row in a separate studio-backend PR.Linear
https://linear.app/wherobots/issue/WBC-820
- from Claude with ❤️