fix(telemetry): only announce skill events the outermost frame saved

[jdx]

https://entire.io/gh/entireio/cli/trails/1124

Review feedback on #2023 that I missed before it landed.

MutateSessionStateSaved reported saved=true for a nested frame on the grounds that the outer frame flushes its mutations. That is a prediction, not a fact: if the outer frame then returns ErrMutationSkip or fails, the caller has already emitted telemetry for an append that never reached disk. The landed doc comment waved this off as "the same exposure the plain error return always had, and self-correcting for events that later re-derive" — which is wrong in the second half, and re-derivation is precisely the mechanism that makes it worse:

• extraction re-derives skill events from transcript offset 0 on every pass;
• dedupe is against the ledger in session state (state.SkillEvents);
• so an event whose ledger entry never landed is re-derived next pass, returned as new, and announced a second time.

The result is duplicate cli_skill_invoked events in PostHog — exactly what the ledger exists to prevent. Not emitting is the recoverable direction; emitting early is not.

The fix

Replace the bool with MutateSessionStateOnSaved(ctx, id, fn, onSaved). The caller hands the effect to the helper instead of deciding from a return value it cannot trust, and the helper runs it from the only frame that knows whether a save happened:

• a nested frame queues its effect on the session gate;
• the outermost frame drains the queue after its own save succeeds, and discards it when it skips, fails, or panics;
• effects still run after release(), so the settings load and detached-process spawn never extend the gate hold.

MutateSessionState becomes a one-line wrapper (onSaved = nil), so the migrated call sites lose their stateSaved branch rather than gaining a new concept.

A panicking effect is contained to that effect (runPostSaveEffect), so it neither skips the effects queued behind it nor unwinds into callers that don't recover — PostCommit is one, and a telemetry bug should not kill a git hook mid-commit over a signal that is fail-open everywhere else. It logs rather than swallows: a panic is a bug wherever it comes from, and a silent recover would trade a crash for an invisible failure. This closes the one path where the panic discipline already applied to the mutation function was missing.

Scope

No behavior change today. I traced every Saved call site before writing this: handleLifecycleSessionStart, handleLifecycleTurnStart, handleLifecycleCompaction, transitionSessionTurnEnd, markSessionEnded, PostCommit, CondenseSessionByID, and CondenseAndMarkFullyCondensed are all reached only at hook-handler or command-body level, so the nested branch is currently unreachable. This closes it as a landmine, not as a live duplicate — the next refactor that moves one of those handlers under a gate (transitionSessionTurnEnd already runs HandleTurnEnd, with its reentrant mutations, inside its own closure) would have gotten silent PostHog duplicates with no failing test.

One gap deliberately left open and documented on the helper: a nested frame whose fn errors has already mutated the shared state, and an outer frame that swallows that error still saves those mutations, with no effect registered. That direction loses an announcement rather than duplicating one, and is inherent to nested frames sharing a state pointer.

Also updates the contract doc comments in skill_events.go and skill_telemetry.go that told callers to emit "after the surrounding MutateSessionState returns" to name the new helper.

Tests

The nested contract as a table — outer saves / skips / fails — asserting the effect never runs while the outer frame is still open, that a skipped outer frame leaves the nested mutation off disk, and that a queued effect does not leak into the next frame on the same session. Plus the panic cases from both directions: a panicking mutation discards the queue and leaves the gate usable, and a panicking effect does not stop the effects behind it or surface as the mutation's error.

Verified each case fails against the old behavior — patching the nested branch to call onSaved() inline reproduces the three nested failures, and reverting runPostSaveEffect to a bare effect() call lets the panic escape and crash the test binary. The existing "runs outside the session gate" test keeps its durability probe, now driven through the new helper.

Verified green: mise run lint 0 issues, all packages, plus both canary suites (56 Vogon, 4 roger-roger).

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Fixes telemetry emission so post-save effects run only after a successful outermost session-state save.

Changes:

• Adds queued post-save effects.
• Migrates lifecycle, post-commit, and condensation callers.
• Updates documentation and adds nested-frame tests.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Summary
cmd/entire/cli/strategy/telemetry_signals.go	Updates telemetry guidance.
cmd/entire/cli/strategy/skill_telemetry.go	Documents the new emission contract.
cmd/entire/cli/strategy/skill_telemetry_test.go	Tests save, failure, skip, and nesting behavior.
cmd/entire/cli/strategy/skill_events.go	Updates durable deduplication guidance.
cmd/entire/cli/strategy/session_state.go	Implements queued post-save effects.
cmd/entire/cli/strategy/manual_commit_hooks.go	Migrates post-commit telemetry.
cmd/entire/cli/strategy/manual_commit_condensation.go	Migrates condensation telemetry.
cmd/entire/cli/lifecycle.go	Migrates lifecycle telemetry calls.

Suppressed comments (2)

cmd/entire/cli/strategy/session_state.go:635

• Because MutateSessionState now delegates here with onSaved == nil, this block still creates a backing array with capacity at least one for every successful outer mutation. Session-state mutations include the PostToolUse hot path (see the helper's own comment above), so this adds an avoidable allocation on every hook even when no effect is registered. Only allocate/copy effects when len(gate.afterSave) > 0 || onSaved != nil.

	effects = make([]func(), 0, len(gate.afterSave)+1)
	effects = append(effects, gate.afterSave...)
	if onSaved != nil {
		effects = append(effects, onSaved)
	}

cmd/entire/cli/strategy/session_state.go:609

• The new contract explicitly promises that queued effects are discarded when the outer frame panics, but the added table only covers save, ErrMutationSkip, and an ordinary error. Please add a panic case that registers a nested effect, recovers outside the mutation, and then runs another frame to prove the queue cannot leak or announce a durable event from the panicking frame.

	var effects []func()
	defer func() {
		gate.activeState = nil
		gate.afterSave = nil
		release()
		for _, effect := range effects {
			effect()
		}
	}()

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jdx]

@evisdren sorry — I pushed after your approval and dismissed it. Here's the delta so you can re-review just that.

You approved at fe2fde6a9. Two things landed on top, and the branch was force-pushed once (rebase), so it's now c8317b1e5.

1. Rebased onto the renamed base. #2024 gained 26c4cd871, which renames the telemetry event cli_checkpoint_condensed → cli_commit_condensed and its Go identifiers. That rippled through this branch mechanically: condensedTelemetrySignal → commitCondensedSignal, emitCheckpointCondensedTelemetry → emitCommitCondensedTelemetry. Two conflicts, both resolved to keep this PR's MutateSessionStateOnSaved shape under the new names. Worth noting one thing I caught in the process: the rebase left my doc comments naming the pre-rename identifiers, so I fixed those — a grep for the four old names now comes back empty.

2. Ten lines of comments at the two condensation onSaved callbacks (c8317b1e5), saying why they emit skill telemetry only and pointing at the invariant that now lives on newCommitCondensedSignal in #2024. This is the readable defect behind the two HIGH trail findings: the omission was deliberate but nothing at those call sites said so, and this PR moving the emission onto a callback puts the asymmetry right where a reviewer looks. Both findings are dismissed with the rationale recorded.

What did not change: anything executable. The whole post-approval delta is git diff fe2fde6a9 c8317b1e5 — every non-comment line in it is the rename, and the only line that changes runtime behaviour is the event-name string in #2024. The alloc guard and the panic-case test you already approved in fe2fde6a9 are untouched.

Lint clean, test:ci green (unit + integration + canary), and -race clean over the strategy package.

The merge-base changed after approval.

[jdx]

Rebased onto the updated #2024, which moved onto current main and gained three review: commits addressing that PR's six trail findings. Nothing here changed on purpose — the rebase produced exactly two conflicts, both mechanical:

• manual_commit_hooks.go — feat(telemetry): missed-opportunity signal at checkpoint condensation #2024 now builds one commitCondensedEmitter per commit (the prior-history git log and the settings load are commit-scoped, not session-scoped). Resolution keeps this PR's onSaved closure and calls condensedTelemetry.emit(iterCtx, condensedSignal) inside it, so the emit still runs as a post-save effect outside the session gate.
• telemetry_signals.go — the function this PR's doc comment described was replaced by commitCondensedEmitter.emit. Resolution keeps the new type and carries this PR's wording forward: the emit is handed to MutateSessionStateOnSaved as the post-save effect rather than called inside the mutation closure.

Also renamed one stale reference: the CondenseSessionByID comment added in docs(telemetry): say why the two condensation paths emit no commit signal named emitCommitCondensedTelemetry, which no longer exists. It now names commitCondensedEmitter.emit. The reasoning it records is unchanged and still correct — that path condenses without a commit, so a commit-scoped payload has nothing to describe.

The review: document the skill-event ledger's size envelope commit that was briefly on this branch was dropped by the rebase as already-upstream — it belongs to #2024.

mise run check green on this branch (lint 0 issues, 56 Vogon + 4 roger-roger canary tests).

[jdx]

Update — unstacked onto main, and the review finding is fixed

This PR is no longer stacked on #2024. Only 3 of its 275 changed lines ever touched anything #2024 introduced (the emit call moving into the onSaved closure, and one doc-comment mention), so the dependency was almost entirely artificial. Its substance — the session gate's afterSave queue, the defer ordering, lifecycle.go, skill_telemetry.go — descends from #2023, which is already merged.

Reparented onto current main (89ebc1ba4). The two coupled lines are gone, and docs(telemetry): say why the two commit-less condensation paths emit no signal moved to #2024, where the signal it describes actually lives. What's left is the gate change alone, which is the right shape for a change to defer ordering and panic paths — it can be reviewed without a telemetry-metric discussion on top.

⚠️ The PR base still says jdx/missed-opportunity-signal. GitHub refuses the change via both GraphQL and REST — Cannot change the base branch because the pull request is part of a stack — so it needs unstacking in the UI. The rendered diff is already correct (GitHub uses the merge base, which is now main), so this is a merge-ordering label rather than a diff problem; but until it's retargeted this can't merge before #2024, which is the constraint unstacking was meant to remove.

Review finding fixed — review: contain a panicking post-save effect to that effect. A panicking effect no longer takes the effects queued behind it down, or escapes into callers that don't recover.

I agreed with the fix but not quite the framing. "Telemetry in an inconsistent state" isn't the risk — telemetry here is best-effort and the effects run after release(), so the gate is already free. The two reasons it's worth fixing:

1. An escaping panic unwinds out of MutateSessionStateOnSaved into callers that don't recover. PostCommit is one, so a telemetry bug would kill a git hook mid-commit over a signal that's fail-open everywhere else.
2. Consistency, which is what makes this a real gap rather than speculative hardening: a panic in the mutation function is already handled deliberately here — the queue is discarded because a panicking frame never saved, and release() runs first so the gate stays usable, both covered by TestMutateSessionStateOnSaved_PanicDiscardsQueuedEffects. The effects were the one path where the same discipline wasn't applied.

Took "wrap the effect execution" over "each individual effect has its own panic handling" — one helper at the loop, rather than asking every caller to defend itself, since callers are the ones least likely to remember. It logs rather than swallows: a panic is a bug wherever it comes from, and a silent recover would trade a crash for an invisible failure.

Verified the test bites — reverted to a bare effect() call, the panic escapes and crashes the test binary.

mise run lint 0 issues, 56 Vogon + 4 roger-roger canary tests green.

[jdx]

Rebased onto 5a157342a; base is now main

The unstack went through — this PR now targets main directly and no longer depends on #2024. Rebase was clean despite #2013 touching the same manual_commit_hooks.go region; the onSaved conversion in the PostCommit loop is unaffected.

mergeable: MERGEABLE, lint 0 issues, 56 Vogon + 4 roger-roger canary tests green. The review finding on the effects loop is fixed and resolved on the trail.