diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml index 9a2d82641..ac30621b6 100644 --- a/.github/workflows/pr-review.yml +++ b/.github/workflows/pr-review.yml @@ -367,12 +367,29 @@ jobs: # threads the deep/audit-only MCP knobs into $GITHUB_ENV. A missing tool # degrades gracefully (::warning:: + skip) — it never fails the workflow. # Pins/candidate are overridable via repo variables; see docs/lsp-pilot.md. + # Cache the LSP toolchain/index across ephemeral runs (story #846) so a warm + # run restores under the 30s cold-start SLA instead of re-installing. Keyed on + # OS + candidate + pinned tool versions + a hash of the indexed Shell sources + # (the "relevant lockfile" for a Shell-only index). SHA-pinned to the same + # actions/cache the Claude CLI cache above uses. Opt-in behind LSP_PILOT_ENABLED; + # its cache-hit output is threaded into the setup step for the cold-start metric. + - name: Cache LSP index + id: lsp-cache + if: ${{ vars.LSP_PILOT_ENABLED == 'true' }} + uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: ~/.local + key: lsp-pilot-${{ runner.os }}-${{ vars.LSP_CANDIDATE || 'agent-lsp' }}-${{ vars.AGENT_LSP_VERSION || 'v0.15.0' }}-bls${{ vars.BASH_LANGUAGE_SERVER_VERSION || '5.6.0' }}-${{ hashFiles('scripts/**/*.sh', '**/package-lock.json') }} + restore-keys: | + lsp-pilot-${{ runner.os }}-${{ vars.LSP_CANDIDATE || 'agent-lsp' }}-${{ vars.AGENT_LSP_VERSION || 'v0.15.0' }}-bls${{ vars.BASH_LANGUAGE_SERVER_VERSION || '5.6.0' }}- + - name: Set up LSP pilot servers (opt-in) if: ${{ vars.LSP_PILOT_ENABLED == 'true' }} env: LSP_CANDIDATE: ${{ vars.LSP_CANDIDATE || 'agent-lsp' }} AGENT_LSP_VERSION: ${{ vars.AGENT_LSP_VERSION || 'v0.15.0' }} BASH_LANGUAGE_SERVER_VERSION: ${{ vars.BASH_LANGUAGE_SERVER_VERSION || '5.6.0' }} + LSP_INDEX_CACHE_HIT: ${{ steps.lsp-cache.outputs.cache-hit }} run: bash scripts/setup-lsp-pilot.sh - name: Enumerate candidate PRs diff --git a/scripts/lib/token-metrics.sh b/scripts/lib/token-metrics.sh index 97864d7df..bea56e0b7 100644 --- a/scripts/lib/token-metrics.sh +++ b/scripts/lib/token-metrics.sh @@ -163,6 +163,54 @@ emit_verification_record() { printf '%s\n' "$record" >> "$TOKEN_LOG_FILE" 2>/dev/null || true } +# emit_lsp_coldstart_record +# [reason] +# Appends one LSP cold-start record to TOKEN_LOG_FILE on the SAME JSONL channel as +# token records (story #846, epic #839). Discriminated by kind:"lsp_cold_start" so +# story #844's comparison harness can compute cold-start P50/P95 against the SLA and +# the cost report skips it (token_report.sh keeps only (.kind // "token_usage") == +# "token_usage" — a cold-start record is not a priced token-usage call). +# cache — "hit" | "miss" | "unknown" (actions/cache outcome) +# skipped — "true" when LSP wiring was skipped (SLA exceeded or toolchain absent) +# No-op when TOKEN_LOG_FILE is unset; swallows I/O errors so it never aborts a run. +emit_lsp_coldstart_record() { + [ -n "${TOKEN_LOG_FILE:-}" ] || return 0 + + # set -u-safe defaults (this lib runs under set -euo pipefail): a short call + # degrades to a defaulted record rather than crashing on an unbound positional. + local workflow="${1:-}" candidate="${2:-}" cold_start_ms="${3:-0}" cache="${4:-unknown}" + local skipped="${5:-false}" sla_ms="${6:-0}" reason="${7:-}" + + local ts run_id record + ts=$(date -u +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || echo "") + run_id="${GITHUB_RUN_ID:-}" + + record=$(jq -cn \ + --arg ts "$ts" \ + --arg workflow "$workflow" \ + --arg candidate "$candidate" \ + --arg cold_start_ms "$cold_start_ms" \ + --arg cache "$cache" \ + --arg skipped "$skipped" \ + --arg sla_ms "$sla_ms" \ + --arg reason "$reason" \ + --arg run_id "$run_id" \ + '{ + kind: "lsp_cold_start", + ts: $ts, + workflow: $workflow, + candidate: $candidate, + cold_start_ms: ($cold_start_ms | tonumber? // 0), + cache: $cache, + skipped: ($skipped == "true"), + sla_ms: ($sla_ms | tonumber? // 0), + reason: $reason, + run_id: $run_id + }' 2>/dev/null) || return 0 + + printf '%s\n' "$record" >> "$TOKEN_LOG_FILE" 2>/dev/null || true +} + # ───────────────────────────────────────────────────────────────────────────── # Real-usage capture (replaces the char/4 estimate when an engine reports usage) # diff --git a/scripts/setup-lsp-pilot.sh b/scripts/setup-lsp-pilot.sh index dfa7199c1..67d9cf9cb 100755 --- a/scripts/setup-lsp-pilot.sh +++ b/scripts/setup-lsp-pilot.sh @@ -62,9 +62,72 @@ AGENT_LSP_REPO="${AGENT_LSP_REPO:-blackwell-systems/agent-lsp}" # steps via $GITHUB_PATH. INSTALL_BIN="${INSTALL_BIN:-$HOME/.local/bin}" +# Cold-start SLA (story #846): toolchain bring-up (install / actions/cache restore +# → ready) must complete within this budget; over it the LSP wiring is auto-skipped +# (graceful degradation — never a workflow failure, docs/lsp-pilot.md §3). The +# index cache is what keeps a warm run under budget. Overridable for a tuning bump. +LSP_COLD_START_SLA_MS="${LSP_COLD_START_SLA_MS:-30000}" + +# Best-effort: the cold-start metric channel (Token Cost Observatory). The review +# job always has the repo checked out, so this resolves; if absent (odd invocation) +# emit_lsp_coldstart_record falls back to a no-op below so the script stays +# self-contained. +if [ -f "$(dirname "${BASH_SOURCE[0]}")/lib/token-metrics.sh" ]; then + # shellcheck source=scripts/lib/token-metrics.sh + source "$(dirname "${BASH_SOURCE[0]}")/lib/token-metrics.sh" +fi +if ! declare -f emit_lsp_coldstart_record >/dev/null 2>&1; then + emit_lsp_coldstart_record() { :; } +fi + warn() { echo "::warning::[lsp-pilot] $*" >&2; } note() { echo "[lsp-pilot] $*"; } +# _lsp_now_ms — current epoch in milliseconds. Falls back to second resolution on +# platforms where `date +%N` is unsupported (the literal `N` is left intact there). +_lsp_now_ms() { + local ns + ns="$(date +%s%N 2>/dev/null || echo '')" + case "$ns" in + # %.0f, not %d: epoch ms (~1.75e12) overflows awk's 32-bit %d conversion on + # common awks (mawk) and would yield a garbage cold-start measurement. + ''|*[!0-9]*) date +%s 2>/dev/null | awk '{ printf "%.0f", $1 * 1000 }' ;; + *) awk "BEGIN { printf \"%.0f\", $ns / 1000000 }" ;; + esac +} + +# _lsp_elapsed_ms — non-negative integer delta. Any non-numeric +# input or a backwards clock yields 0 (a glitch must not look like a slow start). +_lsp_elapsed_ms() { + local a="${1:-0}" b="${2:-0}" d + case "$a$b" in ''|*[!0-9]*) echo 0; return 0 ;; esac + d=$(( b - a )) + [ "$d" -lt 0 ] && d=0 + echo "$d" +} + +# _lsp_sla_exceeded — true (0) when cold-start is over the +# SLA. Pure, for unit coverage. A non-numeric cold-start (clock glitch) is the safe +# default: NOT exceeded, so a measurement error never spuriously skips LSP. +_lsp_sla_exceeded() { + local cold="${1:-}" sla="${2:-}" + case "$cold" in ''|*[!0-9]*) return 1 ;; esac + # sla may be negative (tests use -1 to force the skip path); strip a leading + # minus, then require the remainder to be all digits. + case "${sla#-}" in ''|*[!0-9]*) return 1 ;; esac + [ "$cold" -gt "$sla" ] +} + +# _lsp_cache_status — normalize an actions/cache `cache-hit` output to +# hit/miss/unknown (true→hit, false→miss, unset/other→unknown). +_lsp_cache_status() { + case "${1:-}" in + true|hit) echo "hit" ;; + false|miss) echo "miss" ;; + *) echo "unknown" ;; + esac +} + print_allowed_tools() { printf '%s\n' "$LSP_PILOT_ALLOWED_TOOLS"; } # install_bash_language_server — npm-global install of the pinned server. @@ -178,15 +241,41 @@ main() { fi export PATH="$INSTALL_BIN:$PATH" + # Time the toolchain bring-up (install / actions/cache restore → ready). This is + # the honest, shell-observable cold-start proxy: the language server's first-query + # indexing happens inside the claude subprocess and isn't visible here, but the + # bring-up window is exactly what the index cache (story #846) eliminates. The + # cache outcome is threaded in from the workflow's actions/cache step. + local cache_status t0 t1 cold_ms + cache_status="$(_lsp_cache_status "${LSP_INDEX_CACHE_HIT:-}")" + t0="$(_lsp_now_ms)" local server_ok=1 bls_ok=1 install_candidate || server_ok=0 install_bash_language_server || bls_ok=0 + t1="$(_lsp_now_ms)" + cold_ms="$(_lsp_elapsed_ms "$t0" "$t1")" if [ "$server_ok" -ne 1 ] || [ "$bls_ok" -ne 1 ]; then + emit_lsp_coldstart_record "pr-review" "$LSP_CANDIDATE" "$cold_ms" "$cache_status" \ + "true" "$LSP_COLD_START_SLA_MS" "toolchain-unavailable" warn "LSP toolchain unavailable (candidate=${LSP_CANDIDATE}) — review continues on base capabilities, LSP enrichment skipped" return 0 fi + # Cold-start SLA auto-skip (AC #3): an over-budget bring-up degrades the + # enrichment, not the review — same "warn, don't fail" contract as a missing + # tool. The review proceeds on base capabilities; the workflow never fails. + if _lsp_sla_exceeded "$cold_ms" "$LSP_COLD_START_SLA_MS"; then + emit_lsp_coldstart_record "pr-review" "$LSP_CANDIDATE" "$cold_ms" "$cache_status" \ + "true" "$LSP_COLD_START_SLA_MS" "sla-exceeded" + warn "LSP cold-start ${cold_ms}ms exceeded SLA ${LSP_COLD_START_SLA_MS}ms (cache=${cache_status}) — skipping LSP wiring; review continues on base capabilities" + return 0 + fi + + emit_lsp_coldstart_record "pr-review" "$LSP_CANDIDATE" "$cold_ms" "$cache_status" \ + "false" "$LSP_COLD_START_SLA_MS" "ok" + note "LSP cold-start ${cold_ms}ms within SLA ${LSP_COLD_START_SLA_MS}ms (cache=${cache_status})" + # Thread the opt-in knobs into the job env for the subsequent review step. # engine.sh reads these; with them set it appends --mcp-config/--strict-mcp-config # and merges the navigation tools into the deep/duck --allowed-tools (triage @@ -202,8 +291,12 @@ main() { fi } -case "${1:-}" in - print-allowed-tools) print_allowed_tools ;; - ""|setup) main ;; - *) echo "usage: $(basename "$0") [print-allowed-tools|setup]" >&2; exit 2 ;; -esac +# Run the dispatcher only when executed directly — sourcing (e.g. the unit tests +# for the pure cold-start/SLA helpers) must not trigger install + wiring. +if [ "${BASH_SOURCE[0]:-$0}" = "$0" ]; then + case "${1:-}" in + print-allowed-tools) print_allowed_tools ;; + ""|setup) main ;; + *) echo "usage: $(basename "$0") [print-allowed-tools|setup]" >&2; exit 2 ;; + esac +fi diff --git a/tests/dev-lead/unit/test_lsp_coldstart_sla.bats b/tests/dev-lead/unit/test_lsp_coldstart_sla.bats new file mode 100644 index 000000000..1fb201788 --- /dev/null +++ b/tests/dev-lead/unit/test_lsp_coldstart_sla.bats @@ -0,0 +1,160 @@ +#!/usr/bin/env bats +# Unit tests for the LSP cold-start SLA + index-cache wiring (epic #839, story #846). +# +# Builds on story #842's setup wiring — covers only what #846 adds on top: +# AC #1 — pr-review.yml caches the LSP toolchain/index (SHA-pinned actions/cache) +# and threads its cache-hit into the setup step. +# AC #2 — emit_lsp_coldstart_record writes a discriminated lsp_cold_start JSONL +# record (cold_start_ms, cache, skipped, sla_ms) to the Token Cost +# Observatory; cache status is normalized to hit/miss/unknown. +# AC #3 — an over-budget cold-start auto-skips LSP wiring via the "warn, don't +# fail" contract (no knobs, exit 0); a missing tool degrades the same way. + +SCRIPT_DIR="$(cd "$(dirname "$BATS_TEST_FILENAME")/../../.." && pwd)" +SETUP_SCRIPT="$SCRIPT_DIR/scripts/setup-lsp-pilot.sh" +WORKFLOW="$SCRIPT_DIR/.github/workflows/pr-review.yml" + +# Source the script in a clean subshell so its `set -euo pipefail` never leaks into +# the bats test shell, and the source-guard keeps main() from running. +_call() { run bash -c 'source "$1"; shift; "$@"' bash "$SETUP_SCRIPT" "$@"; } + +# ── AC #3: pure SLA decision helper ─────────────────────────────────────────── + +@test "_lsp_sla_exceeded: over budget → exceeded (0)" { + _call _lsp_sla_exceeded 31000 30000 + [ "$status" -eq 0 ] +} + +@test "_lsp_sla_exceeded: at budget → not exceeded (1)" { + _call _lsp_sla_exceeded 30000 30000 + [ "$status" -eq 1 ] +} + +@test "_lsp_sla_exceeded: under budget → not exceeded (1)" { + _call _lsp_sla_exceeded 12000 30000 + [ "$status" -eq 1 ] +} + +@test "_lsp_sla_exceeded: non-numeric cold-start → safe default, not exceeded (1)" { + _call _lsp_sla_exceeded "oops" 30000 + [ "$status" -eq 1 ] +} + +@test "_lsp_sla_exceeded: negative SLA (test seam) is exceeded by any cold-start (0)" { + _call _lsp_sla_exceeded 0 -1 + [ "$status" -eq 0 ] +} + +@test "_lsp_sla_exceeded: non-numeric SLA → safe default, not exceeded (1)" { + _call _lsp_sla_exceeded 5 abc + [ "$status" -eq 1 ] +} + +# ── AC #2: cache-status normalization + elapsed ─────────────────────────────── + +@test "_lsp_cache_status: true→hit, false→miss, unset→unknown" { + _call _lsp_cache_status true; [ "$output" = "hit" ] + _call _lsp_cache_status false; [ "$output" = "miss" ] + _call _lsp_cache_status ""; [ "$output" = "unknown" ] +} + +@test "_lsp_elapsed_ms: forward delta, and a backwards clock clamps to 0" { + _call _lsp_elapsed_ms 1000 1500; [ "$output" = "500" ] + _call _lsp_elapsed_ms 1500 1000; [ "$output" = "0" ] +} + +# ── AC #2: the cold-start metric record ─────────────────────────────────────── + +@test "emit_lsp_coldstart_record: writes a well-formed lsp_cold_start record" { + local log; log="$(mktemp)" + run bash -c 'source "$1"; TOKEN_LOG_FILE="$2" emit_lsp_coldstart_record pr-review agent-lsp 1234 hit false 30000 ok' \ + bash "$SETUP_SCRIPT" "$log" + [ "$status" -eq 0 ] + run jq -e '.kind=="lsp_cold_start" and .cold_start_ms==1234 and .cache=="hit" and .skipped==false and .sla_ms==30000 and .reason=="ok"' "$log" + [ "$status" -eq 0 ] + rm -f "$log" +} + +@test "emit_lsp_coldstart_record: no-op when TOKEN_LOG_FILE unset" { + run bash -c 'source "$1"; unset TOKEN_LOG_FILE; emit_lsp_coldstart_record pr-review agent-lsp 1 hit false 30000 ok; echo done' \ + bash "$SETUP_SCRIPT" + [ "$status" -eq 0 ] + [ "$output" = "done" ] +} + +# ── AC #2/#3: end-to-end setup paths ────────────────────────────────────────── + +_make_setup_env() { + SANDBOX_BIN="$(mktemp -d)" + export GITHUB_ENV="$(mktemp)" + export TOKEN_LOG_FILE="$(mktemp)" + export PATH="$SANDBOX_BIN:$PATH" +} + +_teardown_setup_env() { + rm -rf "$SANDBOX_BIN" + rm -f "$GITHUB_ENV" "$TOKEN_LOG_FILE" + unset GITHUB_ENV TOKEN_LOG_FILE SANDBOX_BIN +} + +_stub_tools_present() { + printf '#!/usr/bin/env bash\necho agent-lsp 0.15.0\n' > "$SANDBOX_BIN/agent-lsp" + printf '#!/usr/bin/env bash\necho 5.6.0\n' > "$SANDBOX_BIN/bash-language-server" + chmod +x "$SANDBOX_BIN/agent-lsp" "$SANDBOX_BIN/bash-language-server" +} + +@test "setup: cold-start under SLA → knobs wired + record skipped:false" { + _make_setup_env + _stub_tools_present + export LSP_INDEX_CACHE_HIT=true + LSP_COLD_START_SLA_MS=30000 run bash "$SETUP_SCRIPT" + [ "$status" -eq 0 ] + grep -q "^REVIEW_MCP_CONFIG=.github/mcp/lsp.json$" "$GITHUB_ENV" + grep -q "^REVIEW_MCP_ALLOWED_TOOLS=mcp__lsp__" "$GITHUB_ENV" + run jq -e 'select(.kind=="lsp_cold_start") | .skipped==false and .reason=="ok" and .cache=="hit"' "$TOKEN_LOG_FILE" + [ "$status" -eq 0 ] + unset LSP_INDEX_CACHE_HIT + _teardown_setup_env +} + +@test "setup: cold-start over SLA → ::warning:: + no knobs + exit 0 + record skipped:true" { + _make_setup_env + _stub_tools_present + # SLA of -1ms is exceeded by any non-negative cold-start → deterministic skip. + LSP_COLD_START_SLA_MS=-1 run bash "$SETUP_SCRIPT" + [ "$status" -eq 0 ] + [[ "$output" == *"::warning::"* ]] + [[ "$output" == *"exceeded SLA"* ]] + ! grep -q "^REVIEW_MCP_CONFIG=" "$GITHUB_ENV" + ! grep -q "^REVIEW_MCP_ALLOWED_TOOLS=" "$GITHUB_ENV" + run jq -e 'select(.kind=="lsp_cold_start") | .skipped==true and .reason=="sla-exceeded"' "$TOKEN_LOG_FILE" + [ "$status" -eq 0 ] + _teardown_setup_env +} + +@test "setup: toolchain uninstallable → record skipped:true (toolchain-unavailable) + exit 0, no knobs" { + _make_setup_env + # No agent-lsp / bash-language-server on PATH; stub the installers to fail so + # nothing installs and no network is touched. + for cmd in npm gh curl uv; do + printf '#!/usr/bin/env bash\nexit 1\n' > "$SANDBOX_BIN/$cmd" + chmod +x "$SANDBOX_BIN/$cmd" + done + run bash "$SETUP_SCRIPT" + [ "$status" -eq 0 ] + [[ "$output" == *"::warning::"* ]] + ! grep -q "^REVIEW_MCP_CONFIG=" "$GITHUB_ENV" + run jq -e 'select(.kind=="lsp_cold_start") | .skipped==true and .reason=="toolchain-unavailable"' "$TOKEN_LOG_FILE" + [ "$status" -eq 0 ] + _teardown_setup_env +} + +# ── AC #1: workflow wiring ──────────────────────────────────────────────────── + +@test "workflow: pr-review.yml caches the LSP index (SHA-pinned) and threads cache-hit" { + # SHA-pinned actions/cache (the same SHA the Claude CLI cache uses), a Cache LSP + # index step, and the cache-hit output threaded into the setup step's env. + grep -q "name: Cache LSP index" "$WORKFLOW" + grep -q "actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae" "$WORKFLOW" + grep -q "LSP_INDEX_CACHE_HIT: \${{ steps.lsp-cache.outputs.cache-hit }}" "$WORKFLOW" +}