ENG-2055: stop the test suite sending real analytics to production

[alecantu7]

What

One line in tests/conftest.py: os.environ.setdefault("ANTON_ANALYTICS_ENABLED", "false") → a plain assignment.

Why

setdefault writes only when the key is absent. A developer with ANTON_ANALYTICS_ENABLED exported — exactly what someone working on an analytics ticket sets — cancelled the guard silently, and the suite has been posting real events to production PostHog since ENG-1288 shipped on 2026-08-06.

This is the source of the ~70% contamination in turn_completed. The fake rows carry a fingerprint that cannot occur in production — planning_model renders as <AsyncMock name='mock.planning_model' id='…'>, which only make_mock_llm produces. Counted per day, the AsyncMock rows are the script-shaped rows:

Day	script-shaped	AsyncMock
2026-08-27	2,147	2,147
2026-08-25	2,580	2,580
2026-08-19	2,880	2,882

It is also ENG-1964's April complaint, still live: project 355390 holds 41,121 ds_connect_attempt in 14 days across 350 installs — 117 per install, which no real user does. That ticket was filed at ~38% of volume and cancelled in May; four months on it is worse.

ENG-1692 does not cover this. Its guard sits inside _emit_turn_cost alone, so the other families are unprotected.

259 is a floor, not the total (self-review F2). anton has 15 send_event call sites across 9 files emitting ~10 distinct names, of which four route to the direct PostHog sink: turn_completed, tool_completed, rule_retrieval, scratchpad_package_installed. The run above exercised four names; rule_retrieval (cortex.py:89) and scratchpad_package_installed (core/utils/scratchpad.py:100) never fired — not because they are guarded, but because their test paths pass settings objects that fail inside send_event's own try/except before sending. A run that exercised those paths would emit more.

Delivery, stated precisely (self-review F3). This is cheaper than ENG-1692 but not free. conftest.py is read from the developer's checkout, so it takes effect on the next test run after a pull or rebase — no package reinstall, unlike ENG-1692, but a developer on a long-lived branch cut before today does not get it automatically.

CI was never the source — analytics.py::_is_ci() already drops GITHUB_ACTIONS and friends. The leak is local developer runs.

Verification — mutation-verified against a live capture server

Both runs on this branch, same venv, same environment, ANTON_ANALYTICS_ENABLED=true exported, with ANTON_POSTHOG_HOST and ANTON_ANALYTICS_URL pointed at a local HTTP server that logs every GET and POST:

	Result	Events captured
With this fix	2,709 passed, 31 skipped	0
Mutated back to setdefault	2,709 passed, 31 skipped	259

The 259: tool_completed 149, ds_connect_* 89, ask_user_* 16, turn_completed 5.

The test count was identical in both directions on the first commit, which is the point worth noticing — no test observed this guard, so it could never have failed loudly. That is why it went unnoticed for four months, and why the check above is a capture server rather than a code reading.

That gap is now closed. 54f4f816 adds test_the_suite_kill_switch_beats_an_exported_variable, which spawns a child process with the variable exported before the interpreter starts — the only shape in which the bug is visible, and one an in-process assertion cannot reach on a clean machine. Mutation-verified: restoring setdefault fails it with enabled=True. Full suite now 2,711 passed / 31 skipped.

env -u ANTON_IS_CI -u GITHUB_ACTIONS ANTON_ANALYTICS_ENABLED=true \
    ANTON_POSTHOG_HOST=http://127.0.0.1:8899 \
    ANTON_ANALYTICS_URL=http://127.0.0.1:8899/collector \
    .venv/bin/python -m pytest tests/ -q

Nothing loses coverage

The shell-level escape hatch is removed deliberately — and only that one. monkeypatch.setenv still re-enables analytics inside a test, because it runs long after conftest import and AntonSettings reads the environment at construction. That is asserted by test_a_test_can_still_re_enable_analytics_for_itself, so nobody restores the opt-out believing this case was lost with it (self-review F4).

The tests that exercise the analytics layer construct their own settings objects and never read the environment — test_analytics.py::_Settings and ::S, test_tool_completed.py::_PosthogSettings, test_tool_outcome_tracking.py's SimpleNamespace. tests/e2e/harness.py:164 already sets the variable explicitly per subprocess rather than relying on inheritance.

This is the same call cowork-server's own conftest already makes for the database: "Force isolation (assignment, not setdefault, never touch a real DB)."

Expected side effect — not a regression

turn_completed volume should fall by roughly two thirds as developers pull this. That is the fix working. The drop arrives per machine, so expect a few days rather than a cliff, and any alert floor calibrated before it was calibrated on contamination.

The polluted history is deliberately left alone; historical reads keep using the behavioural filter.

Out of scope

Extending the script guard to tool_completed — 149 of the 259 are tool rows and this stops them at the source. A guard in the product code would change a live event whose baseline is three weeks old, need a rule invented separately for ds_connect_*/ask_user_* (which fire outside a turn, so there is no llm_calls to test), and drop rows unauditably. Revisit only if residue survives.

Langfuse was never affected — a mocked LLM never reaches the gateway, so no trace exists.

Security check

Performed. The change strictly reduces data egress: it stops a test process sending to an external service. No credential, authz, endpoint, input-validation or deserialization surface is touched, and no secrets appear in the diff or in history.

Closes ENG-2055.

🤖 Generated with Claude Code

[alecantu7]

Adversarial self-review (ENG-2055)

Reviewed at head 009c5957 against fresh origin/staging (87918945). Full suite run twice on this branch — with the fix: 2,709 passed, 0 events; mutated back to setdefault: 2,709 passed, 259 events. Writer inventory for ANTON_ANALYTICS_ENABLED run across all seven repos in the workspace.

Verdict: safe to merge on correctness. One finding worth fixing first (inline, on conftest.py), plus three corrections to my own PR body below.

What survived refutation

F2 — the body understates the emitter surface. The Why section says "three of the four event families are unprotected". anton actually has 15 send_event call sites across 9 files, ~10 distinct event names, of which four route to the direct PostHog sink: turn_completed, tool_completed, rule_retrieval, scratchpad_package_installed. My capture run observed four names; rule_retrieval (cortex.py:89) and scratchpad_package_installed (core/utils/scratchpad.py:100) never fired — not because they are guarded, but because their test paths pass settings objects that fail inside send_event's own try/except before sending. 259 is a floor, not the total.

F3 — "works on every build, immediately" is overstated. conftest.py is read from the developer's checkout, so this needs a pull or rebase. The contrast with ENG-1692 is still real and worth keeping — that one needs a package reinstall — but a developer on a long-lived branch does not get this automatically.

F4 — "the escape hatch is removed" invites the wrong conclusion. Only the shell-level hatch goes. monkeypatch.setenv still works inside a test; I verified it. As written a reviewer could reasonably conclude the analytics path is no longer testable from the suite, which is false.

What did NOT survive (checked and killed)

• A .env file could still enable it. Verified live: env false + .env saying true → analytics_enabled = False. Env beats .env in pydantic-settings.
• The assignment leaks into subprocesses. setdefault mutated os.environ identically — no change. Both subprocess paths pass explicit values (test_analytics.py's generated script uses class attributes; tests/e2e/harness.py:164 sets the variable itself).
• The named settings classes read the env, so the coverage claim is false. Checked all four. _Settings and _PosthogSettings are plain classes with hardcoded values, and both tests monkeypatch _fire/_fire_posthog so nothing leaves the process. S is a subprocess script pointed at 127.0.0.1. test_tool_outcome_tracking.py's SimpleNamespace(analytics_enabled=True) has no analytics_url, so send_event raises into its own guard and sends nothing.
• Another repo sets this variable. It appears only in anton — README, CLI help, settings, docs, and the two test files. No CI workflow sets it.

Unverified

I proved the mechanism and reproduced the fingerprint locally, but I have no access to the two machines producing the production traffic. If their leak has a different cause, this closes a real hole and the 70% persists. The ticket's ds_connect_attempt criterion is the falsifiable check.

run-tests had not completed when I reviewed; my local run of the same suite on this head passed 2,709 / skipped 31 / failed 0.

[alecantu7]

F1 — medium. Nothing in CI catches a regression of this line.

I grepped origin/staging and this head: no test references ANTON_ANALYTICS_ENABLED outside this file and tests/e2e/harness.py:164. Both of my suite runs returned the same counts — 2,709 passed with the assignment, 2,709 passed mutated back to setdefault. No test observes this guard in either direction.

That is exactly the property that let the leak run for four months, and this PR leaves it intact. A merge conflict, a revert, or a well-meaning "restore the opt-in" reopens it with a green CI.

The cheap version is a partial guard — it only fails on a machine that already has the variable exported:

def test_analytics_is_off_for_the_whole_suite():
    assert os.environ["ANTON_ANALYTICS_ENABLED"] == "false"
    assert AntonSettings().analytics_enabled is False

The airtight version spawns a child process with the variable exported, imports this conftest the way pytest does, and asserts the setting still resolves False — so it fails on every machine when the line regresses. tests/test_analytics.py already has the subprocess + _CI_MARKERS scaffolding to build on (_run_child, _capture_endpoint).

Follow-up commit incoming.

[alecantu7]

Self-review findings addressed

F1 (medium) — fixed in 54f4f816. test_the_suite_kill_switch_beats_an_exported_variable spawns a child process with ANTON_ANALYTICS_ENABLED=true exported before the interpreter starts, imports this conftest the way pytest does, and asserts AntonSettings().analytics_enabled still resolves False. An in-process assertion could not do this — by the time any test runs, conftest has already executed in an interpreter the test did not control, so on a clean machine (which is what CI provides) it would pass for the wrong reason.

Mutation-verified: restoring os.environ.setdefault(...) fails it with - enabled=False / + enabled=True; the fix passes it. Reuses test_analytics.py's existing subprocess + _CI_MARKERS scaffolding rather than introducing a second pattern.

A second test pins the other half — monkeypatch.setenv still re-enables analytics inside a test — so nobody restores the escape hatch believing that case was lost with it.

Full suite: 2,711 passed, 31 skipped.

F2, F3, F4 — corrected in the PR body. The emitter surface is now stated as 15 call sites / ~10 names / 4 direct-sink events with 259 called out as a floor; the delivery claim now says "next test run after a pull or rebase" rather than "immediately"; and the escape-hatch paragraph now says shell-level and points at the test that proves the monkeypatch path survives.

Ready for human review.

[tino097]

Code review

Found 1 issue, inline on the new subprocess helper.

The core change (setdefault -> assignment) checks out. I ran the suite at this head (2711 passed, 31 skipped) and confirmed the new kill-switch test fails with enabled=True when the setdefault form is restored, so the mutation-verification claim in its docstring holds rather than being aspirational.

Also checked and dismissed: nothing else in the repo relies on the old only-if-absent behavior (tests/e2e/harness.py builds its subprocess env explicitly, and the analytics tests construct their own settings objects rather than reading the environment), and the "three of those four families" note in tests/conftest.py is accurate for the four families it enumerates.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[tino097]

This block is inert here, and the comment justifying it is inaccurate for this code path.

_CI_MARKERS only matter to _is_ci(), which is consulted inside send_event in anton/analytics.py#L499-L501. The child spawned here never calls send_event — it imports conftest and prints AntonSettings().analytics_enabled, and that field resolves purely from ANTON_ANALYTICS_ENABLED via pydantic's env_prefix. No CI marker is read anywhere on that path. So leaving a marker set would not change this test's result, and the stated reason ("would make this pass for the wrong reason on CI and fail locally") does not hold.

It reads as a copy of the same block in _run_child above, where the child does call send_event and the pop is genuinely load-bearing.

Worth fixing rather than leaving: this is the regression test for a bug whose entire character was a guard that looked effective and silently wasn't. A comment asserting a guard that does nothing works against exactly the property this test exists to establish.

Smallest fix is to drop both:

Suggested change

	# The suite may itself run under GitHub Actions. `_is_ci()` is a *separate*
	# guard that would drop the traffic anyway, so leaving a marker set would
	# make this pass for the wrong reason on CI and fail locally.
	for marker in _CI_MARKERS:
	env.pop(marker, None)

Alternatively keep the pop and reword the comment to say the markers are cleared for parity with _run_child, not because they affect this assertion.

[alecantu7]

Confirmed and fixed in fb0b3395 — you're right on both halves, and I checked it rather than taking it on the reading.

analytics_enabled is a plain field on AntonSettings (anton/config/settings.py:147) resolved from ANTON_ANALYTICS_ENABLED via env_prefix. _is_ci() is referenced in exactly one place, send_event at analytics.py:500, and this child never gets there. Measured each marker individually and all seven together:

Child env	fix	setdefault mutant
markers popped	enabled=False	enabled=True
every _CI_MARKERS entry set	enabled=False	enabled=True

Inert in both directions, so the comment's stated failure mode ("pass for the wrong reason on CI and fail locally") cannot occur. Copied from _run_child, exactly as you read it.

I took the reword rather than the bare delete, for the reason you gave — this is the regression test for a guard that looked effective and wasn't, so the useful thing to leave behind is why there is no scrub here when the helper 60 lines up has one. Otherwise the next person restores it by symmetry. The comment now names send_event as what makes the scrub load-bearing in _run_child and records the both-directions result.

One thing your comment pushed me into that wasn't in scope: the same commit fixes a stale line in tests/conftest.py claiming this fix "works on every build, immediately". My own self-review (F3) had already corrected that in the PR body and I never carried it back into the code. Same defect class you flagged — a comment asserting something untrue — so it went with it.

Suite re-run at the new head against a local capture server, ANTON_ANALYTICS_ENABLED=true exported: 2,711 passed, 31 skipped, 0 events. Kill-switch test still mutation-verified — restoring setdefault fails it with enabled=True, with or without CI markers set.

[alecantu7]

Self-review at fb0b3395

Re-ran the ticket's acceptance criterion from scratch rather than trusting it — full suite, local capture server, analytics forced on, both directions.

conftest line	events captured	tests
os.environ.setdefault(...) (mutant)	260	1 failed, 2710 passed
os.environ[...] = "false" (this PR)	0	2711 passed, 31 skipped

Family split under the mutant: tool_completed 149, ds_connect_* 89 (the ticket's figure exactly), ask_user_* 15, turn_completed 5. The new test is genuinely mutation-verified — restoring setdefault fails it with enabled=True, and it fails inside a full-suite run rather than only in isolation.

Supporting claims checked and holding:

• Nothing bypasses send_event. All 15 emitters funnel through it, and the analytics_enabled gate at analytics.py:496 sits above the branch to either sink — so the single line covers both the direct PostHog path and the collector path. This was the one that mattered: if the direct sink had its own gate, the fix would have been partial.
• harness.py sets the variable explicitly in stub mode, and in live mode env.update(common) applies it on top of the inherited environment — so the subprocess claim holds in both modes, not just the one.
• No other os.environ.setdefault anywhere in tests/.
• The cited cowork-server precedent is real and verbatim at cowork-server/tests/conftest.py:14.

Finding 1 (medium) — the guard is one layer; the precedent it cites is seven

That cowork-server conftest does not merely prefer assignment to setdefault. It forces seven variables and redirects every sink to a temp dir. This PR takes the "assignment" half and leaves ANTON_ANALYTICS_URL and ANTON_POSTHOG_KEY pointing at production for the whole suite, which is safe today only because of one if at analytics.py:496.

That is the uncomfortable part. This bug exists because ENG-1288 added a new emitter that reached a real sink. The next one — or any refactor that moves that check — reopens it, and the new test would not notice, because it asserts on AntonSettings().analytics_enabled rather than on "no bytes left the process". The ticket's own lesson is that reading the code is how this was missed for four months; asserting on a resolved setting is the same category of evidence.

Demonstrated, with the enabled-check bypassed to stand in for that future emitter:

PR version (flag only):           analytics_enabled=False  url='http://…/collector'  key=set    -> 2 events
hardened (flag + sinks blanked):  analytics_enabled=False  url=''                    key=empty  -> 0 events

Two lines, both already documented in analytics.py as supported kill switches — "Blanking analytics_url has always stopped EVERY event", and ANTON_POSTHOG_KEY="" disables the PostHog sink specifically. Full suite with them added: 2711 passed, 31 skipped, identical to without. No coverage cost.

Pushing this as a follow-up commit.

Finding 2 (low) — the family table is missing one

The ticket and the conftest comment both say four families and 254 events. My run captured five: the same four plus scratchpad_package_installed. Minor in volume, but the "Out of scope" section reasons by enumerating families, so the fifth belongs in the list before anyone judges what residue is left.

Finding 3 (low) — the sibling repos have no guard at all

cowork-server/tests/conftest.py and both scratchpad-controller conftests contain no analytics guard of any kind. The ticket's measurement of cowork-server at zero is correct but incidental — nothing stops its tests emitting; they simply never drive a turn to an emitter today. cowork-server vendors anton, so the first test that does drive one leaks with nothing to catch it, which is precisely the shape of this bug. Worth a one-line guard there rather than relying on the measurement staying true.

Separately, and not covered by the ticket: cowork_evals has no conftest at all and runs anton for real. If eval runs carry a developer's aid, some share of the 41,121 ds_connect_attempt in project 355390 is evals rather than the test suite — which would mean the volume does not fall as far as the Done when predicts, and someone reads a partial drop as the fix having failed. Worth naming in the ticket so the follow-up measurement is interpreted correctly.

[alecantu7]

Verification of 24178e4c, and one refutation

Checked the follow-up independently rather than re-reading it. Full suite at 24178e4c against a local capture server, ANTON_ANALYTICS_ENABLED=true exported: 2,711 passed, 31 skipped, 0 events.

Finding 1 holds and the commit implements it correctly. The two things that could have made it a no-op both check out. The field names are right — analytics_url and posthog_key at settings.py:148,167, so ANTON_ANALYTICS_URL / ANTON_POSTHOG_KEY resolve through env_prefix. And the gate ordering is what the commit assumes: if not settings.analytics_url: return at analytics.py:522 sits above the _POSTHOG_EVENTS branch, so blanking the URL stops the direct sink too, not just the collector. Nothing in the suite is broken by it — the subprocess children use hardcoded settings classes (_SHORT_LIVED_CHILD's S), not inherited env, so blanking in the parent cannot reach them.

New finding (medium) — the fix makes the ticket's own acceptance check vacuous

Done when #1 is "running the suite with ANTON_ANALYTICS_ENABLED=true exported produces zero requests, asserted against a local capture server." That check no longer discriminates, because conftest now overrides the ANTON_ANALYTICS_URL the repro command sets.

Measured, ticket's documented repro verbatim, only the flag line mutated back:

conftest state	capture server	suite
pre-PR (setdefault, no sink blanking)	259	1 failed, 2710 passed
setdefault + sink blanking retained	0	1 failed, 2710 passed
this PR	0	2711 passed, 31 skipped

Row 2 is the problem: the guard is defeated and the acceptance check still reads zero. Anyone re-running the documented repro to confirm the fix gets a green that would be green either way — the same "looks effective, silently isn't" shape this ticket exists to fix.

Not a defect in the code, and not an argument for reverting: the discriminating guard has simply moved into test_the_suite_kill_switch_beats_an_exported_variable, which fails in every row where the flag regresses and runs in CI, which the capture-server ritual never did. That is a better place for it. But Done when #1 and any QA step derived from it need to say so, or the fifth column of that table gets ticked off on a vacuous check.

Finding 2 — refuted

scratchpad_package_installed does not leak, so the four-family enumeration in the conftest comment is right as written. Full breakdown from the pre-PR mutant run above — 259 events, 12 event names, four families:

Family	Count
tool_completed	149
ds_connect_* (success 44 / attempt 40 / failed 5)	89
ask_user_* (7 names)	16
turn_completed	5

Matches the PR body's 149/89/16/5 exactly. scratchpad_package_installed is absent, and the code says why: all six of its tests monkeypatch anton.analytics.send_event itself (test_tool_outcome_tracking.py:224,246,267,294,338,358) or pass _PosthogSettings with _fire_posthog patched (test_analytics.py:181), so it never reaches the wire. The first self-review had this right — scratchpad_package_installed and rule_retrieval never fire — and the later one contradicted it. The only stale figure is the conftest comment's 254 (147/89/16/2), which is a real measurement from a pre-branch build; the PR body already carries the corrected 259.

Finding 3 — agreed, but it belongs in its own ticket

The sibling-repo gap is real and the reasoning is right: cowork-server measuring zero is incidental, not guarded. But it is a change to two other repos, so per convention it wants its own ticket rather than widening this diff.

The cowork_evals half is the part worth carrying into ENG-2055 before anyone reads the follow-up measurement, since it changes how the ds_connect_attempt criterion is interpreted: if some of the 41,121 is eval runs rather than the suite, the volume will not fall as far as Done when predicts, and a partial drop is not evidence the fix failed.

---

Note on review state: the approval is on fb0b3395; 24178e4c landed after it, so the approving reviewer has not seen the sink-blanking commit. All checks green at head (run-tests 4m28s). Leaving the merge to a human.

[alecantu7]

Correcting the Finding 2 refutation — it is five families, and the fifth has a name

The refutation above is wrong, and the reason is worth more than the count.

Re-ran on a clean checkout at 24178e4c, conftest reverted to the pre-PR state, ticket's repro verbatim: 260 events, scratchpad_package_installed present — matching the earlier run exactly, so 260/five is reproduced twice and 259/four once.

Rather than trade counts, I instrumented the emitter with PYTEST_CURRENT_TEST and re-ran. Five tests reach send_package_install_event:

test_tool_outcome_tracking.py::TestPackageInstallTelemetry::test_install_action_also_sends_the_event
test_tool_outcome_tracking.py::TestPackageInstallTelemetry::test_install_success_with_retry_warning_is_not_a_failure
test_tool_outcome_tracking.py::TestPackageInstallTelemetry::test_successful_install_sends_package_name_only
test_tool_outcome_tracking.py::TestPackageInstallTelemetry::test_valid_specifiers_still_install
test_scratchpad_observer_dispatch.py::TestHandleScratchpadObserverIntegration::test_non_exec_actions_do_not_fire_observers

The refutation is right about the four it names — those TestPackageInstallTelemetry tests monkeypatch send_event, so they reach the emitter and fire nothing. The fifth is in a file the enumeration never opened. test_scratchpad_observer_dispatch.py contains zero occurrences of send_event, patch(, monkeypatch, and zero mentions of scratchpad_package_installed. It reaches send_package_install_event incidentally through the dispatch path, unpatched, and fires for real.

That is why the code reading missed it: the enumeration was built by grepping the event name, and a test that triggers an event without naming it is structurally invisible to that method. Which is this ticket's own lesson pointed back at itself — "Reading the code is how this was missed for four months."

Corrected in 6dc28139: the conftest comment now carries 260 across five families, names the caller, and records that it was found by instrumenting the emitter rather than by reading. The volume is trivial — one event — but the "Out of scope" section reasons by enumerating families, so an incomplete list makes that argument unsound.

The new finding lands, and it is a fair hit on the sink-blanking commit

Blanking the sinks does make Done when #1 non-discriminating: send_event returns at if not settings.analytics_url before the flag is consulted, so the documented repro reads zero whether or not the flag guard holds. That is a real cost of 24178e4c that I did not weigh — I checked the hardening broke no tests, not that it broke the acceptance check.

The conclusion is right: the discriminating guard has moved into test_the_suite_kill_switch_beats_an_exported_variable, which runs in CI, which the capture-server ritual never did. Worth splitting the criterion rather than deleting it, because two different properties are involved:

• "the flag guard holds" — now the CI test, and only the CI test.
• "nothing leaves the process" — still the capture server, which remains a real end-to-end check even though it no longer discriminates on the flag.

Finding 3

Agreed on both halves — the sibling-repo guard as its own ticket, and the cowork_evals caveat carried into ENG-2055 before anyone interprets the ds_connect_attempt follow-up, since eval traffic in that 41,121 would mean the volume does not fall as far as Done when predicts and a partial drop is not evidence of failure.

Review state

tino097 approved fb0b3395. Two commits have landed since — 24178e4c blanks the sinks, which is a behavioural change and the one with the acceptance-check consequence above, and 6dc28139 is comment-only. So that approval predates the change that actually alters what the suite does and wants a fresh look rather than a carry-over. All checks green at head.

[alecantu7]

Half right, and the half I got wrong is the useful half

The correction is right that my enumeration was unsound, and right about why. I built it by grepping the event name, which cannot see test_scratchpad_observer_dispatch.py::TestHandleScratchpadObserverIntegration::test_non_exec_actions_do_not_fire_observers — it passes {"action": "install", "packages": ["x"]} through the dispatch path and reaches send_package_install_event while containing no occurrence of send_event, patch(, monkeypatch, or the event name. Confirmed by instrumentation, not by reading:

SEND_EVENT action=scratchpad_package_installed settings_type=MagicMock

My "all six tests monkeypatch send_event" was an enumeration of the tests that name the event. That is exactly the blind method the correction describes, and the methodological point stands on its own.

But it does not leak, and the 260/five figure is an emitter count

Reaching send_event is not sending. I instrumented send_event and _spawn in the same full-suite run as the capture server, pre-PR conftest, ticket's repro verbatim:

Counted at	Total	Names	scratchpad_package_installed
the wire (capture server)	259	12	absent
the emitter (send_event calls)	277	16	present (2)

The wire number is 259 across four families — tool_completed 149, ds_connect_* 89, ask_user_* 16, turn_completed 5 — now reproduced twice, and matching this PR's own body exactly. The 18-event gap is calls that reach send_event and die inside its own try/except before anything is sent: scratchpad_package_installed 2, rule_retrieval 2, anton_started 1, anton_query 1, and the rest turn/ds overcounts.

So 260/five is an emitter count, not a wire count — and counted at the emitter it is not five families either, it is 277 across 16 names. Neither number is wrong; they measure different things, and only one of them reached production.

Why that specific event cannot reach the wire, traced to the line:

POSTHOG_BODY RAISED for scratchpad_package_installed:
TypeError: Object of type MagicMock is not JSON serializable

_fake_session is a MagicMock, so session._settings is a MagicMock, and _posthog_body cannot serialize it. Swallowed by send_event's except Exception: pass. Verified three ways: absent from both full-suite wire captures, zero events when that test runs alone under the fully reverted conftest, and the TypeError above.

The part of this that is actually worth having

That caller passes an explicit settings object, which means neither guard in conftest.py applies to it. settings.analytics_enabled is a MagicMock attribute, so it is truthy — the flag is not consulted in any meaningful sense. And the sink blanking only affects AntonSettings() resolving from env, which this path never does. It is stopped by a serialization error and nothing else.

That is the same shape as the original bug — a guard that looks like it covers a path it does not — except this one currently fails safe by accident. A caller that passed a more realistic settings object would send for real, past both guards, and no test would notice. Worth its own ticket alongside the sibling-repo one; it should not sit in this file reading as covered.

Corrected in 4b87cfc3

The conftest comment now carries both numbers, says which one to quote and why, keeps the grep-blindness lesson, and records the MagicMock corollary as an open hole rather than a covered case. Suite unchanged: 2,711 passed, 31 skipped.

One caveat on my own method, so it does not become the next correction: the combined run above reported a second failure, test_backend_launcher_env.py::test_build_env_without_existing_pythonpath. That is my instrumentation — I injected PYTHONPATH=/tmp to load the counting plugin, which is precisely what that test asserts on. It passes clean without it (4 passed).

[alecantu7]

Pre-merge adversarial review — 2 findings, both fixed in 021168af

Reviewed at 4b87cfc3 against fresh origin/staging (0d713edc, verified against the API, not the local clone). Base is staging, no sibling PRs. Verdict: low risk, safe to merge.

It cannot reach a user, and that is checked rather than assumed: pyproject.toml:68 packages ["anton", "anton_state"], so tests/ never enters the wheel; nothing in anton/ or anton_state/ imports conftest; and no sibling repo references anton's tests. The only surface it can break is anton's own suite, which is what CI covers.

Both findings were claim-accuracy, not behaviour — and both are the class this ticket exists to close.

F1 — the comment subtracted two populations that do not subtract

4b87cfc3 (mine) wrote "the other 18 reach send_event and die inside its own try/except". 277 − 259 = 18 treats an in-process counter and a wire counter as one population. They are not, in both directions.

The tell was already in my own numbers: the wire showed more tool_completed than the emitter (149 vs 148). No amount of swallowed exceptions produces that, and _fire_posthog is documented "no retry, no raise", so retries cannot either.

Cause, isolated and executed — test_cloud_turn_process.py:25 does env = os.environ.copy() and runs the real entrypoint as a child:

that file alone, pre-PR conftest:   WIRE 5   (turn_completed 3, tool_completed 2)
                                    EMITTER 0   send_event invocations

F1b — which turned up something the PR was not claiming credit for

Those subprocess children inherit os.environ, so the assignment reaches them. Same file, same command:

conftest	wire events
pre-PR	5
at head	0

This only works because it is an assignment. With setdefault and an exported true, children inherited true and sent for real. It also corrects my first self-review's "setdefault mutated os.environ identically — no change" — true only when the variable is absent, i.e. not in the case this ticket is about.

F2 — test_a_test_can_still_re_enable_analytics_for_itself passed for a narrower reason than it claimed

Its docstring promised a test could still "exercise the enabled path". Since 24178e4c blanked the sinks, flipping the flag re-enables the setting, not sending. Probe at head:

analytics_enabled = True
analytics_url     = ''
posthog_key       = ''
spawned           = []      <- send_event hands nothing to a sender thread

The test stayed green because it asserted the setting. That is the exact reassurance a future developer would follow before concluding the guard broke and restoring setdefault.

It also closed real coverage. Nothing tested the sink blanking — deleting those two lines from conftest was a silent no-op. The test now asserts at the sender, on both routes, so it fails if they are removed.

Mutation-verified both ways:

Mutation	Result
drop the two sink lines	test_a_test_can_still_re_enable_analytics_for_itself fails
restore setdefault	test_the_suite_kill_switch_beats_an_exported_variable fails

Full suite at 021168af: 2,711 passed, 31 skipped, 0 events against a local capture server with ANTON_ANALYTICS_ENABLED=true exported.

Checked and refuted — did not become findings

• Writer inventory (ENG-597 rule) for ANTON_ANALYTICS_ENABLED / ANTON_ANALYTICS_URL / ANTON_POSTHOG_KEY / ANTON_POSTHOG_HOST across every repo in MindsDB_Repos: every hit is inside an anton clone or worktree. No sibling repo reads or writes them, so the sink blanking cannot reach cowork-server, scratchpad-controller, cowork or cowork_evals. No CI workflow sets them either.
• Blanking breaks the subprocess analytics tests — it does not; _SHORT_LIVED_CHILD's S hardcodes url/key/host rather than inheriting.
• A test asserts on the default analytics_url / posthog_key — none does.
• Windows — tests.yml:19 is ubuntu-latest only, and tests are not packaged.

Not run

/security-review, judged disproportionate for a diff adding no production code, no input handling and no endpoint. Manual pass instead: the change strictly reduces egress; the blanked posthog_key is a public project token, not a credential (settings.py:156); no secrets in the diff or history.

Caveat on my own method

The combined instrumentation run needed PYTHONPATH=/tmp to load a counting plugin, which fails test_backend_launcher_env.py::test_build_env_without_existing_pythonpath — that test asserts on exactly that variable. Clean without it (4 passed). Not a PR defect.

---

Review state: tino097's approval is on fb0b3395, now five commits back, and 24178e4c (sink blanking) was the behavioural one. staging-freeze is currently enforcement=disabled and the PR reads MERGEABLE / CLEAN. Leaving the merge to a human.

[alecantu7]

Settled by measurement: it does leak, and the culprit is a third caller neither of us had named

Corrected in f0f91e23. We were each half right, and the half that was wrong was mine first.

I instrumented send_event itself — not send_package_install_event, which is exactly what made my earlier revision name the wrong culprit — and ran the full suite with conftest reverted to the pre-PR state. Three callers reach it with scratchpad_package_installed, with three different outcomes:

Test	settings	outcome
test_analytics.py::test_scratchpad_package_installed_goes_to_posthog_not_the_collector	_PosthogSettings, host ph.example.test	body builds, goes nowhere real
test_chat_scratchpad.py::TestScratchpadInstallViaChat::test_install_action_dispatch	real AntonSettings	body OK → POSTed to the capture server
test_scratchpad_observer_dispatch.py::TestHandleScratchpadObserverIntegration::test_non_exec_actions_do_not_fire_observers	MagicMock	TypeError: Object of type MagicMock is not JSON serializable, swallowed

Raw probe output for the middle one:

REACHED settings=AntonSettings key='phc_test' host='http://127.0.0.1:8899'
        test=tests/test_chat_scratchpad.py::TestScratchpadInstallViaChat::test_install_action_dispatch
  body OK -> would POST

Wire total 260, third run in a row.

What survives from your revision, unchanged. The MagicMock mechanism is exactly right and I watched it fire — send_event sails past both guards because every MagicMock attribute is truthy, then dies in _posthog_body. That is a real and non-obvious finding, and "reaching send_event is not sending" is the right lesson to write down.

What does not survive is generalising it to "it never leaked". It leaks from the middle caller. And the follow-up you flagged — "a caller that passed a more realistic settings object would send for real" — is not hypothetical: that caller is in the suite today and needs no more realistic object, because it already constructs a real AntonSettings and reads the environment.

My earlier correction was also wrong, in the same shape: I named the observer test as the leaker because I instrumented send_package_install_event, which is one route in. It reaches the emitter and dies. The event on the wire was never coming from it.

So the header now carries 260 across five families, all three callers with their outcomes, and keeps both traps — a name-grep cannot see the third caller, and reaching send_event is not sending — as separate points, since neither subsumes the other.

None of this changes merge safety. The fix stops all three: with the assignment plus blanked sinks the suite emits zero, scratchpad_package_installed included. The whole dispute was about how the header describes the pre-fix leak, and no runtime behaviour depends on it. It was worth correcting only because the previous wording told the next reader a class was closed while a real instance of it sits in the suite.

[alecantu7]

f0f91e23 is right, and both counts were right — it is cold-vs-warm workspace

The conclusion in f0f91e23 holds: scratchpad_package_installed does leak, from test_chat_scratchpad.py::TestScratchpadInstallViaChat::test_install_action_dispatch, which builds a real AntonSettings and reads the environment. My 259/four generalisation was wrong.

But the reason given for why other runs read 259 — "which its own ordering can hide" — is not the mechanism, and that matters, because as written nobody can reproduce the 260.

The real gate. tool_handlers.py:719:

result = await pad.install_packages(packages)
if install_call_installed_something(result):
    send_package_install_event(session, packages)

and the workspace fixture is a persistent directory in the repo, not tmp_path — test_chat_scratchpad.py:20:

base = Path(__file__).resolve().parents[1] / ".pytest-workspace"

So the first run on a machine really pip-installs cowsay and emits. Every run afterwards gets "already satisfied", the gate is false, and the event never fires again on that machine.

Verified by toggling only that state — same commit, same command, isolated test:

workspace	scratchpad_package_installed on the wire
moved aside (cold)	1
restored (warm)	0

.pytest-workspace/.anton/scratchpad-venvs/main/bin/cowsay was already present on my machine — the suite has run here many times this session. That is the whole discrepancy. Both measurements were correct on the machines that produced them; neither of us was mis-measuring.

Ordering cannot explain it: the gate is the state of a directory that outlives the run.

Corrected in 7b7fffe6 — the comment now carries the cold/warm split and the rm -rf .pytest-workspace step needed to reproduce 260. Without that step the next person measures 259 and concludes the comment is wrong, which is how this one question has now been reversed three times.

Two things that follow

For the ticket's Done when. The leak count is machine-state dependent. A verifier on a warm checkout sees 259/four and a fresh CI runner sees 260/five, and neither is evidence of anything being broken. Worth stating wherever the figure is quoted.

Unrelated to this PR, but surfaced by it: on a cold workspace the suite performs a real pip install from PyPI during test_install_action_dispatch. That is live network egress in a unit-test run, and it is why the event fires at all. Not this PR's business — flagging it for the sibling-repo/test-hygiene follow-up rather than widening this diff.

Ran test_analytics.py, test_chat_scratchpad.py, test_scratchpad_observer_dispatch.py, test_tool_outcome_tracking.py at the new head: 99 passed. Comment-only change to conftest; CI runs the full suite.