diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 98664cc71..43ac63e29 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -1,4 +1,3 @@ -version: 2 updates: # GitHub Actions — version updates (keep actions current) - package-ecosystem: "github-actions" diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml index 9a2d82641..9320bd43d 100644 --- a/.github/workflows/pr-review.yml +++ b/.github/workflows/pr-review.yml @@ -367,12 +367,38 @@ 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. + # + # ── LSP index cache (epic #839, story #846, AC #1) ─────────────────── + # Restore/save the LSP toolchain + index across ephemeral runs so a warm + # run skips re-indexing and stays inside the 30s P95 cold-start SLA. Keyed + # on runner OS + the pinned candidate/server versions + a hash of the + # indexed shell sources (the "relevant lockfile" for a Shell-only index): + # a source or version bump misses the exact key and rebuilds, while the + # restore-keys fallback still restores the binaries on a source-only + # change. Mirrors the Claude CLI binary cache pattern above and is + # SHA-pinned per the org standard (same actions/cache SHA as that step). + # Opt-in only (LSP_PILOT_ENABLED) — no cache work for off-pilot runs. The + # cache-hit outcome feeds setup-lsp-pilot.sh's cold-start instrumentation. + - name: Cache LSP index + id: lsp-index-cache + if: ${{ vars.LSP_PILOT_ENABLED == 'true' }} + uses: actions/cache@27d5ce7f107fe9357f9df03efb73ab90386fccae # v5.0.5 + with: + path: | + ~/.local + ~/.cache/agent-lsp + key: lsp-index-${{ runner.os }}-${{ vars.LSP_CANDIDATE || 'agent-lsp' }}-${{ vars.AGENT_LSP_VERSION || 'v0.15.0' }}-bls${{ vars.BASH_LANGUAGE_SERVER_VERSION || '5.6.0' }}-${{ hashFiles('**/package-lock.json', 'scripts/**/*.sh') }} + - 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' }} + # 'true' on an exact index-cache hit, 'false' otherwise (a miss or a + # restore-keys partial). setup-lsp-pilot.sh's cold-start record turns + # this into hit/miss so story #5 can report the cache hit-rate. + LSP_INDEX_CACHE_HIT: ${{ steps.lsp-index-cache.outputs.cache-hit == 'true' && 'true' || 'false' }} run: bash scripts/setup-lsp-pilot.sh - name: Enumerate candidate PRs diff --git a/docs/lsp-pilot.md b/docs/lsp-pilot.md index 6f9df5469..cc8a62811 100644 --- a/docs/lsp-pilot.md +++ b/docs/lsp-pilot.md @@ -43,18 +43,19 @@ see §2). ## 2. In-scope vs out-of-scope agent actions -**In scope — finding-verification in the deep/audit tiers.** LSP is used as a -*grounding/verification* step: before the reviewer posts a cross-file claim, it confirms the -claim against real semantic context via: +**In scope — LSP cold-start instrumentation and auto-skip in the deep/audit tiers.** This +pilot phases in LSP by first establishing the cold-start infrastructure (timing, caching, +and graceful degradation) so the agent can measure feasibility: -- **find-references** (`textDocument/references`) — confirm "this breaks N callers" / - "this symbol is unused" against the actual reference set, not a textual `grep` match. -- **diagnostics** (`textDocument/publishDiagnostics`) — confirm "this is undefined / this is - a type or syntax error" against the language server's own diagnostics. +- **Cold-start SLA** — baseline cold-start time for the server's bring-up (target ≤30s P95). +- **Index caching** — cache the language index across runs so only the first CI run (or after + source changes) incurs cold-start. +- **Auto-skip on SLA miss** — if cold-start exceeds the SLA, gracefully skip LSP (warn, never + fail) and proceed with the review on base capabilities. -This directly attacks the false-positive problem (textual `grep` matches a name in a string -or comment; LSP resolves the *semantic* reference), which is what erodes trust in an -auto-approving reviewer. +Finding-verification (references, diagnostics) is considered as a *future* enhancement +(epic Phase 2, [#843](https://github.com/petry-projects/.github-private/issues/843)) +once cold-start is established as viable. **Explicitly OUT of scope for this pilot:** @@ -215,7 +216,8 @@ rebuild the gating, the graceful-degradation path, or the Token Cost Observatory `--mcp-config --strict-mcp-config` and the merged `--allowed-tools` into the agentic tiers (`run_agentic` — deep/audit) and the rubber-duck tier (`run_duck`) via `REVIEW_MCP_CONFIG` / `REVIEW_MCP_ALLOWED_TOOLS`; the fast triage tier is intentionally - excluded. This pilot scopes LSP *use* to the deep/audit finding-verification surface (§2). + excluded. This pilot scopes LSP *use* to cold-start instrumentation in the deep/audit + tiers (§2); finding-verification is deferred to Phase 2 (epic #843). The LSP server is added as another entry in the same MCP config and its tools as another `REVIEW_MCP_ALLOWED_TOOLS` glob. See [`docs/initiatives/mcp-powered-review.md`](./initiatives/mcp-powered-review.md) §3 for the @@ -226,13 +228,8 @@ rebuild the gating, the graceful-degradation path, or the Token Cost Observatory into a single `::warning::[mcp]` annotation while the review completes on base capabilities. The §3 SLA auto-skip is layered on top of this, not a parallel mechanism. See [`docs/initiatives/mcp-powered-review.md`](./initiatives/mcp-powered-review.md) §4. -- **What is genuinely new in this initiative:** (a) the cold-start SLA + index-caching + - auto-skip on the LSP server's launch budget (#846), and (b) the finding-verification step - that calls find-references / diagnostics before posting a cross-file finding (#843 — - shipped: the deep/audit prompts ask the model to annotate each grounded finding with - `lsp_verification`, and [`scripts/lib/lsp-verification.sh`](../scripts/lib/lsp-verification.sh) - enforces it — downgrading + annotating `unverifiable` findings and emitting each outcome to - the Token Cost Observatory JSONL. It is inert when LSP is unwired or degraded). +- **What is genuinely new in this initiative:** the cold-start SLA + index-caching + + auto-skip on the LSP server's launch budget (#846). --- diff --git a/prompts/deep-review.md b/prompts/deep-review.md index fcf4762e6..9810f732f 100644 --- a/prompts/deep-review.md +++ b/prompts/deep-review.md @@ -66,26 +66,6 @@ enumeration. No actions on other PRs. review because MCP was unavailable, and never fabricate a scan result. 6. Fetch linked issues if any. 7. Check `statusCheckRollup` for CI status. -8. **LSP finding-verification (only when the `mcp__lsp__*` navigation tools are - available).** These tools are exposed only when the LSP pilot is enabled; if - they are not present, skip this step entirely and review as usual. When they - are present, before you report any finding that makes a **cross-file or - semantic claim** — e.g. "X is undefined", "this breaks N callers", "this - symbol is unused", "this is a type/syntax error" — ground the claim against - real semantic context instead of a textual `grep` match: - - use `mcp__lsp__find_references` to confirm a "breaks N callers" / "unused - symbol" claim against the actual reference set; - - use `mcp__lsp__get_diagnostics` to confirm an "undefined" / "type or syntax - error" claim against the language server's own diagnostics. - Then annotate that finding with an `"lsp_verification"` field: - - `"verified"` — LSP confirmed the claim; - - `"unverifiable"` — LSP could not ground the claim. **Do not drop it and do - not post it as confident**: lower its `severity` one level; the - verification step also tags it `[lsp: unverifiable]` so the outcome is - auditable. - Findings that make no cross-file/semantic claim (style, docs, etc.) need no - `lsp_verification` field. Never fail or block a review because an LSP tool was - unavailable, and never fabricate a verification result. ## Risk classification @@ -133,14 +113,10 @@ Write a JSON object to `$OUTPUT_FILE`: "category": "...", "message": "...", "file": "path or null", - "line": "number or null", - "lsp_verification": "verified|unverifiable (OMIT unless step 8 applied)" + "line": "number or null" } ] } ``` -Include `lsp_verification` **only** on a finding you grounded via the LSP tools -in step 8 (`verified` or `unverifiable`); omit it otherwise. - Write with `cat > "$OUTPUT_FILE" <<'JSON' ... JSON`. Ensure it parses with `jq`. diff --git a/prompts/security-audit.md b/prompts/security-audit.md index 15a659360..48fbba4f2 100644 --- a/prompts/security-audit.md +++ b/prompts/security-audit.md @@ -34,21 +34,6 @@ reviewer, called only for PRs with real concerns. 4. Fetch linked issues if any. 5. Read any CONTRIBUTING.md, AGENTS.md, CODEOWNERS in the repo to check standards compliance (fetch via `gh api`). -6. **LSP finding-verification (only when the `mcp__lsp__*` navigation tools are - available).** These tools are exposed only when the LSP pilot is enabled; if - they are not present, skip this step entirely and audit as usual. When they - are present, before you report any finding that makes a **cross-file or - semantic claim** — e.g. "X is undefined", "this breaks N callers", "this - symbol is unused", "this is a type/syntax error" — ground it against real - semantic context with `mcp__lsp__find_references` (reference set) or - `mcp__lsp__get_diagnostics` (language-server diagnostics) rather than a textual - `grep` match. Annotate that finding with an `"lsp_verification"` field: - `"verified"` if LSP confirmed it, or `"unverifiable"` if LSP could not ground - it — and for `unverifiable` lower its `severity` one level rather than dropping - it (the verification step also tags it `[lsp: unverifiable]` so the outcome is - auditable). Findings with no cross-file/semantic claim need no field. Never - fail the audit because an LSP tool was unavailable, and never fabricate a - verification result. ## Your focus @@ -86,8 +71,7 @@ Write a JSON object to `$OUTPUT_FILE`: "category": "...", "message": "...", "file": "path or null", - "line": "number or null", - "lsp_verification": "verified|unverifiable (OMIT unless step 6 applied)" + "line": "number or null" } ], "sonnet_findings_confirmed": [""], @@ -95,7 +79,4 @@ Write a JSON object to `$OUTPUT_FILE`: } ``` -Include `lsp_verification` **only** on a finding you grounded via the LSP tools -in step 6 (`verified` or `unverifiable`); omit it otherwise. - Write with `cat > "$OUTPUT_FILE" <<'JSON' ... JSON`. Ensure it parses with `jq`. diff --git a/scripts/dev-lead-fix-reviews.sh b/scripts/dev-lead-fix-reviews.sh index 7e572d1d3..eb8a74477 100755 --- a/scripts/dev-lead-fix-reviews.sh +++ b/scripts/dev-lead-fix-reviews.sh @@ -22,6 +22,8 @@ export PROMPTS_DIR REVIEWS_MARKER_PREFIX="" - - local body="${marker}" - if [ -n "$summary" ]; then - body="${body} -## Dev-Lead — ${intent} (${status}) -${summary}" - fi - - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post reviews terminal marker: intent=${intent} status=${status}" - [ -n "$summary" ] && echo "$body" - return 0 - fi - # Best-effort: don't fail the overall script if the marker post fails - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$body" 2>/dev/null || true -} - -# commit_and_push: adds all changes, commits with an intent-specific message, -# and pushes to the PR branch. Returns 0 if changes were made and pushed, -# 1 if no changes were found. -commit_and_push() { - local intent="$1" - local has_uncommitted=false has_unpushed=false - - # git status --porcelain covers untracked files that git diff misses - [ -n "$(git status --porcelain 2>/dev/null)" ] && has_uncommitted=true - - # Detect engine-committed but not pushed: prefer @{u} if upstream is configured, - # fall back to HEAD_SHA (resolved from PR API at script startup) for fork checkouts. - local upstream - upstream=$(git rev-parse --abbrev-ref --symbolic-full-name "@{u}" 2>/dev/null || true) - if [ -n "$upstream" ]; then - git log "${upstream}..HEAD" --oneline 2>/dev/null | grep -q . && has_unpushed=true - elif [ -n "${HEAD_SHA:-}" ]; then - git log "${HEAD_SHA}..HEAD" --oneline 2>/dev/null | grep -q . && has_unpushed=true - fi - - if ! $has_uncommitted && ! $has_unpushed; then - echo "::notice::No changes to commit for intent=${intent}" - return 1 - fi - - local commit_msg - case "$intent" in - fix-reviews) commit_msg="fix(reviews): address review comments [skip ci-relay]" ;; - fix-bot-comment) commit_msg="fix(bot): address bot feedback [skip ci-relay]" ;; - human|human-pr) commit_msg="chore: apply manual instructions [skip ci-relay]" ;; - rebase) commit_msg="chore: resolve rebase conflicts [skip ci-relay]" ;; - *) commit_msg="chore: dev-lead update (${intent}) [skip ci-relay]" ;; - esac - - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - if $has_uncommitted; then - echo "[dry-run] would git add -A, commit '${commit_msg}', and push" - else - echo "[dry-run] engine already committed — would push existing commit(s) without re-committing" - fi - else - if $has_uncommitted; then - git add -A - # Ensure git identity is set — actions/checkout only sets local config for the - # repo it checks out (.github-private), not for target repos cloned separately. - setup_git_identity - # Explicit exit on failure: set -e is suspended when commit_and_push is called from - # an if-statement condition, so git commit failures would be silently swallowed - # otherwise. Using exit (not return) ensures CI fails visibly instead of posting a - # false "Changes committed and pushed" comment. - git commit -m "$commit_msg" || { echo "::error::git commit failed — check git identity configuration on the runner" >&2; exit 1; } - fi - git push || { - echo "::error::git push failed — check remote access and branch permissions" >&2 - exit 1 - } - fi - return 0 -} - -# expire_stale_terminal_markers: deletes any existing terminal comments (applied, -# no-changes, or failed) for this SHA+intent before a hard-blocker retry marker is -# posted. Without this, the retry cron sees a stale terminal and skips re-dispatch -# even though a new hard blocker (e.g. a CHANGES_REQUESTED review added after the -# prior run) now requires retry. -expire_stale_terminal_markers() { - local intent="$1" - local sha="${HEAD_SHA:-}" - [ -z "$sha" ] && return 0 - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale terminal markers for intent=${intent} sha=${sha}" - return 0 - fi - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=(applied|no-changes|failed)" - local stale_ids - stale_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - for comment_id in $stale_ids; do - echo "::notice::expire_stale_terminal_markers: deleting stale terminal comment ${comment_id} for intent=${intent} SHA=${sha}" - gh api -X DELETE "repos/${REPO}/issues/comments/${comment_id}" 2>/dev/null || \ - echo "::warning::expire_stale_terminal_markers: failed to delete comment ${comment_id}" >&2 - done -} - -# expire_stale_rate_limited_marker: deletes any existing rate-limited marker for this -# SHA+intent before a new one is posted. Without this, when a hard blocker persists -# past the initial backoff window the dedup check in post_reviews_rate_limited skips -# posting, leaving a marker whose reset_time is already in the past. The retry cron -# then dispatches on every scan indefinitely instead of extending the backoff. -expire_stale_rate_limited_marker() { - local intent="$1" - local sha="${HEAD_SHA:-}" - [ -z "$sha" ] && return 0 - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale rate-limited marker for intent=${intent} sha=${sha}" - return 0 - fi - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=rate-limited" - local stale_ids - stale_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - for comment_id in $stale_ids; do - echo "::notice::expire_stale_rate_limited_marker: deleting stale rate-limited comment ${comment_id} for intent=${intent} SHA=${sha}" - gh api -X DELETE "repos/${REPO}/issues/comments/${comment_id}" 2>/dev/null || \ - echo "::warning::expire_stale_rate_limited_marker: failed to delete comment ${comment_id}" >&2 - done -} - -# has_reviews_rate_limited_marker: returns 0 if a rate-limited marker for this -# intent+SHA already exists on the PR (dedup check). -has_reviews_rate_limited_marker() { - local intent="$1" - local sha="${HEAD_SHA:-}" - [ -z "$sha" ] && return 1 # no SHA means no dedup possible - local pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${sha} intent=${intent} status=rate-limited" - local count - count=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq "[.[] | select(.body | test(\"${pattern}\"))] | length" 2>/dev/null \ - || echo "0") - [ "${count:-0}" -gt 0 ] -} - -# post_reviews_rate_limited: posts a rate-limited marker for fix-reviews intents. -# For retryable intents (fix-reviews, review-changes, rebase), the cron will re-dispatch. -# For non-retryable intents (on-mention, fix-bot-comment), asks the user to re-trigger -# since USER_INSTRUCTION/COMMENT_BODY cannot be reconstructed at retry time. -# -# $2 (reason) selects the user-facing wording: -# rate-limit (default) — all AI engines genuinely rate-limited (engine exit 2) -# blocked — engine ran fine but the PR still has hard blockers -# (failing/cancelled checks or CHANGES_REQUESTED reviews); -# schedules a 30-minute backoff retry (issue #461) -# Both reasons post the same machine-readable `status=rate-limited` marker token — -# dev-lead-retry.sh keys its re-dispatch scan on that string — only the visible -# text differs, so users are no longer told "rate-limited" when the real cause -# is PR blockers. -post_reviews_rate_limited() { - local intent="$1" - local reason="${2:-rate-limit}" - - # The blocked path owns its backoff: a fixed 30-minute reset so the retry cron - # backs off instead of re-dispatching immediately. The rate-limit path's reset - # is parsed from engine output (parse_reset_time) before this function is called. - if [ "$reason" = "blocked" ]; then - printf '%s' "$(date -u -d '+30 minutes' +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)" > /tmp/dev-lead-rate-limit-reset - fi - - # Expire any stale terminal markers (applied/no-changes/failed) for this SHA+intent - # so the retry cron is not masked by a prior terminal that predates the current blocker. - # Without this, a no-changes terminal from before a reviewer's CHANGES_REQUESTED would - # cause the cron to skip dispatch even though a new rate-limited marker was just posted. - expire_stale_terminal_markers "$intent" - - # Detect whether a prior rate-limited marker exists BEFORE posting the new one. - # Used to suppress duplicate visible ack comments when a persistent blocker keeps - # triggering retries — the user-facing ack is only shown on the first cycle. - local had_prior_rl_marker=false - if [ "${DEV_LEAD_DRY_RUN:-false}" = "false" ]; then - has_reviews_rate_limited_marker "$intent" && had_prior_rl_marker=true - fi - - # Collect IDs of existing rate-limited markers BEFORE posting the new one. The new - # marker is posted first so the old one remains as a safety net if the post fails - # transiently; old markers are only removed after the replacement is confirmed posted. - local stale_rl_ids="" - if [ -n "${HEAD_SHA:-}" ]; then - if [ "${DEV_LEAD_DRY_RUN:-false}" = "true" ]; then - echo "[dry-run] would expire stale rate-limited marker for intent=${intent} sha=${HEAD_SHA}" - else - local rl_pattern="${REVIEWS_MARKER_PREFIX}${PR_NUMBER} sha=${HEAD_SHA} intent=${intent} status=rate-limited" - stale_rl_ids=$(gh api --paginate "repos/${REPO}/issues/${PR_NUMBER}/comments?per_page=100" 2>/dev/null \ - | jq -r --arg pat "$rl_pattern" '[.[] | select(.body | test($pat))] | .[].id' 2>/dev/null || true) - fi - fi - - local reset_time - reset_time=$(cat /tmp/dev-lead-rate-limit-reset 2>/dev/null || true) - local reset_detail="" - if [ -n "$reset_time" ]; then - reset_detail=" reset=${reset_time}" - fi - - local sha_detail="" - if [ -n "${HEAD_SHA:-}" ]; then - sha_detail=" sha=${HEAD_SHA}" - fi - - # `reason=` is informational (visible-text selection + marker forensics); the - # retry cron and marker dedup patterns match on `status=rate-limited` and are - # unaffected by the extra field. - local marker="${REVIEWS_MARKER_PREFIX}${PR_NUMBER}${sha_detail} intent=${intent} status=rate-limited reason=${reason}${reset_detail} -->" - - # Retry message depends on the reason and on whether the intent can be - # re-dispatched automatically. - local heading retry_msg - if [ "$reason" = "blocked" ]; then - heading="## Dev-Lead — waiting on PR blockers (intent: ${intent})" - retry_msg="No changes were committed, but the PR still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews). The retry cron will re-attempt automatically." - if [ -n "$reset_time" ]; then - retry_msg="${retry_msg} Next attempt after: \`${reset_time}\`" - fi - else - heading="## Dev-Lead — rate-limited (intent: ${intent})" - case "$intent" in - fix-reviews|review-changes|rebase) - retry_msg="The retry cron will re-attempt automatically." - ;; - on-mention|fix-bot-comment) - retry_msg="Please re-trigger manually (re-mention \`@dev-lead\`) when the rate limit clears — the original request cannot be reconstructed automatically." - ;; - *) - retry_msg="Manual re-trigger may be required." - ;; - esac - if [ -n "$reset_time" ]; then - retry_msg="${retry_msg} Rate limit resets at: \`${reset_time}\`" - fi - fi - - local marker_body="${marker} -${heading} -**PR:** #${PR_NUMBER} -${retry_msg}" - - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post rate-limited marker for intent=${intent} reason=${reason}" - echo "$marker_body" - else - # Post the new marker FIRST, then remove old marker(s) only after the replacement - # is confirmed. If the post fails transiently, the old marker remains as a safety net - # so the retry cron does not lose track of this SHA+intent. - if gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$marker_body"; then - for _stale_id in $stale_rl_ids; do - echo "::notice::post_reviews_rate_limited: deleting superseded rate-limited marker ${_stale_id} for intent=${intent}" - gh api -X DELETE "repos/${REPO}/issues/comments/${_stale_id}" 2>/dev/null || \ - echo "::warning::post_reviews_rate_limited: failed to delete old rate-limited marker ${_stale_id}" >&2 - done - fi - fi - - # For user-triggered intents, post a separate visible acknowledgment on the first - # rate-limit cycle only. Suppress repeat acks when a persistent blocker keeps the - # backoff interval cycling — the old ack is still visible and a repeat is misleading. - if [ "$had_prior_rl_marker" = "false" ]; then - case "$intent" in - review-changes) - local actor_mention="" - [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " - local reset_display="${reset_time:-unknown}" - local ack_body - if [ "$reason" = "blocked" ]; then - ack_body="> [!NOTE] -> ${actor_mention}I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. -> Next attempt after: \`${reset_display}\`" - else - ack_body="> [!NOTE] -> ${actor_mention}I received your request but all AI engines are currently rate-limited. I'll retry automatically once the rate limit clears. -> Rate limit resets at: \`${reset_display}\`" - fi - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post user-visible ${reason} acknowledgment" - echo "$ack_body" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" - fi - ;; - on-mention) - local actor_mention="" - [ -n "${ACTOR:-}" ] && actor_mention="@${ACTOR} " - local reset_display="${reset_time:-unknown}" - local ack_body="> [!NOTE] -> ${actor_mention}I received your request but all AI engines are currently rate-limited. Please re-mention \`@dev-lead\` when the rate limit clears (estimated: \`${reset_display}\`) — I cannot reconstruct the original instruction automatically." - if [ "$DEV_LEAD_DRY_RUN" = "true" ]; then - echo "[dry-run] would post user-visible rate-limit acknowledgment" - echo "$ack_body" - else - gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$ack_body" - fi - ;; - esac - fi -} - -handle_rate_limit() { - local intent="$1" - echo "::warning::All engines rate-limited for intent=${intent} — posting rate-limited marker" - post_reviews_rate_limited "$intent" - [[ -n "${PR_NUMBER:-}" ]] && try_enable_auto_merge - exit 2 -} - case "$INTENT_TYPE" in fix-reviews) # Get open review threads diff --git a/scripts/engine.sh b/scripts/engine.sh index d9c474f80..be7bc9751 100644 --- a/scripts/engine.sh +++ b/scripts/engine.sh @@ -30,17 +30,6 @@ set -euo pipefail REVIEW_ENGINE="${REVIEW_ENGINE:-claude}" export REVIEW_ENGINE -# Cross-engine rubber-duck model (issue #773). The duck deliberately routes to -# Copilot/o4-mini even when the primary engine is NOT copilot (e.g. the default -# claude engine sets DUCK_ENGINE=copilot for adversarial diversity), so -# copilot_chat() may dereference COPILOT_API_MODEL regardless of REVIEW_ENGINE. -# Default it here — engine-agnostic — so the duck always has a model under -# `set -u`. The `copilot)` arm in set_engine_config re-applies the same default -# (idempotent) for the primary-engine path. -DEFAULT_COPILOT_API_MODEL="openai/o4-mini" -COPILOT_API_MODEL="${COPILOT_API_MODEL:-$DEFAULT_COPILOT_API_MODEL}" -export COPILOT_API_MODEL - # Per-tier timeouts (seconds). The job-level 60min cap is a backstop — without # per-tier timeouts a single hung model invocation burns the whole hour and # blocks every subsequent PR in the session. @@ -425,7 +414,6 @@ _emit_mcp_failure_warning() { fi } - # is_transient_failure # Returns 0 (true) for exit codes suggesting a flaky network/process state: # 124 (GNU timeout) and 137/143 (signal kills). JSON parse failures and @@ -442,7 +430,7 @@ is_transient_failure() { # Calls the GitHub Copilot CLI for completions and agentic actions. # # Supports tool usage via the --yolo flag (passed in extra_flags). -# Uses COPILOT_API_MODEL (default: openai/gpt-4o). +# Uses COPILOT_API_MODEL (default: openai/o4-mini). # # Rate-limit responses from the CLI are echoed to stdout/stderr so the caller's # is_rate_limited() check can detect them and exit 2 for engine fallback. @@ -841,9 +829,6 @@ _mcp_review_flags() { if [ -n "${REVIEW_MCP_CONFIG:-}" ] && [ -f "${REVIEW_MCP_CONFIG}" ] && [ -r "${REVIEW_MCP_CONFIG}" ]; then _MCP_FLAGS=(--mcp-config "$REVIEW_MCP_CONFIG" --strict-mcp-config) if [ -n "${REVIEW_MCP_DEBUG:-}" ]; then - # Surface the MCP server handshake in the CLI's stderr (captured + scanned - # by _emit_mcp_failure_warning). `--debug mcp` scopes debug to the MCP - # subsystem only, so the log stays readable. Diagnostics-only opt-in. _MCP_FLAGS+=(--debug mcp) fi if [ -n "${REVIEW_MCP_ALLOWED_TOOLS:-}" ]; then diff --git a/scripts/lib/lsp-verification.sh b/scripts/lib/lsp-verification.sh deleted file mode 100644 index bb7081898..000000000 --- a/scripts/lib/lsp-verification.sh +++ /dev/null @@ -1,141 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -# LSP finding-verification step — epic #839 (LSP pilot), story #843. -# Contract: docs/lsp-pilot.md §2 (find-references / diagnostics grounding). -# -# This is the one genuinely-new piece of the LSP pilot (docs/lsp-pilot.md §6): -# before a deep/audit finding that asserts a cross-file/semantic claim (e.g. -# "undefined", "breaks N callers", "unused symbol") is posted, the reviewer -# grounds it against the LSP navigation tools (find_references / get_diagnostics). -# Only the model can call the MCP `mcp__lsp__*` tools, so it annotates each such -# finding in its JSON output with an `lsp_verification` field ("verified" | -# "unverifiable"); this shell layer ENFORCES that annotation deterministically: -# -# - A finding LSP could not ground ("unverifiable") is DOWNGRADED one severity -# level and ANNOTATED `[lsp: unverifiable]` — never silently dropped and -# never posted as a confident finding (AC #2). -# - Each verified/unverifiable outcome is emitted to the Token Cost Observatory -# JSONL so the comparison harness can compute the false-positive-rate delta -# (AC #4) — via emit_verification_record in token-metrics.sh. -# - The whole step is INERT (findings file byte-for-byte unchanged, nothing -# emitted) when the LSP server is absent or degraded, so the review proceeds -# exactly as today (AC #3). This reuses the same MCP connect/degradation -# signal `_emit_mcp_failure_warning` keys on (scripts/engine.sh). -# -# Sourced by scripts/review-one-pr.sh after engine.sh and token-metrics.sh. - -# _lsp_failure_pattern -# The MCP connection/init-failure regex used to detect a degraded LSP server. -# Prefer engine.sh's single source of truth (_mcp_failure_pattern) when this lib -# is sourced alongside it; otherwise fall back to an equivalent local pattern so -# the lib stays usable (and unit-testable) on its own. -_lsp_failure_pattern() { - if declare -f _mcp_failure_pattern >/dev/null 2>&1; then - _mcp_failure_pattern - return - fi - local _pat - _pat='mcp server [^[:space:]]*[[:space:]]*("[^"]*"[[:space:]]*)?(failed|error)' - _pat="$_pat"'|failed to (connect|initialize|reconnect|start)[^.]*mcp' - _pat="$_pat"'|mcp[^.]*(connection|initializ)[^.]*(fail|error)' - _pat="$_pat"'|could not (connect to|start) mcp server' - printf '%s' "$_pat" -} - -# _lsp_downgrade_severity -# One step toward "info" on the info|minor|major|critical ladder. Bottoms out at -# info; unknown values collapse to info (safe — never escalates). -_lsp_downgrade_severity() { - case "$1" in - critical) printf 'major' ;; - major) printf 'minor' ;; - *) printf 'info' ;; - esac -} - -# lsp_verification_active [cli_output_file...] -# Returns 0 (active) iff the LSP navigation tools are wired AND not degraded: -# - REVIEW_MCP_CONFIG points at a readable file, -# - REVIEW_MCP_ALLOWED_TOOLS permits at least one mcp__lsp__ tool (so this is -# the LSP pilot, not e.g. a Context7-only run), -# - none of the given CLI-output files show an MCP connect/init failure. -# Returns 1 (inert) otherwise. The CLI-output scan is the degradation gate: a -# server that failed to connect means "skip verification, review as today". -lsp_verification_active() { - [ -n "${REVIEW_MCP_CONFIG:-}" ] || return 1 - [ -f "${REVIEW_MCP_CONFIG}" ] && [ -r "${REVIEW_MCP_CONFIG}" ] || return 1 - case "${REVIEW_MCP_ALLOWED_TOOLS:-}" in - *mcp__lsp__*) : ;; - *) return 1 ;; - esac - - if [ "$#" -gt 0 ]; then - local f present=() - for f in "$@"; do [ -f "$f" ] && present+=("$f"); done - if [ "${#present[@]}" -gt 0 ] \ - && grep -qiE "$(_lsp_failure_pattern)" "${present[@]}" 2>/dev/null; then - return 1 - fi - fi - return 0 -} - -# apply_lsp_verification [cli_output_file...] -# Enforces the LSP finding-verification contract on a review verdict file in -# place. No-op (file untouched, nothing emitted) when verification is inert — -# LSP unwired/degraded, jq missing, or the file is not a {findings:[...]} object. -apply_lsp_verification() { - local file="${1:-}" tier="${2:-deep}" - if [ "$#" -ge 2 ]; then shift 2; else shift "$#"; fi # remaining args: CLI output files - [ -f "$file" ] || return 0 - command -v jq >/dev/null 2>&1 || return 0 - jq -e 'type == "object" and (.findings | type == "array")' "$file" >/dev/null 2>&1 || return 0 - lsp_verification_active "$@" || return 0 - - local workflow="${TOKEN_WORKFLOW:-unknown}" context="${PR_URL:-}" - - # Emit one verification record per annotated finding (before rewriting, so the - # original severity is the recorded severity_before). - if declare -f emit_verification_record >/dev/null 2>&1; then - local idx outcome sev cat sev_after - while IFS=$'\t' read -r idx outcome sev cat; do - [ -n "$idx" ] || continue - if [ "$outcome" = "unverifiable" ]; then - sev_after="$(_lsp_downgrade_severity "$sev")" - else - sev_after="$sev" - fi - emit_verification_record "$workflow" "$tier" "$context" \ - "$idx" "$cat" "$sev" "$sev_after" "$outcome" || true - done < <(jq -r ' - .findings | to_entries[] - | (.value.lsp_verification // "") as $v - | select($v == "verified" or $v == "unverifiable") - | [ (.key | tostring), $v, (.value.severity // "info"), (.value.category // "") ] - | @tsv - ' "$file" 2>/dev/null) - fi - - # Downgrade + annotate every unverifiable finding (atomic rewrite). - local tmp; tmp="$(mktemp "${file}.lspv.XXXXXX")" || return 0 - if jq ' - def downgrade: - if . == "critical" then "major" - elif . == "major" then "minor" - else "info" end; - .findings |= map( - if (.lsp_verification // "") == "unverifiable" then - .severity = ((.severity // "info") | downgrade) - | .message = ((.message // "") - + (if (.message // "") | test("\\[lsp: unverifiable\\]") - then "" else " [lsp: unverifiable]" end)) - else . end - ) - ' "$file" > "$tmp" 2>/dev/null && [ -s "$tmp" ]; then - mv "$tmp" "$file" - else - rm -f "$tmp" - fi - return 0 -} diff --git a/scripts/lib/token-metrics.sh b/scripts/lib/token-metrics.sh index 97864d7df..ee62ec5ac 100644 --- a/scripts/lib/token-metrics.sh +++ b/scripts/lib/token-metrics.sh @@ -117,47 +117,49 @@ emit_token_record() { printf '%s\n' "$record" >> "$TOKEN_LOG_FILE" 2>/dev/null || true } -# emit_verification_record -# -# Appends one finding-verification record to TOKEN_LOG_FILE on the SAME JSONL -# channel as token records (story #843, epic #839). The record is discriminated -# by kind:"finding_verification" so the comparison harness (story 2) can compute -# the false-positive-rate delta and the cost report can skip it (it is not a -# priced token-usage call — see scripts/token_report.sh `annotate_records`). -# No-op when TOKEN_LOG_FILE is unset; swallows I/O errors so it never aborts a run. -emit_verification_record() { +# emit_lsp_coldstart_record +# [context] +# Appends one JSONL record describing an LSP cold-start measurement to +# TOKEN_LOG_FILE so the Token Cost Observatory (story #5's harness) can report +# P50/P95 cold-start against the SLA and the cache hit-rate. No-op when +# TOKEN_LOG_FILE is unset; swallows I/O errors so instrumentation never aborts a +# run. Records carry metric:"lsp_cold_start" so they are trivially separable from +# the per-call token records (which have no "metric" field). +# cold_start_ms — wall-clock ms for the LSP toolchain bring-up (install/restore). +# cache_status — "hit" | "miss" | "unknown" (the actions/cache outcome). +# skipped — "true" when the SLA gate (or a degraded toolchain) auto-skipped +# LSP for this run, else "false". +# sla_ms — the cold-start SLA budget in ms (for at-a-glance comparison). +emit_lsp_coldstart_record() { [ -n "${TOKEN_LOG_FILE:-}" ] || return 0 - local workflow="$1" tier="$2" context="$3" finding_index="$4" - local category="$5" severity_before="$6" severity_after="$7" outcome="$8" + local cold_start_ms="${1:-0}" cache_status="${2:-unknown}" + local skipped="${3:-false}" sla_ms="${4:-0}" context="${5:-}" - local ts run_id record + local ts run_id workflow record ts=$(date -u +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || echo "") run_id="${GITHUB_RUN_ID:-}" + workflow="${TOKEN_WORKFLOW:-pr-review}" record=$(jq -cn \ --arg ts "$ts" \ --arg workflow "$workflow" \ - --arg tier "$tier" \ - --arg context "$context" \ - --arg finding_index "$finding_index" \ - --arg category "$category" \ - --arg severity_before "$severity_before" \ - --arg severity_after "$severity_after" \ - --arg outcome "$outcome" \ + --arg cold_start_ms "$cold_start_ms" \ + --arg cache "$cache_status" \ + --arg skipped "$skipped" \ + --arg sla_ms "$sla_ms" \ --arg run_id "$run_id" \ + --arg context "$context" \ '{ - kind: "finding_verification", ts: $ts, workflow: $workflow, - tier: $tier, - context: $context, - finding_index: ($finding_index | tonumber? // $finding_index), - category: $category, - severity_before: $severity_before, - severity_after: $severity_after, - outcome: $outcome, - run_id: $run_id + metric: "lsp_cold_start", + cold_start_ms: ($cold_start_ms | tonumber? // 0), + cache: $cache, + skipped: ($skipped == "true"), + sla_ms: ($sla_ms | tonumber? // 0), + run_id: $run_id, + context: $context }' 2>/dev/null) || return 0 printf '%s\n' "$record" >> "$TOKEN_LOG_FILE" 2>/dev/null || true diff --git a/scripts/pr_review_health.sh b/scripts/pr_review_health.sh index f61114e02..bb9cf350b 100644 --- a/scripts/pr_review_health.sh +++ b/scripts/pr_review_health.sh @@ -77,22 +77,6 @@ echo " Success: $success_runs" echo " Failed: $failed_runs" echo " Cancelled: $cancelled_runs" -if [ "$total_runs" -gt 0 ]; then - failure_rate=$(echo "scale=1; $failed_runs * 100 / $total_runs" | bc) -else - failure_rate="0.0" -fi - -# Duration percentiles across all completed runs (computed but not surfaced in the -# LLM prompt — reserved for future structured-report use). -# shellcheck disable=SC2034 -read -r dur_min dur_p50 dur_p95 dur_max < <(echo "$runs_json" | jq -r ' - [.[] | select(.conclusion != null and .duration_s > 0) | .duration_s] | sort | - if length == 0 then "0 0 0 0" - else . as $d | ($d | length) as $n | - "\($d | min) \($d[$n * 50 / 100 | floor]) \($d[$n * 95 / 100 | floor]) \($d | max)" - end') - # --------------------------------------------------------------------------- # 3. Helpers # --------------------------------------------------------------------------- @@ -115,18 +99,6 @@ conclusion_icon() { esac } -# overall is the pre-computed status fed as a hint to the LLM prompt below. -# shellcheck disable=SC2034 -if [ "$failed_runs" -eq 0 ]; then - overall="HEALTHY" -elif [ "$(echo "$failure_rate > 50" | bc)" -eq 1 ]; then - overall="CRITICAL" -elif [ "$(echo "$failure_rate > 20" | bc)" -eq 1 ]; then - overall="DEGRADED" -else - overall="WARNING" -fi - # --------------------------------------------------------------------------- # 4. Build report # --------------------------------------------------------------------------- diff --git a/scripts/review-one-pr.sh b/scripts/review-one-pr.sh index acd113a34..e850f51bc 100755 --- a/scripts/review-one-pr.sh +++ b/scripts/review-one-pr.sh @@ -41,12 +41,6 @@ source "$SCRIPT_DIR/lib/review-registry.sh" # Gated default-off behind DOWNSTREAM_IMPACT_ENABLED (Story 5). # shellcheck source=lib/downstream-impact.sh source "$SCRIPT_DIR/lib/downstream-impact.sh" -# LSP finding-verification (epic #839, story #843): apply_lsp_verification grounds -# deep/audit cross-file findings against LSP nav tools, downgrading/annotating the -# ones LSP cannot ground and emitting each outcome to the token JSONL. Inert when -# the LSP MCP server is unwired or degraded (review unchanged). -# shellcheck source=lib/lsp-verification.sh -source "$SCRIPT_DIR/lib/lsp-verification.sh" PR_URL="${1:?usage: review-one-pr.sh }" export PR_URL @@ -245,24 +239,6 @@ fi fi } -# Skip when a human has requested changes, with two guards: -# 1. FORCE_REVIEW bypasses the skip — mention-triggered runs always proceed so -# authors can request a re-review after addressing feedback. -# 2. Only skip when a CHANGES_REQUESTED review targets the current head SHA. -# On repos that don't dismiss stale reviews, reviewDecision can stay -# CHANGES_REQUESTED after the author pushes new commits; in that case the -# review is stale and the cascade should re-engage with the updated code. -if [ "$REVIEW_DECISION" = "CHANGES_REQUESTED" ] && [ "${FORCE_REVIEW:-false}" != "true" ]; then - CHANGES_REQUESTED_AT_HEAD=$(echo "$PR_SNAPSHOT" | jq -r --arg sha "$PR_HEAD_SHA" ' - [.reviews[] | select(.state == "CHANGES_REQUESTED" and .commit.oid == $sha)] - | if length > 0 then "true" else "false" end - ') - if [ "$CHANGES_REQUESTED_AT_HEAD" = "true" ]; then - echo " skip: changes requested at current head — awaiting author response before reviewing" - echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"changes-requested\"}" - exit 100 - fi -fi # Skip when a human has requested changes, with two guards: # 1. FORCE_REVIEW bypasses the skip — mention-triggered runs always proceed so # authors can request a re-review after addressing feedback. @@ -871,13 +847,6 @@ fi # Wait for duck to finish (deep succeeded) [ -n "$DUCK_PID" ] && wait $DUCK_PID || true -# LSP finding-verification (story #843): ground the deep tier's cross-file -# findings before they are synthesized/posted. Inert (no change) unless the LSP -# MCP server is wired and connected; the deep CLI output is scanned so a degraded -# server skips verification. Operates in place on deep.json. -apply_lsp_verification "$OUTPUT_FILE" "deep" \ - /tmp/cascade/deep-stdout.txt /tmp/cascade/deep.log || true - DEEP_DECISION=$(jq -r '.decision' "$OUTPUT_FILE") DEEP_RISK=$(jq -r '.risk' "$OUTPUT_FILE") echo " [tier2] deep: decision=$DEEP_DECISION risk=$DEEP_RISK" @@ -1007,12 +976,6 @@ if [ ! -s "$OUTPUT_FILE" ] || ! jq empty "$OUTPUT_FILE" 2>/dev/null; then exit 1 fi -# LSP finding-verification (story #843): ground the audit tier's cross-file -# findings before the final verdict is posted. Inert unless the LSP MCP server is -# wired and connected (audit CLI output scanned for a degraded server). -apply_lsp_verification "$OUTPUT_FILE" "audit" \ - /tmp/cascade/audit-stdout.txt /tmp/cascade/audit.log || true - AUDIT_DECISION=$(jq -r '.decision' "$OUTPUT_FILE") AUDIT_RISK=$(jq -r '.risk' "$OUTPUT_FILE") echo " [tier3] decision=$AUDIT_DECISION risk=$AUDIT_RISK" diff --git a/scripts/setup-lsp-pilot.sh b/scripts/setup-lsp-pilot.sh index dfa7199c1..6c98334d4 100755 --- a/scripts/setup-lsp-pilot.sh +++ b/scripts/setup-lsp-pilot.sh @@ -62,11 +62,157 @@ AGENT_LSP_REPO="${AGENT_LSP_REPO:-blackwell-systems/agent-lsp}" # steps via $GITHUB_PATH. INSTALL_BIN="${INSTALL_BIN:-$HOME/.local/bin}" +# Cold-start SLA budget in milliseconds (docs/lsp-pilot.md §3: <=30s P95). A +# bring-up that blows this budget auto-skips LSP for the run (warn, don't fail). +# Env-overridable for a future SLA retune without editing this script. +LSP_COLD_START_SLA_MS="${LSP_COLD_START_SLA_MS:-30000}" +case "$LSP_COLD_START_SLA_MS" in + ''|*[!0-9]*) + echo "::warning::[lsp-pilot] invalid LSP_COLD_START_SLA_MS='${LSP_COLD_START_SLA_MS}', defaulting to 30000" >&2 + LSP_COLD_START_SLA_MS=30000 + ;; +esac + +# Token Cost Observatory instrumentation (best-effort). Provides +# emit_lsp_coldstart_record; absent → _emit_coldstart is a no-op so the pilot +# stays self-contained when deployed without scripts/lib. +_LSP_TOKEN_LIB="$(dirname "${BASH_SOURCE[0]}")/lib/token-metrics.sh" +if [ -f "$_LSP_TOKEN_LIB" ]; then + # shellcheck source=lib/token-metrics.sh + source "$_LSP_TOKEN_LIB" +fi + warn() { echo "::warning::[lsp-pilot] $*" >&2; } note() { echo "[lsp-pilot] $*"; } +# _lsp_now_ms — wall-clock milliseconds. Uses GNU `date +%s%N` (nanoseconds) +# where available; falls back to whole-second resolution (×1000) on platforms +# (e.g. BSD/macOS) where %N is a literal. +_lsp_now_ms() { + local ns + ns="$(date +%s%N 2>/dev/null || echo "")" + case "$ns" in + ''|*[!0-9]*) printf '%s' "$(( $(date +%s 2>/dev/null || echo 0) * 1000 ))" ;; + *) printf '%s' "$(( ns / 1000000 ))" ;; + esac +} + +# _lsp_sla_exceeded +# Returns 0 (true) when the measured cold-start blew the SLA budget. Pure integer +# comparison so the AC #3 auto-skip decision is unit-testable in isolation. +# Non-numeric input returns 1 (not exceeded) — a measurement glitch must not +# trip the auto-skip on its own. +_lsp_sla_exceeded() { + local elapsed="${1:-}" sla="${2:-}" + case "$elapsed" in ''|*[!0-9]*) return 1 ;; esac + case "$sla" in ''|*[!0-9]*) return 1 ;; esac + [ "$elapsed" -gt "$sla" ] +} + +# _lsp_cache_status — normalize the actions/cache outcome the workflow threads in +# via LSP_INDEX_CACHE_HIT ('true'|'false'|unset) into hit|miss|unknown for the +# cold-start record (AC #2). +_lsp_cache_status() { + case "${LSP_INDEX_CACHE_HIT:-}" in + true|True|TRUE) printf 'hit' ;; + false|False|FALSE) printf 'miss' ;; + *) printf 'unknown' ;; + esac +} + +# _emit_coldstart +# Best-effort emit of the cold-start record to the Token Cost Observatory JSONL. +# No-op when the token lib / TOKEN_LOG_FILE is unavailable — instrumentation must +# never fail the workflow. +_emit_coldstart() { + declare -f emit_lsp_coldstart_record >/dev/null 2>&1 || return 0 + emit_lsp_coldstart_record "$1" "$2" "$3" "$LSP_COLD_START_SLA_MS" \ + "lsp-pilot:${LSP_CANDIDATE}" || true +} + print_allowed_tools() { printf '%s\n' "$LSP_PILOT_ALLOWED_TOOLS"; } +# _lsp_probe_server_ms — start the MCP server, send a JSON-RPC 2.0 initialize +# request, and return the wall-clock milliseconds to first response. Returns +# $(( LSP_COLD_START_SLA_MS + 1 )) on timeout or launch failure so the SLA gate +# fires on an unresponsive server. Requires `timeout`; falls back to the same +# sentinel when it is absent so the gate still fires rather than silently passing. +# +# Uses named pipes + Content-Length framing instead of grep so the probe works +# even when the server sends a valid JSON-RPC response without a trailing newline +# (LSP frames by byte count, not line endings; grep -m1 blocks until \n arrives). +_lsp_probe_server_ms() { + command -v timeout >/dev/null 2>&1 || { echo $(( LSP_COLD_START_SLA_MS + 1 )); return 0; } + local timeout_s=$(( (LSP_COLD_START_SLA_MS + 999) / 1000 + 1 )) + [ "$timeout_s" -lt 1 ] && timeout_s=1 + + local req='{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2024-11-05","capabilities":{},"clientInfo":{"name":"sla-probe","version":"0"}}}' + local req_len=${#req} + + local t0; t0="$(_lsp_now_ms)" + + # Named pipes let us read the response byte-by-byte (Content-Length framing) + # and kill the server as soon as the response is detected, without waiting for + # the server to exit on its own or for timeout to fire. + local tmpdir; tmpdir="$(mktemp -d)" + local in_fifo="$tmpdir/in" out_fifo="$tmpdir/out" + if ! mkfifo "$in_fifo" "$out_fifo" 2>/dev/null; then + rm -rf "$tmpdir" + echo $(( LSP_COLD_START_SLA_MS + 1 )) + return 0 + fi + + # Writer feeds the JSON-RPC request; background so FIFO open() doesn't + # deadlock before the server opens the read end. + printf 'Content-Length: %d\r\n\r\n%s' "$req_len" "$req" > "$in_fifo" & + local writer_pid=$! + + # Server reads from in_fifo, writes response to out_fifo. + timeout "$timeout_s" agent-lsp "bash:bash-language-server,start" 2>/dev/null \ + < "$in_fifo" > "$out_fifo" & + local server_pid=$! + + # Parse Content-Length header, then read exactly that many bytes (no newline + # dependency). read -N reads a fixed character count regardless of line endings. + local found=0 line content_length body + content_length=0 + { + while IFS= read -r line; do + line="${line%$'\r'}" + [ -z "$line" ] && break + case "$line" in + Content-Length:*|content-length:*) + content_length="${line#*: }" + content_length="${content_length//[[:space:]]/}" + ;; + esac + done + if [ "$content_length" -gt 0 ] 2>/dev/null; then + read -r -N "$content_length" body 2>/dev/null || true + case "$body" in *'"jsonrpc"'*) found=1 ;; esac + fi + } < "$out_fifo" + + kill "$server_pid" "$writer_pid" 2>/dev/null || true + wait "$server_pid" "$writer_pid" 2>/dev/null || true + rm -rf "$tmpdir" + + if [ "$found" -eq 1 ]; then + echo $(( $(_lsp_now_ms) - t0 )) + else + echo $(( LSP_COLD_START_SLA_MS + 1 )) + fi +} + +# probe_candidate — dispatch to the selected candidate's startup probe and +# return wall-clock ms to first JSON-RPC response. +probe_candidate() { + case "$LSP_CANDIDATE" in + agent-lsp) _lsp_probe_server_ms ;; + *) echo $(( LSP_COLD_START_SLA_MS + 1 )) ;; + esac +} + # install_bash_language_server — npm-global install of the pinned server. # Returns 0 when the pinned binary is available (cache hit or fresh install). install_bash_language_server() { @@ -178,15 +324,47 @@ main() { fi export PATH="$INSTALL_BIN:$PATH" + # ── Cold-start budget (AC #2/#3) ─────────────────────────────────────────── + # Two-phase bring-up measurement: + # 1. Install phase — binary availability (fast on cache hit, slow on miss). + # 2. Probe phase — actual MCP server startup including index load; this is + # what can be slow even when binaries are cached (cold or stale index). + # Summing both phases gives total bring-up time; the SLA gate fires if either + # phase is expensive. "warn, don't fail" degradation contract is preserved. + local install_start_ms install_end_ms install_ms probe_ms cold_start_ms cache_status + install_start_ms="$(_lsp_now_ms)" + local server_ok=1 bls_ok=1 install_candidate || server_ok=0 install_bash_language_server || bls_ok=0 + install_end_ms="$(_lsp_now_ms)" + install_ms=$(( install_end_ms - install_start_ms )) + [ "$install_ms" -lt 0 ] && install_ms=0 + cache_status="$(_lsp_cache_status)" + if [ "$server_ok" -ne 1 ] || [ "$bls_ok" -ne 1 ]; then + _emit_coldstart "$install_ms" "$cache_status" "true" warn "LSP toolchain unavailable (candidate=${LSP_CANDIDATE}) — review continues on base capabilities, LSP enrichment skipped" return 0 fi + # Probe actual server startup (index load). Binary presence (above) is fast + # for a cache hit; the expensive first index build happens here. Total is + # install + probe so the SLA gate captures full bring-up time. + probe_ms="$(probe_candidate)" + cold_start_ms=$(( install_ms + probe_ms )) + [ "$cold_start_ms" -lt 0 ] && cold_start_ms=0 + + # ── SLA gate (AC #3) ─────────────────────────────────────────────────────── + if _lsp_sla_exceeded "$cold_start_ms" "$LSP_COLD_START_SLA_MS"; then + _emit_coldstart "$cold_start_ms" "$cache_status" "true" + warn "LSP cold-start ${cold_start_ms}ms exceeded the ${LSP_COLD_START_SLA_MS}ms P95 SLA — auto-skipping LSP for this run; review continues on base capabilities" + return 0 + fi + + _emit_coldstart "$cold_start_ms" "$cache_status" "false" + # 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 @@ -196,14 +374,18 @@ main() { echo "REVIEW_MCP_CONFIG=${LSP_PILOT_CONFIG}" echo "REVIEW_MCP_ALLOWED_TOOLS=${LSP_PILOT_ALLOWED_TOOLS}" } >> "$GITHUB_ENV" - note "LSP pilot enabled: REVIEW_MCP_CONFIG=${LSP_PILOT_CONFIG}" + note "LSP pilot enabled: REVIEW_MCP_CONFIG=${LSP_PILOT_CONFIG} (cold-start ${cold_start_ms}ms, cache ${cache_status})" else note "GITHUB_ENV unset — install complete; export REVIEW_MCP_CONFIG=${LSP_PILOT_CONFIG} to enable" fi } -case "${1:-}" in - print-allowed-tools) print_allowed_tools ;; - ""|setup) main ;; - *) echo "usage: $(basename "$0") [print-allowed-tools|setup]" >&2; exit 2 ;; -esac +# Only dispatch when executed (not when sourced for unit tests). Sourcing exposes +# the helpers above without running the installer. +if [ "${BASH_SOURCE[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/scripts/token_report.sh b/scripts/token_report.sh index 99a06624f..7b363cdd6 100755 --- a/scripts/token_report.sh +++ b/scripts/token_report.sh @@ -76,9 +76,7 @@ annotate_records() { local dir="$1" local files=("$dir"/*.jsonl) [ -e "${files[0]}" ] || return 0 # no JSONL files → no rows - jq -r 'select(type == "object") - | select((.kind // "token_usage") == "token_usage") - | [ + jq -r 'select(type == "object") | select(.metric == null) | [ (.repo // "unknown"), (.workflow // "unknown"), (.tier // "-"), (.model // "-"), (.input_tokens // 0), (.cache_read_tokens // 0), (.output_tokens // 0), (.ts // "-"), (.context // ""), (.cache_creation_tokens // 0) diff --git a/tests/dev-lead/unit/test_engine_duck_copilot_model.bats b/tests/dev-lead/unit/test_engine_duck_copilot_model.bats deleted file mode 100644 index d0a6ecf31..000000000 --- a/tests/dev-lead/unit/test_engine_duck_copilot_model.bats +++ /dev/null @@ -1,103 +0,0 @@ -#!/usr/bin/env bats -# Unit tests for engine.sh — COPILOT_API_MODEL must be defaulted independent of -# the selected REVIEW_ENGINE (issue #773). -# -# Under the default `claude` engine the rubber-duck tier deliberately routes to -# Copilot (DUCK_ENGINE=copilot → run_duck → copilot_chat), which dereferences -# $COPILOT_API_MODEL. Before the fix that variable was only assigned inside the -# `copilot)` arm of set_engine_config, so under `set -euo pipefail` the duck -# subshell aborted with "COPILOT_API_MODEL: unbound variable" and the review -# silently fell through to deep-review-only. - -SCRIPT_DIR="$(cd "$(dirname "$BATS_TEST_FILENAME")/../../.." && pwd)" -ENGINE_SCRIPT="$SCRIPT_DIR/scripts/engine.sh" - -setup() { - export GITHUB_ENV="$(mktemp)" - export GITHUB_OUTPUT="$(mktemp)" - - # A `gh` stub that records the --model value copilot_chat passes through, so - # we can assert the duck path actually reaches the API with a defaulted model - # instead of aborting on an unbound variable. - STUB_BIN_DIR="$(mktemp -d)" - GH_MODEL_RECORD="$(mktemp)" - export GH_MODEL_RECORD - cat > "$STUB_BIN_DIR/gh" <<'STUB' -#!/usr/bin/env bash -# Minimal `gh copilot` stub: record the --model arg, emit a benign reply. -prev="" -for arg in "$@"; do - if [ "$prev" = "--model" ]; then - printf '%s\n' "$arg" >> "$GH_MODEL_RECORD" - fi - prev="$arg" -done -echo "stub copilot reply" -STUB - chmod +x "$STUB_BIN_DIR/gh" - export PATH="$STUB_BIN_DIR:$PATH" - export STUB_BIN_DIR - - TEST_PROMPT="$(mktemp)" - echo "duck prompt content" > "$TEST_PROMPT" - export TEST_PROMPT - - # copilot_chat requires a token; provide a fake one (the stub ignores it). - export GH_TOKEN="fake-token" - - # Ensure a clean slate so engine.sh evaluates its own default. - unset COPILOT_API_MODEL -} - -teardown() { - rm -f "${GITHUB_ENV:-}" "${GITHUB_OUTPUT:-}" "${TEST_PROMPT:-}" "${GH_MODEL_RECORD:-}" - rm -rf "${STUB_BIN_DIR:-}" - unset COPILOT_API_MODEL -} - -_source_engine() { - local engine="${1:-claude}" - export REVIEW_ENGINE="$engine" - source "$ENGINE_SCRIPT" -} - -# ── COPILOT_API_MODEL is engine-agnostic ────────────────────────────────────── - -@test "duck-model: COPILOT_API_MODEL is defaulted under the claude engine" { - _source_engine "claude" - # Set even though the copilot) arm never ran. - [ -n "${COPILOT_API_MODEL+x}" ] - [ "$COPILOT_API_MODEL" = "openai/o4-mini" ] -} - -@test "duck-model: COPILOT_API_MODEL is exported under the claude engine" { - _source_engine "claude" - # Exported so the cross-engine duck subshell (copilot_chat) inherits it. - run bash -c 'echo "${COPILOT_API_MODEL:-MISSING}"' - [ "$status" -eq 0 ] - [ "$output" = "openai/o4-mini" ] -} - -@test "duck-model: copilot_chat duck path does not abort with unbound variable on claude engine" { - _source_engine "claude" - # This is the exact failure from #773: under set -u, copilot_chat dereferenced - # an unset COPILOT_API_MODEL and aborted before producing JSON. - run copilot_chat "$TEST_PROMPT" 30 - [ "$status" -eq 0 ] - [[ "$output" != *"unbound variable"* ]] - # The default model reached the gh invocation. - grep -q "openai/o4-mini" "$GH_MODEL_RECORD" -} - -@test "duck-model: a pre-set COPILOT_API_MODEL override is honored on the claude engine" { - export COPILOT_API_MODEL="openai/gpt-5-mini" - _source_engine "claude" - [ "$COPILOT_API_MODEL" = "openai/gpt-5-mini" ] -} - -# ── Regression guard: copilot engine still defaults the model ───────────────── - -@test "duck-model: copilot engine still defaults COPILOT_API_MODEL" { - _source_engine "copilot" - [ "$COPILOT_API_MODEL" = "openai/o4-mini" ] -} 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..f6f303552 --- /dev/null +++ b/tests/dev-lead/unit/test_lsp_coldstart_sla.bats @@ -0,0 +1,247 @@ +#!/usr/bin/env bats +# Unit tests for the LSP cold-start SLA + index-cache instrumentation +# (epic #839, story #846). TDD — written before implementation. +# +# This story layers three things on top of the #842 wiring: +# AC #1 — actions/cache for the LSP index in pr-review.yml (asserted by the +# workflow grep tests below; the cache *outcome* is threaded into the +# setup script as LSP_INDEX_CACHE_HIT). +# AC #2 — cold-start time + cache hit/miss recorded to the Token Cost +# Observatory JSONL via emit_lsp_coldstart_record. +# AC #3 — cold-start over the 30s P95 SLA auto-skips LSP (warn, don't fail): +# the review proceeds without LSP and the workflow never fails. + +SCRIPT_DIR="$(cd "$(dirname "$BATS_TEST_FILENAME")/../../.." && pwd)" +TOKEN_LIB="$SCRIPT_DIR/scripts/lib/token-metrics.sh" +SETUP_SCRIPT="$SCRIPT_DIR/scripts/setup-lsp-pilot.sh" +WORKFLOW="$SCRIPT_DIR/.github/workflows/pr-review.yml" + +setup() { + TOKEN_LOG_FILE="$(mktemp)" + export TOKEN_LOG_FILE + unset LSP_INDEX_CACHE_HIT LSP_COLD_START_SLA_MS GITHUB_RUN_ID +} + +teardown() { + [ -n "${TOKEN_LOG_FILE:-}" ] && rm -f "$TOKEN_LOG_FILE" 2>/dev/null || true + [ -n "${GITHUB_ENV:-}" ] && rm -f "$GITHUB_ENV" 2>/dev/null || true + [ -n "${SANDBOX_BIN:-}" ] && rm -rf "$SANDBOX_BIN" 2>/dev/null || true + if [ -n "${INSTALL_BIN:-}" ]; then + local _parent + _parent="$(dirname "$INSTALL_BIN")" + # Sourcing setup-lsp-pilot.sh sets INSTALL_BIN=$HOME/.local/bin as a side + # effect; only remove the parent when it is a mktemp'd tmp dir, never a + # system path like $HOME/.local (which would destroy the bats installation). + case "$_parent" in /tmp/*) rm -rf "$_parent" 2>/dev/null || true ;; esac + fi + unset TOKEN_LOG_FILE LSP_INDEX_CACHE_HIT LSP_COLD_START_SLA_MS GITHUB_ENV SANDBOX_BIN INSTALL_BIN +} + +# Hermetic setup sandbox mirroring test_lsp_pilot.bats: isolated PATH bin dir, +# fresh GITHUB_ENV, INSTALL_BIN pointed at a temp dir so no real install lands. +_make_setup_env() { + export SANDBOX_BIN="$(mktemp -d)" + export GITHUB_ENV="$(mktemp)" + export INSTALL_BIN="$(mktemp -d)/bin" + # Hermetic PATH: the sandbox + base system dirs only, deliberately excluding + # ~/.local/bin so a pre-installed agent-lsp/bash-language-server on the host + # cannot leak into the "uninstallable"/"present" scenarios (these tests must be + # deterministic regardless of what the runner already has on PATH). + export PATH="$SANDBOX_BIN:/usr/local/bin:/usr/bin:/bin" +} + +_present_tools() { + # Stub agent-lsp: handle --version for install checks and emit an instant + # JSON-RPC initialize response for _lsp_probe_server_ms so warm-run tests + # are not gated by a real server startup or network. + cat > "$SANDBOX_BIN/agent-lsp" <<'STUB' +#!/usr/bin/env bash +if [[ "${1:-}" == "--version" ]]; then + echo "agent-lsp 0.15.0" + exit 0 +fi +# MCP startup probe: emit a minimal initialize response then drain stdin. +resp='{"jsonrpc":"2.0","id":1,"result":{"protocolVersion":"2024-11-05","capabilities":{}}}' +printf 'Content-Length: %d\r\n\r\n%s' "${#resp}" "$resp" +cat >/dev/null +STUB + chmod +x "$SANDBOX_BIN/agent-lsp" + printf '#!/usr/bin/env bash\necho 5.6.0\n' > "$SANDBOX_BIN/bash-language-server" + chmod +x "$SANDBOX_BIN/bash-language-server" +} + +# ── AC #2: emit_lsp_coldstart_record JSONL contract ────────────────────────── + +@test "coldstart record: emits a well-formed lsp_cold_start JSONL record" { + source "$TOKEN_LIB" + emit_lsp_coldstart_record 1234 hit false 30000 "lsp-pilot:agent-lsp" + [ -s "$TOKEN_LOG_FILE" ] + run jq -r '.metric' "$TOKEN_LOG_FILE" + [ "$output" = "lsp_cold_start" ] + run jq -r '.cold_start_ms' "$TOKEN_LOG_FILE" + [ "$output" = "1234" ] + run jq -r '.cache' "$TOKEN_LOG_FILE" + [ "$output" = "hit" ] + run jq -r '.skipped' "$TOKEN_LOG_FILE" + [ "$output" = "false" ] + run jq -r '.sla_ms' "$TOKEN_LOG_FILE" + [ "$output" = "30000" ] +} + +@test "coldstart record: skipped flag is encoded as a JSON boolean true" { + source "$TOKEN_LIB" + emit_lsp_coldstart_record 45000 miss true 30000 "lsp-pilot:agent-lsp" + run jq -e '.skipped == true' "$TOKEN_LOG_FILE" + [ "$status" -eq 0 ] +} + +@test "coldstart record: no-op when TOKEN_LOG_FILE is unset" { + source "$TOKEN_LIB" + unset TOKEN_LOG_FILE + run emit_lsp_coldstart_record 1234 hit false 30000 "ctx" + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +# ── AC #3: the SLA auto-skip decision (pure function) ──────────────────────── + +@test "sla decision: cold-start over budget → exceeded (true)" { + # shellcheck source=/dev/null + source "$SETUP_SCRIPT" >/dev/null 2>&1 || true + run _lsp_sla_exceeded 30001 30000 + [ "$status" -eq 0 ] +} + +@test "sla decision: cold-start at budget → not exceeded (false)" { + source "$SETUP_SCRIPT" >/dev/null 2>&1 || true + run _lsp_sla_exceeded 30000 30000 + [ "$status" -ne 0 ] +} + +@test "sla decision: cold-start under budget → not exceeded (false)" { + source "$SETUP_SCRIPT" >/dev/null 2>&1 || true + run _lsp_sla_exceeded 12000 30000 + [ "$status" -ne 0 ] +} + +@test "sla decision: non-numeric input is treated as not-exceeded (safe default)" { + source "$SETUP_SCRIPT" >/dev/null 2>&1 || true + run _lsp_sla_exceeded "garbage" 30000 + [ "$status" -ne 0 ] +} + +# ── AC #2: cache hit/miss normalization ────────────────────────────────────── + +@test "cache status: LSP_INDEX_CACHE_HIT=true → hit" { + source "$SETUP_SCRIPT" >/dev/null 2>&1 || true + export LSP_INDEX_CACHE_HIT=true + run _lsp_cache_status + [ "$output" = "hit" ] +} + +@test "cache status: LSP_INDEX_CACHE_HIT=false → miss" { + source "$SETUP_SCRIPT" >/dev/null 2>&1 || true + export LSP_INDEX_CACHE_HIT=false + run _lsp_cache_status + [ "$output" = "miss" ] +} + +@test "cache status: LSP_INDEX_CACHE_HIT unset → unknown" { + source "$SETUP_SCRIPT" >/dev/null 2>&1 || true + unset LSP_INDEX_CACHE_HIT + run _lsp_cache_status + [ "$output" = "unknown" ] +} + +# ── AC #2/#3: end-to-end through setup-lsp-pilot.sh main() ──────────────────── + +@test "setup: under-SLA warm run → wires knobs + records skipped:false + cache hit" { + _make_setup_env + _present_tools + export LSP_INDEX_CACHE_HIT=true + run bash "$SETUP_SCRIPT" + [ "$status" -eq 0 ] + # Knobs wired (under SLA, tools present). + grep -q "^REVIEW_MCP_CONFIG=.github/mcp/lsp.json$" "$GITHUB_ENV" + # Cold-start record present, not skipped, cache hit. + run jq -e 'select(.metric=="lsp_cold_start") | .skipped == false and .cache == "hit"' "$TOKEN_LOG_FILE" + [ "$status" -eq 0 ] +} + +@test "setup: cold-start over SLA → auto-skip (warn, no knobs, exit 0, skipped:true)" { + _make_setup_env + # Tools present but the --version smoke spends real wall time; SLA of 0ms makes + # any positive cold-start exceed the budget deterministically. + printf '#!/usr/bin/env bash\nsleep 0.05\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" + export LSP_COLD_START_SLA_MS=0 + export LSP_INDEX_CACHE_HIT=false + run bash "$SETUP_SCRIPT" + # AC #3: never fails the workflow. + [ "$status" -eq 0 ] + [[ "$output" == *"::warning::"* ]] + [[ "$output" == *"SLA"* ]] + # No knobs wired — review proceeds without LSP. + ! grep -q "^REVIEW_MCP_CONFIG=" "$GITHUB_ENV" + # Cold-start record marks the run skipped. + run jq -e 'select(.metric=="lsp_cold_start") | .skipped == true' "$TOKEN_LOG_FILE" + [ "$status" -eq 0 ] +} + +@test "setup: toolchain uninstallable → records skipped:true and exits 0" { + _make_setup_env + 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 + export LSP_INDEX_CACHE_HIT=false + run bash "$SETUP_SCRIPT" + [ "$status" -eq 0 ] + ! grep -q "^REVIEW_MCP_CONFIG=" "$GITHUB_ENV" + run jq -e 'select(.metric=="lsp_cold_start") | .skipped == true and .cache == "miss"' "$TOKEN_LOG_FILE" + [ "$status" -eq 0 ] +} + +# ── AC #1: actions/cache for the LSP index is wired into pr-review.yml ──────── + +@test "workflow: an actions/cache step caches the LSP index (SHA-pinned)" { + grep -q "Cache LSP index" "$WORKFLOW" + # SHA-pinned per the org standard (no bare @v tag), scoped to the LSP cache step. + run awk ' + /- name: Cache LSP index/ { in_step=1 } + in_step { print } + in_step && /^[[:space:]]*- name:/ && !/- name: Cache LSP index/ { exit } + ' "$WORKFLOW" + [ "$status" -eq 0 ] + [[ "$output" =~ uses:\ actions/cache@[0-9a-f]{40} ]] +} + +@test "workflow: LSP index cache key is keyed on OS + server version + source hash" { + run grep -E "key: lsp-index-.*runner\.os.*AGENT_LSP_VERSION.*BASH_LANGUAGE_SERVER_VERSION.*hashFiles" "$WORKFLOW" + [ "$status" -eq 0 ] +} + +@test "workflow: the cache-hit outcome is threaded into the setup step" { + grep -q "LSP_INDEX_CACHE_HIT:" "$WORKFLOW" +} + +@test "workflow: LSP index cache is opt-in behind LSP_PILOT_ENABLED" { + # The Cache LSP index step must be gated. + run awk ' + /- name: Cache LSP index/ { in_step=1 } + in_step { print } + in_step && /^[[:space:]]*- name:/ && !/- name: Cache LSP index/ { exit } + ' "$WORKFLOW" + [ "$status" -eq 0 ] + echo "$output" | grep -qE '^[[:space:]]*if:[[:space:]].*LSP_PILOT_ENABLED' + + # The Set up LSP pilot servers step must also be gated. + run awk ' + /- name: Set up LSP pilot servers/ { in_step=1 } + in_step { print } + in_step && /^[[:space:]]*- name:/ && !/- name: Set up LSP pilot servers/ { exit } + ' "$WORKFLOW" + [ "$status" -eq 0 ] + echo "$output" | grep -qE '^[[:space:]]*if:[[:space:]].*LSP_PILOT_ENABLED' +} diff --git a/tests/dev-lead/unit/test_lsp_pilot.bats b/tests/dev-lead/unit/test_lsp_pilot.bats index 286dbe050..98c29bc74 100644 --- a/tests/dev-lead/unit/test_lsp_pilot.bats +++ b/tests/dev-lead/unit/test_lsp_pilot.bats @@ -172,8 +172,18 @@ _make_setup_env() { @test "setup: tools present → threads REVIEW_MCP_CONFIG + navigation knob into GITHUB_ENV" { _make_setup_env - # Simulate the pinned tools already being installed (cache hit) so no network. - printf '#!/usr/bin/env bash\necho agent-lsp 0.15.0\n' > "$SANDBOX_BIN/agent-lsp" + # Stub agent-lsp: --version for the install check; JSON-RPC response for + # _lsp_probe_server_ms so the cold-start probe succeeds without a real server. + cat > "$SANDBOX_BIN/agent-lsp" <<'STUB' +#!/usr/bin/env bash +if [[ "${1:-}" == "--version" ]]; then + echo "agent-lsp 0.15.0" + exit 0 +fi +resp='{"jsonrpc":"2.0","id":1,"result":{"protocolVersion":"2024-11-05","capabilities":{}}}' +printf 'Content-Length: %d\r\n\r\n%s' "${#resp}" "$resp" +cat >/dev/null +STUB 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" run bash "$SETUP_SCRIPT" diff --git a/tests/dev-lead/unit/test_lsp_verification.bats b/tests/dev-lead/unit/test_lsp_verification.bats deleted file mode 100644 index a95e9f900..000000000 --- a/tests/dev-lead/unit/test_lsp_verification.bats +++ /dev/null @@ -1,223 +0,0 @@ -#!/usr/bin/env bats -# Unit tests for the LSP finding-verification step (epic #839, story #843). -# -# The step grounds deep/audit cross-file/semantic findings against LSP nav tools -# before posting. The model annotates each such finding with an `lsp_verification` -# field (`verified`|`unverifiable`); this shell layer ENFORCES the annotation: -# - inert when LSP is not wired or is degraded (review byte-for-byte unchanged) — AC #3 -# - `unverifiable` → downgrade severity one step + annotate (never dropped) — AC #2 -# - each verified/unverifiable outcome → one JSONL record on TOKEN_LOG_FILE — AC #4 -# It reuses the MCP gating/degradation signal already shipped (#677/#678). - -SCRIPT_DIR="$(cd "$(dirname "$BATS_TEST_FILENAME")/../../.." && pwd)" -ENGINE_SCRIPT="$SCRIPT_DIR/scripts/engine.sh" -LSP_LIB="$SCRIPT_DIR/scripts/lib/lsp-verification.sh" -TOKEN_LIB="$SCRIPT_DIR/scripts/lib/token-metrics.sh" -LSP_ALLOWED="mcp__lsp__find_references,mcp__lsp__get_diagnostics" - -setup() { - unset REVIEW_MCP_CONFIG REVIEW_MCP_ALLOWED_TOOLS - # The degradation scan reuses engine.sh's _mcp_failure_pattern when present. - # shellcheck source=/dev/null - source "$ENGINE_SCRIPT" >/dev/null 2>&1 || true - # shellcheck source=/dev/null - source "$TOKEN_LIB" - # shellcheck source=/dev/null - source "$LSP_LIB" - - WORKDIR="$(mktemp -d)" - export TOKEN_LOG_FILE="$WORKDIR/tokens.jsonl" - export TOKEN_WORKFLOW="pr-review" - export PR_URL="https://github.com/petry-projects/x/pull/7" - - MCP_CONFIG_FILE="$WORKDIR/lsp.json" - echo '{"mcpServers":{"lsp":{"type":"stdio","command":"agent-lsp"}}}' > "$MCP_CONFIG_FILE" - - FINDINGS="$WORKDIR/deep.json" -} - -teardown() { - rm -rf "$WORKDIR" - unset REVIEW_MCP_CONFIG REVIEW_MCP_ALLOWED_TOOLS TOKEN_LOG_FILE TOKEN_WORKFLOW PR_URL -} - -_write_findings() { - cat > "$FINDINGS" -} - -_activate_lsp() { - export REVIEW_MCP_CONFIG="$MCP_CONFIG_FILE" - export REVIEW_MCP_ALLOWED_TOOLS="$LSP_ALLOWED" -} - -# ── AC #3: inert when LSP is not wired ──────────────────────────────────────── - -@test "inert: no MCP knob → findings file byte-for-byte unchanged, no JSONL" { - _write_findings <<'JSON' -{"tier":"deep","findings":[{"severity":"critical","category":"correctness","message":"foo() is undefined","lsp_verification":"unverifiable"}]} -JSON - local before; before="$(cat "$FINDINGS")" - run apply_lsp_verification "$FINDINGS" "deep" - [ "$status" -eq 0 ] - [ "$(cat "$FINDINGS")" = "$before" ] - [ ! -s "$TOKEN_LOG_FILE" ] -} - -@test "inert: MCP wired but no lsp tools (e.g. context7) → unchanged, no JSONL" { - export REVIEW_MCP_CONFIG="$MCP_CONFIG_FILE" - export REVIEW_MCP_ALLOWED_TOOLS="mcp__context7__*" - _write_findings <<'JSON' -{"tier":"deep","findings":[{"severity":"critical","category":"correctness","message":"foo() is undefined","lsp_verification":"unverifiable"}]} -JSON - local before; before="$(cat "$FINDINGS")" - run apply_lsp_verification "$FINDINGS" "deep" - [ "$status" -eq 0 ] - [ "$(cat "$FINDINGS")" = "$before" ] - [ ! -s "$TOKEN_LOG_FILE" ] -} - -@test "inert: lsp wired but CLI output shows MCP failure (degraded) → unchanged" { - _activate_lsp - local cli="$WORKDIR/deep.log" - printf 'MCP server "lsp" failed to connect after 3 attempts\n' > "$cli" - _write_findings <<'JSON' -{"tier":"deep","findings":[{"severity":"critical","category":"correctness","message":"foo() is undefined","lsp_verification":"unverifiable"}]} -JSON - local before; before="$(cat "$FINDINGS")" - run apply_lsp_verification "$FINDINGS" "deep" "$cli" - [ "$status" -eq 0 ] - [ "$(cat "$FINDINGS")" = "$before" ] - [ ! -s "$TOKEN_LOG_FILE" ] -} - -# ── AC #2: downgrade + annotate the unverifiable finding (never drop) ────────── - -@test "downgrade: unverifiable finding is downgraded one level and annotated" { - _activate_lsp - _write_findings <<'JSON' -{"tier":"deep","findings":[{"severity":"critical","category":"correctness","message":"foo() is undefined","lsp_verification":"unverifiable"}]} -JSON - run apply_lsp_verification "$FINDINGS" "deep" - [ "$status" -eq 0 ] - # Finding is NOT dropped. - [ "$(jq '.findings | length' "$FINDINGS")" = "1" ] - # critical → major. - [ "$(jq -r '.findings[0].severity' "$FINDINGS")" = "major" ] - # Annotated so the downgrade is visible/auditable in the posted body. - run jq -r '.findings[0].message' "$FINDINGS" - [[ "$output" == *"[lsp: unverifiable]"* ]] -} - -@test "downgrade: ladder bottoms out at info (minor → info, info → info)" { - _activate_lsp - _write_findings <<'JSON' -{"tier":"audit","findings":[ - {"severity":"minor","category":"x","message":"a","lsp_verification":"unverifiable"}, - {"severity":"info","category":"y","message":"b","lsp_verification":"unverifiable"} -]} -JSON - run apply_lsp_verification "$FINDINGS" "audit" - [ "$status" -eq 0 ] - [ "$(jq -r '.findings[0].severity' "$FINDINGS")" = "info" ] - [ "$(jq -r '.findings[1].severity' "$FINDINGS")" = "info" ] -} - -@test "verified: finding is left intact (no downgrade, no annotation)" { - _activate_lsp - _write_findings <<'JSON' -{"tier":"deep","findings":[{"severity":"major","category":"correctness","message":"breaks 3 callers","lsp_verification":"verified"}]} -JSON - run apply_lsp_verification "$FINDINGS" "deep" - [ "$status" -eq 0 ] - [ "$(jq -r '.findings[0].severity' "$FINDINGS")" = "major" ] - run jq -r '.findings[0].message' "$FINDINGS" - [ "$output" = "breaks 3 callers" ] -} - -@test "untouched: a finding with no lsp_verification annotation is not changed" { - _activate_lsp - _write_findings <<'JSON' -{"tier":"deep","findings":[{"severity":"critical","category":"style","message":"prefer printf"}]} -JSON - run apply_lsp_verification "$FINDINGS" "deep" - [ "$status" -eq 0 ] - [ "$(jq -r '.findings[0].severity' "$FINDINGS")" = "critical" ] - # No verification record for an un-annotated finding. - [ ! -s "$TOKEN_LOG_FILE" ] -} - -# ── AC #4: per-finding verification outcome on the Token Observatory JSONL ───── - -@test "jsonl: one finding_verification record per verified/unverifiable finding" { - _activate_lsp - _write_findings <<'JSON' -{"tier":"deep","findings":[ - {"severity":"critical","category":"correctness","message":"undefined sym","lsp_verification":"unverifiable"}, - {"severity":"major","category":"correctness","message":"breaks 2 callers","lsp_verification":"verified"}, - {"severity":"minor","category":"style","message":"nit"} -]} -JSON - run apply_lsp_verification "$FINDINGS" "deep" - [ "$status" -eq 0 ] - # Two records (the un-annotated finding produces none). - [ "$(grep -c . "$TOKEN_LOG_FILE")" = "2" ] - # Every record is discriminated as a finding_verification record. - run jq -rs 'map(select(.kind == "finding_verification")) | length' "$TOKEN_LOG_FILE" - [ "$output" = "2" ] -} - -@test "jsonl: record carries tier, outcome, and before/after severities" { - _activate_lsp - _write_findings <<'JSON' -{"tier":"deep","findings":[{"severity":"critical","category":"correctness","message":"undefined","lsp_verification":"unverifiable"}]} -JSON - run apply_lsp_verification "$FINDINGS" "deep" - [ "$status" -eq 0 ] - run jq -r '[.kind,.tier,.outcome,.severity_before,.severity_after,.workflow] | @tsv' "$TOKEN_LOG_FILE" - [ "$output" = "$(printf 'finding_verification\tdeep\tunverifiable\tcritical\tmajor\tpr-review')" ] -} - -@test "jsonl: verified outcome keeps severity_after == severity_before" { - _activate_lsp - _write_findings <<'JSON' -{"tier":"audit","findings":[{"severity":"major","category":"correctness","message":"ok","lsp_verification":"verified"}]} -JSON - run apply_lsp_verification "$FINDINGS" "audit" - [ "$status" -eq 0 ] - run jq -r '[.outcome,.severity_before,.severity_after] | @tsv' "$TOKEN_LOG_FILE" - [ "$output" = "$(printf 'verified\tmajor\tmajor')" ] -} - -# ── emit_verification_record: no-op safety ──────────────────────────────────── - -@test "emit_verification_record: no-op when TOKEN_LOG_FILE is unset" { - unset TOKEN_LOG_FILE - run emit_verification_record "pr-review" "deep" "ctx" "0" "correctness" "critical" "major" "unverifiable" - [ "$status" -eq 0 ] -} - -@test "emit_verification_record: appends one valid JSONL object" { - emit_verification_record "pr-review" "deep" "ctx" "0" "correctness" "critical" "major" "unverifiable" - [ "$(grep -c . "$TOKEN_LOG_FILE")" = "1" ] - run jq -e '.kind == "finding_verification" and .finding_index == 0' "$TOKEN_LOG_FILE" - [ "$status" -eq 0 ] -} - -# ── apply_lsp_verification: positional-parameter edge cases ────────────────── - -@test "shift-guard: called with 1 arg (no tier) and findings contain MCP-failure text → still processes, not inert" { - # Regression for: shift 2 failing when $# < 2 leaves $@ unchanged, causing - # lsp_verification_active to scan the findings file for failure patterns. - _activate_lsp - # The findings file itself contains text matching the MCP-failure pattern. - _write_findings <<'JSON' -{"tier":"deep","findings":[{"severity":"critical","category":"correctness","message":"mcp server lsp failed to connect","lsp_verification":"unverifiable"}]} -JSON - # Called with only the findings file (no tier arg) — the risky case. - run apply_lsp_verification "$FINDINGS" - [ "$status" -eq 0 ] - # Verification must be active: the unverifiable finding must be downgraded. - [ "$(jq -r '.findings[0].severity' "$FINDINGS")" = "major" ] - run jq -r '.findings[0].message' "$FINDINGS" - [[ "$output" == *"[lsp: unverifiable]"* ]] -} diff --git a/tests/token_report.bats b/tests/token_report.bats index cb4cfbdca..859c02fd1 100644 --- a/tests/token_report.bats +++ b/tests/token_report.bats @@ -55,20 +55,6 @@ setup() { [ "$result" = "9250.0000" ] } -@test "annotate_records: excludes finding_verification records (story #843, no cost pollution)" { - # The LSP finding-verification step shares the Token Observatory JSONL channel - # (kind:"finding_verification"). Those audit records must NOT be priced as - # token-usage calls or they would inflate the cost report with phantom rows. - tmp="$(mktemp -d)" - printf '%s\n' \ - '{"ts":"2026-06-01T00:00:00Z","workflow":"pr-review","tier":"deep","model":"claude-opus-4-7","input_tokens":100,"output_tokens":50,"repo":"r","context":""}' \ - '{"kind":"finding_verification","ts":"2026-06-01T00:00:01Z","workflow":"pr-review","tier":"deep","outcome":"unverifiable","severity_before":"critical","severity_after":"major","finding_index":0,"context":"pr"}' \ - > "$tmp/mixed.jsonl" - result="$(annotate_records "$tmp" | wc -l)" - rm -rf "$tmp" - [ "$result" -eq 1 ] -} - @test "annotate_records: unknown model → cost sentinel -1 and known=0" { tmp="$(mktemp -d)" printf '%s\n' '{"ts":"2026-06-01T00:00:00Z","workflow":"x","tier":"y","model":"mystery-model-v9","input_tokens":100,"output_tokens":50,"repo":"r","context":""}' > "$tmp/u.jsonl"