Skip to content

fix(telemetry): source frontend app version from build config - #2535

Merged
Chris0Jeky merged 3 commits into
mainfrom
issue-2182/telemetry-build-version
Sep 6, 2026
Merged

fix(telemetry): source frontend app version from build config#2535
Chris0Jeky merged 3 commits into
mainfrom
issue-2182/telemetry-build-version

Conversation

@Chris0Jeky

Copy link
Copy Markdown
Owner

Summary

  • replace the frontend telemetry 0.1.0 literal with a Vite build-time VITE_APP_VERSION value
  • pass the normalized release version through Release Desktop and Release Container builds, with 0.0.0-dev defaults for local and rehearsal builds
  • document the source and cover the injected value, fallback, API forwarding, and release workflow contracts

Closes #2182

Verification

  • npm run test -- src/tests/store/telemetryStore.spec.ts src/tests/api/telemetryApi.spec.ts — 25 passed
  • npm run typecheck — passed
  • scoped ESLint — passed
  • npm run build — passed
  • explicit VITE_APP_VERSION=9.9.9-test production bundle check — passed
  • node --test scripts/ci/release-cache-contract.test.mjs — 28 passed
  • targeted release-desktop version assertions — 4 passed
  • docs governance / golden principles — passed
  • git diff --check — passed

The complete release-desktop contract file remains 70 passed / 6 failed on both this worktree and the unchanged primary checkout. The six failures are the existing Windows/WSL validator injection cases; the redirect case created only a disposable test artifact, which was removed. Hosted workflow lint/CI remains the final proof for the workflow edits.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

The connector usage-limit notice is informational, not a review finding. This PR is being checked through the independent coordinator review path; no code action is requested from this notice.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Fresh-context review (SC-9 gate — Codex credits exhausted). This is the first review posted on this PR.

Verdict: SHIP. No CRITICAL or HIGH. One MEDIUM and two LOWs, all tracked rather than blocking. Reviewed with a release-integrity / supply-chain lens because this is T2 control-plane (.github/workflows/**, scripts/ci/**), so under the ADR-0066 amendment of 2026-09-03 it still merges only after the maintainer's own review plus this review. Queueing it under OUTSTANDING_TASKS.md SC-10.

The core question for a version-stamping change is whether the version can come out wrong or empty in a shipped artifact. It cannot, on any lane that ships one:

  • release-desktop.yml derives the stamp in resolve-source from the validated tag and passes it to the frontend build; release-container.yml passes the two build args through to deploy/Dockerfile.production.
  • buildAppVersion() trims and falls back to 0.0.0-dev, so a missing or whitespace arg degrades to an obviously-non-release marker rather than an empty string.
  • The rehearsal lane correctly yields 0.0.0-dryrun.
  • deploy/docker/frontend.Dockerfile always produces 0.0.0-dev — correct for every one of its callers, since no release lane builds it (see the LOW below).

Telemetry surface is unchanged: no new field, no new recipient, only the value of an existing version stamp.

MEDIUM — two independent copies of the version-normalization logic, with nothing asserting they agree

.github/workflows/release-desktop.yml:171. The PR adds a third copy of the normalization block (version="${tag#v}"; version="${version%%+*}"; grep -Eq '^[0-9A-Za-z][0-9A-Za-z.-]*$') inside resolve-source, while build-backend's pre-existing Resolve product version step (:356-384) keeps re-deriving the identical value from needs.resolve-source.outputs.tag for -p:Version=.

Byte-identical today, so nothing is broken at this head. But the desktop archive now carries a frontend stamped from one derivation and a backend stamped from the other, and nothing compares them — release-desktop-dispatch.test.mjs pins the new resolve-source strings (:230-232) but never pinned build-backend's copy at all. A future edit to one block silently desynchronizes the two halves of the same archive.

Cheap close, worth a follow-up rather than expanding this PR: have build-backend read needs.resolve-source.outputs.version instead of re-deriving (keeping the plain-token check as redundancy), or add one assertion that both jobs stamp from the same source.

LOW — telemetryApi.spec.ts rename overstates what that spec proves

:34. The edit lifts the literal into const injectedAppVersion = '0.2.0' and renames the test to "should preserve the build-injected app version when posting events", but the spec builds the event objects by hand and only checks what http.post received — it never exercises buildAppVersion() or import.meta.env. The identical assertions would pass if the store still hardcoded 0.1.0. #2182's "telemetryApi contract/spec updated to assert the injected value" is genuinely satisfied by the new telemetryStore.spec.ts case, not by this one. Either restore the original name or add a comment pointing at where the real coverage lives.

LOW — frontend.Dockerfile comment is aspirational

deploy/docker/frontend.Dockerfile:11 says "Release builds may override this to keep frontend telemetry aligned with the backend". Grepping every consumer at head: that file is built only by reusable-container-images.yml, cd-staging-gate.yml (explicitly non-deploying), deploy/docker-compose.yml, scripts/deploy/Build-TaskdeckImages.ps1 and the docs' local examples — none pass VITE_APP_VERSION, and the only release container path builds deploy/Dockerfile.production instead. The behaviour is right; only the comment implies a lane that does not exist. The CONFIGURATION_REFERENCE wording is more careful and accurate.

INFO

  • Nothing closes the loop end-to-end — no check greps dist/assets/*.js for the expected version after npx vite build. The author verified it once by hand (VITE_APP_VERSION=9.9.9-test, recorded in the body), and the release-container lane already has the analogous backend assertion. Vite's env plumbing here is the same mechanism VITE_API_BASE_URL already relies on in this exact build step, so the residual risk is low. Optional follow-up.
  • The GitHub Pages demo (pages-frontend.yml) is the one shipped frontend absent from the docs table; it builds with an empty API base, so loadConfig() fails, isActive is false and it emits no telemetry at all. Harmless; optionally add it to that row.

Tests

Adequate for the behaviour changed. telemetryStore.spec.ts tests buildAppVersion() in both directions — injected value with surrounding whitespace (proving the .trim()) and empty/whitespace (proving the 0.0.0-dev fallback) — through the public emit() path rather than reaching inside the store, with vi.stubEnv and a matching vi.unstubAllEnvs() in afterEach, and no assertion depending on VITE_APP_VERSION being absent from the ambient environment. The pre-existing literal was correctly updated for consistency.

Docs

CONFIGURATION_REFERENCE.md is the right home and was done correctly — I checked the rewritten rows line-by-line against the code at head and each claim holds. No ADR is owed: this picks between no competing approaches and is trivially reversible.

The red check is not this PR

Frontend Unit (windows-latest) was FAILURE, and the job never reached vitest. It died in the first step, Run source launcher regression suite (node --test … scripts/ci/dev-up.test.mjs), which runs before npm ci; every later step including the vitest step is skipped. The failure is fail 0 / cancelled 2: no assertion failed anywhere. The parent test at dev-up.test.mjs:1476 declares no timeout of its own, so the CLI-wide --test-timeout=30000 must cover its five serial children; the first four consumed 22 830 ms, leaving ~7.2 s for the fifth, which was cancelled mid-flight. The arithmetic is exact — 2376.5 + 8286.7 + 4294.3 + 7872.9 + 7599.7 = 30 430 ms against a reported parent of 30 430.96 ms. The PowerShell leg of the identical parent test passed in the same run with 3.4× headroom.

This is the tracked class in #2378 and #2161, not a defect in this diff. The PR's own code is proven green on the ubuntu leg of the same run: telemetryStore.spec.ts 22 tests, telemetryApi.spec.ts 3 tests, 368 files / 5574 passed. I have run gh pr update-branch to re-prove against the current base (main had moved 64 commits past the failing run's merge base, so a plain rerun would have reused the stale merge ref).

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Fresh-context re-review against the moved base (coordinator, 2026-09-06; SC-10 delegated, q-1 = A; read-only Opus 5 reviewer over the diff of the updated head against current main).

VERDICT: SHIP. main had touched both release workflows and CONFIGURATION_REFERENCE.md since the 2026-09-04 review. Verified: the head-vs-main diff contains zero deletions in either workflow (every SHA pin, no-cache: true, persist-credentials: false and clean-build step preserved); VITE_APP_VERSION is plumbed consistently (release-container build-args, ARG/ENV before npm run build in both Dockerfiles, step env: in release-desktop build-frontend); every new release-desktop-dispatch.test.mjs assertion matches the current YAML and binds to the new resolve-source lines; no other scripts/ci test pins release-container.yml or the Dockerfiles by exact text; telemetry sends the version only in the pre-existing appVersion field and a missing value falls back to 0.0.0-dev. LOWs recorded for #2182: CONFIGURATION_REFERENCE.md:106 still says "Injection: none" for the rehearsal container build; the normalized version is derived twice in release-desktop (resolve-source and build-backend) with no test asserting agreement; no documented read path for the frontend value; STATUS untouched (coordinator lease covers it). Merge follows the hosted run at f743786d8.

@Chris0Jeky
Chris0Jeky merged commit f6ec8f9 into main Sep 6, 2026
35 checks passed
@Chris0Jeky
Chris0Jeky deleted the issue-2182/telemetry-build-version branch September 6, 2026 13:29
@github-project-automation github-project-automation Bot moved this from Pending to Done in Taskdeck Execution Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Frontend][Telemetry] appVersion is hardcoded to '0.1.0' in telemetry payloads — wire it from build config

1 participant