Repository navigation
fix(ci): stop Windows test runs inflating product telemetry - #4370
Conversation
|
Not a review — I cannot give this one a qualifying approval, and I want to flag one scoping point before it merges. On the login: this PR is authored by The scoping point. The PR's own title is correctly narrow — "stop Windows test runs inflating product telemetry". The framing in the triage room is broader: "fix the DAU inflation". Those are not the same claim, and this repo already documents a second, independent inflation source that this PR does not touch: So after this lands, CI-origin events stop, but person-id churn still inflates absolute counts. Anyone reading the merge as "DAU is now accurate" would be wrong — it is "one known inflation source is closed". Why it matters right now rather than in general: the current numbers are being read as a growth story — DAU has gone 31 → 113 over eight days with new installs at 52/65/80 on the last three. If some fraction of that was Windows CI, the trend shape changes once this lands, and the honest move is to expect a step down and not read it as a regression. Worth a line in the PR body so the next person to look at the dashboard is not surprised by their own fix.
|
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Reviewed at 2691a3209 (exact head), scoped to Windows workflow correctness per Rui's assignment.
Workflow file correct: .github/workflows/windows-runtime.yml adds a genuinely workflow-level env: / SUTANDO_TELEMETRY: "0" block (top-level, column 0, before jobs: — not nested under a job or step), so it applies to every job/step in the workflow. Confirmed by reading the actual file, not just the diff hunk.
Guard extension verified for real, both directions:
- Ran the updated
tests/telemetry-test-optout-guard.test.pyat head: 7/7 pass, including the newwindows-runtime.yml sets SUTANDO_TELEMETRY=0check. - Negative control: reverted just the workflow file back to its pre-PR content (keeping the updated test) and re-ran — guard correctly FAILS with
windows-runtime.yml sets SUTANDO_TELEMETRY=0as the named failure. Confirms the guard actually detects the regression this PR fixes, not just that it passes now.
One structural note, not a blocker: the guard's workflow-level-env detection is two independent regex searches (a top-level ^env:$ line exists somewhere, and an indented SUTANDO_TELEMETRY: "0" line exists somewhere) rather than one anchored match confirming the indented line is actually inside that specific env block. In windows-runtime.yml they are correctly adjacent, so this PR is fine — but the looseness is pre-existing (shared by the ci.yml/python39-compat.yml checks this PR didn't touch), not introduced here. Not gating on it since it's out of this PR's scope.
Diff is exactly the 2 files described, no production runtime or client code touched. Approving on the Windows workflow scope.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 89%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
|
Windows CI result at unchanged head The exact PR base Python standalone tests passed. Remaining required CI is still running. The branch rules do not require |
qingyun-wu
left a comment
There was a problem hiding this comment.
Scope: telemetry regression evidence; Windows workflow scope is Bassil's. Reviewed at head 2691a3209 (base main, merge-base e23d86059), diff cut against the merge-base: 2 files, +6/-2.
Verdict: ready to merge on the telemetry scope — APPROVE. No blockers; one non-blocking note at the end.
1. Mechanism (send path and guard)
- Send path a Windows CI run reaches:
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:3263-3264(task_processed(bucket_source(...), "remote")once per newly queued task) →src/telemetry.py:253capture→:236-238enabled()=bool(_KEY) and not opted_out()→:344-368_dispatch→ daemon-thread_postat:241._KEYfalls back to the embedded public project key (src/telemetry.py:63-65), so on a fresh runner with no env at allenabled()is already True. On a non-darwin runner the install id resolves to~/.local/share/sutando/telemetry-id(:106-110), which does not exist on a fresh runner, so every run mints a new distinct_id — consistent with "each runner appears as a new install". - Gate:
opted_out()atsrc/telemetry.py:113-122returns True forSUTANDO_TELEMETRYin{0,false,no,off}, checked live on every call. The PR sets that variable workflow-wide at.github/workflows/windows-runtime.yml:9-10(top-levelenv:, beforejobs:), and pins it attests/telemetry-test-optout-guard.test.py:39-45. - Propagation: the one subprocess the gateway suite spawns builds its env as
dict(os.environ, ...)(src/remote-gateway-bridge.test.py:560), so a workflow-level variable is not scrubbed on the way down.
2. Reproduced the 34 → 0 claim
Probe = the PR body's script plus two hardening changes: SUTANDO_STATE_DIR is also pinned to the tempdir (so a host-local telemetry-disabled marker in <workspace>/state cannot fake a zero — src/telemetry.py:131-132 checks that location too), and a positive control capture('probe_control') runs before the suite to prove the interception seam counts when not opted out. Run from each checkout root:
# HEAD 2691a3209
env -u SUTANDO_TELEMETRY python3 probe-4370.py
RESULT optout=unset control_send=1 suite_exit=1 intercepted_product_events=34 events=['task_processed']
SUTANDO_TELEMETRY=0 python3 probe-4370.py
RESULT optout=0 control_send=0 suite_exit=1 intercepted_product_events=0 events=[]
# base origin/main e23d86059 (separate worktree)
env -u SUTANDO_TELEMETRY python3 probe-4370.py
RESULT optout=unset control_send=1 suite_exit=1 intercepted_product_events=34 events=['task_processed']
SUTANDO_TELEMETRY=0 python3 probe-4370.py
RESULT optout=0 control_send=0 suite_exit=1 intercepted_product_events=0 events=[]
34 with the variable unset, 0 with it set to 0, identical at base and head (the PR does not touch the emitting code, so the before/after is the environment, which is exactly what the workflow change controls). The control shows 1 when not opted out, so the zero is a measured zero.
Disclosure on suite_exit=1: on this macOS host src/remote-gateway-bridge.test.py reports FAILED (8) — all eight are proactive-delivery cases ([channel: !room] nudge delivers to the routed room, unconfirmed proactive sends stop at initial+5 retries, …), and the FAIL set is byte-identical at e23d86059 and at head (diffed). So it is pre-existing and host-environmental here, not this PR; the author's suite_exit=0 and the Windows CI job both show the suite green on their hosts (see §5). 310 checks pass around them and the 34 task_processed events are emitted regardless.
3. The guard cannot suppress telemetry in production
- The gating variable is
SUTANDO_TELEMETRY, a product-specific name — notCI,GITHUB_ACTIONSor another generic flag.src/telemetry.pyreads 9 env vars and none of them isCI/GITHUB_ACTIONS/RUNNER_OS(grep with the 9-hit count as the positive control). - Nothing in the production tree sets it:
grep -rn 'SUTANDO_TELEMETRY\b' src scripts skills packagesminus tests andtelemetry.pyitself returns exactly one setter,scripts/coverage-gate.sh:37(a test runner). A workflow-levelenv:exists only inside a GitHub Actions run. - The exact trigger is pinned both ways in
tests/telemetry-test-optout-guard.test.py:=0 → opted_out() True / enabled() False(:71-73) and the positive control "key present, no opt-out → enabled() True" (:77-79).
4. Regression side — telemetry still proven to work
SUTANDO_TELEMETRY=0 python3 tests/telemetry.test.py→ALL PASS (24 checks), rc=0;env -u SUTANDO_TELEMETRY python3 tests/telemetry.test.py→ALL PASS (24 checks), rc=0 (rc captured directly, not through a pipe).- Guard at head:
ALL PASS (7 checks). Negative control — workflow file reverted toorigin/maincontent, updated test kept:FAIL windows-runtime.yml sets SUTANDO_TELEMETRY=0 / 1 FAILURE(S); restored afterwards, tree clean. The new check discriminates.
5. Gates and CI
bash scripts/review-checks.sh --diff <diff-file>→ PASS, rc=0 (hardcoded-paths + root-artifacts + prose-cap).bash scripts/lint-workspace-resolution.sh --diff <diff-file>→ rc=0. Added lines scanned by hand for/Users/,/home/,~/.,/usr/bin/: none.ci-triage.py 4370:PowerShell + Windows runtimeis FAILING on this head (run 35247335012). I read the failed step: it iscore-heartbeat.test.py(TestHeartbeatCli.test_once_flag_writes_single_beat_and_exitsFAIL + nineTestHeartbeatWriteERRORs). The same job onmainat the merge-basee23d86059(run 35246724282) fails with the identical FAIL/ERROR set, and the three most recentmainruns are all red — pre-existing, outside this PR's two files. In both runs the gateway suite itself printsPASS — all checks green, so the workflow-level env was exercised on a real Windows runner against the emitting suite. The other two checks were still pending when I read them.
Worst-case disruption
None for users: the variable is set only inside the GitHub Actions run; no production runtime file changes; reverting the workflow restores the leak, nothing else. john-the-dev's scoping note stands — this closes the CI-origin inflation source only, and the person-id churn upper-bound noted at scripts/posthog-usage-report.py:17-19 is untouched.
Not verified
- The PostHog-side "34-event production burst" — I have no PostHog access; I verified the local emission count that would produce it.
- The exact Windows id-file path on the runner is inferred from
src/telemetry.py:106-110(sys.platform != "darwin"branch), not observed in a runner log. - Non-blocking, out of scope:
.github/workflows/claim-machine-nightly.ymlalso runs python tests without the opt-out; the two suites it runs andsrc/outbox*.pycontain no telemetry reference, so it does not emit today, but the guard's fixed workflow tuple would not catch it if that changed (Bassil's regex looseness note is the same family).
-- Sutando core (qingyun-001)
qingyun-wu
left a comment
There was a problem hiding this comment.
The telemetry change itself is correct: the Windows workflow now opts out at workflow scope, and the seven-case opt-out guard passes locally. I found no code issue in this two-file diff.
Approval is withheld because the exact-head Windows job is red: core-heartbeat.test.py cannot import core_lineage.py due the unconditional fcntl import on Windows. That is the separate main-branch defect targeted by #4371, not a failure introduced here, but this PR still needs a green Windows run on its own head before approval.
Reviewed by Qingyun's Personal Codex.
Windows runtime CI runs the real gateway test suite without opting out of product telemetry. Every fresh runner can therefore appear as a new active installation in PostHog. The existing telemetry guard only checked the Linux and Python 3.9 workflows.
This adds workflow-wide
SUTANDO_TELEMETRY: "0"to Windows runtime and includes that workflow in the existing regression guard. Review.github/workflows/windows-runtime.ymlfirst, thentests/telemetry-test-optout-guard.test.py.Evidence
Confirmed on current main
e23d86059. Windows run 35244189661 queued exactly 34 mock gateway tasks during 16:10:21–16:10:29 UTC on September 17, matching a 34-event production telemetry burst. The workflow has no telemetry opt-out, unlikeci.yml.With the updated regression test but the parent workflow:
With the workflow fix:
I also ran the complete real
src/remote-gateway-bridge.test.pytwice, intercepting onlytelemetry._postso no diagnostic events could reach PostHog. With the Windows workflow's old environment and its new environment respectively:Reproduce the boundary check from this checkout, once with
env -u SUTANDO_TELEMETRY python3 ...and once withSUTANDO_TELEMETRY=0 python3 ...:Validation and scope
SUTANDO_TELEMETRY=0 python3 tests/telemetry.test.py: ALL PASS (24 checks), including opt-out mechanisms and positive controls.git diff --check: clean.bash scripts/review-checks.sh --diff <(git diff origin/main...HEAD): PASS (hardcoded-paths, root-artifacts, prose-cap).