feat: record adoption dimensions on every telemetry event - #326
Conversation
cli_run counts invocations and cannot count workspaces, which is what
nearly every adoption metric is actually about. The finest identity carried
today is the anonymous UUID in $XDG_CONFIG_HOME, which is per config
directory: one developer across five repositories is one id, and a fresh CI
container regenerates it every job. Both distortions inflate breadth and
deflate depth, and neither is measurable after the fact.
Adds six super-properties, resolved once per process and attached to
identify and to every capture:
workspaceId hash of the git top level, falling back to cwd
repositoryId hash of {host}/{owner}/{repo}, host-agnostic
envOS process.platform
ci whether CI holds a positive value
ciProvider the provider, [unknown] on CI, [none] off it
languageStack languages evidenced by root manifests
Plus ruleCount on cli_check_completed, so a scan that loaded no rules is
distinguishable from one that loaded rules and found nothing.
repositoryId does NOT reuse canonicalizeGitHubUrl. That function throws
UNSUPPORTED_REMOTE_HOST deliberately as the capability boundary on remote
rule generation, and teaching it other hosts would soften a refusal doing
real work elsewhere. A second, host-agnostic parser sits beside it, so a
GitLab or self-hosted repository is counted as the codebase it is.
ghOwner stays unhashed: an owner is public identity and the value is
load-bearing precisely when legible. repositoryId is hashed because a
repository name can be an unannounced product. Neither hash is a secret and
the spec says so, since a remote URL is reversible by anyone who can
enumerate candidates. workspaceId is the one where hashing protects, local
paths carrying usernames and not being enumerable.
languageStack comes from a bounded root-only probe rather than
detectRepository, which is a recursive glob with manifest parsing and would
run on every invocation including agent fetches. It reads its mapping from
the scan's own LANGUAGE_MARKERS so the two cannot disagree about which
manifest means which language; they differ only in search scope. The cost
is that a language confined to a sub-package is not seen, which a test pins
so it is not "fixed" into a recursive walk.
Every resolution falls back to a sentinel rather than failing, and the
telemetry opt-out still short-circuits before any of it runs.
Single-PR delivery shape, so the spec, the implementation and the archive land together. Verified before archiving, on a scratch commit that was then reset: the standing analytics spec went from 40 scenarios to 57 and from 14 requirements to 17, with `comm` reporting NO scenario present before that is missing after. Both MODIFIED requirements kept their titles byte for byte, including the escaped underscore in `CLI events use cli\_ prefix` — a renamed title applies nothing at all and `validate --strict` passes either way, so the diff is the only thing that catches it.
|
@claude /review |
|
Claude finished @thecodedrift's task in 5m 45s —— View job Code ReviewReview mode: incremental — read 0 prior review thread(s) before reviewing.
Overall this is a well-scoped, well-documented change — the reasoning in the PR body for each design decision (host-agnostic repository parser kept separate from 1. 2. New sequential git spawns added to every telemetry-emitting command invocation ( 3. The new adoption dimensions are unit-tested in isolation but never verified as actually wired into 4. 5. Duplicated JSDoc block above 6. 7. (Minor) Branch: |
Five findings from the review on #326, all verified against the source before acting. repositoryId no longer collapses nested group paths. Keeping only the first and last segments made `acme/team1/api` and `acme/team2/api` one identity, silently undercounting distinct codebases in exactly the nested-group case the parser exists to serve. The original reasoning (a renamed group should not split one repository into two) was sound in one direction and missed the other; a rename is the milder failure and the visible one, and it is the same class of event as a GitHub transfer, which already changes the identity here. The dimensions are now tested where they are ATTACHED, not only where they are resolved. The wiring is two `...dimensions` spreads and had no coverage, so dropping one would have left every resolver test passing while no event carried a single new property. Confirmed by doing exactly that: removing the capture spread fails two of the new tests. resolveAdoptionDimensions handles an absent cwd itself instead of defaulting to process.cwd(), which contradicted the comment claiming it behaved like resolveScaffoldVersion and ghOwner — it resolved live git state instead. Only the three cwd-dependent dimensions sentinel; envOS, ci and ciProvider are properties of the process and stay real. scaffoldVersion, repositoryContext and the dimensions now resolve concurrently. They are independent, each spawns a subprocess or reads the filesystem, and getTelemetry runs on essentially every invocation. The JSDoc above LANGUAGE_MARKERS is one block again; the export note had been stacked above the original, orphaning it from the symbol. Declined: extracting a shared remote parser from canonicalOwnerUrl, which carries an explicit warning that it is a verbatim port compared with === and must stay byte-for-byte identical. And threading valeRuleIds down to save one readdir, which would couple the noRuleFiles gate's error semantics to a counting concern.
All seven findings verified against the source before acting. Five fixed in cf5bfa8, two declined with reasons. 1. 3. Untested wiring — fixed, and this was the important one. 4. Comment contradicted the code — fixed. 2. Sequential spawns — fixed. 5. Orphaned JSDoc — fixed. One block again. 6. Shared remote parser — declined. 7. Third Also tightened the spec's 1412 tests pass; typecheck and lint clean. — AI Coding Agent |
1 participant
Why
cli_runcounts invocations. It cannot count workspaces, and nearly every adoption metric worth reporting is per-workspace.The finest identity we carry today is
cli, the anonymous UUID in$XDG_CONFIG_HOME/taskless/anonymous_id. It is per config directory, so one developer across five repositories is onecli, and a fresh CI container regenerates the file every job. Both distortions push the same way — they inflate breadth and deflate depth — and neither can be repaired after the fact, because no property stored on past events separates a CI run from a human one.Retention cannot be backfilled either. A four-week number needs four weeks of a property that already exists, which is why this change is scoped to the dimensions and defers the analysis built on them.
What changes
Six super-properties on every identify and every capture, resolved once per process:
workspaceIdcwdrepositoryId{host}/{owner}/{repo}, host-agnostic,[unknown]when unresolvableenvOSprocess.platformciCIholds a positive valueciProvider[unknown]on CI,[none]off itlanguageStackPlus
ruleCountoncli_check_completed, so a scan that loaded no rules is distinguishable from one that loaded rules and found nothing.No command changes behaviour, no output changes shape.
Four decisions worth reviewing
workspaceIdanchors on the git top level, notcwd.checkfrompackages/cliandcheckfrom the repository root are one workspace, and they only report the same id if the path resolves upward first. Anchoring oncwdwould make every subdirectory its own workspace and inflate the count without bound. A test asserts that equality directly.repositoryIddoes NOT reusecanonicalizeGitHubUrl. That function throwsUNSUPPORTED_REMOTE_HOSTdeliberately — it is the capability boundary on remote rule generation — so teaching it other hosts would soften a refusal doing real work elsewhere. A second, host-agnostic parser sits beside it. A GitLab or self-hosted repository gets a realrepositoryIdwithghOwner: [unknown], which is the case a GitHub-shaped implementation gets wrong silently, so it has its own test.ghOwnerstays unhashed;repositoryIdis hashed. Different reasons. An owner is public identity and the value is load-bearing precisely when legible: excluding a known owner from external-adoption counts needs the name. A repository name can be an unannounced product. Neither hash is a secret and the spec says so, since a remote URL is reversible by anyone who can enumerate candidates.workspaceIdis the one where hashing genuinely protects — local paths carry usernames and are not enumerable.languageStackis a root-only probe, notdetectRepository. The scan is a recursive glob with manifest parsing; this runs on every invocation, including theagentfetches an agent makes repeatedly. It reads its mapping from the scan's ownLANGUAGE_MARKERSso the two cannot disagree about which manifest means which language — they differ only in search scope. The cost is that a language confined to a sub-package is not seen, and a test pins that so it is not later "fixed" into a recursive walk.One trap found while building it:
LANGUAGE_MARKERShas no JavaScript or TypeScript entry (the scan derives those frompackage.jsondependencies separately), so a probe built from the constant alone reports nothing for the stack this CLI is most used on.Verification
pnpm typecheck,pnpm lintclean;pnpm test1407 passed (was 1388).resolveWorkspaceRootignore the git top level fails the subdirectory-equality test; makingcanonicalRepositoryPathreject non-GitHub hosts fails 3 tests including the GitLab case.commreporting no scenario present before that is missing after, and both MODIFIED titles byte-identical.validate --strictpasses on a delta that drops scenarios, so the diff is the only thing that catches it.Deferred, with reasons in the proposal
cli_rule_verified(reverses the standing "verify rides oncli_runalone" decision), rule-resolution rate (needs stable finding identity — a materially larger privacy question), and the fact thatcli_installedfires oninitand so measures initialization rather than installation.Refs #325