perf(#1032): use blobless fetch with timeout for pre-review clone deepening

docs/review.md

@@ -105,7 +105,7 @@ See [Customizing with AGENTS.md](https://fullsend.sh/docs/guides/user/customizin
 | `REVIEW_SKIP_AUTHORS` | Comma-separated list of forge usernames to skip review for. When a PR/MR is opened by a user in this list, the review dispatch exits early without running the agent. Set in `env.runner` in your harness YAML (consumed by the pre-script on the runner). | _(empty — all PRs/MRs are reviewed)_ | Comma-separated logins, e.g. `app/renovate,app/dependabot` |
 | `REVIEW_PROTECTED_PATHS` | Comma-separated list of path prefixes the review agent treats as protected. PRs that modify files under these paths cannot be approved by the agent — only a human can grant approval. Default is set in `harness/review.yaml` (`env.runner` and `env.sandbox`); an unset value is a misconfiguration (fail-closed). Set to an empty string to deliberately disable protected-path enforcement entirely. When set to a value that parses to no valid paths (e.g. stray or consecutive commas), the script aborts (fail-closed) as a likely misconfiguration. | See [`harness/review.yaml`](../harness/review.yaml) | Comma-separated path prefixes (e.g. `.github/,deploy/,manifests/`) |
 | `REVIEW_RISK_ASSESSMENT_ENABLED` | Enables the risk assessment pre-pass (GitHub only). When `true`, the orchestrator dispatches a risk-assessment sub-agent before the main review dimensions. The sub-agent computes a composite 1–5 risk score from metadata signals, git history, and linked issue context. The post-script applies a `risk/*` label and posts a sticky risk comment. Set in `forge.github.env` in the harness — not in the top-level `env:` block, since the risk assessment scripts depend on the GitHub API and produce fabricated scores on other forges. | `true` (GitHub) | `"true"`, `"false"` |
-| `REVIEW_GIT_FETCH_DEPTH` | Controls clone deepening for git history analysis (risk assessment Tier 2). When set to `"0"`, the pre-script unshallows the target repo clone so the risk-assessment sub-agent can access full commit history. When unset and `REVIEW_RISK_ASSESSMENT_ENABLED` is `true`, defaults to `"0"` automatically — the Tier 2 sub-agent requires full git history. Set explicitly to any other value (e.g., `"1"`) to disable deepening even with risk assessment enabled. Set in `env.runner` in harness YAML (consumed by the pre-script on the runner). | _(auto: `"0"` when risk assessment enabled, no deepening otherwise)_ | `"0"` to fully unshallow |
+| `REVIEW_GIT_FETCH_DEPTH` | Controls clone deepening for git history analysis (risk assessment Tier 2). When set to `"0"`, the pre-script deepens the target repo clone so the risk-assessment sub-agent can access commit history. The deepen is blobless (`git fetch --unshallow --filter=blob:none`) and bounded by a 120s timeout: Tier 2 reads commit and tree metadata only, so historical file contents are never fetched, and a slow fetch degrades the risk signal instead of consuming the review budget. The pre-script also sets `diff.renames=false` in the deepened clone — rename detection is the one history operation that reads file contents, which the sandbox cannot fetch. If the fetch fails or times out the clone stays shallow, which the sub-agent detects and treats as an unavailable tier. When unset and `REVIEW_RISK_ASSESSMENT_ENABLED` is `true`, defaults to `"0"` automatically — the Tier 2 sub-agent requires full git history. Set explicitly to any other value (e.g., `"1"`) to disable deepening even with risk assessment enabled. Set in `env.runner` in harness YAML (consumed by the pre-script on the runner). | _(auto: `"0"` when risk assessment enabled, no deepening otherwise)_ | `"0"` to fully unshallow |
 
 Override any variable by extending the harness file via a `base` reference and setting `env.runner` / `env.sandbox` in your custom harness YAML. `base` composition merges `env.runner`/`env.sandbox` per-key — child values override, everything else inherits from the base (ADR 0045, ADR 0055). Per ADR 0080 and ADR 0081, this harness-level override is the correct path; the CI workflow `env:` block is reserved for infrastructure plumbing, not agent behavior knobs like these.
 

scripts/pre-review-test.sh

@@ -487,6 +487,212 @@ run_gitlab_test_stdout "gitlab-no-token-proceeds" \
   0 \
   "REVIEW_TOKEN="
 
+# --- Clone-deepening tests (risk assessment Tier 2) ---
+#
+# These build a real local repo pair (working clone + bare "remote") so the
+# actual `git fetch --unshallow --filter=blob:none origin` in pre-review.sh
+# runs offline. Stubbing git was rejected deliberately: what these tests need
+# to assert is the state of the resulting clone (still shallow? blobless?
+# rename detection off?), which a stub cannot produce.
+
+# build_deepen_repo creates ${TMPDIR}/deepen/target-repo as a depth-1 clone of
+# a local bare repo whose history contains a rename, and echoes its path.
+build_deepen_repo() {
+  local root="${TMPDIR}/deepen"
+  rm -rf "${root}"
+  mkdir -p "${root}"
+  (
+    set -e
+    git init -q -b main "${root}/src"
+    cd "${root}/src"
+    git config user.email "test@example.com"
+    git config user.name "Test User"
+    local i
+    for i in 1 2 3; do
+      printf 'line%s\n%s\n' "${i}" "$(seq 1 40 | tr '\n' ' ')" > "file${i}.txt"
+      git add -A
+      git commit -qm "commit ${i}"
+    done
+    # A rename *plus* a content edit is what makes a blobless clone lazily
+    # fetch blobs: exact renames are detected from the blob hash alone, so
+    # only inexact (similarity) detection has to read content. Renaming
+    # without editing would make this fixture pass no matter what.
+    git mv file1.txt renamed1.txt
+    printf 'CHANGED HEADER\n%s\nextra tail line\n' "$(seq 1 34 | tr '\n' ' ')" > renamed1.txt
+    git add -A
+    git commit -qm "rename and modify file1"
+    cd "${root}"
+    git clone -q --bare src bare.git
+    # Partial clone is served only when the remote opts in.
+    git -C bare.git config uploadpack.allowFilter true
+    git clone -q --depth=1 "file://${root}/bare.git" target-repo
+  ) >/dev/null 2>&1
+  echo "${root}/target-repo"
+}
+
+# run_deepen_test runs pre-review.sh with REPO_DIR pointed at a fixture clone.
+# Arguments:
+#   $1 — test name
+#   $2 — expected stdout substring
+#   $3 — REPO_DIR value
+#   $4+ — extra KEY=VALUE environment entries
+run_deepen_test() {
+  local test_name="$1"
+  local expected_stdout="$2"
+  local repo_dir="$3"
+  shift 3
+
+  local mock_bin
+  mock_bin="$(build_mock "OPEN" "human-author")"
+
+  local exit_code=0
+  env \
+    PATH="${mock_bin}:${PATH}" \
+    PR_NUMBER="42" \
+    REPO_FULL_NAME="test-org/test-repo" \
+    PR_URL="https://github.com/test-org/test-repo/pull/42" \
+    FULLSEND_FORGE="github" \
+    REVIEW_TOKEN="fake..." \
+    GH_TOKEN="fake..." \
+    REPO_DIR="${repo_dir}" \
+    REVIEW_RISK_ASSESSMENT_ENABLED="true" \
+    "$@" \
+    bash "${SCRIPT_DIR}/pre-review.sh" > "${TMPDIR}/stdout.log" 2>&1 || exit_code=$?
+
+  if [[ ${exit_code} -ne 0 ]]; then
+    echo "FAIL: ${test_name} — expected exit 0, got ${exit_code}"
+    cat "${TMPDIR}/stdout.log"
+    FAILURES=$((FAILURES + 1))
+    return 1
+  fi
+
+  if ! grep -qF "${expected_stdout}" "${TMPDIR}/stdout.log" 2>/dev/null; then
+    echo "FAIL: ${test_name} — expected stdout '${expected_stdout}' not found"
+    cat "${TMPDIR}/stdout.log"
+    FAILURES=$((FAILURES + 1))
+    return 1
+  fi
+
+  echo "PASS: ${test_name}"
+  return 0
+}
+
+# check_repo_state asserts a git config/state value on the fixture clone.
+check_repo_state() {
+  local test_name="$1"
+  local actual="$2"
+  local expected="$3"
+
+  if [[ "${actual}" != "${expected}" ]]; then
+    echo "FAIL: ${test_name} — expected '${expected}', got '${actual}'"
+    FAILURES=$((FAILURES + 1))
+    return
+  fi
+  echo "PASS: ${test_name}"
+}
+
+# Happy path: shallow clone is deepened, blobless, with renames disabled.
+DEEPEN_REPO="$(build_deepen_repo)"
+if run_deepen_test "deepen-shallow-clone" \
+  "Clone deepened successfully" \
+  "${DEEPEN_REPO}"; then
+
+  check_repo_state "deepen-clears-shallow" \
+    "$(git -C "${DEEPEN_REPO}" rev-parse --is-shallow-repository)" "false"
+
+  check_repo_state "deepen-full-history" \
+    "$(git -C "${DEEPEN_REPO}" rev-list --count HEAD)" "4"
+
+  # The filter's config keys are deliberately removed afterwards (see below),
+  # so assert the *effect*: a historical blob is genuinely absent.
+  PRE_RENAME="$(git -C "${DEEPEN_REPO}" log --format=%H -1 --skip=1)"
+  check_repo_state "deepen-applies-blob-filter" \
+    "$(git -C "${DEEPEN_REPO}" cat-file -e "${PRE_RENAME}:file1.txt" 2>/dev/null \
+      && echo "blob-present" || echo "blob-absent")" "blob-absent"
+
+  # Containment: neither key may survive, or the sandbox will try a lazy
+  # fetch it cannot complete. partialclonefilter alone re-registers origin
+  # as a promisor remote, so both are checked.
+  check_repo_state "deepen-unsets-promisor" \
+    "$(git -C "${DEEPEN_REPO}" config --get remote.origin.promisor || echo "unset")" "unset"
+  check_repo_state "deepen-unsets-partialclonefilter" \
+    "$(git -C "${DEEPEN_REPO}" config --get remote.origin.partialclonefilter || echo "unset")" "unset"
+
+  # The point of the whole exercise: Tier 2's change-coupling command must
+  # run offline on a commit containing a rename. Point origin at nothing too,
+  # so a regression that restored the promisor config still could not quietly
+  # succeed by reaching the "remote".
+  git -C "${DEEPEN_REPO}" remote set-url origin "file:///nonexistent/gone.git"
+
+  check_repo_state "deepen-disables-rename-detection" \
+    "$(git -C "${DEEPEN_REPO}" config --get diff.renames)" "false"
+
+  RENAME_COMMIT="$(git -C "${DEEPEN_REPO}" log --format=%H -1)"
+
+  # Negative control: with rename detection on, the same commit *does* need a
+  # blob that is not present. This is what the diff.renames=false line in
+  # pre-review.sh prevents — if that line is dropped, the assertions below
+  # stop being vacuous and start failing.
+  RENAME_ERRS="$(git -C "${DEEPEN_REPO}" -c diff.renames=true show --name-only \
+    --format= "${RENAME_COMMIT}" 2>&1 | grep -ciE 'fatal|could not' || true)"
+  check_repo_state "deepen-rename-detection-fails-without-blobs" \
+    "$([[ "${RENAME_ERRS}" -gt 0 ]] && echo "needs-network" || echo "offline")" \
+    "needs-network"
+
+  COUPLING_OUT="$(git -C "${DEEPEN_REPO}" diff-tree -r --no-commit-id --name-only \
+    --no-renames "${RENAME_COMMIT}" 2>&1)"
+  check_repo_state "deepen-coupling-command-needs-no-blobs" \
+    "$(printf '%s' "${COUPLING_OUT}" | grep -ciE 'fatal|could not fetch')" "0"
+  check_repo_state "deepen-coupling-command-names-both-paths" \
+    "$(printf '%s\n' "${COUPLING_OUT}" | sort | tr '\n' ' ')" "file1.txt renamed1.txt "
+fi
+
+# Explicit REVIEW_GIT_FETCH_DEPTH wins over the risk-assessment auto-default.
+DEEPEN_REPO_2="$(build_deepen_repo)"
+if run_deepen_test "no-deepen-when-depth-explicitly-set" \
+  "proceeding with review agent" \
+  "${DEEPEN_REPO_2}" \
+  REVIEW_GIT_FETCH_DEPTH="1"; then
+
+  check_repo_state "explicit-depth-leaves-clone-shallow" \
+    "$(git -C "${DEEPEN_REPO_2}" rev-parse --is-shallow-repository)" "true"
+fi
+
+# Risk assessment disabled — no deepening, no warning.
+DEEPEN_REPO_3="$(build_deepen_repo)"
+if run_deepen_test "no-deepen-when-risk-assessment-disabled" \
+  "proceeding with review agent" \
+  "${DEEPEN_REPO_3}" \
+  REVIEW_RISK_ASSESSMENT_ENABLED="false"; then
+
+  check_repo_state "disabled-risk-leaves-clone-shallow" \
+    "$(git -C "${DEEPEN_REPO_3}" rev-parse --is-shallow-repository)" "true"
+fi
+
+# Unreachable remote — the script warns and still exits 0, leaving the clone
+# shallow so the Tier 2 sub-agent detects the degraded state itself.
+DEEPEN_REPO_4="$(build_deepen_repo)"
+git -C "${DEEPEN_REPO_4}" remote set-url origin "file:///nonexistent/gone.git"
+if run_deepen_test "deepen-failure-warns-and-continues" \
+  "::warning::Failed to deepen clone" \
+  "${DEEPEN_REPO_4}"; then
+
+  check_repo_state "failed-deepen-leaves-clone-shallow" \
+    "$(git -C "${DEEPEN_REPO_4}" rev-parse --is-shallow-repository)" "true"
+fi
+
+# Missing target directory is reported, not silently skipped.
+run_deepen_test "deepen-missing-target-dir-warns" \
+  "::warning::Clone-deepening skipped" \
+  "${TMPDIR}/deepen/does-not-exist"
+
+# Non-GitHub forge does not attempt a gh-token fetch.
+DEEPEN_REPO_5="$(build_deepen_repo)"
+run_deepen_test "deepen-skipped-without-token" \
+  "::warning::Cannot deepen clone" \
+  "${DEEPEN_REPO_5}" \
+  GH_TOKEN=""
+
 # --- Summary ---
 
 echo ""

scripts/pre-review.sh

@@ -492,9 +492,21 @@ fi
 
 # ---------------------------------------------------------------------------
 # Deepen shallow clone for git history analysis (risk assessment Tier 2).
-# When REVIEW_GIT_FETCH_DEPTH is unset, default to "0" (full unshallow) if
-# risk assessment is enabled — the Tier 2 sub-agent needs full git history.
+# When REVIEW_GIT_FETCH_DEPTH is unset, default to "0" if risk assessment is
+# enabled — the Tier 2 sub-agent needs commit history, not just the tip.
 # Explicit values always take precedence.
+#
+# The deepen is blobless (--filter=blob:none): Tier 2 reads commit and tree
+# metadata only, so fetching historical file contents would be pure cost.
+# The filter is only honoured for a configured remote — with a bare URL git
+# accepts the flag and silently fetches everything — hence "origin", which
+# actions/checkout configures as the target repo's HTTPS URL.
+#
+# Fetching by remote name also widens the ref corpus: actions/checkout leaves
+# the default wildcard refspec, so this pulls every branch and tag rather than
+# the single ref the old bare-URL form fetched. That is deliberate — Tier 2's
+# revert-frequency signal reads `git log --all` — and cheap, since trees and
+# commits are all that come down.
 # ---------------------------------------------------------------------------
 if [[ -z "${REVIEW_GIT_FETCH_DEPTH+set}" && "${REVIEW_RISK_ASSESSMENT_ENABLED:-false}" == "true" ]]; then
   REVIEW_GIT_FETCH_DEPTH="0"
@@ -505,15 +517,59 @@ if [[ "${REVIEW_GIT_FETCH_DEPTH:-}" == "0" ]]; then
     echo "::warning::Clone-deepening skipped — target directory '${_TARGET_DIR}' not found"
   elif git -C "${_TARGET_DIR}" rev-parse --is-shallow-repository 2>/dev/null | grep -q true; then
     echo "Deepening shallow clone for git history analysis..."
-    if [[ "${FULLSEND_FORGE}" == "github" && -n "${GH_TOKEN:-}" && -n "${REPO_FULL_NAME:-}" ]]; then
-      git -C "${_TARGET_DIR}" \
-        -c "http.extraheader=Authorization: basic $(printf 'x-access-token:%s' "${GH_TOKEN}" | base64 -w0)" \
-        fetch --unshallow "https://github.com/${REPO_FULL_NAME}.git" 2>/dev/null \
-        && echo "Clone deepened successfully" \
-        || echo "::warning::Failed to deepen clone — Tier 2 risk signals may be degraded"
-    else
+    # Scope the credential to origin's own host so it cannot be sent anywhere
+    # else. The destination is now config-derived (origin) rather than a
+    # literal URL, so without this the header would follow whatever
+    # remote.origin.url happens to hold. Non-https remotes (the test fixtures
+    # use file://) fall back to an unscoped header, which they ignore anyway.
+    _ORIGIN_SCOPE="$(git -C "${_TARGET_DIR}" remote get-url origin 2>/dev/null \
+      | sed -nE 's#^(https://[^/]+/).*#\1.#p')"
+    _DEEPEN_ERR="$(mktemp)"
+    if [[ "${FULLSEND_FORGE}" != "github" || -z "${GH_TOKEN:-}" || -z "${REPO_FULL_NAME:-}" ]]; then
       echo "::warning::Cannot deepen clone — missing credentials or unsupported forge"
+    elif ! command -v timeout >/dev/null 2>&1; then
+      # Local runs on macOS need coreutils for GNU timeout; without a bound
+      # this fetch is exactly the unbounded cost #1032 was filed about.
+      echo "::warning::Cannot deepen clone — 'timeout' not found (install coreutils); Tier 2 risk signals may be degraded"
+    elif timeout --kill-after=5 120 git -C "${_TARGET_DIR}" \
+        -c "http.${_ORIGIN_SCOPE}extraheader=Authorization: basic $(printf 'x-access-token:%s' "${GH_TOKEN}" | base64 -w0)" \
+        fetch --unshallow --filter=blob:none origin 2>"${_DEEPEN_ERR}"; then
+      # Rename detection is the one thing in the documented Tier 2 command
+      # set that reads blob content, so turn it off repo-locally: every Tier
+      # 2 command is then blob-free, and the coupling signal is unaffected
+      # (a rename reported as delete+add still names both paths). The
+      # promisor containment below is the backstop for anything else.
+      #
+      # Guarded: under `set -e` a failed config write would abort the whole
+      # pre-script, turning a degraded risk signal into a failed review.
+      if git -C "${_TARGET_DIR}" config diff.renames false; then
+        echo "Clone deepened successfully (blobless; rename detection disabled)"
+      else
+        echo "::warning::Deepened clone but could not disable rename detection — Tier 2 history commands may block on unavailable blobs"
+      fi
+    else
+      # A killed or failed fetch leaves the clone shallow, which the Tier 2
+      # sub-agent detects on its own and treats as an unavailable tier.
+      # Surface why: exit 124 (timeout), auth failure and a server that
+      # refuses --filter are all actionable, and all look alike without this.
+      echo "::warning::Failed to deepen clone — Tier 2 risk signals may be degraded"
+      sed -n '1,5p' "${_DEEPEN_ERR}" 2>/dev/null || true
     fi
+    rm -f "${_DEEPEN_ERR}"
+
+    # Containment. The fetch registers origin as a promisor remote, so git
+    # would try to lazily fetch any object it is missing. The sandbox cannot
+    # serve that fetch — its egress policy allows the GitHub REST API, not
+    # the git wire protocol — so the attempt blocks until the run's timeout.
+    # Dropping the registration turns that hang into an immediate "unable to
+    # read object", which is the honest answer and costs no budget.
+    #
+    # Both keys are required: partialclonefilter on its own re-creates the
+    # promisor remote. Unconditional, because a fetch that fails part-way
+    # still registers them — the keys are written during fetch setup, before
+    # any transport runs.
+    git -C "${_TARGET_DIR}" config --unset remote.origin.promisor || true
+    git -C "${_TARGET_DIR}" config --unset remote.origin.partialclonefilter || true
   fi
 fi