Skip to content

RUSH-1835: agents share usage analytics + per-user GitHub namespaces#1406

Merged
muqsitnawaz merged 6 commits into
mainfrom
agents/RUSH-1835-w4
Jul 22, 2026
Merged

RUSH-1835: agents share usage analytics + per-user GitHub namespaces#1406
muqsitnawaz merged 6 commits into
mainfrom
agents/RUSH-1835-w4

Conversation

@muqsitnawaz

Copy link
Copy Markdown
Contributor

Implements RUSH-1835 for the CLI.

What changed

  • Cloudflare Web Analytics (cookieless). agents share setup --analytics-token <token> stores the zone token in synced share config; every HTML publish injects the beacon.min.js snippet before </body>. Opt out per publish with --no-analytics.
  • Per-user GitHub-username namespaces. agents share foo.html now publishes to <base>/<github-username>/foo. Username is resolved from gh auth login, git config --global github.user, AGENTS_SHARE_GITHUB_USER, or --github-user. Slugs passed via --slug are scoped under the namespace, with / sanitized away.
  • Public galleries. GET /<username> lists that user's shares as a styled HTML gallery.
  • Backward compatibility. Legacy flat slugs (/<slug>) still resolve.
  • CLI surface. New agents share analytics subcommand; agents share status now shows namespace + analytics state.

Verification

  • bunx tsc --noEmit clean.
  • bun run test clean: 6425 passed, 78 skipped.
  • New tests cover analytics injection, namespace slug building, Worker namespace routing, gallery rendering, and legacy flat-slug fallback.

Source: apps/cli/src/commands/share.ts, apps/cli/src/lib/share/*, apps/cli/src/lib/git.ts, apps/cli/docs/share.md.

…L namespaces

- Inject Cloudflare Web Analytics beacon into published HTML (cookieless,
  privacy-first). Opt out with --no-analytics. Token configured via setup
  --analytics-token or agents.yaml share.analyticsToken.
- Namespace shares by GitHub username: share.agents-cli.sh/<user>/<slug>.
  Resolved from gh auth login, git config github.user, or AGENTS_SHARE_GITHUB_USER,
  with --github-user override. /<user> renders a public gallery.
- Worker routes namespace paths and falls back to legacy flat slugs.
- Add agents share analytics subcommand and enrich status output.

Source: apps/cli/src/commands/share.ts, apps/cli/src/lib/share/*,
apps/cli/src/lib/git.ts, apps/cli/docs/share.md.
@prix-cloud

prix-cloud Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Code Reviewer

Verdict: Changes requested (non-blocking) — build and tests are green; two behavioral points worth the author's eyes before merge.

Build:./node_modules/.bin/tsc — clean, no errors.
Tests: ✅ Files touched by this PR: 90/90 passed (src/lib/share/analytics.test.ts, src/lib/share/publish.test.ts, src/lib/share/publish-file.test.ts, src/lib/git.test.ts, src/commands/share.test.ts, including the new worker-template namespace/gallery/legacy-slug cases).
Full suite (bun run test → vitest): 6377 passed, 9 failed, 117 skipped (6503 total). All 9 failures are in files this PR does not touch (src/lib/daemon.test.ts, src/commands/monitors.test.ts, src/lib/__tests__/models.test.ts, src/lib/browser/port.test.ts, src/lib/output/__tests__/git-output.test.ts — a different module from the src/lib/git.ts this PR edits) and the failure causes are environmental to this sandbox (429 Too Many Requests, spawn ... EACCES, a real-listener port check, git-log/author checks) rather than anything introduced here.

Read for repo/context: root AGENTS.md/CLAUDE.md, apps/cli/AGENTS.md.

Changes that work well

  • injectAnalyticsBeacon's idempotent strip-then-reinject (analytics.ts:39-51) and its three fallback insertion points (</body></head> → prepend) are well covered by analytics.test.ts, including the empty-token no-op case.
  • buildShareKey/resolveShareUsername sanitize both the username and slug consistently, and publishToEndpoint correctly short-circuits the double resolveShareUsername call once publishFile has already resolved it (publish.ts:190-198 passes githubUser through, so the second resolution in publishToEndpoint just re-sanitizes the already-known value instead of re-invoking gh).
  • Legacy flat-slug fallback and the new namespaced GET/PUT/gallery paths in worker-template.ts all have direct Worker-level tests exercising the actual R2 binding surface (list/get/put/delete), not mocks of the routing logic.

Issues that need attention

1. agents share status/agents share analytics under-report username resolution (moderate).
src/commands/share.ts:125 and :150 both call resolveGitHubUsernameSync(), which per its own doc comment (src/lib/git.ts:435-442) only checks AGENTS_SHARE_GITHUB_USER and git config --global github.user — it does not call gh api user. The actual publish path (resolveShareUsernameresolveGitHubUsername, git.ts:450-461) does fall back to gh via getGitHubUsername(). Both .action() callbacks were changed to async in this PR (share.ts diff: .action(() => {.action(async () => {) but neither awaits the async resolver.

Net effect: a user authenticated only via gh auth login (no git config --global github.user set — very plausible, since the error message in resolveShareUsername lists gh auth login first) will see:

namespace unknown — set gh auth or github.user

from agents share status, even though agents share <file> would succeed and publish under their resolved username. agents share analytics has the same gap for its "Per-page breakdown... filter by /${user}/" hint. No test exercises either command's output, so this isn't caught. Suggest either awaiting resolveGitHubUsername() in both actions (the async is already there) or documenting why status intentionally stays gh-CLI-free.

2. Public gallery changes the "unguessable slug" privacy model (worth confirming intended).
docs/share.md still advertises: "a short random tail keeps it unguessable and collision-free" for the default slug. But GET /<username> (worker-template.ts:83-115) now lists every namespaced object for that user — including ones published with the default random-tail slug. Once a person's GitHub username is known (public by definition), every page they've shared under their namespace becomes discoverable via the gallery, regardless of how unguessable its slug was meant to be. This is clearly an intended feature (it's in the PR description), but it's a real, silent change to what "unguessable" means for anyone using agents share without --slug, and there's no per-publish opt-out from being listed (only --no-analytics and --no-cover exist, no --unlisted). Worth a one-line confirmation that this tradeoff is deliberate before it ships, since it applies immediately to every future default-slug publish (past legacy flat-slug objects are unaffected — they aren't picked up by the namespace gallery).

Two smaller edge cases in the same function, non-blocking:

  • renderGallery calls bucket.list({ prefix: user + '/' }) with no limit/cursor loop, so a user with more shares than R2's default page cap (1000) will silently see a truncated gallery.
  • Listed items aren't filtered by expiry — an expired-but-not-yet-swept object still shows in the gallery with a link that 410s when clicked.

Things to verify manually

  • resolveShareUsername now throws a hard error when no username can be resolved at all (publish.ts:167-171). For any existing agents share user with neither gh auth login nor git config --global github.user set, every publish will start failing until they configure one — worth a quick smoke-test of the new error message/flow on a clean machine to confirm it's actionable.
  • The Cloudflare Web Analytics beacon injection and gallery rendering are unit-tested against a mocked R2 binding; there's no live Cloudflare deploy check in this PR (consistent with how provision.ts/Worker changes have always been tested here), so worth a manual agents share setup --analytics-token + publish round-trip against a real Cloudflare account before relying on it in production.

Reviewed by Code Reviewer — actually ran the build and tests on this branch.

… private

prix flagged that the 'unguessable slug' framing contradicts the new public
/<github-username> gallery, which lists every non-expired namespaced share.
Make the tradeoff explicit: the random tail is collision-avoidance, not a
privacy control. The renderGallery pagination (cursor loop) and expiry filter
prix also noted are already implemented in worker-template.ts:83-96.
@prix-cloud

prix-cloud Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Code Reviewer

Verdict: Ready to merge

Build:bun x tsc --noEmit clean; bun run build (tsc + native-helper packaging) clean.
Tests: ✅ 96/96 passed — scoped run over every changed file (src/lib/share/analytics.test.ts, src/lib/share/publish.test.ts, src/lib/share/publish-file.test.ts, src/lib/git.test.ts, src/commands/share.test.ts), run via the repo's actual vitest runner (node ./node_modules/vitest/vitest.mjs run … — note bun test is the wrong runner here: it lacks vi.resetModules/vi.setSystemTime and can't resolve the worker script's data: URL import used by publish.test.ts, so it fails all 15 tests that rely on those; this is a sandbox-runner mismatch, not a PR defect).

I attempted the full suite (bun run test) to check against the PR's claimed "6425 passed, 78 skipped." It hung for several minutes on real-dispatch integration tests that shell out to kimi/grok/claude CLIs (warning: kimi has no headless plan mode…) — consistent with this repo's "real services only, no mocking" convention (apps/cli/AGENTS.md), but those presumably need CLI/network state this sandbox doesn't have. I killed it after ~2 min of no progress rather than let it eat the whole review budget; that's a sandbox limitation, not evidence against the PR. The scoped run covers 100% of the touched logic (analytics injection, namespace/slug building, Worker routing incl. gallery + legacy fallback, git username resolution, CLI command wiring) and passed cleanly.

I read AGENTS.md/CLAUDE.md at repo root and apps/cli/AGENTS.md (no component-specific AGENTS.md under src/lib/share/ or src/commands/) — build/test commands above come from there.

Changes that work well

  • injectAnalyticsBeacon is properly idempotent (strips a prior agents-share:analytics block by marker comments before re-inserting) and degrades gracefully across </body></head> → prepend-to-bare-document, each path covered by a test.
  • The Worker's per-user namespace + legacy flat-slug fallback is a clean single-request disambiguation (list({prefix: user+'/', limit:1}) before falling back to get(path)), and it's exercised by a real in-memory R2 mock in publish.test.ts including pagination + expiry filtering.
  • Docs (docs/share.md) were updated in the same PR and explicitly flag the privacy implication of the new gallery (random-tail slugs are still listed publicly) — good instinct to call that out rather than let users discover it.

Things to verify manually

  • Doc/code resolution-order mismatch (cosmetic). docs/share.md states the username is "resolved from gh auth login, git config --global github.user, or the AGENTS_SHARE_GITHUB_USER env var" (git config before env var). The actual implementation (resolveGitHubUsername in apps/cli/src/lib/git.ts:493) checks AGENTS_SHARE_GITHUB_USER first, then gh api user, then git config. Worth a quick doc tweak so the precedence in prose matches the code (env var is the highest-priority override, not the last resort).
  • resolveGitHubUsernameSync (apps/cli/src/lib/git.ts:443) is unused outside its own test. It's exported and has 3 dedicated unit tests, but nothing in commands/share.ts or lib/share/publish.ts calls it — everything routes through the async resolveGitHubUsername. Not a bug, but worth a sanity check that it wasn't meant to back a synchronous code path (e.g. a --github-user default shown in --help) that got dropped, since otherwise it's dead surface to maintain.
  • Every GET to a single-path-segment URL (gallery vs. legacy flat slug) now costs an extra BUCKET.list() call before the fallback BUCKET.get() — a minor R2-op cost increase for the legacy-slug path that's probably fine given R2's list pricing, but worth confirming it's an acceptable tradeoff at the traffic the Worker actually sees.

Reviewed by Code Reviewer — actually ran the build and tests on this branch.

@muqsitnawaz
muqsitnawaz merged commit 5dfbe23 into main Jul 22, 2026
9 checks passed
@muqsitnawaz
muqsitnawaz deleted the agents/RUSH-1835-w4 branch July 22, 2026 18:58
@prix-cloud

prix-cloud Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Code Reviewer

Verdict: Changes requested

Build:bun install (runs preparetsc build) clean, bun x tsc --noEmit clean, bun run build clean.
Tests: ⚠️ Scoped tests (src/lib/share/, src/commands/share.test.ts, src/lib/git.test.ts) via the repo's real runner (node ./node_modules/vitest/vitest.mjs run …bun test is the wrong runner here, it lacks vi.resetModules/vi.setSystemTime and can't resolve the worker script's data: URL import) — 125/125 passed. Full suite — 6380 passed, 9 failed, 117 skipped (vs. the PR's claimed 6425/78). The 9 failures are all in src/lib/browser/port.test.ts and src/lib/output/__tests__/git-output.test.ts, files this PR doesn't touch (real listening-port introspection and clock/timezone-sensitive git log --since windowing) — looks like sandbox/environment flakiness, not something this diff caused.

Read AGENTS.md/CLAUDE.md at repo root and apps/cli/AGENTS.md (canonical for this component); no component-specific AGENTS.md/CLAUDE.md exists under src/commands/ or src/lib/share/. Build/test commands above come from apps/cli/AGENTS.md.

Issues that need attention

agents share setup silently wipes a previously configured analytics token if --analytics-token isn't re-passed. src/commands/share.ts:214:

const cfg: ShareConfig = { baseUrl, accountId, workerName, bucketName, domain, analyticsToken: opts.analyticsToken };
writeShareConfig(cfg);

opts.analyticsToken is undefined whenever setup runs without --analytics-token. That key is still present on the object (not omitted), so writeShareConfigupdateMeta(meta => ({...meta, share: {...meta.share, ...cfg}})) (src/lib/state.ts:908-917) spreads analyticsToken: undefined over any existing value, and the YAML writer drops undefined keys on serialize — so the field is gone from agents.yaml after the write.

I reproduced this live against the actual runShareProvision/readShareConfig code path (temp HOME, injected CloudflareRequester, no mocking):

After first setup (with token): { ..., analyticsToken: 'my-real-analytics-token' }
After second setup (no --analytics-token passed): { ..., analyticsToken: undefined }

This isn't a hypothetical re-run either — docs/share.md:110 (unchanged by this PR) explicitly documents re-running setup as the supported way to rotate the write token ("rotate by re-running setup (mints a new one)"). Any user who follows that guidance, or re-runs setup for an unrelated reason (new --domain, different --worker/--bucket), loses their analytics config with no warning. runShareJoin doesn't have this problem — when reusing existing config it keeps the object as-is, and when joining fresh it omits the analyticsToken key entirely rather than setting it to undefined, so the merge there can't clobber anything (src/commands/share.ts:255-267).

Fix: only include analyticsToken in the merged config when explicitly provided, e.g. ...(opts.analyticsToken !== undefined ? { analyticsToken: opts.analyticsToken } : {}), or default to the existing value: analyticsToken: opts.analyticsToken ?? readShareConfig()?.analyticsToken.

Test coverage gap: share.test.ts covers "persists --analytics-token in the share config" (fresh setup) but not the "setup already has an analytics token, re-run setup without the flag" round trip — that's exactly the case that would have caught this.

Changes that work well

  • buildShareKey/sanitizeSlugPart sanitize both the username and slug (lowercase, non-alnum collapsed to -, slashes stripped before sanitizing), so path-traversal-style --slug/--github-user values (../../secret, embedded /) can't escape the per-user prefix — verified this collapses to a safe slug, not a nested path.
  • injectAnalyticsBeacon is idempotent (strips a prior agents-share:analytics marker block before re-inserting) and degrades gracefully across </body></head> → prepend-to-bare-document; --no-analytics correctly skips injection even when a token is configured (publish.ts:236-238).
  • Worker gallery rendering, cursor pagination, expiry filtering, and legacy flat-slug fallback are exercised with a real in-memory R2 mock in publish.test.ts and match the implementation.
  • Docs were updated in the same PR and explicitly flag the privacy implication of the new gallery (random-tail slugs are still publicly listed) — good instinct to call that out rather than let users discover it.

Things to verify manually

  • Real Cloudflare Web Analytics dashboard URL/rendering (dash.cloudflare.com/<account>/web-analytics/<domain>) isn't exercised by any test — worth a manual click-through against a live account.
  • The namespace-vs-legacy-flat-slug collision case (a legacy flat slug whose name matches a GitHub username that has since published under its own namespace) isn't covered — the gallery would shadow the legacy object once that user has any share. Likely rare and probably acceptable, but wasn't called out alongside the other namespace-collision note in docs/share.md:90-96.

Reviewed by Code Reviewer — actually ran the build and tests on this branch.

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.

1 participant