feat(telemetry): missed-opportunity signal at checkpoint condensation - #2024
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new, content-free telemetry signal emitted on successful post-commit checkpoint condensation to better measure “missed opportunity” adoption: sessions that committed files with prior AI checkpoint history without using entire search.
Changes:
- Introduces a
cli_checkpoint_condensedtelemetry event payload and detached sender (CheckpointCondensedSignal, builder, tracker). - Adds strategy-side signal computation:
used_search(substring probe over in-memory transcript) andprior_ai_history(boundedgit logscan for recent commits with anEntire-Checkpointtrailer touching committed files). - Wires emission into the post-commit condensation flow and adds unit tests for transcript probing and the git-log-based prior-history detector.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/telemetry/detached.go | Adds the new condensed-checkpoint telemetry signal type, payload builder, and detached tracking function. |
| cmd/entire/cli/telemetry/detached_test.go | Adds payload-building tests for the new event and centralizes the agent test constant. |
| cmd/entire/cli/strategy/telemetry_signals.go | Implements used_search detection + prior AI-history probing and emits the condensed-checkpoint telemetry signal (telemetry opt-in gated). |
| cmd/entire/cli/strategy/telemetry_signals_test.go | Adds unit tests for transcript search detection and prior-checkpoint-touch detection via a temp git repo. |
| cmd/entire/cli/strategy/manual_commit_types.go | Extends CondenseResult with UsedSearch for downstream telemetry emission. |
| cmd/entire/cli/strategy/manual_commit_hooks.go | Hooks telemetry emission into the successful condensation path. |
| cmd/entire/cli/strategy/manual_commit_condensation.go | Populates CondenseResult.UsedSearch from the extracted raw transcript. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address Copilot review on #2024: - emitCheckpointCondensedTelemetry resolves state.AgentType (display name, "Claude Code") to the agent registry key ("claude-code") so the agent property lines up with skill and command events; unknown agent types fall back to the stored string rather than dropping the signal. - CheckpointCondensedSignal doc now states the actual invariant (content-free metadata, no file paths/prompts/transcript content) instead of "booleans and counts only", and documents Agent as the registry key. - BuildCheckpointCondensedPayload defaults an empty agent to "auto" like the other payload builders; the shared literal moved to the autoAgentName const. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3d4737d to
6f6eda2
Compare
|
Both findings addressed in 6f6eda2:
Rebased on the updated #2023. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
cmd/entire/cli/strategy/telemetry_signals.go:93
- The environment opt-out is not checked until
TrackCheckpointCondensedDetached, after this function has already run thegit logprobe. A user who opted in previously and then setsENTIRE_TELEMETRY_OPTOUTstill incurs the repository scan, contrary to the PR's guarantee that no work/probe occurs for opted-out users. Include the environment opt-out in this early gate.
s, err := settings.Load(ctx)
if err != nil || s.Telemetry == nil || !*s.Telemetry {
return
cmd/entire/cli/strategy/manual_commit_hooks.go:1411
- This call executes inside the
MutateSessionStatecallback at lines 1004-1010, so settings I/O, agit logsubprocess, machine-ID lookup, and detached-process spawn all extend the per-session gate hold. The established telemetry pattern explicitly requires emission after mutation returns (cmd/entire/cli/lifecycle.go:1579-1581) to avoid blocking concurrent hooks. Capture the signal while locked, then run this telemetry work afterMutateSessionStatecompletes.
// Content-free adoption signal (opt-in telemetry gated inside).
emitCheckpointCondensedTelemetry(ctx, state, result)
cmd/entire/cli/strategy/telemetry_signals.go:40
- The acknowledged quoting behavior makes
prior_ai_historyincorrect for ordinary non-ASCII paths (for example, Git outputs"caf\303\251.go"whileFilesTouchedcontainscafé.go), and line-based parsing also cannot represent filenames containing newlines. This systematically creates false negatives in the metric. Request NUL-delimited names (-z) and parse those names without trimming or Git quote decoding.
// Paths git quotes in --name-only output (e.g. non-ASCII names) won't match
// their unquoted FilesTouched form; that false-negative is acceptable for a
// telemetry boolean.
Address Copilot review on #2024: - emitCheckpointCondensedTelemetry resolves state.AgentType (display name, "Claude Code") to the agent registry key ("claude-code") so the agent property lines up with skill and command events; unknown agent types fall back to the stored string rather than dropping the signal. - CheckpointCondensedSignal doc now states the actual invariant (content-free metadata, no file paths/prompts/transcript content) instead of "booleans and counts only", and documents Agent as the registry key. - BuildCheckpointCondensedPayload defaults an empty agent to "auto" like the other payload builders; the shared literal moved to the autoAgentName const. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6f6eda2 to
d96d143
Compare
5d52152 to
4015a35
Compare
Address Copilot review on #2024: - emitCheckpointCondensedTelemetry resolves state.AgentType (display name, "Claude Code") to the agent registry key ("claude-code") so the agent property lines up with skill and command events; unknown agent types fall back to the stored string rather than dropping the signal. - CheckpointCondensedSignal doc now states the actual invariant (content-free metadata, no file paths/prompts/transcript content) instead of "booleans and counts only", and documents Agent as the registry key. - BuildCheckpointCondensedPayload defaults an empty agent to "auto" like the other payload builders; the shared literal moved to the autoAgentName const. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d96d143 to
5a79a16
Compare
|
Addressed the three remaining review findings in 5a79a16, and rebased onto the updated #2023:
Full suite re-run on the stack: 9,324 unit / 514 integration / canary green. |
4015a35 to
9f5e2e2
Compare
Address Copilot review on #2024: - emitCheckpointCondensedTelemetry resolves state.AgentType (display name, "Claude Code") to the agent registry key ("claude-code") so the agent property lines up with skill and command events; unknown agent types fall back to the stored string rather than dropping the signal. - CheckpointCondensedSignal doc now states the actual invariant (content-free metadata, no file paths/prompts/transcript content) instead of "booleans and counts only", and documents Agent as the registry key. - BuildCheckpointCondensedPayload defaults an empty agent to "auto" like the other payload builders; the shared literal moved to the autoAgentName const. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5a79a16 to
7d418f4
Compare
Address Copilot review on #2024: - emitCheckpointCondensedTelemetry resolves state.AgentType (display name, "Claude Code") to the agent registry key ("claude-code") so the agent property lines up with skill and command events; unknown agent types fall back to the stored string rather than dropping the signal. - CheckpointCondensedSignal doc now states the actual invariant (content-free metadata, no file paths/prompts/transcript content) instead of "booleans and counts only", and documents Agent as the registry key. - BuildCheckpointCondensedPayload defaults an empty agent to "auto" like the other payload builders; the shared literal moved to the autoAgentName const. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7d418f4 to
cdd3faf
Compare
Address Copilot review on #2024: - emitCheckpointCondensedTelemetry resolves state.AgentType (display name, "Claude Code") to the agent registry key ("claude-code") so the agent property lines up with skill and command events; unknown agent types fall back to the stored string rather than dropping the signal. - CheckpointCondensedSignal doc now states the actual invariant (content-free metadata, no file paths/prompts/transcript content) instead of "booleans and counts only", and documents Agent as the registry key. - BuildCheckpointCondensedPayload defaults an empty agent to "auto" like the other payload builders; the shared literal moved to the autoAgentName const. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cdd3faf to
0fe22f1
Compare
c6d2c44 to
d5c69a3
Compare
Address Copilot review on #2024: - emitCheckpointCondensedTelemetry resolves state.AgentType (display name, "Claude Code") to the agent registry key ("claude-code") so the agent property lines up with skill and command events; unknown agent types fall back to the stored string rather than dropping the signal. - CheckpointCondensedSignal doc now states the actual invariant (content-free metadata, no file paths/prompts/transcript content) instead of "booleans and counts only", and documents Agent as the registry key. - BuildCheckpointCondensedPayload defaults an empty agent to "auto" like the other payload builders; the shared literal moved to the autoAgentName const. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
0fe22f1 to
fc10900
Compare
26c4cd8 to
e311936
Compare
|
Rebased onto current
|
Address Copilot review on #2024: - emitCheckpointCondensedTelemetry resolves state.AgentType (display name, "Claude Code") to the agent registry key ("claude-code") so the agent property lines up with skill and command events; unknown agent types fall back to the stored string rather than dropping the signal. - CheckpointCondensedSignal doc now states the actual invariant (content-free metadata, no file paths/prompts/transcript content) instead of "booleans and counts only", and documents Agent as the registry key. - BuildCheckpointCondensedPayload defaults an empty agent to "auto" like the other payload builders; the shared literal moved to the autoAgentName const. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
e311936 to
07d7b32
Compare
Update — rebased again, and #2101 needed a follow-upRebased onto current #2101 was a semantic conflict, not a textual one. It converted the four Corrected an overstated rejection rationale. The Gained a commit from #2100.
|
Raw search-command counts can't say whether adoption is low — most sessions have nothing worth searching for. The denominator that makes the rate meaningful is: sessions that edited files carrying AI checkpoint history without ever consulting search. Entire is the only tool that can compute it, and until now nothing emitted it. On each successful post-commit condensation, emit cli_checkpoint_condensed with two booleans and a count: used_search (substring probe over the already in-memory transcript, canonical and legacy spellings), prior_ai_history (did any committed file appear in a recent commit carrying an Entire-Checkpoint trailer — one bounded git-log subprocess, --skip=1 to exclude the commit just made), and files_committed. Content-free by construction: no file paths, prompts, or transcript content leave the machine. Gated on the opt-in telemetry setting before any work happens — the git-log probe never runs for opted-out users — plus ENTIRE_TELEMETRY_OPTOUT; the PostHog call itself is the existing detached-child path and never blocks the hook. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Address Copilot review on #2024: - emitCheckpointCondensedTelemetry resolves state.AgentType (display name, "Claude Code") to the agent registry key ("claude-code") so the agent property lines up with skill and command events; unknown agent types fall back to the stored string rather than dropping the signal. - CheckpointCondensedSignal doc now states the actual invariant (content-free metadata, no file paths/prompts/transcript content) instead of "booleans and counts only", and documents Agent as the registry key. - BuildCheckpointCondensedPayload defaults an empty agent to "auto" like the other payload builders; the shared literal moved to the autoAgentName const. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…h -z
Three review findings:
The env opt-out was only checked inside TrackCheckpointCondensedDetached,
after the git-log density probe had already run — a user who opted in
previously and then set ENTIRE_TELEMETRY_OPTOUT still paid for the repository
scan. The emit path now checks the new telemetry.IsEnvOptedOut() before any
work (and the other trackers share the helper).
emitCheckpointCondensedTelemetry ran inside condenseAndUpdateState, i.e.
inside PostCommit's MutateSessionState closure, extending the session gate
hold with settings I/O, a git subprocess, machine-ID lookup, and a process
spawn. Condensation now snapshots a cheap condensedTelemetrySignal while
locked; the PostCommit loop emits it after the session's mutation saves,
alongside the skill-event emission.
git log --name-only quotes non-ASCII paths ("caf\303\251.go"), so
prior_ai_history could never match their unquoted FilesTouched form, and
line-based parsing could not represent names containing newlines. The probe
now passes -z and splits the NUL-terminated, unquoted name list.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cli_checkpoint_condensed promises "a checkpoint was condensed" and delivers "a commit condensed a checkpoint". That gap already produced a HIGH review finding reading the absent emission on the doctor and session-end condensation paths as data loss, and it would mislead anyone querying total condensations. Every property is commit-scoped — files_committed counts that commit's files, prior_ai_history asks whether commits before this one touched them (hence the git-log probe's --skip=1) — so name the event for the commit: cli_commit_condensed, with the Go identifiers following. Renaming is free exactly now: this PR is unmerged, so the event has never been emitted in a release and no dashboard keys on it. After it ships it stops being free. Also records the scoping invariant on newCommitCondensedSignal: the sole caller is condenseAndUpdateState via postCommitProcessSessionLocked, and folding in the commit-less paths would add rows indistinguishable from genuine misses — inflating the denominator rather than completing it. Covering them is a metric change (trigger discriminator, nullable commit-scoped fields), not a bug fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
transcriptMentionsEntireSearch was bytes.Contains over the raw transcript.
Measured across 328 real Claude Code transcripts: 18 would set
used_search=true, 1 actually ran the command. The false positives are
structural, and Entire installs the artifacts that trip its own probe --
setup_search_skill.go embeds `entire search --json` in the search skill's
description and body, investigate/prompt.go injects it into every
investigate prompt, and any session that reads this repository's source
matches. Direction is what made it blocking: inflating used_search
deflates the missed-opportunity rate, so the metric reads "adoption is
fine" for the wrong reason and nothing in the data reveals it.
Match a recorded tool invocation instead. Two things make the obvious
version of that fix -- "match a Bash command field in the pass that
already walks tool_use blocks" -- wrong in the opposite direction.
First, most agents have no such pass. That walk is
agent.ExtractSkillEvents, which dispatches through SkillEventExtractor:
one implementer, and the dispatcher returns nil for codex, cursor,
copilotcli, geminicli, opencode, factoryaidroid, pi and external without
touching the transcript. used_search is an unconditional payload
property, so all of them would have reported a fabricated false. Cursor
is not merely unimplemented but unprobeable -- its transcripts contain no
tool_use blocks at all (see ExtractModifiedFilesFromOffset) -- so
sniffing the shape per line cannot rescue this either: a Cursor line does
unmarshal as a transcript.Line and would score "understood, negative".
Trading a few inflating false positives for a whole population of
deflating false negatives is a worse metric, not a better one, because
the second kind is invisible.
So the "can I see tool calls at all" question has to be answered per
agent, and the answer has to reach the payload. New ToolInvocationScanner
capability (built-in only, same rationale as AsSkillEventExtractor) whose
dispatcher returns (found, supported); Claude Code is the sole
implementer. Agents that cannot be walked simply do not implement it, and
that is reported rather than guessed: used_search is now *bool and is
omitted from the payload when unmeasurable, with a new always-present
used_search_source ("unsupported" | "none" | "command" | "subagent").
A consumer filtering `used_search = false` therefore excludes unknowns
instead of absorbing them, because a missing PostHog property is not
false. Changing the payload is free exactly now, for the same reason the
rename one commit earlier was: the event has never been emitted in a
release and no dashboard keys on it.
Second, a Bash-only matcher fails hardest on the sessions that adopted
search the way we ship it. setup_search_skill.go installs the affordance
as a subagent (.claude/agents/entire-search.md, plus the Codex and Gemini
equivalents), so the intended path records an Agent/Task tool_use with
subagent_type=entire-search and runs the actual command in a *separate*
subagent transcript that condensation never reads. Matching the dispatch
is the primary case, not a nicety. Reading the subagent files instead was
not considered: the analogous token path is measured at ~29x (see the
comment above CalculateTokenUsage), and the dispatch carries the same
signal for free.
The command matcher requires command position -- start of command, or
after a shell separator, tolerating env assignments and a path prefix --
which is what separates `cd sub && entire search x` from `grep -rn
"entire search" cmd/` and `git commit -m "... entire search ..."`, where
the phrase sits inside an argument. Its internal separators are spelled
as single spaces rather than \s+ on purpose: that is what makes the
scanner's byte prefilter provably a performance filter and not a
correctness one, and TestSearchHintsCoverPattern fails if the pattern is
loosened without extending the hints. Residual false negatives (xargs,
loop variables, wrapper scripts) are documented and are the safer
direction -- the rate now reads as an upper bound.
On the placement half of the finding, which asked for this to fold into
an existing traversal: there was no second traversal to remove.
extractSessionData already runs CalculateTokenUsage *and*
ExtractSkillEvents in the same gate-held block, each a full
ParseFromBytes plus a per-assistant-line unmarshal, beside which the
9.1ms/10MB scan was a rounding error. What this buys is precision at
comparable cost -- two byte prefilters (hints, then the literal
"tool_use", which is exact because the quotes bracket the token and so
does not match the "tool_use_id" on every tool_result line) mean the
miss path stays a single pass with no allocations, and JSON work happens
only on candidate lines. It does NOT move work out of the session gate,
and the flag is now derived where every other transcript-derived field
is derived. Lines are split with bytes.IndexByte rather than
bufio.Scanner, whose 64KiB default token limit a single Claude line
routinely exceeds.
Tests: the three fixtures the old probe was tested against
({"tool":"Bash","command":...}) are shapes no agent writes, which is part
of how an 18-to-1 rate went unnoticed; they are replaced by real JSONL
covering each documented false-positive class (grep, commit message, tool
output, assistant prose, the scaffolded skill body being written, the
investigate prompt) plus both accepting forms.
TestDetectSearchUsage_UnprobeableAgentsReportUnsupported feeds cursor, pi
and copilot-cli a transcript that does contain a real invocation and
requires "unsupported" -- that is the test that keeps the metric honest,
and it is the case the suggested fix got wrong. Cursor and Pi also gained
MustNotImplementToolInvocationScanner tests carrying the reason.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…on NUL Three findings on the same function, landed together because each part would otherwise be written twice: the hoist changes its signature, and the format fix rewrites the body it returns. Per-session probe -> per-commit (MEDIUM). emitCommitCondensedTelemetry ran inside PostCommit's per-session loop and each call shelled out to `git log -z --skip=1 -n 50 --name-only`. Measured p50 of 25 runs: 13.8ms for the probe, and 8.7ms for `git --version` alone -- so roughly 9ms of it is process startup, and the timing is flat across a 60x range of output size (4.8KB to 133KB). The walk is nearly free; the process is the cost. Since the output is commit-scoped and identical for every session in the commit -- only the per-session intersection differs -- paying it per session was paying for spawns. So priorAICommitTouchedFiles (predicate) becomes priorAICommitFiles (set), and commitCondensedEmitter memoizes both the scan and the settings load for one commit. Constructed once in PostCommit from the worktreePath already in scope, which also drops the per-session paths.WorktreeRoot call and its error branch. Memoization is not the gate. Both halves resolve on FIRST USE, and the order inside emit is the promise the doc comment already made: a commit where no session condenses runs neither settings.Load nor git log, and an opted-out user never reaches the probe. Two tests pin that rather than trusting the reading -- a nil signal must not even resolve the gate, and an opted-out user (explicitly, and with the key absent, since telemetry is opt-in) must see zero probe calls. Plain flags rather than sync.Once because the loop is sequential and the struct's other state is unguarded anyway; that constraint is stated on the type. Don't oversell the settings half: settings.Load is uncached and already called several times per hook process elsewhere (see the OPF note in CLAUDE.md). This memoizes one hook, it does not fix that pattern. Telemetry gate idiom (LOW). The hand-rolled `s.Telemetry == nil || !*s.Telemetry` is gone; allowed() calls s.IsTelemetryEnabled(), the helper #2023 extracted precisely to stop this being copied a fourth time. The load is retained because s.Enabled is still needed at send time. Also adds the emitCommitCondensed send seam, matching emitSkillTelemetry, so the gate is testable without a PostHog client. Unescaped git-log format (LOW). `--format=%x1e%B%x1f` took both delimiters from the raw body, so a commit message containing either control character split a record early -- dropping the real Entire-Checkpoint trailer with it. Verified against git 2.53.0: the old format on such a commit yields a record whose message is " and ", whose trailer is absent, and whose "file list" is the rest of the message. That makes prior_ai_history read false, which suppresses a genuine miss. Same failure direction as the used_search finding, which is why it is worth fixing properly rather than narrowing. Replaced with `--format=%x00%H%n%B`. -z NUL-terminates the format output itself and a commit message cannot contain NUL, so splitting the whole output on NUL makes an empty field an unambiguous record marker -- one that rests on nothing about message content. %H earns its four characters by guaranteeing the post-marker field is non-empty, so an empty-message commit cannot emit two adjacent markers and mis-frame the record after it; TestPriorAICommitFiles_EmptyMessageCommit pins that, and the hash is a free debugging anchor. trailers.ParseCheckpoint stays the single definition of what a checkpoint trailer is, and its regex is unanchored, so the leading hash line is inert. Rejected %(trailers:key=Entire-Checkpoint,valueonly), which would keep the body out of the output entirely, on semantics rather than compatibility. It hands trailer detection to git's parser, which only recognises a trailer block in the message's final paragraph, while trailers.ParseCheckpoint matches anywhere. Commits whose trailer is not last-paragraph -- reworded commits, hand-edited squash bodies, and the multi-trailer squash-merge case ParseAllCheckpoints exists for -- would newly be missed, and the divergence would be invisible in the data. Keeping %B also keeps trailers.ParseCheckpoint as the single definition of what a checkpoint trailer is, which is worth more here than the narrower injection surface. (The placeholder also predates git 2.15, but that is 2017 and not a real constraint; the semantics are the reason.) Rejected a two-pass hashes-then-names design outright: spawns are the cost, per above. Merge commits still contribute nothing, and the reasoning is now recorded on the function instead of being implicit. --name-only emits no names for a merge, and the obvious fix (--diff-merges=first-parent) would make a "Merge origin/main into X" commit attribute every file it merged in, inflating prior_ai_history and therefore the miss rate. A merge's content is already attributed to the individual trailer-carrying commits it brings in, which are in this window on their own account -- 51 of the last 300 merges on main carry the trailer, so this is a frequent path, not a rare one. Squash merges are single-parent, so their files do appear. TestPriorAICommitFiles_MergeCommitContributesNothing pins the decision so a future "fix" has to argue with a test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pre-existing from #2023 rather than introduced here, filed on this trail for continuity with the rest of the review pass. Comment-only. SessionState.SkillEvents grows for the life of the session, and session state is read and written whole on every MutateSessionState -- i.e. every hook, including PostToolUse -- so the ledger is paid per hook for the rest of the session. Measured JSON round-trip: 0 events / 106 B / 1.6us; 10 / 6.0 KB / 42us; 50 / 29.7 KB / 201us; 200 / 119 KB / 785us. That is ~594 B per event and sub-millisecond at any realistic N, so this is not a blocker -- the point is that a 100 KB session state should read as by design rather than as a leak. It also cannot simply be trimmed, and the note says why: the durable ledger is what makes skill telemetry exactly-once, since extraction re-derives from transcript offset 0 on every pass and dedupes against it. Capping re-enables double-reporting for exactly the long sessions a cap would target -- a correctness regression bought with a sub-millisecond saving, which is the wrong trade in general and a particularly bad one on a branch about whether the numbers can be trusted. If the envelope ever does need shrinking, the move named in the comment is a narrower ledger (persist only the dedupe keys, ~40 B/event) rather than a truncation. The finding pointed at ExtractedSessionData.SkillEvents, which is the transient per-condensation copy and not the field that costs anything, so the substantive note goes on the durable one and the transient field gets a pointer to it. Leaving that pointer out is how the next reader re-derives the same wrong conclusion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…no signal Two HIGH trail findings read the absent commit-condensed emission in CondenseSessionByID and CondenseAndMarkFullyCondensed as data loss. The omission is deliberate, but nothing at those call sites said so — the invariant lived only on newCommitCondensedSignal, which is not where a reviewer looking at a condensation path arrives. Name the reason where the question comes up. These are the two pointers; the invariant itself stays with the signal. Originally written on jdx/skill-telemetry-nested-save, because that PR moving the surviving emission onto an onSaved callback put the asymmetry right where a reviewer looks. That PR is now based on main and no longer contains the commit-condensed signal at all, so the comments follow the code they describe. Comments only; no behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
07d7b32 to
8b40f77
Compare
Rebased onto
|
Two findings, both about the same seam between the matcher and the byte prefilter that decides whether a line is parsed at all. Case-folded matcher behind a case-sensitive prefilter. detectSearchUsage compared SubagentType with strings.EqualFold, so it accepted "Entire-Search" -- but the hint list is applied by claudecode.lineMayCarryInvocation through case-sensitive bytes.Contains, which discards that line before the matcher ever sees it. A silent false negative, and precisely the hazard ToolInvocationScanner's own doc warns callers about; TestSearchHintsCoverPattern missed it because it pinned only the canonical lowercase constant. Dropped EqualFold rather than widening the hints, per the finding: the subagent name is a literal we scaffold ourselves, so matcher and prefilter agreeing on exactness is the property worth having, and extra case variants would widen the prefilter for nothing. The test now asserts both halves -- that the hint list does not cover the case variants, and that the matcher rejects them -- so the two cannot drift apart again without failing. Cursor is not unprobeable, and I should not have written that it was. The claim came from cursor/transcript.go, which says Cursor "transcripts do not contain tool_use blocks" -- and I repeated it in ToolInvocationScanner's doc and in the cursor test's justification as though it were established. It is not: it dates to 366e4ee (2026-03-03, "copilot feedback"), it is restated in four comments plus AGENT.md, and no test pins it anywhere. Cursor uses the same JSONL format as Claude Code (cursor/transcript.go's own GetTranscriptPosition says so) and the shared transcript.ContentBlock type carries tool_use, so the shape supports it. The finding measured six real Cursor transcripts carrying tool_use blocks with name and input (Glob, Write, Read). I could not reproduce that locally -- this machine has no Cursor transcripts -- so I am taking the measurement on its specificity plus the stale, untested provenance of the claim it contradicts. Corrected both comments I wrote. Cursor still does not implement the scanner, but for the narrow, true reason: ToolInvocation.Command assumes Claude's `command` input key, the finding's own sample contains no shell-tool invocation, and guessing the key would manufacture the false negative this interface exists to prevent. "Needs a confirmed mapping" is not "impossible", and the test now says so and tells the next person to delete it once the mapping is known. Deliberately not fixed here: the stale premise in cursor/transcript.go, cursor.go, cursor_test.go and cursor/AGENT.md. It makes ExtractModifiedFilesFromOffset return nothing for every Cursor session, which is a file-detection question rather than a telemetry one and wants its own change with real fixtures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two more findings — and a correction to something I asserted aboveBoth fixed in LOW — case-folded matcher behind a case-sensitive prefilter. Dropped MEDIUM — I was wrong that Cursor is unprobeable, and I stated it as fact in this PR. My earlier comment said Cursor "is genuinely unprobeable, not just unimplemented." I took that from
The finding measured six real Cursor transcripts carrying Both comments now say "no walker yet" and explicitly warn against reading absence as impossibility. Cursor still doesn't implement the scanner, but for the narrow true reason rather than the false one: Filed separately, and bigger than this PR: the same stale premise makes Lint 0 issues, 56 Vogon + 4 roger-roger canary tests green. Both findings resolved on the trail. |
Two findings, one defect: a session that condenses without a readable
transcript carried searchProbe's zero value all the way to PostHog as
used_search=false with used_search_source="". That is precisely the
fabricated negative the tri-state was added to prevent, and it wore no
label to reveal itself -- the worst of both designs.
Three paths reached it, and skipIfNothingToCondense lets all three
condense on FilesTouched or task records alone, so these were real events
rather than a theoretical shape:
- extractSessionData and extractSessionDataFromLiveTranscript assigned
the probe inside `if len(data.Transcript) > 0`.
- extractOrCreateSessionData's default branch (no shadow branch, no
transcript path -- the Codex null-transcript_path case its own comment
names) built ExtractedSessionData without touching the field at all.
Fixed at every level, because the underlying defect is that the zero
value was unsafe rather than that three call sites forgot it.
The probe is now called unconditionally in both extractors.
detectSearchUsage already maps an empty transcript to unsupported -- that
is documented on it as "seeing nothing because there is nothing to see is
not evidence that the session did not search" -- so gating the call was
what manufactured the zero value in the first place. The default branch
sets unsupported explicitly.
The emit check was the structural mistake, and it is the part worth
keeping in mind: `source != searchSourceUnsupported` is a DENYLIST, so it
admitted "" and would admit any future source added without revisiting
it. Replaced with searchProbe.measured(), an allowlist over the three
known-measured sources, plus label(), which maps "" onto unsupported so
UsedSearchSource honours its documented contract of always being one of
the four names. Forgetting to set the probe now fails safe.
Tests pin the invariant rather than the three call sites, since the call
sites are what will change next: the zero value is not a measurement, an
unrecognised source is not a measurement, the three real sources are, and
an emit carrying a zero-value probe omits used_search and still labels
itself unsupported. Verified they bite -- restoring the denylist
reproduces the reported symptom exactly, UsedSearch=false with
UsedSearchSource="".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six confirmed findings from the pre-review pass: - The search probe is now computed at exactly one choke point (CondenseSession), so the interrupted-condensation recovery result carries it like every other path and no future extractor branch can ship the zero-value probe by omission. The three per-branch assignments are gone. - The scan is gated BEFORE it runs: condensation consults the emitter's memoized telemetry gate (condenseOpts.searchProbeAllowed), so opted-out and not-opted-in users never pay the full-transcript pass, and the two never-emitting condensation paths (doctor repair, session-end leftovers) never scan at all. - files_committed and prior_ai_history are commit-scoped by construction: newCommitCondensedSignal re-intersects FilesTouched against the commit's file set, so the one upstream path that deliberately does not narrow (filterFilesTouched on an empty committed set — --allow-empty, failed file detection) can no longer count a session's uncommitted files. - The command matcher no longer reads separators inside quoted arguments or heredoc bodies as command boundaries: sanitizeShellCommandForMatching blanks those spans (removal-only, so the hint-prefilter contract holds by construction) before the position regex runs. `git commit -m "wip; entire search notes"`, `rg "foo|entire search bar"`, and a heredoc writing the search skill's own body no longer count as invocations. - The prior-history probe is anchored: it walks git log from the explicit commit hash PostCommit is reporting on (HEAD can move before the post-gate probe runs) and scrubs the GIT_DIR/GIT_WORK_TREE that git exports to hooks, via gitEnvWithoutRepoOverrides like this package's other `git -C` call. - The entire-search subagent name is one exported constant (strategy.EntireSearchSubagentName): the installer scaffolds its paths from it and a new test pins the template bodies to it, so a rename fails a test instead of silently zeroing used_search_source="subagent". Also aligns the payload docs with the deliberate scoping split (used_search is session-scoped, the other properties commit-scoped) and fixes the stale "Cursor can never be probed" test comment that contradicted agent/tool_invocations.go. resolveShadowRefAndTree is extracted from postCommitProcessSessionLocked, which the new emitter parameter had pushed past the maintidx threshold. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The three findings deliberately deferred from the previous pass, now applied: - prior_ai_history gets the same honesty contract as used_search: the git-log probe reports measurability (ok=false on any failure), the emitter carries it as *bool, and the payload OMITS the property rather than fabricating a measured false — a shallow clone, missing git, or cancelled hook ctx must not read as "touched no AI-dense files". A commit that landed no files is a measured false and never probes at all. - cli_commit_condensed is at-most-once per checkpoint: an amend re-runs PostCommit with the SAME trailer checkpoint ID and an ACTIVE session re-condenses unconditionally, so one logical commit was counted twice in both halves of the ratio. newCommitCondensedSignal now keeps a per-session ledger (SessionState.CommitCondensedSignalCheckpointID) that rides the same state save gating the emit; a failed save retries cleanly, and unit fixtures with a zero checkpoint ID are exempted explicitly. - The tool_use walker gains the assistant-envelope gate every other tool_use consumer in the claudecode package applies, so a non-assistant envelope whose message decodes into tool_use-shaped content (replayed/sidechain or summary lines) can never count as a live invocation. Pinned at both the claudecode layer and through the strategy probe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PrepareCommitMsg stamps an Entire-Checkpoint trailer on a session it judges
eligible; PostCommit then decides independently whether to condense that
session. When the two disagree the commit ships a trailer naming a checkpoint
nobody wrote, so `entire explain` on it resolves to nothing. Each half looks
like a routine skip on its own, so the divergence is invisible today.
Log it at DEBUG when no session claims the trailer. DEBUG rather than WARN
because the condition cannot distinguish that bug from a trailer Entire never
stamped on this commit: `git merge --squash` and `git commit -C` copy one out
of an existing commit message without being sequencer operations, and
PrepareCommitMsg deliberately never stamps a squash. Pinning that down exactly
would mean recording at stamp time that Entire minted this trailer for this
commit, and PrepareCommitMsg writes no session state at all today — so it
would add a lock-taking write to a read-only hook on the latency-critical
path. An authoritative check belongs in `entire doctor`, which can afford to
ask the store whether the ID resolves.
Two cases are cheap to exclude and are excluded, so the line still means
something when grepped:
- Amend. handleAmendCommitMsg preserves or restores the trailer and a
message-only amend has nothing new to condense. Ownership is seeded from
the pre-loop snapshot, which also covers an amend whose owning session has
since ended and is skipped by the loop.
- Cherry-pick, revert, rebase replay. The trailer rides along from a source
commit whose session may not exist in this worktree, so ownership cannot be
established locally. The isRebase guard covers it.
Condensation is reported out of postCommitProcessSessionLocked rather than
inferred from session state afterwards: carry-forward on a partial commit
clears LastCheckpointID that condenseAndUpdateState had just set, so the
post-loop state of a successful partial condensation is indistinguishable from
never having condensed. Each of the three guards, and the partial-commit case,
has a test that fails when its guard is removed.
Rebased onto #2024's condensation telemetry, which added a per-session
*commitCondensedSignal result. Claim is deliberately NOT read from that signal:
newCommitCondensedSignal returns nil for an amend re-condensing a checkpoint it
already reported, so the signal is absent in exactly the case that must count
as claimed. A test covers that, and reusing the signal fails it.
Also corrects filterSessionsWithNewContent's doc: a withheld stale record is
not rescued by this commit's PostCommit, which returns at its no-trailer early
exit before reaching any session. The rescue is endSessionNow or a later
trailered commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M0TN61CAYP48Q3XH1MECRAM1
PrepareCommitMsg stamps an Entire-Checkpoint trailer on a session it judges
eligible; PostCommit then decides independently whether to condense that
session. When the two disagree the commit ships a trailer naming a checkpoint
nobody wrote, so `entire explain` on it resolves to nothing. Each half looks
like a routine skip on its own, so the divergence is invisible today.
Log it at DEBUG when no session claims the trailer. DEBUG rather than WARN
because the condition cannot distinguish that bug from a trailer Entire never
stamped on this commit: `git merge --squash` and `git commit -C` copy one out
of an existing commit message without being sequencer operations, and
PrepareCommitMsg deliberately never stamps a squash. Pinning that down exactly
would mean recording at stamp time that Entire minted this trailer for this
commit, and PrepareCommitMsg writes no session state at all today — so it
would add a lock-taking write to a read-only hook on the latency-critical
path. An authoritative check belongs in `entire doctor`, which can afford to
ask the store whether the ID resolves.
Two cases are cheap to exclude and are excluded, so the line still means
something when grepped:
- Amend. handleAmendCommitMsg preserves or restores the trailer and a
message-only amend has nothing new to condense. Ownership is seeded from
the pre-loop snapshot, which also covers an amend whose owning session has
since ended and is skipped by the loop.
- Cherry-pick, revert, rebase replay. The trailer rides along from a source
commit whose session may not exist in this worktree, so ownership cannot be
established locally. The isRebase guard covers it.
Condensation is reported out of postCommitProcessSessionLocked rather than
inferred from session state afterwards: carry-forward on a partial commit
clears LastCheckpointID that condenseAndUpdateState had just set, so the
post-loop state of a successful partial condensation is indistinguishable from
never having condensed. Each of the three guards, and the partial-commit case,
has a test that fails when its guard is removed.
Rebased onto #2024's condensation telemetry, which added a per-session
*commitCondensedSignal result. Claim is deliberately NOT read from that signal:
newCommitCondensedSignal returns nil for an amend re-condensing a checkpoint it
already reported, so the signal is absent in exactly the case that must count
as claimed. A test covers that, and reusing the signal fails it.
Also corrects filterSessionsWithNewContent's doc: a withheld stale record is
not rescued by this commit's PostCommit, which returns at its no-trailer early
exit before reaching any session. The rescue is endSessionNow or a later
trailered commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M0TN61CAYP48Q3XH1MECRAM1
https://entire.io/gh/entireio/cli/trails/1071
Raw search-command counts can't say whether adoption is low — most sessions have nothing worth searching for. The denominator that makes the rate meaningful is commits that landed files already carrying AI checkpoint history without the session ever consulting search. Entire is the only tool that can compute it, and nothing emitted it.
On each successful post-commit condensation, emit
cli_commit_condensed.Properties
used_searchentire search? Omitted when not measurable — see below.used_search_sourceused_searchwas determined:command,subagent,none, orunsupported. Always present.prior_ai_historyEntire-Checkpointtrailer?files_committedThe event is named for the commit, not the checkpoint, because every property is commit-scoped:
files_committedcounts that commit's files, andprior_ai_historyasks whether commits before this one touched them — which is what the probe's--skip=1is for. The commit-less condensation paths (doctor repair, session-end leftovers) deliberately emit nothing; rows without a commit would be indistinguishable from genuine misses and would inflate the denominator rather than complete it. Covering them would need atriggerdiscriminator plus nullable commit-scoped fields — a change to what the metric means, not a bug fix. The rationale is recorded at both call sites and onnewCommitCondensedSignal.Measuring
used_searchThe first version was a
bytes.Containsprobe. Review measured it at 18 false positives to 1 true positive across 328 real transcripts, and the false positives are structural — Entire installs the artifacts that trip its own probe (setup_search_skill.goembedsentire search --jsonin the search skill's body;investigate/prompt.goinjects it into every investigate prompt). The direction is what made it unusable: inflatingused_searchdeflates the missed-opportunity rate, so the metric reads "adoption is fine" for the wrong reason.It now matches a recorded tool invocation through a new built-in-only
ToolInvocationScannercapability whose dispatcher returns(found, supported):command— a shell tool ran the command, matched in command position (start of command or after a shell separator, tolerating env assignments and a path prefix), socd sub && entire search xmatches whilegrep -rn "entire search" cmd/does not.subagent— theentire-searchsubagent was dispatched. This is the primary path, not a nicety:setup_search_skill.goinstalls search as a subagent, so the intended usage records anAgent/Tasktool_useand runs the actual command in a separate subagent transcript that condensation never reads. A shell-only matcher would report "did not search" for exactly the sessions that adopted the feature.unsupported— no walker exists for this agent's transcript shape.used_searchis then omitted from the payload rather than sent as false, so a consumer filteringused_search = falseexcludes unknowns instead of absorbing them; a missing PostHog property is notfalse. Claude Code is the only implementer today.Consumers should compute the rate over the measurable population:
Privacy and cost
Content-free by construction: booleans, a count, and the agent registry key — never file paths, prompts, or transcript content. Gated on
ENTIRE_TELEMETRY_OPTOUTand then the opt-in telemetry setting before any probe work, so an opted-out user never pays for the git-log subprocess. The PostHog call is the existing detached-child path and never blocks the hook.Both commit-scoped costs are memoized once per commit rather than per session: the settings load and the single
git log, whose 13.8ms is almost entirely process spawn (8.7ms forgit --versionalone, and flat across a 60x range of output size). Both resolve on first use, so a commit where no session condenses pays nothing.The
git logrecord framing is NUL-anchored (--format=%x00%H%n%B). A commit message cannot contain NUL, so the record marker rests on nothing about message content — the previous%x1e/%x1fframing took its delimiters from the raw body, and a body containing either control character split a record early and dropped a real trailer, makingprior_ai_historyread false and suppressing a genuine miss.Tests
Regression coverage for each documented false-positive class (grep, commit message, tool output, assistant prose, the scaffolded skill body being written, the injected investigate prompt); unprobeable agents required to report
unsupportedwhen fed a transcript that does contain a real invocation; the hint/matcher case-sensitivity relationship pinned in both directions; the zero-value probe required not to present itself as a measurement; a commit body containing literal\x1e/\x1fstill attributed; empty-message and merge commits pinned; and the probe and settings load asserted to run once across several sessions.Verified green:
mise run lint0 issues, all 64 packages, plus both canary suites (56 Vogon, 4 roger-roger).🤖 Generated with Claude Code