Skip to content

feat(http): attach user/device identity to swamp-club HTTP traffic (swamp-club#461)#1457

Merged
stack72 merged 2 commits into
mainfrom
feat/cli-telem
May 27, 2026
Merged

feat(http): attach user/device identity to swamp-club HTTP traffic (swamp-club#461)#1457
stack72 merged 2 commits into
mainfrom
feat/cli-telem

Conversation

@johnrwatson
Copy link
Copy Markdown
Contributor

Summary

  • Both ExtensionApiClient and SwampClubClient get an optional identity constructor arg. Their private fetch() wrappers now attach Authorization: Bearer <token> when the user is logged in and Swamp-Distinct-Id: <uuid> always.
  • Identity is resolved once at the CLI composition root via a new loadIdentity() helper, then threaded through libswamp function signatures. libswamp itself never reads auth.json or identity.json — keeps the domain boundary clean.
  • Header merge in the fetch wrapper is caller-wins: identity headers spread first, init.headers spread second. Preserves the per-method x-api-key paths and SwampClubClient.getCurrentUser's session-token Bearer.
  • Per-method apiKey? parameters are kept intact — this is purely additive.
  • Closes swamp-club#461.

Follow-up (separate, swamp-club repo)

This PR is the CLI half. The swamp-club server needs:

  1. Middleware (routes/_middleware.ts) reads Swamp-Distinct-Id, stamps it on spans, prefers it over the per-request anon_* hash. Authorization-derived identity still wins; Swamp-Distinct-Id is treated as untrusted hint data.
  2. Docs update at content/manual/reference/api-key-authentication.md.

Until that ships, the new header is silently dropped server-side. No CLI-visible breakage.

Test plan

  • deno check clean
  • deno lint clean
  • deno fmt clean
  • deno run test: 6304 passed (1 unrelated pre-existing failure on extension quality: missing README, fails on main too)
  • deno run compile: binary builds
  • Unit tests cover both clients: bearerToken + distinctId, distinctId-only, neither, both halves of precedence contract (caller-wins and constructor-wins), and a two-mock-server regression test ensuring the S3 presigned-URL hop carries no identity headers (no token leak to S3 access logs)
  • Manual smoke against local swamp-club: confirm Authorization + Swamp-Distinct-Id headers on outbound requests once swamp-club middleware lands

🤖 Generated with Claude Code

johnrwatson and others added 2 commits May 27, 2026 20:12
Both ExtensionApiClient and SwampClubClient gain an optional identity
constructor arg ({ bearerToken?, distinctId? }). Their private fetch()
wrappers attach `Authorization: Bearer <token>` when logged in and
`Swamp-Distinct-Id: <uuid>` always — letting swamp-club's telemetry
attribute CLI traffic to a user/device instead of an anonymous IP+UA
hash. Identity is resolved once at the CLI composition root via
loadIdentity() and threaded through libswamp function signatures so
libswamp itself never reads auth.json/identity.json. Per-method apiKey
parameters are preserved for backwards compatibility; the new header
path is additive.

The swamp-club server-side middleware change (read Swamp-Distinct-Id,
prefer it over per-request anon_* hash) ships separately.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AuthRepository.load() re-throws errors other than NotFound, including
the "HOME environment variable is not set" thrown by getSwampConfigDir()
on Windows test envs (which use USERPROFILE and may strip HOME in
isolated test runners). Because loadIdentity() runs at CLI startup for
every command — including `swamp --version` and `swamp --help` — that
throw was crashing the CLI with empty stdout on windows-latest CI.

Wrap the AuthRepository call in a try/catch that returns an empty
identity on any failure. Identity resolution is best-effort: a missing
config dir means anonymous traffic, never a CLI crash.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

CLI UX Review

Blocking

None.

Suggestions

None.

Verdict

PASS — this PR makes no user-facing changes. Every modification is internal wiring: threading ClientIdentity through HTTP client constructors and adding a loadIdentity() composition-root helper. No flags, help text, output formats, error messages, or command behavior change. Errors are swallowed silently so identity resolution never crashes a command. Nothing here affects users.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Code Review

Clean, well-designed PR that threads user/device identity through the HTTP clients via constructor injection. The architecture follows DDD principles correctly: identity resolution lives in the CLI composition root (loadIdentity), the domain and libswamp layers never touch the file system, and the ClientIdentity value object is threaded through function signatures cleanly.

Blocking Issues

None.

Suggestions

  1. Missing co-located test for mergeIdentityHeaders: Per project convention (foo.tsfoo_test.ts), src/infrastructure/http/client_identity.ts should have a client_identity_test.ts. The merge function is well-covered indirectly through the 10+ identity tests across both client test files, so this is not blocking — but a direct unit test would make the merge logic independently verifiable (e.g., edge cases like empty callerHeaders, conflicting header names with different casing).

What looks good

  • S3 presigned URL isolation: uploadArchive and downloadArchive correctly bypass the identity-injecting fetch wrapper, and the two-mock-server regression test (downloadArchive does NOT send identity headers to the S3 presigned URL) locks this contract in. No token leak to S3 access logs.
  • Caller-wins precedence: Identity headers spread first, caller headers second — preserving createApiKey's session-token Authorization and per-method x-api-key paths. Both halves of the contract are tested for each client.
  • Resilient loadIdentity: Catches all AuthRepository failures (including missing HOME env var) and leans on UserIdentityRepository.getUserId()'s internal catch-all. Every CLI command calls this at startup — crashing here would break everything.
  • Clean domain boundary: libswamp never reads auth.json or identity.json directly. The CLI composition root resolves identity and passes it through.
  • License headers, formatting, linting: All new files have the AGPLv3 header. CI passes on both Linux and Windows.

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

Adversarial Review

Critical / High

None found.

Medium

  1. Multiple loadIdentity() calls per CLI invocationsrc/cli/mod.ts:1154 calls loadIdentity() for the auto-resolver, and then every individual command handler (e.g., extension_push.ts:285, extension_update.ts:118) calls it again. Some paths like extension update load identity 3+ times (once in runCli, once in the command handler, once in the installExtension callback via createInstallContext). Each call reads auth.json and identity.json from disk. Not a correctness bug — the data won't change within a single CLI invocation — but it's redundant I/O. A single load at the composition root passed through would be cleaner.

  2. ClientIdentity not re-exported from libswamp/mod.ts — The factory functions createExtensionInfoDeps, createExtensionPullDeps, createExtensionPushPrepareDeps, etc. are all re-exported from src/libswamp/mod.ts and their signatures now accept ClientIdentity. But ClientIdentity itself is only exported from src/infrastructure/http/client_identity.ts (and re-exported from the two client modules). CLI code imports it directly from the infrastructure path (src/cli/mod.ts:100). This works via structural typing, but exporting the type from the barrel would be more consistent with the project's import convention documented in CLAUDE.md.

Low

  1. loadIdentity test env var mutation (src/cli/load_identity_test.ts:31-33) — The test deletes HOME, USERPROFILE, and XDG_CONFIG_HOME from the process environment. The finally block restores them, but if the test runner parallelizes across files, any concurrent test depending on HOME (which is most of them) will fail during the race window. This is safe under Deno's default sequential test execution, but fragile if parallel execution is ever enabled.

Verdict

PASS — Clean, additive change with correct header-merge precedence (identity-first, caller-wins), proper S3 presigned-URL isolation (both downloadArchive and uploadArchive use bare fetch not this.fetch), and thorough test coverage including the token-leak regression test. The medium items are minor inefficiency and API hygiene, not correctness issues.

@stack72 stack72 merged commit ec40452 into main May 27, 2026
11 checks passed
@stack72 stack72 deleted the feat/cli-telem branch May 27, 2026 19:34
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