From c3641c74b48bdbeaa3338243663e5cb475f142c3 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 9 Sep 2026 12:11:38 -0700 Subject: [PATCH 1/3] feat(cli): record adoption dimensions on every telemetry event 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. --- .changeset/telemetry-adoption-dimensions.md | 17 ++ .../.openspec.yaml | 2 + .../telemetry-adoption-dimensions/proposal.md | 119 ++++++++ .../specs/analytics/spec.md | 286 ++++++++++++++++++ .../telemetry-adoption-dimensions/tasks.md | 55 ++++ packages/cli/src/commands/check.ts | 26 +- packages/cli/src/detect/scan.ts | 12 +- packages/cli/src/telemetry.ts | 16 + packages/cli/src/util/adoption-dimensions.ts | 220 ++++++++++++++ packages/cli/src/util/git-remote.ts | 85 ++++++ packages/cli/test/adoption-dimensions.test.ts | 210 +++++++++++++ 11 files changed, 1045 insertions(+), 3 deletions(-) create mode 100644 .changeset/telemetry-adoption-dimensions.md create mode 100644 openspec/changes/telemetry-adoption-dimensions/.openspec.yaml create mode 100644 openspec/changes/telemetry-adoption-dimensions/proposal.md create mode 100644 openspec/changes/telemetry-adoption-dimensions/specs/analytics/spec.md create mode 100644 openspec/changes/telemetry-adoption-dimensions/tasks.md create mode 100644 packages/cli/src/util/adoption-dimensions.ts create mode 100644 packages/cli/test/adoption-dimensions.test.ts diff --git a/.changeset/telemetry-adoption-dimensions.md b/.changeset/telemetry-adoption-dimensions.md new file mode 100644 index 00000000..802d08b3 --- /dev/null +++ b/.changeset/telemetry-adoption-dimensions.md @@ -0,0 +1,17 @@ +--- +"@taskless/cli": patch +--- + +Telemetry now records six adoption dimensions on every event: `workspaceId` and +`repositoryId` (both hashed), `envOS`, `ci`, `ciProvider`, and `languageStack`. +`cli_check_completed` also reports `ruleCount`, so a scan that loaded no rules +is distinguishable from one that loaded rules and found nothing. + +Nothing to react to. No command changes behaviour, no output changes shape, and +every dimension falls back to a sentinel rather than failing — telemetry is not +a precondition for any command. `TASKLESS_TELEMETRY_DISABLED=1` and +`DO_NOT_TRACK=1` continue to short-circuit before any of it is resolved, so the +opt-out remains an opt-out of the work rather than only of the send. + +`patch` rather than `minor` because the package is pre-1.0, where added surface +does not earn a `minor`, and because none of this is API a consumer can call. diff --git a/openspec/changes/telemetry-adoption-dimensions/.openspec.yaml b/openspec/changes/telemetry-adoption-dimensions/.openspec.yaml new file mode 100644 index 00000000..7a8e2be6 --- /dev/null +++ b/openspec/changes/telemetry-adoption-dimensions/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-08 diff --git a/openspec/changes/telemetry-adoption-dimensions/proposal.md b/openspec/changes/telemetry-adoption-dimensions/proposal.md new file mode 100644 index 00000000..b6e036e9 --- /dev/null +++ b/openspec/changes/telemetry-adoption-dimensions/proposal.md @@ -0,0 +1,119 @@ +## Why + +`cli_run` counts invocations. It cannot count _workspaces_, and almost every +adoption metric worth reporting is per-workspace rather than per-invocation. + +The gap is structural, not a matter of tuning dashboards. Today the finest +identity we carry is `cli` — the anonymous UUID in +`$XDG_CONFIG_HOME/taskless/anonymous_id`. It is per config directory, so: + +- one developer working across five repositories is one `cli`, and their five + projects are indistinguishable from five runs in one; +- a fresh CI container regenerates the file on every job, so each CI run looks + like a brand-new install. + +Both distortions push the same direction — they inflate breadth and deflate +depth — and neither is measurable after the fact. There is no property stored on +past events that separates a CI run from a human one, which means the existing +series cannot be cleaned retroactively, only replaced going forward. + +**Retention cannot be backfilled.** A four-week retention number needs four +weeks of a property that already exists. Every week the dimensions are absent is +a week the cohort clock is not running, which is why this change is scoped to +the dimensions and defers the analysis built on top of them. + +## What Changes + +Five new super-properties on every captured event, and one count added to an +existing event. + +**`workspaceId`** — a SHA-256 hash of the workspace root's absolute path, where +the root is the git top-level when there is one and the resolved working +directory otherwise. Anchoring on the top-level is what makes it a workspace +identifier rather than a directory identifier: `check` run from `packages/cli` +and from the repository root must report the same workspace, and they only do if +the path is resolved upward first. + +**`repositoryId`** — a SHA-256 hash of a canonical `{host}/{owner}/{repo}` +derived from the `origin` remote, and **not GitHub-specific**. `ghOwner` is +GitHub-only by construction, because it exists to answer a GitHub question. A +repository identifier answers "how many distinct codebases", which a GitLab or +self-hosted repository participates in exactly as much as a GitHub one, so +excluding them would understate deployment breadth and do it silently. Where no +remote resolves, the sentinel `[unknown]` is sent, matching `ghOwner`'s existing +treatment. + +**`envOS`** — `process.platform`. + +**`ci`** — a boolean, true when `process.env.CI` holds a positive value. + +**`ciProvider`** — the detected provider, `[unknown]` when `ci` is true but no +provider is recognized, `[none]` when `ci` is false. + +**`languageStack`** — the languages evidenced by manifest files at the workspace +root, so the rule corpus can be prioritized against the stacks that actually run +it. + +**`ruleCount` on `cli_check_completed`** — the number of rules the scan had +loaded. The event reports findings but not how many rules were live, so a scan +with zero findings and a scan with zero rules are the same event today. That +also blocks the two ratios that ask whether authored rules become recurring +infrastructure, which is the product's central claim. + +### Two decisions worth stating rather than assuming + +**`ghOwner` stays unhashed.** A GitHub owner is public identity, and the value +is load-bearing precisely because it is legible: excluding `taskless` from +external-owner counts, and eyeballing a cohort for plausibility, both need the +name. Hashing it would buy no privacy that matters and cost the metric its +usefulness. + +**`repositoryId` is hashed, and the reason is not the same reason.** A repository +_name_ can be an unannounced product; an owner name generally cannot. That is +the line: owner legible, repository not. + +Neither hash is a secret, and the spec says so, because a hash of a public +repository URL is reversible by anyone who can enumerate candidate URLs. It is a +stable pseudonym. `workspaceId` is different in kind — local absolute paths +contain usernames and are not enumerable — so hashing there is genuinely +protective. Recording which is which prevents a later reader from assuming a +guarantee that was never made. + +## Capabilities + +### Modified Capabilities + +- `analytics`: the standard-properties requirement covers five new dimensions; + the taxonomy requirement adds `ruleCount` to `cli_check_completed`. + +### Added Capabilities + +- `analytics`: workspace and repository identity, execution environment + dimensions, and the language stack dimension, each as its own requirement, so + the hashing boundary and the sentinel treatment are stated where they apply. + +## Impact + +- `openspec/specs/analytics/spec.md` — two requirements amended, three added. +- `packages/cli/src/telemetry.ts` — resolves and attaches the new super-properties. +- `packages/cli/src/util/git-remote.ts` — gains a host-agnostic repository + canonicalization beside the existing GitHub-only one. +- `packages/cli/src/detect/scan.ts` — `LANGUAGE_MARKERS` is exported for reuse. +- `packages/cli/src/commands/check.ts` — `scanCounts` gains `ruleCount`. +- No command behavior changes, and no failure path is added: every resolution + falls back to a sentinel, consistent with the existing rule that telemetry is + never a precondition. + +## Deferred, deliberately + +- **`cli_rule_verified`.** Fixture-verification counts would need a new event on + `rule verify`, which reverses the standing decision that verify rides on + `cli_run` alone. It serves one lower-tier metric and is not worth reopening + that decision inside a change whose value is time-sensitive. +- **Rule-resolution rate.** Deriving "a finding was fixed" needs stable finding + identity across runs — rule, file, line — which is a materially larger privacy + question than anything here. It should be proposed on its own. +- **`cli_installed` is misnamed.** It fires on `init` and the wizard, so it + measures initialization, not installation. Renaming it is a separate change; + until then, install counts should be derived as the first `cli_run` per `cli` + id rather than from the event that sounds like it. diff --git a/openspec/changes/telemetry-adoption-dimensions/specs/analytics/spec.md b/openspec/changes/telemetry-adoption-dimensions/specs/analytics/spec.md new file mode 100644 index 00000000..fc75c418 --- /dev/null +++ b/openspec/changes/telemetry-adoption-dimensions/specs/analytics/spec.md @@ -0,0 +1,286 @@ +## ADDED Requirements + +### Requirement: Workspace and repository identity + +Telemetry SHALL attach a `workspaceId` and a `repositoryId` property to every +identify and every captured event. + +`workspaceId` SHALL be the SHA-256 hash, hex-encoded, of the absolute path of +the workspace root. The workspace root SHALL be the git top-level directory when +the working directory sits inside a git working tree, and the resolved working +directory otherwise. Resolving upward to the top-level is what makes the value a +workspace identifier rather than a directory identifier: an invocation from a +subdirectory SHALL report the same `workspaceId` as one from the root. + +`repositoryId` SHALL be the SHA-256 hash, hex-encoded, of a canonical +`{host}/{owner}/{repo}` string derived from the `origin` remote, lowercased, +with any `.git` suffix and trailing slash removed and any userinfo, port, query, +and fragment discarded. It SHALL NOT be restricted to GitHub remotes: a GitLab, +Bitbucket, or self-hosted repository SHALL receive a `repositoryId` on the same +terms. Where no `origin` remote resolves — for any reason, including a directory +that is not a repository and a host where `git` is unavailable — `repositoryId` +SHALL be the literal sentinel `[unknown]`, present rather than omitted, so those +runs stay countable. + +Neither hash SHALL be described or relied upon as a secret. `repositoryId` is a +stable pseudonym: a hash of a remote URL is reversible by anyone who can +enumerate candidate URLs. It is hashed because a repository NAME can be an +unannounced product, which is a different question from whether the value is +confidential. `workspaceId` hashes a local absolute path, which commonly contains +a username and is not enumerable, so hashing there is protective in a way that +hashing `repositoryId` is not. + +`ghOwner` SHALL remain unhashed. A GitHub owner is public identity, and the +value is load-bearing precisely because it is legible: excluding a known owner +from external-adoption counts, and judging a cohort for plausibility, both +require the name. The boundary is therefore owner legible, repository not. + +Resolution SHALL NOT fail a command. Every unresolvable case SHALL produce a +value, never an exception. + +#### Scenario: Invocation from a subdirectory reports the workspace root + +- **WHEN** a command runs in a subdirectory of a git working tree +- **THEN** `workspaceId` SHALL be the hash of the git top-level path +- **AND** SHALL equal the `workspaceId` reported by the same command run at the root + +#### Scenario: Working directory is not a git working tree + +- **WHEN** a command runs outside any git working tree +- **THEN** `workspaceId` SHALL be the hash of the resolved working directory + +#### Scenario: Non-GitHub remote still yields a repository identity + +- **WHEN** a command runs in a repository whose `origin` is hosted somewhere + other than GitHub +- **THEN** `repositoryId` SHALL be a hash of that repository's canonical + `{host}/{owner}/{repo}` +- **AND** `ghOwner` SHALL be `[unknown]`, since the GitHub-owner question has no + answer for that remote + +#### Scenario: No origin remote + +- **WHEN** a command runs in a directory with no resolvable `origin` remote +- **THEN** `repositoryId` SHALL be `[unknown]` +- **AND** the property SHALL be present rather than omitted + +#### Scenario: The same repository cloned twice + +- **WHEN** the same repository is cloned to two paths on one machine +- **THEN** the two clones SHALL report the same `repositoryId` +- **AND** SHALL report different `workspaceId` values + +#### Scenario: The GitHub owner is not hashed + +- **WHEN** `ghOwner` and `repositoryId` are recorded for the same GitHub repository +- **THEN** `ghOwner` SHALL be the owner segment verbatim +- **AND** `repositoryId` SHALL be a hash + +### Requirement: Execution environment dimensions + +Telemetry SHALL attach `envOS`, `ci`, and `ciProvider` properties to every +identify and every captured event. + +`envOS` SHALL be the value of `process.platform`. + +`ci` SHALL be a boolean, true when the `CI` environment variable is set to a +positive value. Unset, empty, `"0"`, and `"false"` SHALL each be treated as +false; any other non-empty value SHALL be treated as true. + +`ciProvider` SHALL name the detected continuous-integration provider. When `ci` +is true and no provider is recognized, it SHALL be the literal sentinel +`[unknown]`. When `ci` is false, it SHALL be the literal sentinel `[none]`. Both +SHALL be present rather than omitted, so unrecognized and non-CI runs remain +countable and remain distinguishable from each other. + +These dimensions exist because the anonymous identity cannot separate automated +runs from human ones. `$XDG_CONFIG_HOME/taskless/anonymous_id` is regenerated in +a fresh container, so without `ci` every CI job is indistinguishable from a new +install, and no property stored on past events can repair that after the fact. + +#### Scenario: Local run + +- **WHEN** a command runs with `CI` unset +- **THEN** `ci` SHALL be false +- **AND** `ciProvider` SHALL be `[none]` + +#### Scenario: CI run on a recognized provider + +- **WHEN** a command runs under a recognized CI provider +- **THEN** `ci` SHALL be true +- **AND** `ciProvider` SHALL name that provider + +#### Scenario: CI run on an unrecognized provider + +- **WHEN** a command runs with `CI` set to a positive value and no known + provider environment variable present +- **THEN** `ci` SHALL be true +- **AND** `ciProvider` SHALL be `[unknown]` + +#### Scenario: CI is set to a negative value + +- **WHEN** a command runs with `CI` set to `"0"`, `"false"`, or the empty string +- **THEN** `ci` SHALL be false + +### Requirement: Language stack dimension + +Telemetry SHALL attach a `languageStack` property to every identify and every +captured event, listing the languages evidenced by manifest files present at the +workspace root. + +The property exists to prioritize the rule corpus against the stacks that +actually run it, so its value is a coarse dimension rather than a detection +result. + +It SHALL be resolved from a bounded, root-only probe and SHALL NOT invoke +`detectRepository`. The detection scan performs a recursive walk with manifest +parsing, which is acceptable for a command the user asked for and is not +acceptable on every invocation, including the `agent` fetches an agent makes +repeatedly. + +The probe SHALL read its language-to-manifest mapping from the same +`LANGUAGE_MARKERS` constant the detection scan uses, extended with the Node +manifest that the scan derives JavaScript and TypeScript from. The two SHALL +differ only in search scope. This is stated so a future reader does not +reconcile them by making the telemetry probe recursive: a root-only probe misses +a language confined to a sub-package of a monorepo, and that is the accepted +cost of the property being free. + +Where no manifest is found, `languageStack` SHALL be an empty array rather than +omitted. + +#### Scenario: Root manifests are reported + +- **WHEN** a command runs in a workspace whose root contains a `package.json` + and a `go.mod` +- **THEN** `languageStack` SHALL include the languages both manifests evidence + +#### Scenario: No manifests present + +- **WHEN** a command runs in a workspace root with no recognized manifest +- **THEN** `languageStack` SHALL be an empty array +- **AND** the property SHALL be present rather than omitted + +#### Scenario: The probe does not run the detection scan + +- **WHEN** telemetry resolves `languageStack` +- **THEN** it SHALL NOT call `detectRepository` + +#### Scenario: A language confined to a sub-package + +- **WHEN** a monorepo's root carries only a `package.json` and a Python service + lives in a sub-directory +- **THEN** `languageStack` SHALL report the root evidence only +- **AND** this SHALL NOT be treated as a defect in the property + +## MODIFIED Requirements + +### Requirement: All capture calls include standard properties + +Every `capture()` call SHALL include the `cli` property (anonymous UUID), the `cliVersion` property (the version the build reports as its own, baked in at build time), and the `scaffoldVersion` property (the `version` field from `.taskless/taskless.json`, or `0` if the manifest is absent or unreadable). It SHALL also include the adoption dimensions `workspaceId`, `repositoryId`, `envOS`, `ci`, `ciProvider`, and `languageStack`, each defined by its own requirement. When authenticated, the `groups` parameter SHALL include `{ organization: String(orgId) }`. The `cliVersion` and `scaffoldVersion` values SHALL be resolved once at telemetry initialization and attached to every subsequent `capture()` call without re-reading the source files, and the adoption dimensions SHALL be resolved once on the same terms. + +Resolving the adoption dimensions SHALL NOT be a precondition for any command. Each one has a defined value for every failure of resolution, so a capture never has to choose between omitting a property and failing. + +#### Scenario: Anonymous capture includes standard properties + +- **WHEN** `capture("cli_run")` is called without authentication +- **THEN** the event SHALL include `{ cli: anonymousUuid, cliVersion: , scaffoldVersion: }` +- **AND** the event SHALL NOT include a `groups` parameter + +#### Scenario: Authenticated capture includes standard properties and group + +- **WHEN** `capture("cli_rule_created")` is called with authentication +- **THEN** the event SHALL include `{ cli: anonymousUuid, cliVersion: , scaffoldVersion: }` +- **AND** the `groups` parameter SHALL include `{ organization: String(orgId) }` + +#### Scenario: Scaffold version falls back to 0 when manifest missing + +- **WHEN** `getTelemetry(cwd)` is initialized in a directory with no `.taskless/taskless.json` +- **THEN** every `capture()` call from the returned client SHALL include `scaffoldVersion: 0` + +#### Scenario: CLI version is the version the build reports as its own + +- **WHEN** `getTelemetry()` is initialized +- **THEN** `cliVersion` SHALL be the version the build reports as its own, bundled at build time +- **AND** for every build target but `nightly` that SHALL be the version in `packages/cli/package.json` +- **AND** for a `nightly` it SHALL be the version the nightly is published under, so events are attributed to the build that emitted them rather than to the release it anticipates +- **AND** SHALL be attached to every event emitted through the returned client + +#### Scenario: Every event carries the adoption dimensions + +- **WHEN** any event is captured, authenticated or not +- **THEN** it SHALL include `workspaceId`, `repositoryId`, `envOS`, `ci`, `ciProvider`, and `languageStack` + +#### Scenario: The dimensions are resolved once + +- **WHEN** several events are captured within one invocation +- **THEN** each adoption dimension SHALL be resolved once at initialization +- **AND** SHALL NOT be re-read per event + +### Requirement: CLI events use cli\_ prefix + +CLI events SHALL use the `cli_` prefix, with the taxonomy organized as a +`cli_run` denominator plus concrete state-transition events: + +- `cli_run` — exactly one per invocation (see the dedicated requirement). This + replaces every previous `cli_` start event and `cli__completed` + event; the `success`/`durationMs`/`command` signal lives here. +- Concrete state-transition events, each fired at the point the state actually + changes, carrying counts/ids/booleans only (never rule content, prompts, or + matched source): + - `cli_rule_created`, `cli_rule_improved`, `cli_rule_deleted` + - `cli_authenticated`, `cli_logged_out` + - `cli_installed`, `cli_onboarded` + - `cli_check_completed` — error/warning counts and the number of rules the + scan had loaded: `errorCount`, `warningCount`, `findings`, `ruleCount`. + Counts only, never rule content, rule names, or matched source + - `cli_error` — a single failure event with `command` and `code` (a stable + `CLIErrorCode`) +- `cli_agent` — fired when the `agent` command serves a request, with a `topic` + property (the served topic; the exact literal `"(index)"` when invoked with no + topic; the attempted topic for an unknown request). This replaces the previous + `help_index`, `help_`, and `help_unknown` events. + +Commands that carry no concrete state beyond the invocation (e.g. `info`, +`detect`, `update`, `auth status`, `rule verify`, `rule meta`) SHALL rely on +`cli_run` alone and SHALL NOT emit a bespoke event. The previous taxonomy +(`cli_`, `cli__completed`, `help_index`, `help_`, +`help_unknown`) SHALL be removed in this release; there is no dual-emit window. + +#### Scenario: Rule creation emits a concrete state event plus cli_run + +- **WHEN** a user runs `taskless rule create --from req.json` and a rule is written +- **THEN** PostHog SHALL receive one `cli_run` event with `command: "rule create"` +- **AND** SHALL receive a `cli_rule_created` event +- **AND** SHALL NOT receive `cli_rule_create` or `cli_rule_create_completed` + +#### Scenario: Recipe fetch emits cli_agent with a topic + +- **WHEN** an agent runs `taskless agent create-sg-rule` +- **THEN** PostHog SHALL receive a `cli_agent` event with `topic: "create-sg-rule"` +- **AND** SHALL NOT receive a `help_create_sg_rule` event + +#### Scenario: Fetch with no topic emits cli_agent with the index marker + +- **WHEN** an agent runs `taskless agent` +- **THEN** PostHog SHALL receive a `cli_agent` event with `topic: "(index)"` +- **AND** SHALL NOT receive a `help_index` event + +#### Scenario: A command failure emits cli_error + +- **WHEN** a command fails with a known `CLIErrorCode` +- **THEN** PostHog SHALL receive a `cli_error` event with `command` and `code` + +#### Scenario: Old event names are not emitted + +- **WHEN** any CLI command runs in this release +- **THEN** PostHog SHALL NOT receive any event named `cli__completed`, + `help_index`, `help_`, or `help_unknown` + +#### Scenario: A completed scan reports how many rules were loaded + +- **WHEN** a scan completes +- **THEN** the `cli_check_completed` event SHALL include `ruleCount`, the number + of rules the scan had loaded +- **AND** a scan that loaded no rules SHALL be distinguishable from a scan that + loaded rules and found nothing diff --git a/openspec/changes/telemetry-adoption-dimensions/tasks.md b/openspec/changes/telemetry-adoption-dimensions/tasks.md new file mode 100644 index 00000000..939ec2ec --- /dev/null +++ b/openspec/changes/telemetry-adoption-dimensions/tasks.md @@ -0,0 +1,55 @@ +# Tasks + +**Delivery shape: single PR.** The dimensions are only useful together — a +`workspaceId` without `ci` produces workspace counts that CI still inflates, and +shipping them in sequence would start each cohort clock on a different date, so +the first comparable window would be the last property's. One reviewable diff, +no stack. + +## 1. Resolve workspace and repository identity + +- [x] 1.1 Add a workspace-root resolver: `git rev-parse --show-toplevel`, falling back to the resolved working directory when the command fails for any reason, including git being absent. Never throws — the existing `resolveRepositoryContext` is the shape to follow +- [x] 1.2 Hash the root path with SHA-256, hex-encoded, as `workspaceId`. Assert in a test that a subdirectory and the root produce the same value, since that equality is the whole reason the resolver looks upward +- [x] 1.3 Add a host-agnostic repository canonicalization beside `canonicalizeGitHubUrl`. **Do not extend the GitHub one**: it throws `UNSUPPORTED_REMOTE_HOST` deliberately, and that refusal is a capability boundary on remote rule generation that this change must not soften +- [x] 1.4 Cover the remote forms `canonicalOwnerUrl` already handles — scp-like SSH, `ssh://`, `git://`, `https://`, bare owner — at repository granularity rather than owner granularity +- [x] 1.5 Hash it as `repositoryId`; emit `[unknown]` when no remote resolves, present rather than omitted +- [x] 1.6 Test that a GitLab or self-hosted remote yields a real `repositoryId` while `ghOwner` is `[unknown]`. That combination is the point of the property and is the case a GitHub-shaped implementation silently gets wrong + +## 2. Resolve the execution environment + +- [x] 2.1 Add `envOS` from `process.platform` +- [x] 2.2 Add `ci`, treating unset, empty, `"0"`, and `"false"` as false and any other non-empty value as true. `init.ts` already reads `process.env.CI` for interactivity with a narrower test (`"true"`/`"1"`); leave it alone and note the difference — one decides whether to prompt, the other classifies a run, and they are allowed to disagree +- [x] 2.3 Add `ciProvider` with a provider table, `[unknown]` when `ci` is true and nothing matches, `[none]` when `ci` is false +- [x] 2.4 Test all three branches of `ciProvider`. The `[unknown]`/`[none]` distinction is the one that matters: collapsing them loses the ability to tell an unrecognized provider from a local run + +## 3. Resolve the language stack + +- [x] 3.1 Export `LANGUAGE_MARKERS` from `detect/scan.ts` and read the probe's mapping from it, so the two cannot disagree about which manifest means which language +- [x] 3.2 Add the Node manifest to the probe's inputs. `LANGUAGE_MARKERS` has no entry for JavaScript or TypeScript — the scan derives those from `package.json` separately — so a probe built from the constant alone reports nothing for the stack the CLI is most used on +- [x] 3.3 Probe the workspace root only, with `existsSync`, and assert in a test that `detectRepository` is not called. The scan is a recursive walk with manifest parsing, and this runs on every invocation including `agent` +- [x] 3.4 Emit an empty array, not an omitted property, when nothing matches + +## 4. Attach the dimensions + +- [x] 4.1 Resolve all six once in `getTelemetry` and attach them to `identify` and to every `capture`, alongside `cli`/`cliVersion`/`scaffoldVersion`/`ghOwner` +- [x] 4.2 Keep every resolution inside the existing failure-tolerant path: a dimension that cannot be resolved yields its sentinel, and telemetry that fails entirely still falls back to the no-op client +- [x] 4.3 Confirm `TASKLESS_TELEMETRY_DISABLED=1` and `DO_NOT_TRACK=1` still short-circuit before any resolution runs — no git spawn, no filesystem probe, no `anonymous_id` read. The opt-out has to stay an opt-out of the work, not only of the send + +## 5. Report the rule count + +- [x] 5.1 Add `ruleCount` to `scanCounts` in `check.ts`, set from the rules the scan loaded +- [x] 5.2 Set it on the path that assigns `scanCounts` today, so a scan that throws after loading rules keeps reporting nothing rather than reporting a partial count + +## 6. Say so in the spec + +- [x] 6.1 Add the three new requirements: workspace and repository identity, execution environment dimensions, language stack dimension +- [x] 6.2 Amend the standard-properties requirement to list the six dimensions +- [x] 6.3 Amend the taxonomy requirement's `cli_check_completed` bullet to include `ruleCount` +- [x] 6.4 Carry every existing scenario into both MODIFIED deltas — 4 in standard-properties, 5 in the taxonomy requirement — and keep both TITLES byte-identical, including the escaped underscore in `CLI events use cli\_ prefix`. A delta replaces a requirement rather than patching it, and a renamed title applies nothing at all +- [ ] 6.5 Verify by archiving on a scratch commit and grepping the standing spec for every prior scenario, then resetting to the recorded SHA + +## 7. Close out + +- [x] 7.1 Run `pnpm typecheck`, `pnpm lint`, and `pnpm test` +- [x] 7.2 Add a changeset. Pre-1.0, added telemetry surface is a `patch`: no consumer must react to it +- [ ] 7.3 Archive the change diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 618738f9..4f237aa7 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -124,7 +124,12 @@ export const checkCommand = defineCommand({ // Set when a scan actually runs; drives cli_check_completed with counts // only (never matched code). let scanCounts: - | { errorCount: number; warningCount: number; findings: number } + | { + errorCount: number; + warningCount: number; + findings: number; + ruleCount: number; + } | undefined; try { const positionalPaths = extractPositionalPaths(rawArgs); @@ -269,7 +274,24 @@ export const checkCommand = defineCommand({ if (result.severity === "error") errorCount++; else if (result.severity === "warning") warningCount++; } - scanCounts = { errorCount, warningCount, findings: results.length }; + // `ruleCount` is how many rules the scan LOADED, across all three + // engines. Without it a scan with no findings and a scan with no rules + // are the same event, which is exactly the pair the metrics need to + // tell apart. Runtime rules the plan skipped still count: the question + // is how many rules this workspace has configured, not how many + // executed on this run. + // + // `listRuleIds` swallows its own read errors and returns `[]`, so this + // cannot turn an unreadable directory into a failed scan. A telemetry + // count must never be the thing that fails a command. + const valeRuleIds = await listRuleIds(cwd, "vale"); + scanCounts = { + errorCount, + warningCount, + findings: results.length, + ruleCount: + astGrepRuleIds.length + valeRuleIds.length + runtimeRules.length, + }; // Computed by `runEngines`, not here: the exit code is a fact about a // completed dispatch, and an engine failure has to fail the check even diff --git a/packages/cli/src/detect/scan.ts b/packages/cli/src/detect/scan.ts index 6c019a05..c0d94241 100644 --- a/packages/cli/src/detect/scan.ts +++ b/packages/cli/src/detect/scan.ts @@ -232,7 +232,17 @@ const LINTER_SIGNALS: readonly LinterSignal[] = [ * the tree. JavaScript and TypeScript are resolved separately (they share * `package.json`). */ -const LANGUAGE_MARKERS: ReadonlyArray<{ language: string; files: string[] }> = [ +/** + * Exported so telemetry's root-only `languageStack` probe reads the same + * language-to-manifest mapping this scan does. The two differ only in search + * scope: this walks the tree, the probe checks the workspace root. Sharing the + * mapping keeps them from disagreeing about which manifest means which + * language; the scope difference is deliberate and documented there. + */ +export const LANGUAGE_MARKERS: ReadonlyArray<{ + language: string; + files: string[]; +}> = [ { language: "Python", files: [ diff --git a/packages/cli/src/telemetry.ts b/packages/cli/src/telemetry.ts index ee7dacc9..14fab3fc 100644 --- a/packages/cli/src/telemetry.ts +++ b/packages/cli/src/telemetry.ts @@ -6,6 +6,7 @@ import { PostHog } from "posthog-node"; import { decodeJwt } from "jose"; import { decodeOrgId, NIL_ORG_ID } from "./auth/jwt"; +import { resolveAdoptionDimensions } from "./util/adoption-dimensions"; import { resolveRepositoryContext, UNKNOWN_GH_OWNER } from "./util/git-remote"; import { getConfigDirectory, getToken } from "./auth/token"; import { CLI_VERSION } from "./version"; @@ -187,6 +188,19 @@ export async function getTelemetry(cwd?: string): Promise { const repository = cwd ? await resolveRepositoryContext(cwd) : undefined; const ghOwner = repository ? repository.ghOwner : UNKNOWN_GH_OWNER; + // The adoption dimensions: workspace and repository identity, execution + // environment, and language stack. Resolved ONCE here, like cliVersion and + // scaffoldVersion above, and attached to identify and to every capture. + // + // Everything below this point runs only when telemetry is enabled: the + // opt-out returns the no-op client before `getTelemetry` reaches here, so + // `DO_NOT_TRACK=1` costs no git spawn and no filesystem probe. The opt-out + // has to be an opt-out of the WORK, not only of the send. + // + // No `cwd` is treated as no workspace, matching `resolveScaffoldVersion` + // and `ghOwner` above; every real call site passes one. + const dimensions = await resolveAdoptionDimensions(cwd ?? process.cwd()); + posthog = new PostHog(POSTHOG_PROJECT_TOKEN, { host: POSTHOG_HOST, flushAt: 1, @@ -201,6 +215,7 @@ export async function getTelemetry(cwd?: string): Promise { cliVersion: CLI_VERSION, scaffoldVersion, ghOwner, + ...dimensions, }, }); @@ -225,6 +240,7 @@ export async function getTelemetry(cwd?: string): Promise { cliVersion: CLI_VERSION, scaffoldVersion, ghOwner, + ...dimensions, }, ...(!anonymous && orgSubject !== undefined ? { groups: { organization: String(orgSubject) } } diff --git a/packages/cli/src/util/adoption-dimensions.ts b/packages/cli/src/util/adoption-dimensions.ts new file mode 100644 index 00000000..3179ada5 --- /dev/null +++ b/packages/cli/src/util/adoption-dimensions.ts @@ -0,0 +1,220 @@ +import { execFile } from "node:child_process"; +import { createHash } from "node:crypto"; +import { existsSync } from "node:fs"; +import { resolve } from "node:path"; + +import { LANGUAGE_MARKERS } from "../detect/scan"; +import { resolveRepositoryPath } from "./git-remote"; + +/** + * The value a dimension carries when it cannot be resolved. + * + * A sentinel rather than an omitted property, matching `UNKNOWN_GH_OWNER`: + * runs that cannot resolve a dimension stay countable instead of vanishing + * from aggregates. Hashes are hex, so no real value can collide with it. + */ +export const UNKNOWN_DIMENSION = "[unknown]"; + +/** `ciProvider` when the run is not CI at all, as distinct from unrecognized. */ +export const NO_CI_PROVIDER = "[none]"; + +/** Hex SHA-256. Shared so `workspaceId` and `repositoryId` cannot diverge. */ +function hashIdentity(value: string): string { + return createHash("sha256").update(value).digest("hex"); +} + +/** + * The absolute path of the workspace root: the git top-level when `cwd` is + * inside a working tree, otherwise `cwd` resolved. + * + * Resolving upward is the entire reason this exists. `check` run from + * `packages/cli` and `check` run from the repository root are the same + * workspace, and they only report the same `workspaceId` if the path is taken + * to the top level first. Anchoring on `cwd` would make every subdirectory its + * own workspace and inflate the count without bound. + * + * Asks git rather than looking for a `.git` directory, for the reason + * `isGitWorkTree` gives: a worktree, a submodule and a `GIT_DIR` override are + * all real working trees with no `.git` directory at `cwd`. + * + * Never rejects. + */ +export function resolveWorkspaceRoot(cwd: string): Promise { + return new Promise((resolvePromise) => { + execFile( + "git", + ["rev-parse", "--show-toplevel"], + { cwd }, + (error, stdout) => { + const top = stdout.trim(); + resolvePromise(error || !top ? resolve(cwd) : resolve(top)); + } + ); + }); +} + +/** + * A stable identifier for the workspace, hashed. + * + * Hashing here is genuinely protective, unlike in `resolveRepositoryId` below: + * a local absolute path commonly contains a username, and the space of local + * paths is not enumerable, so the hash cannot be walked back to its input. + * + * Two clones of one repository report DIFFERENT values, which is correct — + * they are two workspaces — while `repositoryId` reports the same for both. + * That split is what lets "how many checkouts" and "how many codebases" be + * counted separately. + */ +export async function resolveWorkspaceId(cwd: string): Promise { + return hashIdentity(await resolveWorkspaceRoot(cwd)); +} + +/** + * A stable identifier for the repository, hashed, or `[unknown]`. + * + * The hash is NOT a secret and must not be described as one: a remote URL is + * reversible by anyone who can enumerate candidate URLs. It is hashed because + * a repository NAME can be an unannounced product, which is a different + * question from confidentiality. `ghOwner` stays unhashed alongside it, + * because 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. Owner legible, repository not. + */ +export async function resolveRepositoryId(cwd: string): Promise { + const path = await resolveRepositoryPath(cwd); + return path ? hashIdentity(path) : UNKNOWN_DIMENSION; +} + +/** + * Whether `CI` holds a positive value. + * + * Unset, empty, `"0"` and `"false"` are false; any other non-empty value is + * true. This is DELIBERATELY wider than `init.ts`'s interactivity check, which + * accepts only `"true"`/`"1"`. The two answer different questions — one + * decides whether to prompt a human, this one classifies a run for analytics — + * and a provider exporting `CI=yes` should count as CI even where erring + * toward prompting would be wrong. They are allowed to disagree. + */ +export function isContinuousIntegration( + environment: NodeJS.ProcessEnv = process.env +): boolean { + const value = environment.CI?.trim().toLowerCase(); + if (!value) return false; + return value !== "0" && value !== "false"; +} + +/** + * Environment variables that name a CI provider, most specific first. + * + * Ordered because providers nest: Codespaces and many self-hosted setups also + * export `CI`, and GitHub Actions sets both `GITHUB_ACTIONS` and `CI`. The + * first match wins, so a more specific marker is never shadowed by a generic + * one. + */ +const CI_PROVIDERS: ReadonlyArray<{ variable: string; name: string }> = [ + { variable: "GITHUB_ACTIONS", name: "github_actions" }, + { variable: "GITLAB_CI", name: "gitlab_ci" }, + { variable: "CIRCLECI", name: "circleci" }, + { variable: "BUILDKITE", name: "buildkite" }, + { variable: "TRAVIS", name: "travis" }, + { variable: "APPVEYOR", name: "appveyor" }, + { variable: "DRONE", name: "drone" }, + { variable: "TEAMCITY_VERSION", name: "teamcity" }, + { variable: "BITBUCKET_BUILD_NUMBER", name: "bitbucket_pipelines" }, + { variable: "TF_BUILD", name: "azure_pipelines" }, + { variable: "CODEBUILD_BUILD_ID", name: "aws_codebuild" }, + { variable: "JENKINS_URL", name: "jenkins" }, + { variable: "WOODPECKER", name: "woodpecker" }, + { variable: "VERCEL", name: "vercel" }, + { variable: "NETLIFY", name: "netlify" }, +]; + +/** + * The detected CI provider, `[unknown]` on CI with no recognized marker, or + * `[none]` off CI. + * + * Two sentinels rather than one, because collapsing them loses the ability to + * tell "a provider we have not taught this list about" from "a laptop". The + * first is a gap in the table worth closing; the second is the ordinary case + * and needs no action. + */ +export function resolveCiProvider( + environment: NodeJS.ProcessEnv = process.env +): string { + if (!isContinuousIntegration(environment)) return NO_CI_PROVIDER; + const matched = CI_PROVIDERS.find( + ({ variable }) => (environment[variable] ?? "").trim() !== "" + ); + return matched ? matched.name : UNKNOWN_DIMENSION; +} + +/** + * Node's manifest, which `LANGUAGE_MARKERS` has no entry for. + * + * The detection scan derives JavaScript and TypeScript from `package.json` + * dependencies rather than from a marker, so a probe built from the shared + * constant alone reports NOTHING for the stack this CLI is most used on. Added + * here rather than to `LANGUAGE_MARKERS` itself, since the scan already has a + * richer answer for Node and would then have two. + */ +const NODE_MARKER = { + language: "JavaScript/TypeScript", + files: ["package.json"], +}; + +/** + * The languages evidenced by manifest files AT THE WORKSPACE ROOT. + * + * Deliberately not `detectRepository`. That performs a recursive glob with + * manifest parsing, which is fine for a command the user asked for and is not + * fine on every invocation — including the `agent` fetches an agent makes + * repeatedly. This is a bounded `existsSync` per marker. + * + * The mapping comes from the scan's own `LANGUAGE_MARKERS` so the two cannot + * disagree about which manifest means which language; they differ only in + * search scope. That difference has a real cost: a language confined to a + * sub-package of a monorepo does not appear here. It is accepted, because this + * is a coarse telemetry dimension rather than a detection result, and it is + * the price of the property being free. Do not "fix" it by making this + * recursive. + */ +export function resolveLanguageStack(workspaceRoot: string): string[] { + const languages: string[] = []; + for (const marker of [NODE_MARKER, ...LANGUAGE_MARKERS]) { + if (marker.files.some((file) => existsSync(resolve(workspaceRoot, file)))) { + languages.push(marker.language); + } + } + return languages; +} + +/** Every adoption dimension, resolved once per process by `getTelemetry`. */ +export interface AdoptionDimensions { + workspaceId: string; + repositoryId: string; + envOS: string; + ci: boolean; + ciProvider: string; + languageStack: string[]; +} + +/** + * Resolve all six dimensions. + * + * Never rejects: each dimension has a defined value for every failure of + * resolution, so a capture never has to choose between omitting a property and + * failing. Telemetry is not a precondition for any command. + */ +export async function resolveAdoptionDimensions( + cwd: string +): Promise { + const workspaceRoot = await resolveWorkspaceRoot(cwd); + return { + workspaceId: hashIdentity(workspaceRoot), + repositoryId: await resolveRepositoryId(cwd), + envOS: process.platform, + ci: isContinuousIntegration(), + ciProvider: resolveCiProvider(), + languageStack: resolveLanguageStack(workspaceRoot), + }; +} diff --git a/packages/cli/src/util/git-remote.ts b/packages/cli/src/util/git-remote.ts index 16d2946e..95a978d2 100644 --- a/packages/cli/src/util/git-remote.ts +++ b/packages/cli/src/util/git-remote.ts @@ -257,3 +257,88 @@ export async function listRemoteOwnerUrls(cwd: string): Promise { } return owners; } + +/** + * Reduce a git remote reference to a canonical REPOSITORY path, + * `{host}/{owner}/{repo}`, or `null` when it carries no owner/repo pair. + * + * Host-agnostic, unlike `canonicalizeGitHubUrl` above. That function throws + * `UNSUPPORTED_REMOTE_HOST` for a non-GitHub remote, and the refusal is + * load-bearing: it is the capability boundary on REMOTE rule generation. This + * answers a different question, "which codebase is this", which a GitLab, + * Bitbucket or self-hosted repository participates in exactly as much as a + * GitHub one. Teaching the GitHub parser to accept other hosts would have + * softened a refusal doing real work elsewhere, so the two sit side by side. + * + * Parsing follows `canonicalOwnerUrl` — scp-like SSH, `ssh://`, `git://`, + * `https://`, scheme-relative and bare paths — differing only in taking two + * path segments rather than one. Host, owner and repository are lowercased: + * GitHub treats all three case-insensitively, so `Foo/Bar` and `foo/bar` are + * one repository and must not become two identities. + * + * Never throws. A remote it cannot parse yields `null`, which the caller turns + * into the `[unknown]` sentinel. + */ +export function canonicalRepositoryPath(remote: string): string | null { + const raw = remote.trim(); + if (!raw) return null; + + let host = "github.com"; + let path = raw; + + const sshRemote = /^[^@/]+@([^:/]+):(.+)$/.exec(raw); + if (sshRemote) { + host = sshRemote[1] ?? host; + path = sshRemote[2] ?? path; + } else if (/^[a-z][a-z0-9+.-]*:\/\//i.test(raw) || raw.startsWith("//")) { + try { + const url = new URL(raw.startsWith("//") ? `https:${raw}` : raw); + host = url.hostname; + path = url.pathname; + } catch { + // Not parseable as a URL — treat the input as a path, as + // `canonicalOwnerUrl` does for the same inputs. + } + } + + host = host.toLowerCase().replace(/^www\./, ""); + const segments = path + .replace(/^\/+/, "") + .split("/") + .filter((segment) => segment.length > 0); + if (segments.length < 2) return null; + + // The repository is the LAST segment and the owner the FIRST. A self-hosted + // GitLab serves repositories under nested subgroups, and anything between + // the two is part of the address rather than the identity: keeping it would + // make one repository read as several the moment a group were renamed. + const owner = segments[0]!.toLowerCase(); + const repository = segments + .at(-1)! + .replace(/\.git$/i, "") + .toLowerCase(); + if (!owner || !repository) return null; + + return `${host}/${owner}/${repository}`; +} + +/** + * The canonical repository path for `cwd`'s `origin` remote, or `null`. + * + * Reads `origin` alone rather than walking the `REMOTE_PRECEDENCE` fallback + * `listRemoteOwnerUrls` uses. A repository identity has to be the SAME value + * for every clone of one codebase, and precedence makes it depend on which + * remotes a given clone happens to have configured: a fork with `origin` on + * the fork and `upstream` on the source would report whichever the ordering + * picked, so two clones of the same fork could disagree. + * + * Never throws. Not a repository, no `origin`, an unparseable remote, and git + * missing from the host all yield `null`. + */ +export async function resolveRepositoryPath( + cwd: string +): Promise { + const remotes = await listRemoteConfig(cwd); + const origin = remotes.find((remote) => remote.name === "origin"); + return origin ? canonicalRepositoryPath(origin.url) : null; +} diff --git a/packages/cli/test/adoption-dimensions.test.ts b/packages/cli/test/adoption-dimensions.test.ts new file mode 100644 index 00000000..f0e80eb2 --- /dev/null +++ b/packages/cli/test/adoption-dimensions.test.ts @@ -0,0 +1,210 @@ +import { execFileSync } from "node:child_process"; +import { mkdtempSync, mkdirSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; + +import { describe, expect, it } from "vitest"; + +import { + NO_CI_PROVIDER, + UNKNOWN_DIMENSION, + isContinuousIntegration, + resolveCiProvider, + resolveLanguageStack, + resolveRepositoryId, + resolveWorkspaceId, + resolveWorkspaceRoot, +} from "../src/util/adoption-dimensions"; +import { canonicalRepositoryPath } from "../src/util/git-remote"; + +/** + * Real git repositories rather than a mocked spawn. + * + * The behaviour under test IS git's answer — what `--show-toplevel` reports + * from a subdirectory, and what a remote URL looks like once configured — so a + * stub would only assert that the stub was written to match. The unresolvable + * cases (git absent, spawn failure) are covered by the existing mocked suite + * in `git-remote-context.test.ts`, which is the right shape for those. + */ +function makeRepository(remote?: string): string { + const root = mkdtempSync(join(tmpdir(), "taskless-dimensions-")); + execFileSync("git", ["init", "-q"], { cwd: root }); + if (remote) { + execFileSync("git", ["remote", "add", "origin", remote], { cwd: root }); + } + return root; +} + +describe("workspaceId identifies the workspace, not the directory", () => { + it("reports the git top level from a subdirectory", async () => { + const root = makeRepository(); + const nested = join(root, "packages", "cli"); + mkdirSync(nested, { recursive: true }); + + // The equality IS the requirement. `check` from a package directory and + // `check` from the repository root are one workspace, and anchoring on + // cwd instead of the top level would make every subdirectory its own. + expect(await resolveWorkspaceId(nested)).toBe( + await resolveWorkspaceId(root) + ); + }); + + it("falls back to the working directory outside a git work tree", async () => { + const plain = mkdtempSync(join(tmpdir(), "taskless-plain-")); + expect(await resolveWorkspaceRoot(plain)).toBe(resolve(plain)); + expect(await resolveWorkspaceId(plain)).toMatch(/^[\da-f]{64}$/); + }); + + it("distinguishes two clones of one repository", async () => { + const remote = "git@github.com:taskless/cli.git"; + const first = makeRepository(remote); + const second = makeRepository(remote); + + // Different workspaces, same codebase. That split is the whole point of + // carrying both properties. + expect(await resolveWorkspaceId(first)).not.toBe( + await resolveWorkspaceId(second) + ); + expect(await resolveRepositoryId(first)).toBe( + await resolveRepositoryId(second) + ); + }); +}); + +describe("repositoryId is host-agnostic", () => { + it("resolves a non-GitHub remote", async () => { + const root = makeRepository("git@gitlab.com:acme/widgets.git"); + + // The case a GitHub-shaped implementation silently gets wrong: a GitLab + // repository is a codebase like any other and must be counted. + expect(await resolveRepositoryId(root)).toMatch(/^[\da-f]{64}$/); + expect(await resolveRepositoryId(root)).not.toBe(UNKNOWN_DIMENSION); + }); + + it("reports the sentinel with no origin remote", async () => { + const root = makeRepository(); + expect(await resolveRepositoryId(root)).toBe(UNKNOWN_DIMENSION); + }); + + it("reports the sentinel outside a repository", async () => { + const plain = mkdtempSync(join(tmpdir(), "taskless-plain-")); + expect(await resolveRepositoryId(plain)).toBe(UNKNOWN_DIMENSION); + }); +}); + +describe("canonicalRepositoryPath", () => { + it("reduces every remote form to one identity", () => { + const expected = "github.com/taskless/cli"; + for (const remote of [ + "git@github.com:taskless/cli.git", + "git@github.com:taskless/cli", + "https://github.com/taskless/cli.git", + "https://github.com/taskless/cli", + "ssh://git@github.com/taskless/cli.git", + "git://github.com/taskless/cli.git", + "https://user:token@github.com:443/taskless/cli.git?ref=main#top", + "//github.com/taskless/cli", + ]) { + expect(canonicalRepositoryPath(remote), remote).toBe(expected); + } + }); + + it("lowercases, because GitHub treats the path case-insensitively", () => { + expect(canonicalRepositoryPath("https://GitHub.com/Taskless/CLI")).toBe( + "github.com/taskless/cli" + ); + }); + + it("keeps non-GitHub hosts distinct", () => { + expect(canonicalRepositoryPath("git@gitlab.com:acme/widgets.git")).toBe( + "gitlab.com/acme/widgets" + ); + expect(canonicalRepositoryPath("https://git.internal/acme/widgets")).toBe( + "git.internal/acme/widgets" + ); + }); + + it("takes the first and last segments of a nested group path", () => { + // A GitLab subgroup is part of the address, not the identity. Keeping the + // middle would make one repository read as several after a group rename. + expect( + canonicalRepositoryPath("git@gitlab.com:acme/team/sub/widgets.git") + ).toBe("gitlab.com/acme/widgets"); + }); + + it("returns null when there is no owner/repo pair", () => { + expect(canonicalRepositoryPath("")).toBeNull(); + expect(canonicalRepositoryPath("taskless")).toBeNull(); + expect(canonicalRepositoryPath("https://github.com/taskless")).toBeNull(); + }); +}); + +describe("the CI dimensions", () => { + it("treats any non-negative value as CI", () => { + for (const value of ["1", "true", "yes", "woodpecker"]) { + expect(isContinuousIntegration({ CI: value }), value).toBe(true); + } + }); + + it("treats unset, empty, 0 and false as not CI", () => { + expect(isContinuousIntegration({})).toBe(false); + for (const value of ["", " ", "0", "false", "FALSE"]) { + expect(isContinuousIntegration({ CI: value }), value).toBe(false); + } + }); + + it("names a recognized provider", () => { + expect(resolveCiProvider({ CI: "true", GITHUB_ACTIONS: "true" })).toBe( + "github_actions" + ); + expect(resolveCiProvider({ CI: "true", BUILDKITE: "true" })).toBe( + "buildkite" + ); + }); + + it("distinguishes an unrecognized provider from a local run", () => { + // The two sentinels carry different meanings and must not collapse: one + // is a gap in the provider table, the other is an ordinary laptop. + expect(resolveCiProvider({ CI: "true" })).toBe(UNKNOWN_DIMENSION); + expect(resolveCiProvider({})).toBe(NO_CI_PROVIDER); + }); + + it("prefers the specific marker over the generic one", () => { + // Providers nest — GitHub Actions sets CI as well — so ordering decides. + expect( + resolveCiProvider({ + CI: "true", + GITHUB_ACTIONS: "true", + JENKINS_URL: "x", + }) + ).toBe("github_actions"); + }); +}); + +describe("languageStack", () => { + it("reports the languages its root manifests evidence", () => { + const root = mkdtempSync(join(tmpdir(), "taskless-stack-")); + writeFileSync(join(root, "package.json"), "{}"); + writeFileSync(join(root, "go.mod"), "module example\n"); + + const stack = resolveLanguageStack(root); + expect(stack).toContain("JavaScript/TypeScript"); + expect(stack).toContain("Go"); + }); + + it("reports an empty array rather than nothing when no manifest is present", () => { + const root = mkdtempSync(join(tmpdir(), "taskless-empty-")); + expect(resolveLanguageStack(root)).toEqual([]); + }); + + it("does not see a language confined to a sub-package", () => { + // Documented and accepted: the probe is root-only so it can run on every + // invocation. Pinned so nobody "fixes" it into a recursive walk. + const root = mkdtempSync(join(tmpdir(), "taskless-mono-")); + writeFileSync(join(root, "package.json"), "{}"); + mkdirSync(join(root, "services", "api"), { recursive: true }); + writeFileSync(join(root, "services", "api", "pyproject.toml"), ""); + + expect(resolveLanguageStack(root)).not.toContain("Python"); + }); +}); From 6d7366999a9a0737c2b400e2c1f6600c025890ab Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 9 Sep 2026 12:12:33 -0700 Subject: [PATCH 2/3] chore(cli): archive telemetry-adoption-dimensions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../.openspec.yaml | 0 .../proposal.md | 0 .../specs/analytics/spec.md | 0 .../tasks.md | 4 +- openspec/specs/analytics/spec.md | 201 +++++++++++++++++- 5 files changed, 200 insertions(+), 5 deletions(-) rename openspec/changes/{telemetry-adoption-dimensions => archive/2026-09-09-telemetry-adoption-dimensions}/.openspec.yaml (100%) rename openspec/changes/{telemetry-adoption-dimensions => archive/2026-09-09-telemetry-adoption-dimensions}/proposal.md (100%) rename openspec/changes/{telemetry-adoption-dimensions => archive/2026-09-09-telemetry-adoption-dimensions}/specs/analytics/spec.md (100%) rename openspec/changes/{telemetry-adoption-dimensions => archive/2026-09-09-telemetry-adoption-dimensions}/tasks.md (98%) diff --git a/openspec/changes/telemetry-adoption-dimensions/.openspec.yaml b/openspec/changes/archive/2026-09-09-telemetry-adoption-dimensions/.openspec.yaml similarity index 100% rename from openspec/changes/telemetry-adoption-dimensions/.openspec.yaml rename to openspec/changes/archive/2026-09-09-telemetry-adoption-dimensions/.openspec.yaml diff --git a/openspec/changes/telemetry-adoption-dimensions/proposal.md b/openspec/changes/archive/2026-09-09-telemetry-adoption-dimensions/proposal.md similarity index 100% rename from openspec/changes/telemetry-adoption-dimensions/proposal.md rename to openspec/changes/archive/2026-09-09-telemetry-adoption-dimensions/proposal.md diff --git a/openspec/changes/telemetry-adoption-dimensions/specs/analytics/spec.md b/openspec/changes/archive/2026-09-09-telemetry-adoption-dimensions/specs/analytics/spec.md similarity index 100% rename from openspec/changes/telemetry-adoption-dimensions/specs/analytics/spec.md rename to openspec/changes/archive/2026-09-09-telemetry-adoption-dimensions/specs/analytics/spec.md diff --git a/openspec/changes/telemetry-adoption-dimensions/tasks.md b/openspec/changes/archive/2026-09-09-telemetry-adoption-dimensions/tasks.md similarity index 98% rename from openspec/changes/telemetry-adoption-dimensions/tasks.md rename to openspec/changes/archive/2026-09-09-telemetry-adoption-dimensions/tasks.md index 939ec2ec..abc43356 100644 --- a/openspec/changes/telemetry-adoption-dimensions/tasks.md +++ b/openspec/changes/archive/2026-09-09-telemetry-adoption-dimensions/tasks.md @@ -46,10 +46,10 @@ no stack. - [x] 6.2 Amend the standard-properties requirement to list the six dimensions - [x] 6.3 Amend the taxonomy requirement's `cli_check_completed` bullet to include `ruleCount` - [x] 6.4 Carry every existing scenario into both MODIFIED deltas — 4 in standard-properties, 5 in the taxonomy requirement — and keep both TITLES byte-identical, including the escaped underscore in `CLI events use cli\_ prefix`. A delta replaces a requirement rather than patching it, and a renamed title applies nothing at all -- [ ] 6.5 Verify by archiving on a scratch commit and grepping the standing spec for every prior scenario, then resetting to the recorded SHA +- [x] 6.5 Verify by archiving on a scratch commit and grepping the standing spec for every prior scenario, then resetting to the recorded SHA ## 7. Close out - [x] 7.1 Run `pnpm typecheck`, `pnpm lint`, and `pnpm test` - [x] 7.2 Add a changeset. Pre-1.0, added telemetry surface is a `patch`: no consumer must react to it -- [ ] 7.3 Archive the change +- [x] 7.3 Archive the change diff --git a/openspec/specs/analytics/spec.md b/openspec/specs/analytics/spec.md index 778ff629..1445a825 100644 --- a/openspec/specs/analytics/spec.md +++ b/openspec/specs/analytics/spec.md @@ -107,7 +107,9 @@ The PostHog client SHALL be created with `flushAt: 1` and `flushInterval: 0` bec ### Requirement: All capture calls include standard properties -Every `capture()` call SHALL include the `cli` property (anonymous UUID), the `cliVersion` property (the version the build reports as its own, baked in at build time), and the `scaffoldVersion` property (the `version` field from `.taskless/taskless.json`, or `0` if the manifest is absent or unreadable). When authenticated, the `groups` parameter SHALL include `{ organization: String(orgId) }`. The `cliVersion` and `scaffoldVersion` values SHALL be resolved once at telemetry initialization and attached to every subsequent `capture()` call without re-reading the source files. +Every `capture()` call SHALL include the `cli` property (anonymous UUID), the `cliVersion` property (the version the build reports as its own, baked in at build time), and the `scaffoldVersion` property (the `version` field from `.taskless/taskless.json`, or `0` if the manifest is absent or unreadable). It SHALL also include the adoption dimensions `workspaceId`, `repositoryId`, `envOS`, `ci`, `ciProvider`, and `languageStack`, each defined by its own requirement. When authenticated, the `groups` parameter SHALL include `{ organization: String(orgId) }`. The `cliVersion` and `scaffoldVersion` values SHALL be resolved once at telemetry initialization and attached to every subsequent `capture()` call without re-reading the source files, and the adoption dimensions SHALL be resolved once on the same terms. + +Resolving the adoption dimensions SHALL NOT be a precondition for any command. Each one has a defined value for every failure of resolution, so a capture never has to choose between omitting a property and failing. #### Scenario: Anonymous capture includes standard properties @@ -134,6 +136,17 @@ Every `capture()` call SHALL include the `cli` property (anonymous UUID), the `c - **AND** for a `nightly` it SHALL be the version the nightly is published under, so events are attributed to the build that emitted them rather than to the release it anticipates - **AND** SHALL be attached to every event emitted through the returned client +#### Scenario: Every event carries the adoption dimensions + +- **WHEN** any event is captured, authenticated or not +- **THEN** it SHALL include `workspaceId`, `repositoryId`, `envOS`, `ci`, `ciProvider`, and `languageStack` + +#### Scenario: The dimensions are resolved once + +- **WHEN** several events are captured within one invocation +- **THEN** each adoption dimension SHALL be resolved once at initialization +- **AND** SHALL NOT be re-read per event + ### Requirement: CLI events use cli\_ prefix CLI events SHALL use the `cli_` prefix, with the taxonomy organized as a @@ -148,8 +161,9 @@ CLI events SHALL use the `cli_` prefix, with the taxonomy organized as a - `cli_rule_created`, `cli_rule_improved`, `cli_rule_deleted` - `cli_authenticated`, `cli_logged_out` - `cli_installed`, `cli_onboarded` - - `cli_check_completed` — error/warning counts only (e.g. `errorCount`, - `warningCount`, `findings`) + - `cli_check_completed` — error/warning counts and the number of rules the + scan had loaded: `errorCount`, `warningCount`, `findings`, `ruleCount`. + Counts only, never rule content, rule names, or matched source - `cli_error` — a single failure event with `command` and `code` (a stable `CLIErrorCode`) - `cli_agent` — fired when the `agent` command serves a request, with a `topic` @@ -193,6 +207,14 @@ Commands that carry no concrete state beyond the invocation (e.g. `info`, - **THEN** PostHog SHALL NOT receive any event named `cli__completed`, `help_index`, `help_`, or `help_unknown` +#### Scenario: A completed scan reports how many rules were loaded + +- **WHEN** a scan completes +- **THEN** the `cli_check_completed` event SHALL include `ruleCount`, the number + of rules the scan had loaded +- **AND** a scan that loaded no rules SHALL be distinguishable from a scan that + loaded rules and found nothing + ### Requirement: Wrong-topic re-routing is observable as a derivable funnel The taxonomy SHALL keep wrong-topic re-routing derivable as a funnel signal from @@ -343,3 +365,176 @@ An unresolvable owner SHALL NOT affect the command: it is a telemetry value, not - **WHEN** `ghOwner` is recorded - **THEN** the CLI SHALL NOT infer or record whether the owner is an organization or a user account + +### Requirement: Workspace and repository identity + +Telemetry SHALL attach a `workspaceId` and a `repositoryId` property to every +identify and every captured event. + +`workspaceId` SHALL be the SHA-256 hash, hex-encoded, of the absolute path of +the workspace root. The workspace root SHALL be the git top-level directory when +the working directory sits inside a git working tree, and the resolved working +directory otherwise. Resolving upward to the top-level is what makes the value a +workspace identifier rather than a directory identifier: an invocation from a +subdirectory SHALL report the same `workspaceId` as one from the root. + +`repositoryId` SHALL be the SHA-256 hash, hex-encoded, of a canonical +`{host}/{owner}/{repo}` string derived from the `origin` remote, lowercased, +with any `.git` suffix and trailing slash removed and any userinfo, port, query, +and fragment discarded. It SHALL NOT be restricted to GitHub remotes: a GitLab, +Bitbucket, or self-hosted repository SHALL receive a `repositoryId` on the same +terms. Where no `origin` remote resolves — for any reason, including a directory +that is not a repository and a host where `git` is unavailable — `repositoryId` +SHALL be the literal sentinel `[unknown]`, present rather than omitted, so those +runs stay countable. + +Neither hash SHALL be described or relied upon as a secret. `repositoryId` is a +stable pseudonym: a hash of a remote URL is reversible by anyone who can +enumerate candidate URLs. It is hashed because a repository NAME can be an +unannounced product, which is a different question from whether the value is +confidential. `workspaceId` hashes a local absolute path, which commonly contains +a username and is not enumerable, so hashing there is protective in a way that +hashing `repositoryId` is not. + +`ghOwner` SHALL remain unhashed. A GitHub owner is public identity, and the +value is load-bearing precisely because it is legible: excluding a known owner +from external-adoption counts, and judging a cohort for plausibility, both +require the name. The boundary is therefore owner legible, repository not. + +Resolution SHALL NOT fail a command. Every unresolvable case SHALL produce a +value, never an exception. + +#### Scenario: Invocation from a subdirectory reports the workspace root + +- **WHEN** a command runs in a subdirectory of a git working tree +- **THEN** `workspaceId` SHALL be the hash of the git top-level path +- **AND** SHALL equal the `workspaceId` reported by the same command run at the root + +#### Scenario: Working directory is not a git working tree + +- **WHEN** a command runs outside any git working tree +- **THEN** `workspaceId` SHALL be the hash of the resolved working directory + +#### Scenario: Non-GitHub remote still yields a repository identity + +- **WHEN** a command runs in a repository whose `origin` is hosted somewhere + other than GitHub +- **THEN** `repositoryId` SHALL be a hash of that repository's canonical + `{host}/{owner}/{repo}` +- **AND** `ghOwner` SHALL be `[unknown]`, since the GitHub-owner question has no + answer for that remote + +#### Scenario: No origin remote + +- **WHEN** a command runs in a directory with no resolvable `origin` remote +- **THEN** `repositoryId` SHALL be `[unknown]` +- **AND** the property SHALL be present rather than omitted + +#### Scenario: The same repository cloned twice + +- **WHEN** the same repository is cloned to two paths on one machine +- **THEN** the two clones SHALL report the same `repositoryId` +- **AND** SHALL report different `workspaceId` values + +#### Scenario: The GitHub owner is not hashed + +- **WHEN** `ghOwner` and `repositoryId` are recorded for the same GitHub repository +- **THEN** `ghOwner` SHALL be the owner segment verbatim +- **AND** `repositoryId` SHALL be a hash + +### Requirement: Execution environment dimensions + +Telemetry SHALL attach `envOS`, `ci`, and `ciProvider` properties to every +identify and every captured event. + +`envOS` SHALL be the value of `process.platform`. + +`ci` SHALL be a boolean, true when the `CI` environment variable is set to a +positive value. Unset, empty, `"0"`, and `"false"` SHALL each be treated as +false; any other non-empty value SHALL be treated as true. + +`ciProvider` SHALL name the detected continuous-integration provider. When `ci` +is true and no provider is recognized, it SHALL be the literal sentinel +`[unknown]`. When `ci` is false, it SHALL be the literal sentinel `[none]`. Both +SHALL be present rather than omitted, so unrecognized and non-CI runs remain +countable and remain distinguishable from each other. + +These dimensions exist because the anonymous identity cannot separate automated +runs from human ones. `$XDG_CONFIG_HOME/taskless/anonymous_id` is regenerated in +a fresh container, so without `ci` every CI job is indistinguishable from a new +install, and no property stored on past events can repair that after the fact. + +#### Scenario: Local run + +- **WHEN** a command runs with `CI` unset +- **THEN** `ci` SHALL be false +- **AND** `ciProvider` SHALL be `[none]` + +#### Scenario: CI run on a recognized provider + +- **WHEN** a command runs under a recognized CI provider +- **THEN** `ci` SHALL be true +- **AND** `ciProvider` SHALL name that provider + +#### Scenario: CI run on an unrecognized provider + +- **WHEN** a command runs with `CI` set to a positive value and no known + provider environment variable present +- **THEN** `ci` SHALL be true +- **AND** `ciProvider` SHALL be `[unknown]` + +#### Scenario: CI is set to a negative value + +- **WHEN** a command runs with `CI` set to `"0"`, `"false"`, or the empty string +- **THEN** `ci` SHALL be false + +### Requirement: Language stack dimension + +Telemetry SHALL attach a `languageStack` property to every identify and every +captured event, listing the languages evidenced by manifest files present at the +workspace root. + +The property exists to prioritize the rule corpus against the stacks that +actually run it, so its value is a coarse dimension rather than a detection +result. + +It SHALL be resolved from a bounded, root-only probe and SHALL NOT invoke +`detectRepository`. The detection scan performs a recursive walk with manifest +parsing, which is acceptable for a command the user asked for and is not +acceptable on every invocation, including the `agent` fetches an agent makes +repeatedly. + +The probe SHALL read its language-to-manifest mapping from the same +`LANGUAGE_MARKERS` constant the detection scan uses, extended with the Node +manifest that the scan derives JavaScript and TypeScript from. The two SHALL +differ only in search scope. This is stated so a future reader does not +reconcile them by making the telemetry probe recursive: a root-only probe misses +a language confined to a sub-package of a monorepo, and that is the accepted +cost of the property being free. + +Where no manifest is found, `languageStack` SHALL be an empty array rather than +omitted. + +#### Scenario: Root manifests are reported + +- **WHEN** a command runs in a workspace whose root contains a `package.json` + and a `go.mod` +- **THEN** `languageStack` SHALL include the languages both manifests evidence + +#### Scenario: No manifests present + +- **WHEN** a command runs in a workspace root with no recognized manifest +- **THEN** `languageStack` SHALL be an empty array +- **AND** the property SHALL be present rather than omitted + +#### Scenario: The probe does not run the detection scan + +- **WHEN** telemetry resolves `languageStack` +- **THEN** it SHALL NOT call `detectRepository` + +#### Scenario: A language confined to a sub-package + +- **WHEN** a monorepo's root carries only a `package.json` and a Python service + lives in a sub-directory +- **THEN** `languageStack` SHALL report the root evidence only +- **AND** this SHALL NOT be treated as a defect in the property From cf5bfa8b2f05b903b6d4f6e21b29849feb586637 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Wed, 9 Sep 2026 12:56:07 -0700 Subject: [PATCH 3/3] fix(cli): address review findings on the adoption dimensions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../specs/analytics/spec.md | 6 +- openspec/specs/analytics/spec.md | 6 +- packages/cli/src/detect/scan.ts | 7 +- packages/cli/src/telemetry.ts | 36 +++--- packages/cli/src/util/adoption-dimensions.ts | 42 ++++++- packages/cli/src/util/git-remote.ts | 26 +++-- packages/cli/test/adoption-dimensions.test.ts | 16 ++- packages/cli/test/telemetry.test.ts | 107 ++++++++++++++++++ 8 files changed, 204 insertions(+), 42 deletions(-) diff --git a/openspec/changes/archive/2026-09-09-telemetry-adoption-dimensions/specs/analytics/spec.md b/openspec/changes/archive/2026-09-09-telemetry-adoption-dimensions/specs/analytics/spec.md index fc75c418..e8737a8f 100644 --- a/openspec/changes/archive/2026-09-09-telemetry-adoption-dimensions/specs/analytics/spec.md +++ b/openspec/changes/archive/2026-09-09-telemetry-adoption-dimensions/specs/analytics/spec.md @@ -13,7 +13,9 @@ workspace identifier rather than a directory identifier: an invocation from a subdirectory SHALL report the same `workspaceId` as one from the root. `repositoryId` SHALL be the SHA-256 hash, hex-encoded, of a canonical -`{host}/{owner}/{repo}` string derived from the `origin` remote, lowercased, +`{host}/{owner}/…/{repo}` string derived from the `origin` remote — the +WHOLE remote path, so two repositories sharing an owner and a leaf name under +different nested groups stay distinct — lowercased, with any `.git` suffix and trailing slash removed and any userinfo, port, query, and fragment discarded. It SHALL NOT be restricted to GitHub remotes: a GitLab, Bitbucket, or self-hosted repository SHALL receive a `repositoryId` on the same @@ -54,7 +56,7 @@ value, never an exception. - **WHEN** a command runs in a repository whose `origin` is hosted somewhere other than GitHub - **THEN** `repositoryId` SHALL be a hash of that repository's canonical - `{host}/{owner}/{repo}` + `{host}/{owner}/…/{repo}` - **AND** `ghOwner` SHALL be `[unknown]`, since the GitHub-owner question has no answer for that remote diff --git a/openspec/specs/analytics/spec.md b/openspec/specs/analytics/spec.md index 1445a825..07ec1004 100644 --- a/openspec/specs/analytics/spec.md +++ b/openspec/specs/analytics/spec.md @@ -379,7 +379,9 @@ workspace identifier rather than a directory identifier: an invocation from a subdirectory SHALL report the same `workspaceId` as one from the root. `repositoryId` SHALL be the SHA-256 hash, hex-encoded, of a canonical -`{host}/{owner}/{repo}` string derived from the `origin` remote, lowercased, +`{host}/{owner}/…/{repo}` string derived from the `origin` remote — the +WHOLE remote path, so two repositories sharing an owner and a leaf name under +different nested groups stay distinct — lowercased, with any `.git` suffix and trailing slash removed and any userinfo, port, query, and fragment discarded. It SHALL NOT be restricted to GitHub remotes: a GitLab, Bitbucket, or self-hosted repository SHALL receive a `repositoryId` on the same @@ -420,7 +422,7 @@ value, never an exception. - **WHEN** a command runs in a repository whose `origin` is hosted somewhere other than GitHub - **THEN** `repositoryId` SHALL be a hash of that repository's canonical - `{host}/{owner}/{repo}` + `{host}/{owner}/…/{repo}` - **AND** `ghOwner` SHALL be `[unknown]`, since the GitHub-owner question has no answer for that remote diff --git a/packages/cli/src/detect/scan.ts b/packages/cli/src/detect/scan.ts index c0d94241..1de74565 100644 --- a/packages/cli/src/detect/scan.ts +++ b/packages/cli/src/detect/scan.ts @@ -231,13 +231,14 @@ const LINTER_SIGNALS: readonly LinterSignal[] = [ * Languages inferred from the presence of a manifest or marker file anywhere in * the tree. JavaScript and TypeScript are resolved separately (they share * `package.json`). - */ -/** + * * Exported so telemetry's root-only `languageStack` probe reads the same * language-to-manifest mapping this scan does. The two differ only in search * scope: this walks the tree, the probe checks the workspace root. Sharing the * mapping keeps them from disagreeing about which manifest means which - * language; the scope difference is deliberate and documented there. + * language; the scope difference is deliberate and documented there. Note the + * probe adds its own Node entry, since the JavaScript/TypeScript resolution + * mentioned above happens outside this constant. */ export const LANGUAGE_MARKERS: ReadonlyArray<{ language: string; diff --git a/packages/cli/src/telemetry.ts b/packages/cli/src/telemetry.ts index 14fab3fc..1a164562 100644 --- a/packages/cli/src/telemetry.ts +++ b/packages/cli/src/telemetry.ts @@ -163,7 +163,27 @@ export async function getTelemetry(cwd?: string): Promise { orgSubject = decodeOrgId(token) ?? NIL_ORG_ID; } - const scaffoldVersion = await resolveScaffoldVersion(cwd); + // These three are independent of each other and each spawns a subprocess + // or reads the filesystem, so they run concurrently rather than in a + // chain. `getTelemetry` runs on essentially every invocation, and this was + // three sequential git/fs round trips in front of every command. + // + // Everything here runs only when telemetry is enabled: the opt-out returns + // the no-op client before `getTelemetry` reaches this point, so + // `DO_NOT_TRACK=1` costs no git spawn and no filesystem probe. The opt-out + // has to be an opt-out of the WORK, not only of the send. + const [scaffoldVersion, repository, dimensions] = await Promise.all([ + resolveScaffoldVersion(cwd), + // Which GitHub owner is using the CLI, including anonymously — see the + // note below on why this is resolved from the git remote. + cwd ? resolveRepositoryContext(cwd) : undefined, + // The adoption dimensions: workspace and repository identity, execution + // environment, and language stack. Resolved ONCE, like `cliVersion` and + // `scaffoldVersion`, and attached to identify and to every capture. It + // handles an absent `cwd` itself, sentinelling only the three dimensions + // that depend on one. + resolveAdoptionDimensions(cwd), + ]); // Which GitHub owner is using the CLI, including anonymously — that is // the question this property exists to answer, so it is resolved from the @@ -185,22 +205,8 @@ export async function getTelemetry(cwd?: string): Promise { // never throws, so a host with no git installed lands here like any other // unresolvable case. No `cwd` is treated the same way, matching // `resolveScaffoldVersion` above; every real call site passes one. - const repository = cwd ? await resolveRepositoryContext(cwd) : undefined; const ghOwner = repository ? repository.ghOwner : UNKNOWN_GH_OWNER; - // The adoption dimensions: workspace and repository identity, execution - // environment, and language stack. Resolved ONCE here, like cliVersion and - // scaffoldVersion above, and attached to identify and to every capture. - // - // Everything below this point runs only when telemetry is enabled: the - // opt-out returns the no-op client before `getTelemetry` reaches here, so - // `DO_NOT_TRACK=1` costs no git spawn and no filesystem probe. The opt-out - // has to be an opt-out of the WORK, not only of the send. - // - // No `cwd` is treated as no workspace, matching `resolveScaffoldVersion` - // and `ghOwner` above; every real call site passes one. - const dimensions = await resolveAdoptionDimensions(cwd ?? process.cwd()); - posthog = new PostHog(POSTHOG_PROJECT_TOKEN, { host: POSTHOG_HOST, flushAt: 1, diff --git a/packages/cli/src/util/adoption-dimensions.ts b/packages/cli/src/util/adoption-dimensions.ts index 3179ada5..ea9c5989 100644 --- a/packages/cli/src/util/adoption-dimensions.ts +++ b/packages/cli/src/util/adoption-dimensions.ts @@ -204,17 +204,49 @@ export interface AdoptionDimensions { * Never rejects: each dimension has a defined value for every failure of * resolution, so a capture never has to choose between omitting a property and * failing. Telemetry is not a precondition for any command. + * + * With no `cwd`, only the three dimensions that DEPEND on one fall back to + * their sentinels, matching how `resolveScaffoldVersion` and `ghOwner` treat + * the same case. `envOS`, `ci` and `ciProvider` are properties of the process + * rather than of a directory, so they stay real — sentinelling them would + * discard a known answer to look consistent. + * + * The earlier version defaulted to `process.cwd()` here, which contradicted + * the comment at the call site claiming it behaved like its neighbours: it + * resolved live git state instead. Unreachable today, since every call site + * passes a `cwd`, and a trap for the next one that does not. */ export async function resolveAdoptionDimensions( - cwd: string + cwd: string | undefined ): Promise { - const workspaceRoot = await resolveWorkspaceRoot(cwd); - return { - workspaceId: hashIdentity(workspaceRoot), - repositoryId: await resolveRepositoryId(cwd), + const environment = { envOS: process.platform, ci: isContinuousIntegration(), ciProvider: resolveCiProvider(), + }; + + if (!cwd) { + return { + workspaceId: UNKNOWN_DIMENSION, + repositoryId: UNKNOWN_DIMENSION, + languageStack: [], + ...environment, + }; + } + + // Independent lookups, so they run concurrently: each spawns its own git + // process and neither reads the other's answer. `languageStack` is the + // exception and stays sequential — it probes the workspace root, so it + // cannot start until that root is known. + const [workspaceRoot, repositoryId] = await Promise.all([ + resolveWorkspaceRoot(cwd), + resolveRepositoryId(cwd), + ]); + + return { + workspaceId: hashIdentity(workspaceRoot), + repositoryId, languageStack: resolveLanguageStack(workspaceRoot), + ...environment, }; } diff --git a/packages/cli/src/util/git-remote.ts b/packages/cli/src/util/git-remote.ts index 95a978d2..9388860c 100644 --- a/packages/cli/src/util/git-remote.ts +++ b/packages/cli/src/util/git-remote.ts @@ -308,18 +308,22 @@ export function canonicalRepositoryPath(remote: string): string | null { .filter((segment) => segment.length > 0); if (segments.length < 2) return null; - // The repository is the LAST segment and the owner the FIRST. A self-hosted - // GitLab serves repositories under nested subgroups, and anything between - // the two is part of the address rather than the identity: keeping it would - // make one repository read as several the moment a group were renamed. - const owner = segments[0]!.toLowerCase(); - const repository = segments - .at(-1)! - .replace(/\.git$/i, "") - .toLowerCase(); - if (!owner || !repository) return null; + // The WHOLE path is kept, not just the first and last segments. A + // self-hosted GitLab serves repositories under nested subgroups, and + // collapsing them looked attractive because a renamed group would then not + // change the identity. It trades the wrong way: dropping the middle makes + // `acme/team1/api` and `acme/team2/api` — two unrelated repositories that + // happen to share an owner and a leaf name — one identity, silently + // undercounting in exactly the nested-group case this parser exists to + // serve. A rename is the milder failure and the visible one: it starts a + // new id on a date someone can find, where a collision is never visible at + // all. It is also the same class of event as a GitHub transfer, which + // already changes the identity here and is accepted. + const cleaned = segments.map((segment) => segment.toLowerCase()); + cleaned[cleaned.length - 1] = cleaned.at(-1)!.replace(/\.git$/i, ""); + if (cleaned.some((segment) => segment.length === 0)) return null; - return `${host}/${owner}/${repository}`; + return `${host}/${cleaned.join("/")}`; } /** diff --git a/packages/cli/test/adoption-dimensions.test.ts b/packages/cli/test/adoption-dimensions.test.ts index f0e80eb2..c70e120b 100644 --- a/packages/cli/test/adoption-dimensions.test.ts +++ b/packages/cli/test/adoption-dimensions.test.ts @@ -124,12 +124,20 @@ describe("canonicalRepositoryPath", () => { ); }); - it("takes the first and last segments of a nested group path", () => { - // A GitLab subgroup is part of the address, not the identity. Keeping the - // middle would make one repository read as several after a group rename. + it("keeps the whole nested group path", () => { expect( canonicalRepositoryPath("git@gitlab.com:acme/team/sub/widgets.git") - ).toBe("gitlab.com/acme/widgets"); + ).toBe("gitlab.com/acme/team/sub/widgets"); + }); + + it("does not collapse two repositories that share an owner and a leaf name", () => { + // The reason the whole path is kept (taskless/cli#326 review). Dropping + // the middle segments merged these two into one identity, silently + // undercounting distinct codebases in exactly the nested-group case this + // parser exists to serve. + expect( + canonicalRepositoryPath("git@gitlab.com:acme/team1/api.git") + ).not.toBe(canonicalRepositoryPath("git@gitlab.com:acme/team2/api.git")); }); it("returns null when there is no owner/repo pair", () => { diff --git a/packages/cli/test/telemetry.test.ts b/packages/cli/test/telemetry.test.ts index 9ebf84cd..f3087429 100644 --- a/packages/cli/test/telemetry.test.ts +++ b/packages/cli/test/telemetry.test.ts @@ -468,3 +468,110 @@ describe("ghOwner", () => { } }); }); + +/** The properties on the most recent call recorded by a posthog mock. */ +function lastProperties(mock: { + mock: { calls: unknown[][] }; +}): Record { + const call = mock.mock.calls.at(-1)?.[0] as { + properties?: Record; + }; + return call.properties ?? {}; +} + +/** Every adoption dimension, by name, as attached to identify and capture. */ +const DIMENSION_KEYS = [ + "workspaceId", + "repositoryId", + "envOS", + "ci", + "ciProvider", + "languageStack", +] as const; + +/** + * The dimensions reach `identify` and `capture`. + * + * `adoption-dimensions.test.ts` covers how each value is RESOLVED. This covers + * that the resolved values are actually attached, which is a separate claim + * and the one the whole change rests on: the wiring is two `...dimensions` + * spreads, and dropping either would leave every resolver test passing while + * no event carried a single new property. + */ +describe("adoption dimensions are attached to events", () => { + it("carries every dimension on identify", async () => { + const cwd = await mkdtemp(join(tmpdir(), "taskless-dimensions-")); + try { + await execFileAsync("git", ["init"], { cwd }); + + await getTelemetry(cwd); + + const properties = lastProperties(mockIdentify); + for (const key of DIMENSION_KEYS) { + expect(properties, key).toHaveProperty(key); + } + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + it("carries every dimension on capture", async () => { + const cwd = await mkdtemp(join(tmpdir(), "taskless-dimensions-capture-")); + try { + await execFileAsync("git", ["init"], { cwd }); + + const telemetry = await getTelemetry(cwd); + telemetry.capture("cli_run", { command: "check" }); + + const properties = lastProperties(mockCapture); + for (const key of DIMENSION_KEYS) { + expect(properties, key).toHaveProperty(key); + } + // The event's own properties survive the spread rather than being + // overwritten by it. + expect(properties.command).toBe("check"); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + it("resolves real values, not just present keys", async () => { + const cwd = await mkdtemp(join(tmpdir(), "taskless-dimensions-values-")); + try { + await execFileAsync("git", ["init"], { cwd }); + await execFileAsync( + "git", + ["remote", "add", "origin", "git@github.com:acme/widgets.git"], + { cwd } + ); + await writeFile(join(cwd, "package.json"), "{}"); + + const telemetry = await getTelemetry(cwd); + telemetry.capture("cli_run"); + + const properties = lastProperties(mockCapture); + // Hashes, so asserted by shape rather than by value — the values + // themselves are covered in `adoption-dimensions.test.ts`. + expect(properties.workspaceId).toMatch(/^[\da-f]{64}$/); + expect(properties.repositoryId).toMatch(/^[\da-f]{64}$/); + expect(properties.envOS).toBe(process.platform); + expect(properties.languageStack).toContain("JavaScript/TypeScript"); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + it("sentinels only the cwd-dependent dimensions when no cwd is given", async () => { + // `envOS`, `ci` and `ciProvider` are properties of the process rather than + // of a directory, so they stay real. Sentinelling them would discard a + // known answer for the sake of looking consistent. + await getTelemetry(); + + const properties = lastProperties(mockIdentify); + expect(properties.workspaceId).toBe("[unknown]"); + expect(properties.repositoryId).toBe("[unknown]"); + expect(properties.languageStack).toEqual([]); + expect(properties.envOS).toBe(process.platform); + expect(properties.ciProvider).toEqual(expect.any(String)); + }); +});