diff --git a/prompts/deep-review.md b/prompts/deep-review.md index eed9f0994..3f83a548f 100644 --- a/prompts/deep-review.md +++ b/prompts/deep-review.md @@ -26,6 +26,13 @@ to approve or escalate further to the security auditor (Tier 3). downstream consumer repos that pin a reusable workflow / shell lib / prompt this PR changes. May be the literal `(none)`. This is an **informational signal** — annotate the impacted consumers; it is NOT an auto-escalation trigger. +- `$SYMBOL_CONTEXT_FILE` — (optional; set only when the symbol-context pass is + enabled) path to a file containing the `SYMBOL_CONTEXT` block: for each function + touched in the diff, its caller/callee/type-definition reference contexts + (path + snippet) sourced from code search. May be the literal `(none)`, and a + symbol may be annotated `search unavailable` if navigation degraded. Use it to + reason about **why** a touched function exists and to catch cross-file logic + bugs the hunk alone hides — it is context, NOT an auto-escalation trigger. - `$SAFETY_CHECKS_FILE` — (optional; set only when the safety-checks pass is enabled) path to a file containing the deterministic `SAFETY_CHECKS` block: the pre-computed hard-stop flags (`CI_WEAKENING_DETECTED`, @@ -54,6 +61,13 @@ enumeration. No actions on other PRs. surface them), and escalate ONLY if the change is independently risky per the taxonomy below (e.g. an interface-breaking edit to a consumed surface). Downstream impact alone — even with many consumers — is not a reason to escalate. + Also, if `$SYMBOL_CONTEXT_FILE` is set and the file exists and its contents are + not `(none)`, read it: for each function this PR touches it lists the + caller/callee/type-definition reference contexts across the repo. Use it to + verify the diff against how the symbol is actually used elsewhere (broken + callers, unhandled return shapes, type mismatches) — a cross-file + inconsistency it surfaces is a `correctness` finding; the context itself is + not an escalation trigger. Then, if `$SAFETY_CHECKS_FILE` is set and the file exists, read it. If `CI_WEAKENING_DETECTED` or `PROMPT_INJECTION_DETECTED` is `true`, that is a **blocking** deterministic verdict: set `risk: HIGH` and do not approve — diff --git a/prompts/rubber-duck.md b/prompts/rubber-duck.md index b17086e49..1a4fe52ef 100644 --- a/prompts/rubber-duck.md +++ b/prompts/rubber-duck.md @@ -19,6 +19,12 @@ otherwise. Prioritize concrete findings over vague concerns. `signals` array explaining why it escalated. - `$PRIOR_REVIEW_BODY` — prior review body if this is a re-review (empty if first). - `$PRIOR_REVIEW_SHA` — prior SHA if re-review. +- `$SYMBOL_CONTEXT_FILE` — (optional; set only when the symbol-context pass is + enabled) path to a file containing the `SYMBOL_CONTEXT` block: for each function + touched in the diff, its caller/callee/type-definition reference contexts + (path + snippet) from code search. May be the literal `(none)`. You get the + same context as the primary reviewer so you reason on equal footing — use it to + hunt cross-file logic bugs and broken callers the hunk alone hides. - `$OUTPUT_FILE` — absolute path where you MUST write your JSON verdict. ## Scope @@ -34,8 +40,13 @@ enumeration. No actions on other PRs. yourself to those signals. Your value comes from finding what others miss. 2. `gh pr view "$PR_URL" --json number,title,body,author,isDraft,baseRefName,headRefName,headRefOid,url,repository,labels,reviewDecision,mergeable,mergeStateStatus,statusCheckRollup,reviewRequests,reviews,comments,commits,closingIssuesReferences,additions,deletions,changedFiles,files` 3. `gh pr diff "$PR_URL"` — read the diff thoroughly. -4. Fetch linked issues if any. -5. Check `statusCheckRollup` for CI status. +4. If `$SYMBOL_CONTEXT_FILE` is set and the file exists and its contents are not + `(none)`, read it: it lists caller/callee/type-definition reference contexts + for each function this PR touches. Cross-check the diff against how each symbol + is used elsewhere — broken callers, unhandled return shapes, type mismatches + are exactly the cross-file bugs the primary reviewer's model family may miss. +5. Fetch linked issues if any. +6. Check `statusCheckRollup` for CI status. ## Adversarial focus areas diff --git a/scripts/lib/symbol-context.sh b/scripts/lib/symbol-context.sh new file mode 100644 index 000000000..ff78ebaf4 --- /dev/null +++ b/scripts/lib/symbol-context.sh @@ -0,0 +1,289 @@ +#!/usr/bin/env bash +set -euo pipefail +# Semantic symbol-context assembler for the PR-review cascade (issue #1090, +# epic #1088, Phase 2). +# +# Goal: give the deep-review (run_agentic deep) and rubber-duck (run_duck) tiers +# caller / callee / type-definition context for the SYMBOLS touched in the diff, +# not just the hunk — so the reviewer can reason about why a function exists and +# catch cross-file logic bugs the diff alone hides. +# +# Context source (Phase-1 audit + #839 status). The LSP pilot (#839) that would +# have exposed real LSP navigation tools (find_references, definitions) is NOT +# present in this repo, so per docs/initiatives/mcp-powered-review.md this uses +# the fallback: the **GitHub `search_code` path** (`gh search code`) to collect +# reference contexts for each touched symbol. This mirrors the `gh`-fetch +# structure of scripts/lib/downstream-impact.sh, keeping the assembler +# deterministic and unit-testable, and it lands the context into exactly the +# MCP-eligible deep + duck tiers that engine.sh's _mcp_review_flags already +# threads (triage — no tools — is untouched). +# +# Opt-in + inert by default (AC #4). The caller (review-one-pr.sh) only invokes +# assemble_symbol_context when SYMBOL_CONTEXT_ENABLED=true. When it is off, +# SYMBOL_CONTEXT_FILE is never set, the deep/duck prompts read no symbol block, +# and review behavior + cost are byte-identical to pre-feature. If the code +# search is unavailable, the assembler degrades with an MCP-style ::warning:: +# (fail loud, never fake) and the review continues on the model's base +# capabilities — it never aborts (always exits 0). +# +# Measurability (AC #2). For every touched function the assembler emits one +# ::notice::[symbol-context] log line counting the attached contexts, plus a +# SYMBOL_CONTEXT_COUNTS summary line inside the written block. + +# --------------------------------------------------------------------------- +# sc_touched_functions (PURE — no network / gh I/O) +# --------------------------------------------------------------------------- +# Parses a unified diff and prints the set of function symbols the diff touches, +# one per line, sorted + unique. A definition on an added OR removed line counts +# as "touched" (a deleted function is as relevant to cross-file reasoning as an +# added one). Language coverage is heuristic but spans the stacks this org sees: +# bash/sh : `name() {` and `function name` +# python : `def name(` +# js/ts : `function name(` and `const name = (`/`=>` arrow +# go : `func name(` and `func (recv) name(` +sc_touched_functions() { + local diff="${1:-}" + [ -n "$diff" ] || return 0 + printf '%s\n' "$diff" | awk ' + # Only consider added/removed content lines and @@ hunk headers; skip diff + # file headers (+++/---). + /^\+\+\+/ || /^---/ { next } + /^@@/ { + # Extract the trailing context field after the second @@ (e.g. + # "@@ -10,3 +10,3 @@ func ServeHTTP(...) {" -> "func ServeHTTP(...) {"). + # This is the only source of the touched function name when the diff is a + # body-only change (signature line was not added or removed). + line = $0 + if (match(line, /@@ [^@]* @@ /)) { + s = substr(line, RSTART + RLENGTH) + sub(/^[[:space:]]+/, "", s) + } else if (match(line, /@@ [^@]* @@$/)) { + next # no context field on this hunk header + } else { + next + } + name = "" + # go: func (recv) Name( | func Name( + if (match(s, /^func[[:space:]]+(\([^)]*\)[[:space:]]*)?[A-Za-z_][A-Za-z0-9_]*[[:space:]]*\(/)) { + t = s + sub(/^func[[:space:]]+/, "", t) + sub(/^\([^)]*\)[[:space:]]*/, "", t) + if (match(t, /^[A-Za-z_][A-Za-z0-9_]*/)) { name = substr(t, 1, RLENGTH) } + } + # python: def name( + else if (match(s, /^def[[:space:]]+[A-Za-z_][A-Za-z0-9_]*[[:space:]]*\(/)) { + t = s; sub(/^def[[:space:]]+/, "", t) + match(t, /^[A-Za-z_][A-Za-z0-9_]*/); name = substr(t, 1, RLENGTH) + } + # js/ts: (export) function name( + else if (match(s, /^(export[[:space:]]+)?(async[[:space:]]+)?function[[:space:]]+[A-Za-z_$][A-Za-z0-9_$]*[[:space:]]*\(/)) { + t = s + sub(/^export[[:space:]]+/, "", t) + sub(/^async[[:space:]]+/, "", t) + sub(/^function[[:space:]]+/, "", t) + match(t, /^[A-Za-z_$][A-Za-z0-9_$]*/); name = substr(t, 1, RLENGTH) + } + # bash: function name + else if (match(s, /^function[[:space:]]+[A-Za-z_][A-Za-z0-9_-]*/)) { + t = s; sub(/^function[[:space:]]+/, "", t) + match(t, /^[A-Za-z_][A-Za-z0-9_-]*/); name = substr(t, 1, RLENGTH) + } + # bash/generic: name() { + else if (match(s, /^[A-Za-z_][A-Za-z0-9_-]*[[:space:]]*\(\)/)) { + match(s, /^[A-Za-z_][A-Za-z0-9_-]*/); name = substr(s, 1, RLENGTH) + } + if (name != "") print name + next + } + /^[+-]/ { + line = substr($0, 2) # strip the +/- marker + s = line + sub(/^[[:space:]]+/, "", s) # left-trim + + name = "" + # go: func (recv) Name( | func Name( + if (match(s, /^func[[:space:]]+(\([^)]*\)[[:space:]]*)?[A-Za-z_][A-Za-z0-9_]*[[:space:]]*\(/)) { + t = s + sub(/^func[[:space:]]+/, "", t) + sub(/^\([^)]*\)[[:space:]]*/, "", t) # drop optional receiver + if (match(t, /^[A-Za-z_][A-Za-z0-9_]*/)) { name = substr(t, 1, RLENGTH) } + } + # python: def name( + else if (match(s, /^def[[:space:]]+[A-Za-z_][A-Za-z0-9_]*[[:space:]]*\(/)) { + t = s; sub(/^def[[:space:]]+/, "", t) + match(t, /^[A-Za-z_][A-Za-z0-9_]*/); name = substr(t, 1, RLENGTH) + } + # js/ts: (export) function name( + else if (match(s, /^(export[[:space:]]+)?(async[[:space:]]+)?function[[:space:]]+[A-Za-z_$][A-Za-z0-9_$]*[[:space:]]*\(/)) { + t = s + sub(/^export[[:space:]]+/, "", t) + sub(/^async[[:space:]]+/, "", t) + sub(/^function[[:space:]]+/, "", t) + match(t, /^[A-Za-z_$][A-Za-z0-9_$]*/); name = substr(t, 1, RLENGTH) + } + # bash: function name + else if (match(s, /^function[[:space:]]+[A-Za-z_][A-Za-z0-9_-]*/)) { + t = s; sub(/^function[[:space:]]+/, "", t) + match(t, /^[A-Za-z_][A-Za-z0-9_-]*/); name = substr(t, 1, RLENGTH) + } + # js/ts arrow: (export) const name = ( ... ) => | const name = async + else if (match(s, /^(export[[:space:]]+)?(const|let|var)[[:space:]]+[A-Za-z_$][A-Za-z0-9_$]*[[:space:]]*=[[:space:]]*(async[[:space:]]*)?(\(|[A-Za-z_$])/) && s ~ /=>/) { + t = s + sub(/^export[[:space:]]+/, "", t) + sub(/^(const|let|var)[[:space:]]+/, "", t) + match(t, /^[A-Za-z_$][A-Za-z0-9_$]*/); name = substr(t, 1, RLENGTH) + } + # bash/generic: name() { + else if (match(s, /^[A-Za-z_][A-Za-z0-9_-]*[[:space:]]*\(\)/)) { + match(s, /^[A-Za-z_][A-Za-z0-9_-]*/); name = substr(s, 1, RLENGTH) + } + + if (name != "") print name + } + ' | sort -u +} + +# --------------------------------------------------------------------------- +# _symbol_context_gh +# --------------------------------------------------------------------------- +# Run `gh` for code-search reads. Cross-repo / higher-rate reads prefer the bot +# PAT surfaced as GH_PAT (same convention as _downstream_gh in +# downstream-impact.sh); otherwise fall back to whatever auth gh already has. +_symbol_context_gh() { + if [ -n "${GH_PAT:-}" ]; then + GH_TOKEN="$GH_PAT" gh "$@" + else + gh "$@" + fi +} + +# --------------------------------------------------------------------------- +# sc_search_symbol_contexts +# --------------------------------------------------------------------------- +# Gathers up to caller/callee/type-def reference contexts for via +# the GitHub search_code path (`gh search code`). Prints one context per line as +# "\t". The GitHub code-search index covers both call sites +# (callers/callees) and the definition/type — the closest available proxy to LSP +# find_references given #839 is a no-go. +# +# Exit status: 0 on a successful search (even with zero results); 2 when the +# search itself fails (auth/scope/rate/unavailable) so the caller can emit the +# graceful-degradation warning and keep the review alive. +sc_search_symbol_contexts() { + local symbol="$1" repo="$2" max="${3:-3}" + [ -n "$symbol" ] || return 0 + + local json_fields="path,textMatches" + local args=(search code "$symbol" --limit "$max" --json "$json_fields") + [ -n "$repo" ] && args+=(--repo "$repo") + + local raw rc=0 + raw=$(_symbol_context_gh "${args[@]}" 2>/dev/null) || rc=$? + if [ "$rc" -ne 0 ]; then + return 2 + fi + # An empty/invalid payload is treated as "no results", not a failure. + [ -n "$raw" ] || return 0 + + printf '%s' "$raw" | jq -r ' + if type == "array" then .[] else empty end + | (.path // "" | tostring) as $p + | ((.textMatches // []) | (.[0]?.fragment // "")) as $frag + | ($frag | gsub("[[:space:]]+"; " ") | gsub("^ +| +$"; "")) as $snip + | "\($p)\t\($snip)" + ' 2>/dev/null || return 0 + return 0 +} + +# --------------------------------------------------------------------------- +# assemble_symbol_context [out_file] +# --------------------------------------------------------------------------- +# For each function touched in , gather >= SYMBOL_CONTEXT_MIN (default 3) +# reference contexts from via the search_code path and write a +# human-readable SYMBOL_CONTEXT block to , whose path is exported as +# SYMBOL_CONTEXT_FILE for the deep/audit tiers and run_duck (mirroring +# DOWNSTREAM_IMPACT_FILE). Emits a per-function context-count ::notice:: (AC #2). +# +# Bounds (AC #4 cost cap): +# - at most SYMBOL_CONTEXT_MAX_FUNCS functions searched per PR (default 20), +# - at most SYMBOL_CONTEXT_MIN contexts per function, +# - the assembled block truncated to SYMBOL_CONTEXT_MAX_BYTES (default 8000). +# +# Graceful degradation (AC #4): a diff touching no functions -> literal "(none)" +# with zero searches; a search failure for a symbol -> a single MCP-style +# ::warning:: and that symbol is annotated "search unavailable". Always exits 0. +assemble_symbol_context() { + local diff="${1:-}" + local repo="${2:-}" + local out_file="${3:-${SYMBOL_CONTEXT_FILE:-/tmp/cascade/symbol-context.txt}}" + local min="${SYMBOL_CONTEXT_MIN:-3}" + local max_funcs="${SYMBOL_CONTEXT_MAX_FUNCS:-10}" + local max_bytes="${SYMBOL_CONTEXT_MAX_BYTES:-8000}" + + mkdir -p "$(dirname "$out_file")" 2>/dev/null || true + rm -f "$out_file" + + local functions + functions=$(sc_touched_functions "$diff") + + # No touched functions -> literal "(none)", zero searches (AC #4). + if [ -z "$functions" ]; then + printf '%s' "(none)" > "$out_file" + export SYMBOL_CONTEXT_FILE="$out_file" + return 0 + fi + + local block="Symbol context (caller/callee/type-def references for functions touched in this diff):"$'\n' + local counts_csv="" total=0 fn_count=0 warned=0 + local fn + while IFS= read -r fn; do + [ -n "$fn" ] || continue + if [ "$fn_count" -ge "$max_funcs" ]; then + block+=$'\n'"(function cap reached: $max_funcs; remaining touched functions not searched)"$'\n' + break + fi + fn_count=$((fn_count + 1)) + + local ctx rc=0 n=0 + ctx=$(sc_search_symbol_contexts "$fn" "$repo" "$min") || rc=$? + + block+=$'\n'"- $fn"$'\n' + if [ "$rc" -ne 0 ]; then + block+=" search unavailable"$'\n' + if [ "$warned" -eq 0 ]; then + echo "::warning::[symbol-context] symbol navigation unavailable (code search failed) — review continues on the model's base capabilities (no symbol context)" >&2 + warned=1 + fi + echo "::notice::[symbol-context] $fn: 0 contexts attached (search unavailable)" >&2 + counts_csv="${counts_csv:+$counts_csv,}$fn=0" + continue + fi + + if [ -n "$ctx" ]; then + local l path snip + while IFS= read -r l; do + [ -n "$l" ] || continue + path="${l%%$'\t'*}" + snip="${l#*$'\t'}" + block+=" $path: $snip"$'\n' + n=$((n + 1)) + done <<< "$ctx" + fi + [ "$n" -eq 0 ] && block+=" (no references found)"$'\n' + + total=$((total + n)) + counts_csv="${counts_csv:+$counts_csv,}$fn=$n" + echo "::notice::[symbol-context] $fn: $n context(s) attached" >&2 + done <<< "$functions" + + # Machine-readable summary inside the block (AC #2 — measurable in the output). + block+=$'\n'"SYMBOL_CONTEXT_COUNTS: $counts_csv"$'\n' + echo "::notice::[symbol-context] total: $total context(s) across $fn_count function(s)" >&2 + + # Cap total size so a large touched-function set cannot blow up the prompt + # (mirrors the 8 KB DOWNSTREAM_IMPACT cap). + block="${block:0:max_bytes}" + printf '%s' "$block" > "$out_file" + export SYMBOL_CONTEXT_FILE="$out_file" + return 0 +} diff --git a/scripts/review-one-pr.sh b/scripts/review-one-pr.sh index d6daa254c..6752ce1b6 100755 --- a/scripts/review-one-pr.sh +++ b/scripts/review-one-pr.sh @@ -56,6 +56,13 @@ source "$SCRIPT_DIR/lib/downstream-impact.sh" # Gated behind SAFETY_CHECKS_ENABLED (default ON; off => byte-identical prompt). # shellcheck source=lib/safety-checks.sh source "$SCRIPT_DIR/lib/safety-checks.sh" +# Semantic symbol-context pass (issue #1090, epic #1088): assemble_symbol_context +# gathers caller/callee/type-def reference contexts (via the GitHub search_code +# path) for each function touched in the diff and writes them to a file whose +# path is exported as SYMBOL_CONTEXT_FILE for the deep + duck tiers. Gated +# default-off behind SYMBOL_CONTEXT_ENABLED (inert + byte-identical when off). +# shellcheck source=lib/symbol-context.sh +source "$SCRIPT_DIR/lib/symbol-context.sh" # Shared PR-context prefetch (epic #1101, Story 2): prefetch_pr_context persists # the FULL diff + superset metadata to SHA-bound files (PR_CONTEXT_DIFF_FILE / # PR_CONTEXT_METADATA_FILE) for the agentic tiers. Gated default-off behind @@ -726,7 +733,10 @@ TRIAGE_RESULT=$( # Drop the bulky locals now that the prompt file is on disk. Keeps later # subprocess forks (jq, claude) from hitting E2BIG on hundreds-of-KB diffs. +# PR_DIFF is saved as a local shell variable (not exported) for tier-2 +# symbol-context assembly and unset after that pass completes. # Advisory feedback is capped at 8 KB; write to disk for Tier 2/3 to read. +_sc_diff="${PR_DIFF:-}" ADVISORY_BOT_FEEDBACK_FILE="/tmp/cascade/advisory-bot-feedback.txt" rm -f "$ADVISORY_BOT_FEEDBACK_FILE" if [[ -n "${ADVISORY_BOT_FEEDBACK//[[:space:]]/}" ]]; then @@ -920,6 +930,21 @@ if [ -f "/tmp/cascade/downstream-impact.txt" ]; then export DOWNSTREAM_IMPACT_FILE="/tmp/cascade/downstream-impact.txt" fi +# Semantic symbol-context pass (issue #1090). Runs here in tier-2 so +# triage-cleared PRs never pay the gh search code cost (only deep-review and +# rubber-duck read SYMBOL_CONTEXT_FILE). Gated default-off: when disabled +# SYMBOL_CONTEXT_FILE is never set and the deep/duck prompts + cost are +# byte-identical to pre-feature behavior (holdout-eval stability + rollback). +# Best-effort — assemble_symbol_context always exits 0, degrading to a warning. +if [ "${SYMBOL_CONTEXT_ENABLED:-false}" = "true" ]; then + if [ -z "${GITHUB_REPOSITORY:-}" ]; then + echo "::warning::[symbol-context] GITHUB_REPOSITORY is unset — skipping symbol context to avoid an unscoped cross-repository search" >&2 + else + assemble_symbol_context "$_sc_diff" "$GITHUB_REPOSITORY" "/tmp/cascade/symbol-context.txt" || true + fi +fi +unset _sc_diff + # --- Tier 2: Deep review + Rubber duck (parallel, cross-engine) --- echo " [tier2] type=$TRIAGE_TYPE specialist=$DEEP_TIER_PROMPT" echo " [tier2] deep review ($ENGINE_DEEP_MODEL) + rubber duck ($DUCK_MODEL via $DUCK_ENGINE)" diff --git a/tests/dev-lead/unit/test_symbol_context_assemble.bats b/tests/dev-lead/unit/test_symbol_context_assemble.bats new file mode 100644 index 000000000..30df86624 --- /dev/null +++ b/tests/dev-lead/unit/test_symbol_context_assemble.bats @@ -0,0 +1,218 @@ +#!/usr/bin/env bats +# Unit tests for the I/O half of the symbol-context assembler (issue #1090, +# epic #1088): assemble_symbol_context [out_file]. +# +# Unlike sc_touched_functions (pure), the assembler performs `gh` I/O: it +# extracts the touched functions from the diff, then for each one gathers +# caller/callee/type-def reference contexts via the GitHub search_code path +# (`gh search code`), writes a human-readable SYMBOL_CONTEXT block to a file +# (path exported as SYMBOL_CONTEXT_FILE), and emits a per-function context-count +# log line (AC #2). It must: +# - assemble at least SYMBOL_CONTEXT_MIN (default 3) contexts per function, +# - emit a measurable per-function count log line, +# - degrade gracefully (search unavailable -> MCP-style ::warning::, exit 0), +# - write literal "(none)" and perform zero searches when the diff touches no +# functions. +# +# `gh` is stubbed via a PATH shim that logs every invocation and returns a fixed +# set of search results, so we can assert behavior and search volume. +# +# Run with: bats tests/dev-lead/unit/test_symbol_context_assemble.bats + +setup() { + source "$(cd "$(dirname "$BATS_TEST_FILENAME")/../../.." && pwd)/scripts/lib/symbol-context.sh" + + STUB_DIR="$(mktemp -d "$BATS_TEST_TMPDIR/stub.XXXXXX")" + export PATH="$STUB_DIR:$PATH" + export GH_CALL_LOG="$STUB_DIR/gh_calls.log" + : > "$GH_CALL_LOG" + OUT_FILE="$STUB_DIR/symbol-context.txt" + REPO="petry-projects/.github-private" + + # A one-function bash diff used by most tests. + DIFF='--- a/scripts/foo.sh ++++ b/scripts/foo.sh +@@ -10,1 +10,3 @@ ++assemble_thing() { ++ helper_fn ++} +' + export DIFF +} + +teardown() { + rm -rf "$STUB_DIR" + unset SYMBOL_CONTEXT_FILE SYMBOL_CONTEXT_MIN SYMBOL_CONTEXT_MAX_FUNCS SYMBOL_CONTEXT_MAX_BYTES +} + +# Installs a gh stub. GH_STUB_MODE: +# found — `gh search code` returns 4 result items with path + textMatches. +# two — `gh search code` returns 2 result items (for SYMBOL_CONTEXT_MIN tests). +# fail — `gh search code` exits 1 (search unavailable / no scope). +make_gh_stub() { + cat > "$STUB_DIR/gh" << 'STUBEOF' +#!/usr/bin/env bash +printf '%s\n' "$*" >> "$GH_CALL_LOG" +[ "$1" = "search" ] && [ "$2" = "code" ] || exit 0 + +if [ "${GH_STUB_MODE:-found}" = "fail" ]; then + echo "gh: code search failed" >&2 + exit 1 +fi + +if [ "${GH_STUB_MODE:-found}" = "two" ]; then + cat <<'JSON' +[ + {"path":"scripts/a.sh","textMatches":[{"fragment":" assemble_thing arg1"}]}, + {"path":"scripts/b.sh","textMatches":[{"fragment":"result=$(assemble_thing)"}]} +] +JSON + exit 0 +fi + +# Emit a JSON array shaped like `gh search code --json path,textMatches`. +cat <<'JSON' +[ + {"path":"scripts/a.sh","textMatches":[{"fragment":" assemble_thing arg1"}]}, + {"path":"scripts/b.sh","textMatches":[{"fragment":"result=$(assemble_thing)"}]}, + {"path":"scripts/c.sh","textMatches":[{"fragment":"assemble_thing() {"}]}, + {"path":"scripts/d.sh","textMatches":[{"fragment":"# calls assemble_thing later"}]} +] +JSON +STUBEOF + chmod +x "$STUB_DIR/gh" +} + +# --------------------------------------------------------------------------- +# Happy path: >=3 contexts per function, count log line, file + export +# --------------------------------------------------------------------------- + +@test "assembles at least 3 contexts for a touched function and lists them" { + export GH_STUB_MODE=found + make_gh_stub + run assemble_symbol_context "$DIFF" "$REPO" "$OUT_FILE" + [ "$status" -eq 0 ] + [ -f "$OUT_FILE" ] + grep -q "assemble_thing" "$OUT_FILE" + # At least 3 reference-context lines were written for the function. + [ "$(grep -c 'scripts/[a-d]\.sh' "$OUT_FILE" || true)" -ge 3 ] + run grep -qx "(none)" "$OUT_FILE" + [ "$status" -eq 1 ] + # Searches actually happened. + [ -s "$GH_CALL_LOG" ] +} + +@test "emits a measurable per-function context-count log line (AC #2)" { + export GH_STUB_MODE=found + make_gh_stub + run assemble_symbol_context "$DIFF" "$REPO" "$OUT_FILE" + [ "$status" -eq 0 ] + # The count log line names the function and its attached-context count. + [[ "$output" == *"[symbol-context]"* ]] + [[ "$output" == *"assemble_thing"* ]] + [[ "$output" =~ assemble_thing.*[0-9]+.*context ]] +} + +@test "SYMBOL_CONTEXT_FILE is exported and points at the written file" { + export GH_STUB_MODE=found + make_gh_stub + assemble_symbol_context "$DIFF" "$REPO" "$OUT_FILE" + [ "$SYMBOL_CONTEXT_FILE" = "$OUT_FILE" ] + [ -f "$SYMBOL_CONTEXT_FILE" ] +} + +@test "the configured minimum contexts-per-function is honored" { + export GH_STUB_MODE=two + make_gh_stub + SYMBOL_CONTEXT_MIN=2 run assemble_symbol_context "$DIFF" "$REPO" "$OUT_FILE" + [ "$status" -eq 0 ] + [ "$(grep -c 'scripts/[a-b]\.sh' "$OUT_FILE")" -eq 2 ] +} + +# --------------------------------------------------------------------------- +# Graceful degradation: search unavailable -> MCP-style warning, exit 0 +# --------------------------------------------------------------------------- + +@test "search failure degrades with an MCP-style warning and exits 0 (AC #4)" { + export GH_STUB_MODE=fail + make_gh_stub + run assemble_symbol_context "$DIFF" "$REPO" "$OUT_FILE" + [ "$status" -eq 0 ] + [ -f "$OUT_FILE" ] + # Reuses the MCP graceful-degradation phrasing: warn, never fake. + [[ "$output" == *"::warning::"* ]] + [[ "$output" == *"symbol-context"* ]] + [[ "$output" == *"base capabilities"* ]] +} + +# --------------------------------------------------------------------------- +# No touched functions -> literal "(none)", zero searches +# --------------------------------------------------------------------------- + +@test "a diff touching no functions writes literal (none) and performs zero searches" { + export GH_STUB_MODE=found + make_gh_stub + local docs_diff='--- a/README.md ++++ b/README.md +@@ -1,1 +1,1 @@ +-old ++new +' + run assemble_symbol_context "$docs_diff" "$REPO" "$OUT_FILE" + [ "$status" -eq 0 ] + [ "$(cat "$OUT_FILE")" = "(none)" ] + [ ! -s "$GH_CALL_LOG" ] +} + +# --------------------------------------------------------------------------- +# Bounded block size +# --------------------------------------------------------------------------- + +@test "the assembled block is truncated to the configured byte cap" { + export GH_STUB_MODE=found + make_gh_stub + SYMBOL_CONTEXT_MAX_BYTES=40 run assemble_symbol_context "$DIFF" "$REPO" "$OUT_FILE" + [ "$status" -eq 0 ] + [ -f "$OUT_FILE" ] + [ "$(wc -c < "$OUT_FILE")" -le 40 ] +} + +# --------------------------------------------------------------------------- +# SYMBOL_CONTEXT_MAX_FUNCS: multi-function diff + cap enforcement +# --------------------------------------------------------------------------- + +# Two-function diff used by the cap tests below. +MULTI_DIFF='--- a/scripts/foo.sh ++++ b/scripts/foo.sh +@@ -10,3 +10,3 @@ ++assemble_thing() { ++ helper_fn ++} +@@ -20,3 +20,3 @@ ++another_function() { ++ more stuff ++} +' + +@test "SYMBOL_CONTEXT_MAX_FUNCS caps the number of functions searched" { + export GH_STUB_MODE=found + make_gh_stub + SYMBOL_CONTEXT_MAX_FUNCS=1 run assemble_symbol_context "$MULTI_DIFF" "$REPO" "$OUT_FILE" + [ "$status" -eq 0 ] + grep -q "function cap reached: 1" "$OUT_FILE" + # Only one gh search was issued for the one allowed function. + [ "$(grep -c 'search code' "$GH_CALL_LOG")" -eq 1 ] +} + +@test "each function keeps its own context output and logging isolated" { + export GH_STUB_MODE=found + make_gh_stub + run assemble_symbol_context "$MULTI_DIFF" "$REPO" "$OUT_FILE" + [ "$status" -eq 0 ] + # Both functions have their own section in the output file. + grep -q "assemble_thing" "$OUT_FILE" + grep -q "another_function" "$OUT_FILE" + # Both functions get their own per-function count log line. + [[ "$output" =~ assemble_thing.*[0-9]+.*context ]] + [[ "$output" =~ another_function.*[0-9]+.*context ]] +} diff --git a/tests/dev-lead/unit/test_symbol_context_map.bats b/tests/dev-lead/unit/test_symbol_context_map.bats new file mode 100644 index 000000000..56b1aff47 --- /dev/null +++ b/tests/dev-lead/unit/test_symbol_context_map.bats @@ -0,0 +1,150 @@ +#!/usr/bin/env bats +# Unit tests for the PURE half of the symbol-context assembler (issue #1090, +# epic #1088): sc_touched_functions . +# +# sc_touched_functions parses a unified diff and returns the set of function +# symbols the diff touches (added/removed/hunk-header signatures), with NO +# network / `gh` I/O — so the extraction is deterministic and unit-testable in +# isolation (mirrors the pure jq mapper in scripts/lib/downstream-impact.sh). +# +# Run with: bats tests/dev-lead/unit/test_symbol_context_map.bats + +setup() { + source "$(cd "$(dirname "$BATS_TEST_FILENAME")/../../.." && pwd)/scripts/lib/symbol-context.sh" +} + +# --------------------------------------------------------------------------- +# Empty / no-function diffs +# --------------------------------------------------------------------------- + +@test "empty diff yields no touched functions" { + run sc_touched_functions "" + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +@test "a docs-only diff with no function definitions yields nothing" { + local diff='--- a/README.md ++++ b/README.md +@@ -1,2 +1,2 @@ +-old line ++new line +' + run sc_touched_functions "$diff" + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +# --------------------------------------------------------------------------- +# Language coverage +# --------------------------------------------------------------------------- + +@test "bash function definitions (name() and function name) are extracted" { + local diff='--- a/scripts/foo.sh ++++ b/scripts/foo.sh +@@ -10,3 +10,6 @@ ++assemble_thing() { ++ : ++} ++function helper_fn { ++ : ++} +' + run sc_touched_functions "$diff" + [ "$status" -eq 0 ] + [[ "$output" == *"assemble_thing"* ]] + [[ "$output" == *"helper_fn"* ]] +} + +@test "python def and js/ts function/const-arrow definitions are extracted" { + local diff='--- a/app/util.py ++++ b/app/util.py +@@ -1,1 +1,2 @@ ++def compute_total(items): ++ return 0 +--- a/web/util.ts ++++ b/web/util.ts +@@ -1,1 +1,3 @@ ++export function renderCard(props) {} ++const useThing = (x) => x +' + run sc_touched_functions "$diff" + [ "$status" -eq 0 ] + [[ "$output" == *"compute_total"* ]] + [[ "$output" == *"renderCard"* ]] + [[ "$output" == *"useThing"* ]] +} + +@test "go func definitions (plain and method receiver) are extracted" { + local diff='--- a/svc/handler.go ++++ b/svc/handler.go +@@ -1,1 +1,3 @@ ++func ServeHTTP(w http.ResponseWriter, r *http.Request) {} ++func (s *Server) Start() error { return nil } +' + run sc_touched_functions "$diff" + [ "$status" -eq 0 ] + [[ "$output" == *"ServeHTTP"* ]] + [[ "$output" == *"Start"* ]] +} + +# --------------------------------------------------------------------------- +# Determinism: unique + sorted, and removed-line defs also count as "touched" +# --------------------------------------------------------------------------- + +@test "the same function touched twice is reported once (unique)" { + local diff='--- a/scripts/foo.sh ++++ b/scripts/foo.sh +@@ -10,2 +10,2 @@ +-assemble_thing() { ++assemble_thing() { +' + run sc_touched_functions "$diff" + [ "$status" -eq 0 ] + # exactly one line of output + [ "$(printf '%s\n' "$output" | grep -c 'assemble_thing' || true)" -eq 1 ] +} + +# --------------------------------------------------------------------------- +# Hunk-header (@@ line) signature extraction +# --------------------------------------------------------------------------- + +@test "body-only change: function signature in @@ hunk header is extracted" { + # The signature appears only in the @@ line context field — not on any +/- line. + local diff='--- a/svc/handler.go ++++ b/svc/handler.go +@@ -10,3 +10,3 @@ func ServeHTTP(w http.ResponseWriter, r *http.Request) { +- doOldThing() ++ doNewThing() + } +' + run sc_touched_functions "$diff" + [ "$status" -eq 0 ] + [[ "$output" == *"ServeHTTP"* ]] +} + +@test "body-only bash change: function name in @@ hunk header is extracted" { + local diff='--- a/scripts/foo.sh ++++ b/scripts/foo.sh +@@ -5,3 +5,3 @@ assemble_thing() { +- old_cmd ++ new_cmd + } +' + run sc_touched_functions "$diff" + [ "$status" -eq 0 ] + [[ "$output" == *"assemble_thing"* ]] +} + +@test "body-only python change: function name in @@ hunk header is extracted" { + local diff='--- a/app/util.py ++++ b/app/util.py +@@ -3,3 +3,3 @@ def compute_total(items): +- return 0 ++ return sum(items) + } +' + run sc_touched_functions "$diff" + [ "$status" -eq 0 ] + [[ "$output" == *"compute_total"* ]] +}