[Tim020]

Summary

Server-side half of the anonymous usage-telemetry feature designed in plans/PHONE_HOME_TELEMETRY_PLAN.md. The collector side (Cloudflare Worker/D1/Analytics Engine, shared across dreamteamprod apps) is already deployed and live at https://telemetry.digiscript.dreamteamprod.co.uk — see the dreamteam-telemetry repo.

• services/telemetry_service.py: TelemetryService, mirroring the existing VersionChecker pattern (AsyncHTTPClient + PeriodicCallback, start/stop lifecycle, never raises on failure). Sends a check-in after a jittered initial delay, then roughly daily; retries failed sends up to 3x with exponential backoff, never blocking startup or request handling.
  • Envelope: instance_id (lazily-generated UUIDv4, persisted in settings — never derived from anything identifying), app_version, os_platform/os_arch, deployment_method (docker vs bare-metal, via /.dockerenv), days_since_install, checkin_count, error_count_recent (counted via a small logging.Handler attached to the root logger, read-and-reset each check-in).
  • DigiScript metrics: shows/users/sessions counts, bucketed into ranges — never exact counts — plus rbac_enabled (any row across the dynamically-created rbac_* tables) and collab_editing_used (hardcoded false for now; see note below).
  • Payload shape matches the collector's validateEnvelope/validateDigiscriptMetrics exactly — the collector rejects anything that doesn't.
• digi_server/settings.py: four new settings — telemetry_enabled (opt-out by default, user-editable, new "Telemetry" category) and three hidden internal ones (telemetry_instance_id, telemetry_first_seen_at, telemetry_checkin_count).
• digi_server/app_server.py: start_telemetry_service/stop_telemetry_service/toggle_telemetry, wired the same way as the existing version-checker and following the same live-toggle pattern other settings use. Checks the DIGISCRIPT_TELEMETRY env var first (an independent override), then the telemetry_enabled setting, before the service is ever started.

Known gap

collab_editing_used is hardcoded false. The collaborative editor's drafts (ScriptDraft/RoomManager) are transient — cleaned up on save/discard — so a current-row count would read near-zero even for heavy users. Doing this properly needs a persistent "ever used" flag set from the save path, which is out of scope here.

Not included

The docs/ transparency page and the Grafana dashboard panel — both still open per the plan doc's "Implementation sketch".

Test plan

• ruff check / ruff format --check clean
• New unit tests for TelemetryService (buckets, instance-id/first-seen-at persistence, payload shape, check-in retry/backoff behavior, env-var/setting gating) — 14 tests, all passing
• Full backend suite (pytest, 1284 tests) passes
• Manual smoke test against the live collector once this is deployed to a real instance

[github-actions]

Client Test Results

102 tests   102 ✅  0s ⏱️
  9 suites    0 💤
  1 files      0 ❌

Results for commit e5d75d9.

♻️ This comment has been updated with latest results.

[github-actions]

Client V3 Test Results

375 tests   375 ✅  3s ⏱️
 29 suites    0 💤
  1 files      0 ❌

Results for commit e5d75d9.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

    1 files      1 suites   1m 25s ⏱️
1 314 tests 1 314 ✅ 0 💤 0 ❌
1 345 runs  1 345 ✅ 0 💤 0 ❌

Results for commit e5d75d9.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

246 tests   246 ✅  2m 34s ⏱️
 15 suites    0 💤
  1 files      0 ❌

Results for commit e5d75d9.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

246 tests   246 ✅  2m 36s ⏱️
 15 suites    0 💤
  1 files      0 ❌

Results for commit e5d75d9.

♻️ This comment has been updated with latest results.

[Tim020]

Review round 1

Head SHA reviewed: c56e5cdb0d5acf8bbe597f26f29141a1db766b48

Verdict: changes needed. This is a well-structured, privacy-conscious telemetry service with solid test coverage for its happy paths, but there are two real lifecycle bugs in the start/stop machinery that should be fixed before merge, plus a metric that will over-report for most installs.

Previous round follow-up: none — this is round 1, no prior threads.

SonarQube: Quality gate PASSES (project key dreamteamprod_DigiScript, analysis confirmed fresh for this head SHA). No failing conditions, 0 security hotspots, 0% duplication. 3 open MINOR issues, all rule python:S7503 ("async function never awaits") in telemetry_service.py at lines 85, 103, 166 — covered in the inline comments below. None of these block the gate.

New findings: 2 Critical, 4 Important, 6 Suggestion (2 Sonar-sourced). None could not be anchored to a diff line, but a few broader points are summarized here:

• Test gaps (server/test/services/test_telemetry_service.py): the jitter → _schedule_first_check_in → _first_check_in → PeriodicCallback chain is never actually driven in any test (only that start_telemetry_service() sets telemetry_service to non-None), so the bug in the first Critical comment below would pass the whole suite. Also untested: toggle_telemetry()'s sync wrapper / the settings callback_fn wiring itself (only the async _toggle_telemetry is called directly), _compute_rbac_enabled with an actual non-default RBAC role assigned (only the all-false default case is covered — would have caught the rbac_enabled finding below), exact retry attempt-count/backoff-timing assertions, and concurrency edge cases (check_in() racing stop(), double-start()).
• Docs: per this repo's own "Important Notes from Tim" in CLAUDE.md, a new user-facing setting + env var should come with a docs/ page describing what's collected and how to opt out (both the setting and DIGISCRIPT_TELEMETRY). None was added in this PR.
• Docstring coverage is inconsistent across the new private helpers: __init__ has a full reST docstring, _get_or_create_instance_id has a partial one, but its structural twin _get_or_create_first_seen_at, plus _build_payload, _get_checkin_count and _set_checkin_count, have none at all. Not blocking, but worth a pass given CLAUDE.md's reST docstring convention.
• The module docstring's reference to the dreamteam-telemetry collector repo's exact validator function names (validateEnvelope/validateDigiscriptMetrics) is a reasonable authoritative-contract pointer, but is comment-rot risk if that repo's code moves — low priority, just flagging.

Strengths:

• Deliberate, well-reasoned privacy design: no show/user content, random UUID instance id, bucketed counts, clear settings help text that matches what's actually sent (verified field-by-field).
• check_in()'s "never raises" contract largely holds up under tracing, and the outer except Exception around _build_payload() is correctly-scoped best-effort suppression with full traceback logging (.exception()), not silent swallowing.
• Good WHY comments: the collab_editing_used: False hardcoding rationale, and the env-var-still-overrides-the-setting note in _toggle_telemetry, both verified accurate against the code.
• DB session handling (short sync sessionmaker() blocks, no await while a session is open, SQLAlchemy 2.0 select() API) follows this codebase's established async/DB pattern correctly, and every session use goes through a context manager that rolls back/closes in a finally regardless of exception — no resource leaks found.
• Bucket-boundary tests and payload-shape tests against the collector's contract are thorough and will catch accidental field drift.

🤖 Generated with Claude Code

[Tim020]

Suggestion: this guard is unreachable, and the real failure mode it's meant to catch is worse than what it currently does nothing for.

IOLoop.current() called with its default instance=True creates and returns a new IOLoop when none exists, rather than returning None/falsy (confirmed against the installed Tornado source). So if not IOLoop.current(): can never be true in normal operation — this error-logging branch is dead code, giving false confidence that "no IOLoop" is being defended against. The actual failure mode (calling from a thread/context with no running loop) would instead silently create a throwaway orphan loop, schedule _toggle_telemetry on it, and that callback would simply never run on the real server loop — with no error logged at all, which is a worse silent failure than the one this code appears to handle. Not blocking today if toggle_telemetry() is always invoked from the server's own IOLoop thread, but consider removing the dead branch or replacing it with an actual thread/loop-identity check.

🤖 Generated with Claude Code

[Tim020]

Agreed this is dead code — not fixing it here, with a reason: toggle_telemetry() copies this exact if not IOLoop.current(): error; else: add_callback(...) pattern verbatim from four pre-existing call sites in this same file (toggle_mdns_advertising, regen_logging, validate_has_admin, show_changed), none of which this PR touches. Fixing it only in the new telemetry code would leave the file internally inconsistent — four call sites still doing the (confirmed) wrong thing right next to one that doesn't. A repo-wide cleanup of this pattern (plus whatever the "real failure mode" guard should actually look like, per your point about orphaned loops) seems like the right scope for a separate PR rather than folding it into this one.

[Tim020]

Review round 1 — all findings addressed

Replied inline on each of the 11 anchored comments (2 Critical, 4 Important, 5 Suggestion) with what changed and the commit it landed in — see those threads for specifics. One (the dead IOLoop.current() guard) was deliberately not fixed, with a written reason in its thread: it's copied verbatim from four pre-existing call sites in app_server.py that this PR doesn't touch, so fixing it in isolation would leave the file inconsistent; proposed as a separate repo-wide cleanup instead.

On the broader, non-anchored points from the review summary:

• Docs: added docs/pages/telemetry.md (what's collected, what's never sent, both opt-out mechanisms), linked from docs/index.md and cross-referenced from docs/pages/user_config.md's System Settings section.
• Docstring coverage: added docstrings to _get_or_create_first_seen_at, _get_checkin_count, _set_checkin_count, and _build_payload (all four you named).
• Module docstring comment-rot risk: resolved as part of the Important DIGI-5: Update readme with setup instructions #5 fix (the plans/ reference) — it no longer points at anything that could move independently of this code.
• Test gaps: addressed the ones tied to real bugs — the jitter→first-check-in→PeriodicCallback chain is now actually driven (test_stop_during_jitter_window_prevents_first_check_in), toggle_telemetry()'s sync wrapper is covered (test_toggle_telemetry_sync_wrapper_dispatches_via_ioloop), and _compute_rbac_enabled now has three scenario tests including the revoked-role case I found while fixing the Important DIGI-4: Add node CV action #4 thread. Not added: exact retry attempt-count/backoff-timing assertions, or a true check_in()-racing-stop() concurrency test beyond the jitter-window race — flagging these as still-open rather than claiming full coverage.

Full backend suite: 1295 passed. Latest commit: eeee7cb.

[Tim020]

Review round 2

Head SHA reviewed: eeee7cb311fc7eff93118ba5d50c1a0daa3a443c

Verdict: close to ready. Every Critical from round 1 is genuinely fixed, with good test coverage pinning each one down. No new Critical issues. Two new Important findings below are worth a look before merge, but neither is a regression — they're residual edge cases in logic that was already substantially improved this round.

Previous round follow-up: 10 of 11 round-1 threads verified fixed and resolved; 1 still open.

• Fixed & resolved (10): both Critical lifecycle bugs (stop() not cancelling the pending check-in; the shared AsyncHTTPClient singleton breaking VersionChecker), the DB-write-conflated-with-network-failure bug, the rbac_enabled overcounting-almost-every-install bug (including a residual revoke-but-row-persists nuance the author caught and fixed in a follow-up commit before I could even raise it), the dead plans/ doc reference, the double-start guard, the _toggle_telemetry WHAT-only docstring, the narrow env-var matching, and both SonarQube python:S7503 suggestions.
• Still open (1): the dead/unreachable IOLoop.current() falsy-check in toggle_telemetry() (app_server.py:749-754) — not touched this round, still a minor dead-code suggestion, not blocking.

SonarQube: Quality gate PASSES. Zero open issues of any kind (the 3 round-1 python:S7503 MINOR issues are confirmed gone), zero security hotspots, zero duplication. Analysis is fresh for the reviewed head.

New findings: 2 Important, 5 Suggestion. Both Important findings are anchored below.

• server/services/telemetry_service.py lines 353-384 (_compute_rbac_enabled): fixed for the single-lone-admin case, but two different people each independently creating a cue type (each auto-granted a role on their own cue type) still makes rbac_enabled read True with zero deliberate RBAC administration.
• server/services/telemetry_service.py lines 186-210 (check_in()'s retry loop): empirically confirmed (by reproduction) that a concurrent stop() firing mid-retry nulls self._http_client, and the next attempt(s) throw AttributeError that gets logged as a misleading "check-in failed" warning, burning the remaining retry budget instead of recognizing the service stopped. Doesn't crash (the broad except catches it), but produces confusing logs.

Five Suggestions (all in the test file, not blocking): test_stop_during_jitter_window_prevents_first_check_in's comment overclaims that it covers "the exact race" when it only exercises the guard flags sequentially after stop() has fully completed; test_toggle_telemetry_sync_wrapper_dispatches_via_ioloop's fixed await asyncio.sleep(0.05) is a minor flaky-test risk under CI load; test_start_telemetry_service_is_safe_to_call_twice's docstring claims relevance to toggle_telemetry's IOLoop dispatch path that it doesn't actually exercise; a latent (not currently reachable) issue where restarting a stopped TelemetryService instance would silently fail to send anything, since stop() nulls _http_client and start() doesn't rebuild it; and several test classes (TestTelemetryServiceState, TestTelemetryServicePayload, TestTelemetryServiceCheckIn) construct a TelemetryService — and so a force_instance=True AsyncHTTPClient — in setUp with no tearDown calling .stop(), leaking clients across tests (only TestTelemetryServiceLifecycle does this correctly).

Strengths: the round-1 fixes are well-targeted and well-tested — test_http_client_uses_force_instance, test_rbac_enabled_true_for_multiple_distinct_users/test_rbac_enabled_false_when_all_roles_revoked, and test_check_in_does_not_retry_a_local_persistence_failure all genuinely pin down the behavior they claim (verified via mutation testing). The author caught and fixed the RBAC revoke-row-persistence nuance proactively, in a follow-up commit, before this review got to it. Missing docstrings flagged in round 1's body (on _get_or_create_first_seen_at, _get_checkin_count, _set_checkin_count, _compute_rbac_enabled, and now _build_payload) have all been added.

🤖 Generated with Claude Code

[Tim020]

Review round 2 — both Important findings fixed, all 5 Suggestions addressed

Since neither Important finding was anchored as a separate inline thread this round (both described in the review body prose rather than as GitHub review comments), replying here instead of inline.

rbac_enabled still false-positive on two independent auto-grants: confirmed — the distinct-user-count fix from round 1 couldn't actually distinguish that from genuine administration, since nothing in the RBAC tables or the resources they reference records how a grant was made. Rather than patch the heuristic further, replaced it entirely: a persisted flag (telemetry_rbac_deliberately_used) set only by RBACRolesGrantHandler, the one @require_admin-gated grant endpoint — not the auto-grant call sites in cues.py/shows.py/scripts.py. _compute_rbac_enabled now just reads that flag. New tests: test_rbac_enabled_false_without_deliberate_grant (two users, two independent auto-grants, still reads False) and test_rbac_enabled_true_after_deliberate_grant.

check_in()'s retry loop racing a concurrent stop(): confirmed via the same mechanism you described — asyncio.sleep during backoff yields, stop() runs and nulls _http_client, next attempt hits AttributeError on None.fetch(). Added a self._stopped/self._http_client is None check at the top of each retry attempt that bails out cleanly instead. New test: test_check_in_aborts_cleanly_if_stopped_mid_retry.

Also fixed, since I was already in this code: the latent restart bug you flagged as a Suggestion (start() not rebuilding _http_client after a prior stop() nulled it) — closing it now rather than leaving it for later, since a stop-then-start is exactly what toggling the setting off and back on does. New test: test_restart_after_stop_rebuilds_http_client.

All 5 test-file Suggestions addressed: the jitter-test comment no longer claims to cover "the exact race" (it replays the guard sequentially, not concurrently); the toggle-dispatch test now polls (bounded) instead of a fixed sleep(0.05); the double-start test's docstring no longer claims relevance to toggle_telemetry's IOLoop path; and tearDown (calling .stop()) was added to the three test classes that were leaking AsyncHTTPClient instances.

Full backend suite: 1296 passed, including the existing test/controllers/api/v1/test_rbac.py (unaffected by the new telemetry call in the grant handler). Latest commit: f81756e.

[Tim020]

Review round 3

Head SHA reviewed: f81756ebdbf27a5b994cc33541edbd40d84824c7

Verdict: close to ready — one Important finding worth fixing, nothing blocking. This round is another strong pass: every one of round 2's 6 findings is genuinely fixed, including a root-cause redesign (not a patch) of the rbac_enabled metric.

Previous round follow-up: all 6 round-2 findings verified fixed and resolved (none disputed, none incomplete).

• _compute_rbac_enabled was redesigned around a persisted RBAC_DELIBERATE_USE_KEY flag set only from the @require_admin-gated grant endpoint, rather than counting rows — this fully eliminates the two-independent-creators false positive (not just the single-creator case round 1 fixed), confirmed by new tests covering both the negative and positive case.
• check_in()'s retry loop now checks self._stopped or self._http_client is None before every attempt, cleanly aborting instead of producing a misleading AttributeError-as-failure log when a concurrent stop() fires mid-retry.
• start() now rebuilds self._http_client if None (from a prior stop()), fixing a silent-failure-on-restart bug.
• Three test-quality nits (an overclaiming comment, a flaky fixed asyncio.sleep(0.05), a misleading docstring) were all cleanly fixed with accurate comments and a bounded poll loop. Test hygiene (missing tearDown/stop() leaking HTTP clients) was also fixed across all three affected test classes.

The 1 thread still open from round 1 (the dead/unreachable IOLoop.current() falsy-check in toggle_telemetry(), app_server.py:749-754) remains untouched this round — still a minor non-blocking suggestion.

SonarQube: Quality gate PASSES. Zero open issues, zero security hotspots, zero duplication. Analysis confirmed fresh for this exact head SHA.

New findings: 1 Important, 1 Suggestion.

• server/controllers/api/v1/rbac.py lines 213-219: the new mark_rbac_deliberately_used() call has no error handling, so a transient DB failure there turns an already-successfully-committed RBAC grant into a reported 500 for the admin — and silently skips the WS GET_CURRENT_RBAC refresh notification to the grantee's connected clients too, since that runs later in the same method.
• server/services/telemetry_service.py lines 371-390: minor docstring nit — the flag is forward-looking only (it can't retroactively capture RBAC administration that happened before this code shipped), worth a one-line caveat.

Also noted, not blocking, no inline anchor: no test exercises the real rbac.py → flag wiring end-to-end through the actual HTTP endpoint — the telemetry tests call TelemetryService.mark_rbac_deliberately_used(session) directly, bypassing the controller. A test in the existing server/test/controllers/api/v1/test_rbac.py doing a real grant via the endpoint and then checking the flag would close this gap.

Strengths: the project's own established "telemetry must never affect real app behavior" convention (visible in check_in()'s own carefully-isolated DB-write try/except) is exactly what's missing at the one new call site outside the telemetry module itself — this reads as an oversight in otherwise very consistent code, not a pattern problem. The RBAC redesign is a genuinely better design than the row-counting heuristic it replaced, and the fix-test pairing across all three rounds has been thorough throughout.

🤖 Generated with Claude Code

[Tim020]

Review round 3 — both findings fixed, coverage gap closed

Neither finding was anchored as a separate inline thread, so replying here.

Important — mark_rbac_deliberately_used() with no error handling: confirmed. Wrapped it in its own try/except logged with .exception(), so a failure there can never turn an already-successful grant into a reported 500 or skip the WS GET_CURRENT_RBAC notification that runs after it. This matches the pattern already established in TelemetryService.check_in() for isolating telemetry writes from real app behavior — good catch that it hadn't been applied at the one call site outside the telemetry module itself.

Suggestion — forward-looking-only caveat: added to _compute_rbac_enabled's docstring.

Also closed the non-blocking gap you flagged (no test through the real endpoint): added test_successful_grant_marks_rbac_deliberately_used_for_telemetry to test/controllers/api/v1/test_rbac.py — does a real grant via POST /api/v1/rbac/user/roles/grant and checks the flag afterward, rather than only unit-testing mark_rbac_deliberately_used() in isolation.

Full backend suite: 1297 passed. Latest commit: 5cefa2c.

[Tim020]

Review round 4

Head SHA reviewed: 5cefa2c05a1f9d1f4370db0066c5e06dfb0ae02a

Verdict: ready to merge. This round's diff is small and purely addresses round 3's two findings — nothing else changed, and both fixes are correct.

Previous round follow-up: both round-3 findings verified fixed and resolved.

• The uncaught-exception issue in rbac.py's grant handler (Important) is fixed: the telemetry flag write is now wrapped in its own try/except with a WHY comment, matching the established check_in() pattern, so a failure there can no longer turn a successful RBAC grant into a reported error or skip the WS GET_CURRENT_RBAC notification. The accompanying test gap noted alongside that finding is also closed — test_successful_grant_marks_rbac_deliberately_used_for_telemetry now exercises the real HTTP endpoint end-to-end, not just the static method in isolation.
• The docstring forward-looking-only caveat (Suggestion) is fixed, worded almost exactly as suggested.

The 1 thread still open since round 1 (the dead/unreachable IOLoop.current() falsy-check in toggle_telemetry(), app_server.py:749-754) remains untouched — still a minor non-blocking suggestion, not revisited this round since nothing in this diff touched it.

SonarQube: Quality gate PASSES. Zero open issues, zero security hotspots, zero duplication. Analysis confirmed fresh for this exact head SHA.

New findings: none. No new Critical/Important/Suggestion issues found in this round's diff.

Strengths: across all four rounds, every finding raised was either fixed correctly (often with a better root-cause fix than the minimal suggestion, as with the rbac_enabled redesign) or a genuine non-issue conceded with reasoning. Test coverage was extended alongside every fix, including closing gaps noted only in review-body prose rather than as formal inline threads (the controller-level RBAC wiring test). This PR is in good shape to merge.

🤖 Generated with Claude Code

[Tim020]

Added: first-run telemetry opt-in/out, gated start

On request, telemetry no longer starts at boot on a fresh install before an admin account exists. The choice now lives on the existing first-run "create an admin user" screen in both client/ and client-v3/, pre-checked to match the default:

• start_telemetry_service() gates on has_admin_user, in addition to the env var and telemetry_enabled checks it already had.
• utils/web/user_admin.py's create_user() — shared by both /api/v1/auth/create and /api/v2/users — saves an optional telemetry_enabled field, but only on the very first user creation, then explicitly re-triggers start_telemetry_service(). Omitting the field (older clients, direct API callers) still gets the existing default.
• docs/pages/telemetry.md now mentions this.

New coverage: backend tests for the first-run save/gate behavior, and new Playwright assertions in 01-first-run.spec.ts (checkbox present, checked by default) and 03-system-config.spec.ts (the saved value is actually correct in System Settings afterward). Full backend suite (1303), full E2E suite (241, Chromium), and both clients' Vitest/lint/typecheck/build all verified green.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Tim020]

Review round 5

Head SHA reviewed: e5d75d90f80017624918a06f9d9ac1371f5d8888

Verdict: changes needed. This round adds a substantial new feature — a first-run telemetry opt-in/opt-out checkbox on the admin-creation screen in both clients, gated server-side on a new has_admin_user check so telemetry can never start before first-run setup completes. The core design is sound and the gating logic itself is correct (verified the settings-callback interaction and startup ordering don't race), but two Important issues and a few Suggestions turned up.

Previous round follow-up: nothing to validate this round — this is new feature work, not a reply to an earlier finding. The 1 thread still open since round 1 (the dead/unreachable IOLoop.current() falsy-check in toggle_telemetry(), now at app_server.py:765) remains untouched, unrelated to this round's diff.

SonarQube: Quality gate PASSES. Zero open issues, zero security hotspots, zero duplication. Analysis confirmed fresh for this exact head SHA (ran 15 seconds after the commit).

New findings: 2 Important, 3 Suggestion.

• Both clients: the new "See what's collected" link on the first-run screen opens /help/telemetry in a new tab, but that tab is itself a fresh load of the app with no admin yet — so it renders the same "create an admin user" screen instead of the help page, since both App.vues gate on has_admin_user === false unconditionally, before any route is matched. The one way to read the disclosure before deciding is broken at exactly the moment it matters.
• user_admin.py's new start_telemetry_service() call fires for every first-user creation through /api/v1/auth/create and /api/v2/users — including the ~77+ call sites of DigiScriptTestCase._create_and_login_admin used throughout the backend test suite. Nothing sets DIGISCRIPT_TELEMETRY=0 for pytest, and conftest.py's tearDown never stops the service, so every such test leaks an _ErrorCountHandler onto the process-global logger (confirmed: get_logger() returns a module-level singleton shared across the whole pytest-xdist worker) and an unclosed AsyncHTTPClient, accumulating for the life of the worker process — plus a non-zero (if small per-test) chance of an actual network POST firing mid-suite.

Three Suggestions (all low-risk, not blocking): the Vue 2 client always sends telemetry_enabled in the user-creation payload regardless of relevance, while Vue 3 explicitly omits it for non-first-admin creation — a deliberate-but-divergent parity gap and latent footgun if the server-side has_any_users gate is ever loosened; bool(telemetry_enabled) in user_admin.py would silently coerce a direct API caller's string "false" to True (both real clients send actual booleans, so this only affects hand-written API calls); and 03-system-config.spec.ts's new test claims to confirm "the default saved during first-run" but can't actually distinguish that from the UI's own default, since both happen to be True.

Strengths: the has_admin_user gate itself is correctly placed and doesn't race with existing installs (verified the settings-callback deferral via IOLoop.add_callback and the __init__-time load order). The new unit tests in test_users.py and test_telemetry_service.py exercise real behavior, not trivial mocks. Docs and e2e coverage were added alongside the feature as this project's own conventions ask for — the gaps found are in the details, not the approach.

🤖 Generated with Claude Code

[Tim020]

Important: this link can't actually be reached during first-run — clicking it just shows the same "create an admin user" screen again, not the help page.

target="_blank" opens /help/telemetry in a new tab, which loads the app fresh. App.vue (lines 119-121) gates on settings.has_admin_user === false before any route is matched — it renders the first-run <CreateUser> block unconditionally whenever no admin exists yet, regardless of path. Since no admin exists yet (that's the whole premise of this screen), the new tab shows "Welcome to DigiScript / create an admin user" instead of the privacy disclosure. The new docs text ("The choice is presented right there... nothing is ever sent before that screen has actually been completed") implies this link works, but it's unreachable at exactly the point someone would want to check it before deciding.

Fix: add a route exception to the first-run gate in App.vue, e.g. v-else-if="settings?.has_admin_user === false && !route.path.startsWith('/help')" (adjust to however route/$route is accessed in this component), so /help/* can render normally even pre-first-run. A short inline summary or modal would also work and avoids touching the routing gate at all.

🤖 Generated with Claude Code

[Tim020]

Important: same issue as the Vue 3 client — this link is unreachable during first-run.

App.vue (line 113: v-else-if="SETTINGS && SETTINGS.has_admin_user === false") gates unconditionally on has_admin_user, before any route is matched, so opening /ui-old/help/telemetry in a new tab during first-run just re-renders the "create an admin user" screen instead of the help page. See the companion finding on client-v3/src/components/user/CreateUser.vue:43 for the full explanation — the fix needs to land in both clients' App.vue for parity.

🤖 Generated with Claude Code

[Tim020]

Important: this new call starts a real TelemetryService on every first-admin creation in the backend test suite, with no cleanup — a real resource leak, not just a theoretical one.

/api/v1/auth/create (via controllers/api/v1/auth/user.py) and /api/v2/users both route through this function, and DigiScriptTestCase._create_and_login_admin — used across roughly 77+ call sites in the backend test suite — hits exactly this path. Nothing sets DIGISCRIPT_TELEMETRY=0 anywhere in the pytest environment (pyproject.toml, conftest.py, and python-test.yml are all silent on it), and telemetry_enabled defaults to True, so start_telemetry_service() genuinely starts a live service in every such test. conftest.py's tearDown never calls stop_telemetry_service(), so each test leaks:

• An _ErrorCountHandler added to get_logger() — confirmed this returns the module-level logger singleton (digi_server/logger.py), shared across every test in the same pytest-xdist worker process, so these accumulate for the life of the worker.
• An unclosed AsyncHTTPClient(force_instance=True).
• A scheduled first check-in after random.uniform(0, 3600) seconds — individually low-probability, but across hundreds of tests per CI run, not negligible, and a real network POST to the live collector from CI would be a privacy/correctness problem in its own right.

This is exactly the class of "shared global state across tests" hazard this project's own test-isolation conventions (see CLAUDE.md) are otherwise careful about.

Two smaller, related points while you're in this block:

• No try/except around start_telemetry_service() here — a failure (unlikely, but see the analogous RBAC-grant fix from round 3) would turn an already-successful user creation into a reported 500.
• bool(telemetry_enabled) at line 115 coerces non-bool input — a direct API caller sending the string "false" gets coerced to True. Both real clients send actual booleans, so this only affects hand-written API calls, but worth a type check or letting the setting's own validation reject it.

Fix for the main issue: set DIGISCRIPT_TELEMETRY=0 in DigiScriptTestCase.setUp (restoring/clearing it in tearDown), and/or call self._app.telemetry_service.stop() in conftest.py's tearDown if it's set.

🤖 Generated with Claude Code

[Tim020]

Suggestion: this client always sends telemetry_enabled on every user-creation request, unlike client-v3's CreateUser.vue, which explicitly omits the field unless isFirstAdmin (...(props.isFirstAdmin ? { telemetry_enabled: ... } : {})).

This is confirmed deliberate (per the comment here) and currently harmless, since the server only reads the field inside if not has_any_users: — but it's a parity divergence between the two clients for the same feature, and a latent footgun if that server-side gate is ever loosened or refactored (every subsequent user creation would silently resend telemetry_enabled: true, which could unintentionally re-enable a setting an admin had since turned off, if the server ever started trusting this field more broadly).

Fix: build the request payload explicitly, matching client-v3's pattern: { username, password, is_admin, ...(this.isFirstAdmin ? { telemetry_enabled: this.state.telemetry_enabled } : {}) }, and update the comment accordingly. Not blocking, but worth the parity fix while this is fresh.

🤖 Generated with Claude Code

[Tim020]

Suggestion: this test's name and comment claim more than it actually proves.

"Reflecting the default saved during first-run" would also pass if the checkbox's value never reached the server at all — the UI's own default and the server's own default are both True, so this test can't distinguish "the first-run choice was persisted" from "the settings page just shows its own hardcoded default." It doesn't catch a regression where the first-run submission silently stopped writing the setting.

Fix: in 01-first-run.spec.ts, uncheck the telemetry checkbox before saving (safe now that e2e runs with DIGISCRIPT_TELEMETRY=0), then assert not.toBeChecked() here in 03-system-config.spec.ts. That actually proves the value round-trips through the save.

🤖 Generated with Claude Code