feat(#844): instrument the real pr-review to emit LSP-pilot records (gated stream-json capture)

[don-petry]

Epic: #839 (LSP pilot) · Story: #844 · Builds on: #952 (standalone runner + lsp_pilot_measure.sh)

Summary

Upgrade the LSP-pilot comparison from the standalone probe shipped in #952 to riding the real production pr-review. Make the review path emit one pilot-schema JSONL record per reviewed PR, so every auto-review becomes a free LSP-off ("A") sample, and flipping LSP_PILOT_ENABLED yields the matching LSP-on ("B") sample through the same pipeline — a true apples-to-apples A/B with zero synthetic data.

Why

PR #952 landed the extractor (scripts/lsp_pilot_measure.sh), the harness (scripts/lsp_pilot_compare.sh, #841), and a standalone scripts/lsp_pilot_run.sh that re-runs reviews with a simplified prompt. The standalone driver does not use the production review, so its "A" side isn't what production actually does.

The blocker for using the real review: the headline metrics nav_tokens/tool_calls are only computable from the model's per-tool-call stream-json transcript, but the engine runs claude --output-format json (aggregate usage only — scripts/engine.sh:660) and never captures that transcript. So today a real review cannot produce a pilot record.

What to build

Behind the pilot being active (REVIEW_MCP_CONFIG set / LSP_PILOT_ENABLED=true), capture what's needed and emit a pilot record from the real review.

Acceptance criteria

1. Gated stream capture. When the pilot is active, the deep/audit/rubber-duck claude tiers additionally capture the --output-format stream-json --verbose transcript to a temp file. When the pilot is OFF, behaviour is byte-for-byte unchanged — no stream capture, no extra flags, no new records. (The five consumer repos that don't set LSP_PILOT_ENABLED must be wholly unaffected.)
2. Record emission. After a PR review completes under the pilot, invoke scripts/lsp_pilot_measure.sh on the captured transcript + TOKEN_LOG_FILE to append exactly one pilot-schema record to TOKEN_LOG_FILE — the existing Token Cost Observatory channel already uploaded as the token-usage-<run_id> artifact (.github/workflows/pr-review.yml:445). Discriminate it with a kind (e.g. kind:"lsp_pilot_run") so scripts/token_report.sh keeps excluding it from cost aggregation (its filter is (.kind // "token_usage") == "token_usage").
3. Record contents. pr join key (<repo>#<number>@<head_sha>), variant (lsp-off when the flag is off / lsp-on when on), candidate, nav_tokens, tool_calls, findings/false_positives (from the [Phase 2] Add the LSP finding-verification step to the deep/audit pr-review tiers #843 verification records), cold_start_s (from the [Phase 2] Cache the LSP index and enforce the 30s cold-start SLA with auto-skip #846 lsp_cold_start record), real reported usage, and wall_time_s.
4. Harness compatibility. Emitted records must join by pr and render via scripts/lsp_pilot_compare.sh unchanged (reuse feat(#844): live LSP off-vs-on comparison runner + measurement extractor #952's schema exactly).
5. Tests. Unit coverage for: the capture gate (active vs off), record emission/shape, and the off-pilot no-op. shellcheck --severity=warning -x scripts/*.sh clean; existing test_lsp_pilot_measure / test_lsp_coldstart_sla / test_token_metrics suites stay green.
6. Scope & safety. Additive + opt-in only. Do not modify the thin caller stubs beyond allowed inputs. Preserve all action/SHA pinning. pr-review.yml is consumed by 5 repos — no off-pilot change.

Out of scope

• Enabling LSP_PILOT_ENABLED in production — that's the owner-gated A/B run, not this change.
• Any change to lsp_pilot_compare.sh or the extractor's record schema.
• The go/no-go decision ([Phase 3] Record the human go/no-go decision and fleet-wide recommendation #845).

Done when

A real pr-review running under the pilot drops one lsp_pilot_run record into its token-usage-<run_id> artifact, and lsp_pilot_compare.sh renders an off-vs-on table from two such records (flag off vs on) with no synthetic input.

[github-actions]

Thanks for the detailed write-up and for laying out the acceptance criteria so clearly! Before we dig in: is there an existing integration test environment or staging workflow where we can validate the gated stream capture end-to-end before this touches any of the five consumer repos, or will we be relying solely on the unit tests described in criterion 5?

[don-petry]

Dev-Lead Implementation Plan

Issue: #960 — feat(#844): instrument the real pr-review to emit LSP-pilot records (gated stream-json capture)

Scope

Make the production pr-review path, when the LSP pilot is active, capture the deep/audit/rubber-duck claude tiers' --output-format stream-json --verbose transcript and emit exactly one kind:"lsp_pilot_run" pilot-schema record per PR onto TOKEN_LOG_FILE — additive and opt-in, byte-for-byte unchanged when off.

Design (key decisions)

• Capture gate = LSP_PILOT_ENABLED == "true" (the owner-gated A/B master switch). The five consumer repos never set it ⇒ wholly unaffected.
• Variant = lsp-on when REVIEW_MCP_CONFIG is a readable file (LSP MCP wired by setup-lsp-pilot.sh), else lsp-off. Candidate = agent-lsp / baseline respectively.
• Stream vs usage: when active + a capturing tier opts in, the claude call switches --output-format json → --output-format stream-json --verbose. The terminal result event still carries top-level .result + .usage, so the existing claude usage/text parsers work unchanged (token logging unaffected); the raw NDJSON is appended to a per-call file under a per-PR stream dir (avoids concurrent-append corruption from parallel deep+duck).
• Emission at review exit (EXIT trap, fires once per PR process; no-op when no transcript was captured, e.g. skip/noop paths). Runs lsp_pilot_measure.sh unchanged, then tags the record with kind:"lsp_pilot_run" via jq so token_report.sh keeps excluding it — the extractor schema is left untouched (out of scope to modify).
• Tiers captured: deep, audit, single (run_agentic) + rubber-duck (run_duck). triage/action/writer never capture.

Implementation Checklist

• scripts/engine.sh: add _lsp_pilot_active; gate stream-json capture in _claude_chain_invoke (honoring caller flag _LSP_PILOT_CAPTURE); append raw NDJSON to per-call file in LSP_PILOT_STREAM_DIR; reduce stream to terminal result event for usage/text parsing.
• scripts/engine.sh: set _LSP_PILOT_CAPTURE in run_agentic (deep/audit/single) and run_duck.
• scripts/lsp_pilot_emit.sh (new): pure helpers lpe_pilot_active, lpe_variant, lpe_candidate, lpe_pr_key, lpe_wall_seconds, plus lpe_emit_record (no-op when off / no transcript; appends one kind:"lsp_pilot_run" record).
• scripts/review-one-pr.sh: when pilot active, create per-PR stream dir + EXIT trap that emits the record and cleans up.
• tests/dev-lead/fixtures/engines/stub-claude: emit stream-json NDJSON (tool_use + result event) when --output-format stream-json is requested.

Tests to Write

• tests/dev-lead/unit/test_lsp_pilot_emit.bats: gate (active vs off), variant/candidate/pr-key parsing, record emission shape (+kind), empty-stream and off-pilot no-op.
• tests/dev-lead/unit/test_engine_lsp_pilot.bats: deep tier under pilot → --output-format stream-json --verbose + stream file written + token record still emitted; pilot off → --output-format json, no stream file (byte-for-byte); triage/action under pilot → no stream-json.
• Keep test_lsp_pilot_measure / test_lsp_coldstart_sla / test_token_metrics / test_engine_mcp green.

Test Command

bats tests/dev-lead/unit/ and bash .dev-lead/scripts/dev-lead-lint.sh

Note: test_engine_mcp.bats test 97 (REVIEW_MCP_DEBUG healthy-handshake ::notice::) is RED on a clean checkout before any change here — pre-existing and out of scope.

[don-petry]

✅ Implementation complete — issue #960

Instrumented the real production pr-review to emit one LSP-pilot record per review, behind the owner-gated LSP_PILOT_ENABLED flag. Off-pilot behaviour is byte-for-byte unchanged.

What changed

• scripts/engine.sh — gated stream-json capture in _claude_chain_invoke:
  • When LSP_PILOT_ENABLED=true and the caller is a capturing tier, the claude call switches --output-format json → --output-format stream-json --verbose. The terminal result event still carries top-level .usage/.result, so token logging and text extraction are unchanged.
  • The raw NDJSON is teed to a unique per-call file under $LSP_PILOT_STREAM_DIR (unique name → concurrent deep+duck calls never interleave).
  • Capturing tiers opt in via _LSP_PILOT_CAPTURE: run_agentic deep/audit/single and run_duck. Triage and action (writer/synthesis) never capture.
  • New helper _lsp_pilot_active.
• scripts/lsp_pilot_emit.sh (new, sourced helper) — lpe_pilot_active, lpe_variant (lsp-on when REVIEW_MCP_CONFIG is a readable file, else lsp-off — matching engine.sh's MCP activation), lpe_candidate (baseline / ${LSP_CANDIDATE:-agent-lsp}), lpe_pr_key (<owner/repo>#<num>@<sha>), lpe_wall_seconds, and lpe_emit_record. emit_record folds the per-call transcripts, runs lsp_pilot_measure.sh verbatim (its schema is reused, not modified), tags the line kind:"lsp_pilot_run" via jq, and appends to TOKEN_LOG_FILE. Strict no-op off-pilot / no transcript / no jq / no token log.
• scripts/review-one-pr.sh — when the pilot is on, source the emitter, create a per-PR LSP_PILOT_STREAM_DIR, record a start timestamp, and set an EXIT trap that emits exactly one record (any exit path) then cleans up. Inert off-pilot.
• tests/dev-lead/fixtures/engines/stub-claude — emit a realistic NDJSON transcript (tool_use + tool_result + terminal result with usage) for --output-format stream-json.

Why it joins apples-to-apples

kind:"lsp_pilot_run" keeps token_report.sh's cost aggregation excluding the record (its filter is (.kind // "token_usage") == "token_usage"). The record schema is lsp_pilot_measure.sh's output unchanged, so scripts/lsp_pilot_compare.sh joins/renders these production samples against lsp_pilot_run.sh's synthetic ones with no change. Owner runs the A leg (no MCP → lsp-off/baseline) and B leg (REVIEW_MCP_CONFIG=lsp.json → lsp-on/agent-lsp) through the same pipeline.

Verification

• New tests (TDD, written red-first):
  • tests/dev-lead/unit/test_lsp_pilot_emit.bats — 16 tests (gate, variant/candidate, pr-key, wall-seconds, emit_record no-op/active/lsp-on/empty-dir/multi-transcript/kind-filter).
  • tests/dev-lead/unit/test_engine_lsp_pilot.bats — 8 tests (off-pilot json unchanged + no capture; pilot deep/audit/duck → stream-json + transcript + usage still recorded; triage/action never capture; stream-dir-unset no crash).
• Full unit suite: bats tests/dev-lead/unit/ → 648 tests, 1 failure which is pre-existing and unrelated (test_engine_mcp.bats refactor: migrate to machine user PAT auth (closes #27) #28 "REVIEW_MCP_DEBUG … ::notice:: surfaced" — confirmed failing on a clean checkout with my changes stashed; out of scope).
• Lint: bash .dev-lead/scripts/dev-lead-lint.sh → all checks passed. shellcheck --severity=warning -x clean on all changed scripts and the stub fixture.

Out of scope (untouched, per the issue)

Enabling LSP_PILOT_ENABLED in prod; scripts/lsp_pilot_compare.sh and the lsp_pilot_measure.sh record schema; the go/no-go decision.

Note: per the dev-lead process, no commit/push/PR was created here — CI handles git.