Conversation
A git or svn diff that exceeded CRG_GIT_TIMEOUT was caught by a broad OSError/SubprocessError handler and returned an empty dict, indistinguishable from a genuine no-change diff. The process still exited 0, so an MCP caller received a well-formed zero-change review with no indication the underlying command never completed (tirth8205#913). subprocess.TimeoutExpired is now caught separately and raises GitTimeoutError, a RuntimeError carrying the command name and timeout value. Callers can distinguish an actual timeout from a legitimate empty diff and surface it.
code-review-graph reviewOverall risk: 0.40 (MEDIUM) — 12 changed function(s)/class(es), 0 affected flow(s), 3 test gap(s) Risk-scored changes
Test gaps
Token savings: this graph-backed report used ~3,138 fewer tokens (~55%) than reading every changed file in full (estimated, chars/4 approximation). Powered by code-review-graph — local-first analysis; no code leaves the CI runner. |
|
The premise is right — a timeout that reads as "no changes" is the worst possible failure for a review tool, which is why I filed #913. But this doesn't fix the path the issue describes, and it breaks one that works today. I'd like it reworked rather than merged. It doesn't fix the reported symptom. #913's complaint is a well-formed response saying zero changed functions, zero flows, risk 0.00. On this branch, with a forced timeout: The reason is upstream of your change: except (FileNotFoundError, subprocess.TimeoutExpired):
return []— a silent It converts a designed degradation into a crash.
What I'd take instead:
Two smaller things for the rework: One warning about the interaction with #921: that PR accepts Gates are green on the branch (2991 passed, ruff and mypy clean, 13/13) — all six new tests patch |
|
Changes required: the false-clean response remains at changed-file discovery, before the modified range parser is reached. On a built Git graph, make |
|
Checks pass and the code looks right. Missing: tests for the edge cases it introduces.
Every new branch in the code should have a test, including the failure branch. Disagree with any of these? Say so here first. PRs now target |
) Three of the four causes in the tirth8205#262 diagnosis, measured rather than assumed. Cause 4 (a timed-out `git diff` returning an empty list, tirth8205#913) is deliberately untouched: PR tirth8205#922 is open for it. A. One offload helper, not seven copies. `_offload` in main.py runs a tool body on a worker thread and, when `CRG_TOOL_TIMEOUT` is set above 0, bounds it. 28 of the 30 registered tools now route through it; only `get_docs_section_tool` and `list_repos_tool` stay inline, and each reads one small file. `detect_changes_tool` loses its private copy of the timeout logic and keeps its exact observable behaviour, including the advice naming `CRG_MAX_CHANGED_FUNCS` / `CRG_MAX_TRANSITIVE_FRONTIER`. Measured against origin/staging, driving a real `code-review-graph serve` over stdio: the loop is NOT blocked on staging today, because fastmcp 3 runs sync tool bodies in a worker thread itself (`run_in_thread=True`). A negative-control server with `run_in_thread=False` shows the probe can detect blocking, so that result is the instrument working, not failing. What staging does not do is bound the call: with `CRG_TOOL_TIMEOUT=5`, `get_impact_radius_tool` ran 25.2s and answered only when it was ready; it now answers at 6.0s with `status: error` naming the tool and the budget. An unbounded request is what a client reports as -32001. B. Change discovery gets its own budget. `CRG_DISCOVERY_TIMEOUT` (default 5s, never above `CRG_GIT_TIMEOUT`, read at call time rather than frozen at import) applies only to the read-only discovery chain. `CRG_GIT_TIMEOUT` keeps its 30-second default for build, update and watch, which legitimately run long. The two byte-identical `_GIT_TIMEOUT` definitions, at changes.py:35 and incremental.py:731, are now one constant in constants.py that both modules alias. `discover_review_changes` is that chain: resolve the base, diff, fall back to the working tree. review.py (3 sites), query.py and cli.py (2 sites) call it instead of repeating the three steps, so the budget and the scope below cannot be applied to five of six call sites. C. The working-tree fallback stops walking every untracked file. `get_staged_and_unstaged` takes `untracked=`, defaulting to `"all"`, so no existing caller changes. Discovery passes `"normal"`: a new module in a tracked package is still named, a wholly-untracked directory is not descended into, and the unusable `dir/` placeholder is dropped rather than handed on as a file path. Verified first that no caller needs `"all"`: all six are the discovery fallback, and `incremental_update` never calls it at all -- on origin/staging as on this branch, `update` does not pick up a never-committed module, so scoping this walk cannot regress it. A test pins that. On a repository with an empty `git diff HEAD~1` and 250,000 untracked files, `get_impact_radius_tool` goes from 16.8s -- reporting all 250,000 as "changed" -- to 0.02s and "No changed files detected". The discovery chain alone: 0.397s to 0.016s, the walk 0.377s to 0.006s. That is macOS with a warm page cache; the report is from Windows, where each of those stats costs far more. D. Tests. test_main.py: the heavy-tool guard covers all 28 offloaded tools and checks delegation plus one threading implementation; a new guard fails when a tool is added as a plain `def`; the loop-responsiveness test drives a real event loop rather than asserting `iscoroutinefunction`, and fails when `_offload` is mutated to run inline. test_incremental.py: the budget's precedence, call-time reads, invalid values, the chain's wiring, and that build-side callers keep the git budget. test_integration_git.py: the three untracked modes against real git. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merges origin/staging (PR tirth8205#1029, which this branch predated and no longer applied over), then fixes what three reviewers found. Reproduced each one first; each fix is verified with the reporter's own reproducer. C is withdrawn, not repaired. `get_staged_and_unstaged(untracked=)` and the `--untracked-files=normal` discovery walk are gone; the walk is back to `all`. Under `normal`, git collapses a wholly-untracked directory to one `dir/` record, which is not a path any caller can open, and the code dropped it -- so the first commit of any new package vanished from all four review tools and both CLI review commands, with `status: ok` and no warning at any log level. Reproduced: a repo whose only change is `feature/newmod/{a.py,b.py}` reported "No changes detected"; it now reports both files. The measured prize was 0.377s -> 0.006s of git time on 250,000 untracked files. Being slow is a bug; confidently reviewing nothing is a worse one, and the task's own instruction was to say so rather than trade one for the other. The 5s budget stays, because it no longer lies. staging landed `ChangeDiscoveryError` and `require_vcs` while this branch was out. `discover_review_changes` now passes `require_vcs=True` to all three steps, so exhausting the short budget raises instead of returning `[]`. That is what licenses shortening it at all: the reviewers were right that cutting 30s -> 5s on a path whose timeout returns an empty list makes tirth8205#913's false all-clear several times easier to hit, and extends it to `resolve_review_base`, where a timed-out merge base silently degrades a three-dot diff to two-dot. tirth8205#913 itself is still untouched -- `get_changed_files`'s default is unchanged and `require_vcs` is staging's own flag, so PR tirth8205#922 keeps its ground. Verified with a `git` shim that sleeps 7s: before, `detect_changes_tool` answered `status: ok` with `changed_files: ["scratch_note.txt"]` -- an unrelated file. Now all four tools answer `status: error` naming the budget and `CRG_DISCOVERY_TIMEOUT`, and `detect-changes` exits 1 rather than 0. `discovery_timeout()` no longer clamps to `min(5, GIT_TIMEOUT)`: an explicitly set `CRG_GIT_TIMEOUT` is honoured verbatim. It predates this variable and is what tirth8205#262's reporters were already told to raise; a documented knob that silently stops working is worse than one that never existed. `CRG_GIT_TIMEOUT=120` with the same 7s git returns the correct file list again. `CRG_TOOL_TIMEOUT` keeps its scope and its guard. `_offload` gained `bounded=`, and the five writing tools pass False: build, postprocess, embed, wiki and apply_refactor. `asyncio.wait_for` cancels the await, never the worker, so bounding these reported failure to the client while the build kept writing graph.db (verified: the "failed" run left 52,000 nodes behind) and while apply_refactor's rename landed on disk, whose retry then failed again with "not found or expired". They were also unbounded before the shared helper existed, and `CRG_TOOL_TIMEOUT` is what tirth8205#262 tells a user to set -- the documented remedy must not break builds. Parsing goes through staging's `env_int`, so `CRG_TOOL_TIMEOUT=2.5` (or "", or "abc") no longer raises `ValueError` out of every tool. This also satisfies staging's `test_no_new_unguarded_numeric_env_parse` gate, which the pre-merge branch failed in both directions. The offload runs on anyio's limiter, not asyncio's executor. `anyio.to_thread.run_sync(abandon_on_cancel=True)` replaces `asyncio.to_thread`. Making these tools `async def` had moved them off the 40-slot limiter FastMCP dispatches sync bodies through and onto asyncio's default executor, capped at `min(32, cpu+4)` -- 18 here, 8 on a 4-core Windows box. At the reviewer's N=19 burst a cheap tool waited 10.15s and got a false "timed out"; it now answers in 0.01s, and still does at N=30. anyio is declared in pyproject rather than borrowed from fastmcp. `get_minimal_context_tool` is on the budget too. The entry point CLAUDE.md tells agents to call first was the one tool the previous commit skipped: `resolve_review_base` (30s) + two hardcoded 10s probes + `get_changed_files` (30s). One `discover_review_changes` call replaces all five subprocesses. Measured with a 6s git: 31.2s -> 6.0s. A discovery failure appends a "Degraded:" note, following the churn note's existing contract, rather than reporting risk it could not compute. Also: the timeout hint no longer names `changed_files` / `max_depth` / `max_results` as though every bounded tool accepted them; `_vcs_unavailable` names the budget that actually expired and the knob that governs it; and the CHANGELOG and all five README env-var tables say which tools `CRG_TOOL_TIMEOUT` bounds and that a discovery timeout is an error rather than an all-clear. Tests: the two that pinned the collapsed-directory drop are replaced by ones that pin the opposite, including an end-to-end check that a new package reaches all four review tools against real git (verified to fail when the drop is reintroduced). New coverage for per-step require_vcs, the budget precedence, the unbounded writing tools, malformed CRG_TOOL_TIMEOUT, and the anyio limiter. Gates: 4163 passed, 848 skipped, 2 xfailed, 2 xpassed (both xpassed are pre-existing R-notebook tests). ruff and mypy clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This no longer merges into Worth knowing before you resolve:
git fetch origin
git merge origin/staging
# resolve, then
uv run pytest tests/ -q
uv run ruff check code_review_graph/
uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional
git pushI have not reviewed the change itself yet. That comes once it merges and the checks run against the merged state, since staging has moved a long way and the result is what matters. |
This branch has not been deployed
2 participants
Summary
A git or svn diff that exceeded
CRG_GIT_TIMEOUTwas caught by a broadOSError/SubprocessErrorhandler and returned an empty dict — indistinguishable from a genuine no-change diff. The process still exited 0, so an MCP caller received a well-formed zero-change review with no indication the underlying command never completed (#913).subprocess.TimeoutExpiredis now caught separately at both the git diff and svn diff call sites and raisesGitTimeoutError, aRuntimeErrorsubclass carrying:command— which VCS command timed out ("git diff"or"svn diff")timeout— the configuredCRG_GIT_TIMEOUTvalueNon-timeout
OSErrorandSubprocessErrorcases still return{}as before — the distinction is only for the timeout path where silence is most dangerous.Testing
New
tests/test_git_timeout.py(6 tests):GitTimeoutErroris aRuntimeError; its message names the command and timeout; attributes are accessible.parse_git_diff_rangesraisesGitTimeoutErrorwhensubprocess.runraisesTimeoutExpired(mocked).SubprocessErrorandOSErrorstill return{}(no behavior change for those paths).Fixes #913