Skip to content

ENG-1390: prefix the rule-retrieval analytics action so the collector relays it - #332

Open
alecantu7 wants to merge 1 commit into
stagingfrom
alejandrocantu/eng-1390-prefix-the-rule-retrieval-analytics-action
Open

ENG-1390: prefix the rule-retrieval analytics action so the collector relays it#332
alecantu7 wants to merge 1 commit into
stagingfrom
alejandrocantu/eng-1390-prefix-the-rule-retrieval-analytics-action

Conversation

@alecantu7

Copy link
Copy Markdown
Contributor

Follow-up to ENG-1390 (merged in #330). Two-line behaviour change plus two tests.

The bug

rule_retrieval shipped and produced nothing in PostHog project 355390. The collector lambda relays an action only if it starts with anton_ or ds_connect — a prefix rule, not a list — and drops everything else while still returning HTTP 200. send_event is fire-and-forget behind a bare except, so there is no client-side signal whatsoever.

Measured against the live collector:

action=rule_retrieval        -> posthogResult: false   (silently dropped)
action=anton_rule_retrieval  -> posthogResult: true

Renamed to anton_rule_retrieval. It also matches every other action on this channel — anton_started, anton_query, anton_first_query, ds_connect_* — so the unprefixed name was off-convention as well as broken.

Scope: this fixes ONE of two gates

The lambda also allowlists property names: action, aid, engine, llm_provider, has_mdb_key. So after this, the event arrives but carries none of its data until ENG-1355 lands.

Verified by probing a passing action name with the real payload:

Sent Arrived
outcome, when_rules, kept_rules, stop_reason, duration_ms all dropped
llm_provider ✅ (pre-allowlisted)

So this PR is necessary, not sufficient. Merging it does not make ENG-1390's Done-when #2 pass — that needs ENG-1355, and specifically the generic pass-through option rather than adding turn_completed's 28 names, since rule_retrieval's properties aren't among those either.

Tests

Two, deliberately in their own class. TestRuleRetrievalObservability autouse-stubs _emit_rule_retrieval — which is exactly what these must not have stubbed, since they assert what reaches send_event. I hit that while writing them: the first version silently exercised the stub and failed with KeyError: 'action'.

  • test_the_action_name_carries_the_prefix_the_collector_requires — pins the constant.
  • test_the_emitted_action_is_the_prefixed_one — pins the name that actually goes out, so a refactor reintroducing a literal is caught.

Both mutation-verified against the old name:

AssertionError: 'rule_retrieval' would be silently dropped by the collector
AssertionError: {'action': 'rule_retrieval', 'props': {...}}

Suite: 1745 passed, 28 skipped.

Why a test rather than just a fixed string

The constraint lives in a lambda whose code is in no repo in the workspace — ENG-1355 documents that. So nothing in this codebase can enforce it structurally, and a test with the measured posthogResult values in its docstring is the only place the constraint can be recorded where the next person will trip over it.

Note on turn_completed

turn_completed fails the same gate (posthogResult: false) and would be fixed the same cheap way. Deliberately not touched here: ENG-1355 is in progress and its Done-when queries name the literal string turn_completed, so renaming it unilaterally would invalidate acceptance criteria someone is actively working against. Worth a conversation rather than a surprise — flagged on the ticket.

🤖 Generated with Claude Code

…tor relays it (ENG-1390)

The event shipped as `rule_retrieval` and produced nothing in PostHog project
355390. The collector lambda relays an action only if it starts with `anton_` or
`ds_connect` — a prefix rule, not a list — and drops anything else while still
returning HTTP 200. `send_event` is fire-and-forget behind a bare `except`, so
there is no client-side signal at all. Measured 2026-08-10:

    action=rule_retrieval        -> posthogResult: false   (silently dropped)
    action=anton_rule_retrieval  -> posthogResult: true

Renamed to `anton_rule_retrieval`, which also matches every other action on this
channel (`anton_started`, `anton_query`, `anton_first_query`, `ds_connect_*`) —
the unprefixed name was off-convention as well as broken.

SCOPE: this fixes ONE of two gates. The lambda also allowlists property NAMES
(`action`, `aid`, `engine`, `llm_provider`, `has_mdb_key`), so the event will
arrive carrying none of its data until ENG-1355 lands. Verified by probing a
passing action name with the real properties: `outcome`, `when_rules`,
`kept_rules`, `stop_reason` and `duration_ms` were all dropped; only the
pre-allowlisted `llm_provider` survived. So this is necessary, not sufficient.

Tests: 2, in their own class deliberately. `TestRuleRetrievalObservability`
autouse-stubs `_emit_rule_retrieval`, which is precisely what these must not
have stubbed — they assert what reaches `send_event`. One pins the constant, one
pins the name that actually goes out, so a refactor reintroducing a literal is
caught. Both mutation-verified against the old name.

The constraint lives in a lambda whose code is in no repo in the workspace, so a
test is the only place this codebase can record it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant