feat(analytics): behavior signals — session.outcome event + github/slack action-outcome emits#106
feat(analytics): behavior signals — session.outcome event + github/slack action-outcome emits#106xBalbinus wants to merge 13 commits into
Conversation
Emit a session.outcome analytics event at every terminal transition in the SessionAgent DO (terminated, hibernated, error, recovery_exhausted), one emit per transition. recovery_exhausted funnels through handleStop and is labelled distinctly. error paths carry an error_code. Consumers take the last outcome per session; a wake supersedes a prior hibernated. Update value-metrics proposal section 3 item 5 to note it shipped.
The execution context already carries ctx.analytics through ActionSource.execute
to each plugin's executeAction. Emit low-cardinality behavior signals on success:
- github: pr_created {repo,number,draft}, pr_merged {repo,number},
issue_created {repo,number}
- slack: message_sent/updated/deleted {channel}
Emit only on genuine success (merge guarded on data.merged). Add emit-on-success
and no-emit-on-failure tests for both plugins. Linear is a remote-MCP passthrough
with no per-action success branch, so it is deferred.
Note the shipped signals in the value-metrics proposal.
|
Preview deployment: https://pr-106.dev-valet-turnkey-client.pages.dev |
yourbuddyconner
left a comment
yourbuddyconner
left a comment
There was a problem hiding this comment.
The new terminal outcome is emitted after handleStop has already called flushMetrics(). emitEvent only writes to the DO-local SQLite buffer, while flushMetrics is what persists buffered rows to D1. That means stopped/recovery-exhausted sessions (and similarly hibernation/error paths) can leave session.outcome at flushed = 0 indefinitely, so the new analytics signal is not visible to consumers. Please emit before the existing flush, or flush again after emitting, and add a persistence-level test rather than only mocking flushMetrics.
emitSessionOutcome only wrote the DO-local SQLite buffer, so stopped/
hibernated/errored sessions could leave the row at flushed=0 forever.
Make it async, flush to D1 immediately after emitting, and await it at
every terminal call site (stop/hibernate/recovery/spawn/wake).
Add a persistence-level test that drives handleStop('recovery_exhausted')
through the real emit + flush path (real SQLite DO buffer + real D1 sink,
no mocked emitEvent/flushMetrics) and asserts the row landed in
analytics_events.
A GitHub merge is the ultimate indicator that an authoring session
shipped value. When a merged-PR webhook lands for a session that
authored the PR, write a session.outcome{reason:pr_merged} straight to
analytics_events — out-of-band, since the session may be terminated /
hibernated / gone, so no DO wake. Never fires for source_pr matches.
The event id is deterministic so GitHub redeliveries dedupe via
INSERT OR IGNORE.
Extend makeD1Adapter with raw() so drizzle-wrapped callers (getDb) work
under the fake, then cover the authoring / source_pr / non-merge /
redelivery cases.
Document the out-of-band, webhook-sourced pr_merged terminal outcome and that emitSessionOutcome now persists (emit + await flush) at every DO terminal transition. Disambiguate it from the agent-sourced github.pr_merged action-outcome.
|
Addressed the review:
Rebased on current main (keeps #105's turn_error emit). The broader 'track external constructs generically (PRs, Linear issues, …)' direction is captured as TKAI-181 — kept out of this PR to stay scoped. |
yourbuddyconner
left a comment
yourbuddyconner
left a comment
There was a problem hiding this comment.
Code review — 1 finding.
| @@ -4777,6 +4778,10 @@ export class SessionAgentDO { | |||
| }); | |||
|
|
|||
| this.emitAuditEvent('session.terminated', 'Session terminated'); | |||
There was a problem hiding this comment.
handleStop collapses error-caused terminations to reason: 'terminated', mislabeling them as normal end-of-life.
performHibernate catches SandboxSnapshotFailedError → terminateSandbox() → handleStop('snapshot_failed'), and catches SandboxAlreadyExitedError → handleStop('sandbox_exited'). Both funnel into the new emit here — the ternary maps everything except recovery_exhausted to terminated, so these two anomalous exits persist as session.outcome{ reason: 'terminated' } with no errorCode.
The rest of the diff carefully distinguishes these — the peer hibernate/wake/spawn/recover error paths emit reason: 'error' with a specific errorCode (hibernate_failed, wake_missing_config, sandbox_spawn_failed, recover_missing_config). Consumers grouping by reason to compute session error rate will under-count these two failure modes as successful terminations, which is exactly the signal this PR is trying to produce.
Suggest either routing snapshot_failed / sandbox_exited through emitSessionOutcome('error', 'snapshot_failed' | 'sandbox_exited') before handleStop, or teaching the ternary about them.
There was a problem hiding this comment.
Fixed in 3e9ef2a. snapshot_failed and sandbox_exited now emit session.outcome{ reason: 'error' } with the stop reason as the errorCode, matching the peer hibernate/wake/spawn/recover paths; everything else still collapses to terminated. Went with teaching the ternary rather than emitting before handleStop, so the already-errored guard still applies and a session that failed earlier keeps its original outcome.
Covered by a parametrized case over both reasons asserting the error outcome is emitted and no superseding terminated follows. Worker suite green (135 across session-agent/webhooks/adoption-metrics), typecheck clean.
Guard handleStop so an already-errored session is not superseded by a later terminated outcome — consumers take the last outcome per session, so the terminated row would mask the failure. Recovery exhaustion runs with status 'recovering' and is unaffected. Drain the analytics buffer in a loop until empty. A terminal transition flushes once, so the old single LIMIT-100 query stranded a just-emitted session.outcome behind any backlog of >100 unflushed events. Split the drain into flushAnalyticsEvents so emitSessionOutcome persists its row without re-running the message/tool metric scans the stop/hibernate paths already ran. Wrap the pr_merged analytics write in try/catch so a D1 failure cannot abort the git-state DO notify for this or later targets. Reuse noopAnalytics for the workflow tool node's deferred analytics.
flushAnalyticsEvents now runs its initial SELECT inside the try, so a DO-local storage error is logged and swallowed instead of propagating out of emitSessionOutcome and aborting handleStop's termination tail (event-bus/owner/parent-wake) after status is already terminated. Restore flushMetrics on the three error paths that emit an outcome without a preceding flush (sandbox_spawn_failed, recover_missing_config, wake_missing_config), which the switch to flushAnalyticsEvents dropped. Correct the outcome contract: hibernated is not self-superseding (no outcome is emitted on wake/run), so consumers must join sessions.status for liveness. Fixed in the docstring and the value-metrics spec, and the spec's flushMetrics reference updated to flushAnalyticsEvents.
When several unlinked sessions share a merged PR's head branch, link only the biggest contributor (deterministic tie-break by session id) as the author, and only when no session is already linked by pr_number. The other co-branch sessions are left untouched, so the pr_merged outcome and the merged-PR counts are recorded once instead of being multiplied. Also gate getAdoptionMetrics.totalPrsMerged on pr_number, matching totalPrsCreated: a row can carry prState='merged' without a pr_number, which otherwise inflated merged past created and pushed mergeRate over 100%.
yourbuddyconner
left a comment
yourbuddyconner
left a comment
There was a problem hiding this comment.
Code review findings from a medium-effort pass. All three are correctness/quality nits — the PR shape is solid.
| // the failure. Recovery exhaustion runs with status 'recovering', so its | ||
| // outcome is unaffected. | ||
| if (currentStatus !== 'error') { | ||
| await this.emitSessionOutcome(reason === 'recovery_exhausted' ? 'recovery_exhausted' : 'terminated'); |
There was a problem hiding this comment.
Non-user termination reasons (snapshot_failed, sandbox_exited, completed) collapse into a bare terminated outcome with no reason preserved.
Compare with the error paths at 4650, 5925, 6069, 6103 — each passes a distinguishing error_code. Here the reason argument is only used to pick between terminated and recovery_exhausted; every other reason is dropped. In the analytics stream, a snapshot failure looks identical to a clean user close, which undermines §3's "separates abandoned from done" goal.
Cheap fix: pass reason through as properties.reason (or errorCode) whenever it isn't the default 'user_stopped'.
There was a problem hiding this comment.
Fixed in e326a92. Only user_stopped stays a bare close now — every other reason (completed, parent_stopped) carries the stop reason as the errorCode, and snapshot_failed/sandbox_exited emit reason: "error" with the same code.
Used the errorCode carrier you offered rather than widening the outcome union, so the reason taxonomy consumers group on is unchanged. Covered by parametrized cases over both anomalous exits and both non-user stop reasons, plus one asserting user_stopped still has no code.
| let best: { sessionId: string; commits: number } | undefined; | ||
| for (const c of candidates) { | ||
| const commits = c.commit_count ?? 0; | ||
| if (!best || commits > best.commits || (commits === best.commits && c.session_id < best.sessionId)) { |
There was a problem hiding this comment.
Tie-break is purely lexicographic on session_id, so when co-branch candidates all have equal (or null) commit_count the earliest-sorting session wins regardless of who actually pushed.
Scenario: sessions A and B both start from feature/x in acme/repo; B opens the PR from its sandbox, but session_git_state.commit_count is still null for both when the merge webhook fires. null ?? 0 === 0 → tie → lex-smallest id wins. If A's id sorts before B's, the pr_merged outcome and pr_number stamp go to A even though B authored the PR.
Consider a secondary signal for the tie — most recently active on the branch, or only emitting pr_merged when at least one candidate has commit_count > 0.
There was a problem hiding this comment.
Fixed in e326a92. You are right that the id tie-break was arbitrary — determinism is not correctness, and since commit_count is null until the runner reports it, the tie was the common path rather than a rare one.
Selection now falls back to the most recently active session on the branch (your first suggestion), keeping the id only as a final anchor so concurrent and redelivered webhooks converge on the same winner. Chose recency over gating on commit_count > 0, since that gate would drop credit entirely for a legitimately authored PR whose count had not synced yet. findSessionsByRepoBranch now joins sessions.last_active_at to supply the signal.
Covered by a case encoding your exact scenario: two candidates, both null counts, the later-active one wins despite sorting later by id.
| console.error('[SessionAgentDO] Failed to sync error status to D1:', e), | ||
| ); | ||
| } | ||
| await this.emitSessionOutcome('error', 'hibernate_failed'); |
There was a problem hiding this comment.
The hibernate_failed error path skips the pre-outcome flushMetrics() that every other terminal error path performs.
Compare 4649-4650, 5924-5925, 6102-6103 — each does await this.flushMetrics(); await this.emitSessionOutcome('error', ...). Here we go straight to emitSessionOutcome. The flushMetrics at 5979 covers work up to the snapshot attempt, but anything emitted between 5979 and the throw (e.g. instrumentation inside lifecycle.snapshotSandbox, deleteRuntimeGrantsByScope side effects) buffers locally without reaching D1 before the outcome. Minor, but inconsistent with the pattern the other three error paths establish.
There was a problem hiding this comment.
Fixed in e326a92 — added the flushMetrics() before the outcome. It was the only terminal error path skipping it, so work emitted after the flush at 5979 could never reach D1 once the session went terminal.
performHibernate routes SandboxSnapshotFailedError and SandboxAlreadyExitedError
through handleStop, where every reason other than recovery_exhausted collapsed to
session.outcome{ reason: 'terminated' }. Both are failures, so they were recorded
as clean end-of-life and any consumer deriving an error rate by grouping on reason
under-counted them, while the peer hibernate/wake/spawn/recover paths already
report reason 'error' with a specific errorCode.
Both reasons now emit reason 'error' carrying the stop reason as the errorCode.
…outcome, tie-break author by activity Terminations other than a plain user close lost their reason: 'completed' and 'parent_stopped' both collapsed to a bare terminated outcome, so a session that finished its work was indistinguishable from one that was abandoned. Both now carry the stop reason as the errorCode. The hibernate failure path emitted its outcome without first draining metrics, unlike the other terminal error paths, so anything buffered after the last flush never reached D1 for a session that was about to go terminal. Branch-linked author selection tied on commit_count and fell back to session id, which is unrelated to who pushed — and commit_count is null until the runner reports it, so a merge landing early tied every candidate at zero. Selection now falls back to the most recently active session on the branch, keeping the id only so concurrent and redelivered webhooks converge on the same winner.
|
Flagging a gap I found reviewing my own change here: the single-author rule this PR adds to the webhook handler is bypassable, because a second path still stamps
Its own comment says "the session(s) working on its head branch" — plural, by design. So for sessions A and B both on This route is the more common linking path in practice: it exists specifically so agent-authored PR outcomes are trackable when repo webhooks are not configured. So the webhook-side fix does not apply to PRs created through it. The underlying invariant is "at most one session is linked to a given pr_number", and right now it is enforced in one place and violated in another. I will fix it here rather than leave the fix half-applied, unless you would rather it went in its own change — it touches a file outside this PR's current scope. |
…alytics drain
The merge webhook credits every session linked to a PR by pr_number, so the
create-PR route stamping each session on the head branch gave one PR several
authors and multiplied the shipped-value signal. The request carries no session
id, so the route now picks a single session with the same rule the webhook uses.
Author selection compared last_active_at as text. The column is written by
datetime('now') today, but nothing stops an ISO value being stored, and the two
shapes sort against each other by separator rather than by instant, so a session
active an hour ago could lose to one from the previous day. Both shapes are now
read as UTC milliseconds.
The analytics drain looped until the buffer emptied, with nothing to stop a
batch that inserts but marks nothing from being re-sent forever while a terminal
path awaits it. It now stops when a pass marks no rows and is capped.
2 participants
What
First foundations for user-behavior analytics, on top of the existing
analytics_eventspipeline.session.outcomeevent. Emitted at every terminal session transition (terminated/hibernated/error/recovery_exhausted), with anerrorCodeon error paths. Hardens the Value tab's resolution proxies, which today infer "done" from status +last_active_at.hibernatedrecurs across a session's life, so multiple rows per session are expected and correct — consumers take the last outcome (a wake supersedes a priorhibernated).Plugin action-outcome signals. The execution context already carries
ctx.analyticsthrough to each plugin's executor; nothing used it. Emits low-cardinality success-path events (ids/enums/counts only, never on failure):github.pr_created{repo, number, draft},github.pr_merged{repo, number},github.issue_created{repo, number}slack.message_sent/message_updated/message_deleted{channel}This makes "which integrations/actions do users actually rely on" answerable beyond the coarse
tool_execcounter.Why
Server-side analytics currently can't tell how a session ended (only its last status) or which integration actions produce value. These are the two cheapest, highest-signal additions that need no schema changes and no client work.
Test plan
session.outcomeemitted once per terminal transition (worker suite); github/slack emit on success and NOT on failure (plugin suites)Checklist
docs/specs/2026-07-07-value-metrics-proposal.mdupdated (session_outcome + plugin signals marked shipped)Deferred (need a design call — not in this PR)
McpActionSource→mcp.linear.app) with no local per-action executor to hook. Emittinglinear.issue_createdetc. requires generic{service}.{tool}emission insideMcpActionSource.execute, which affects every MCP-backed plugin and raises event-type cardinality — worth doing deliberately, not surgically here.workflows/nodes/tool.ts, currently a documented no-op).analytics_events.session_idis NOT NULL / FK tosessions.id, but a workflow tool node has only aworkflow_executions.idand no session row. Wiring it needs a decision on workflow-originsession_idsemantics that doesn't break the admin Events-feed joins.