Repository navigation
Conversation
Three of the four causes in the #262 diagnosis, measured rather than assumed. Cause 4 (a timed-out `git diff` returning an empty list, #913) is deliberately untouched: PR #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>
`staging` reaching `testing` was a manual click every day for a change that is, by then, already green: ci.yml runs on push, so the eight required contexts are satisfied on the `staging` tip before anyone opens the promotion PR. This automates that one step and nothing else. `.github/workflows/auto-promote.yml` runs on a daily schedule and on workflow_dispatch. When `staging` has commits `testing` lacks and every required check is green on its tip, it opens or updates the promotion PR and merges it with a merge commit, so contributor authorship on every promoted commit survives. A hand-started run defaults to a dry run. Every rule lives in `scripts/auto_promote.py`, which is importable and tested with no network; the YAML fetches facts and obeys, matching the shape of scripts/promotion_gate.py. Two decisions worth naming: * The required contexts are read from the `testing` ruleset at run time, through the branch-rules endpoint that needs only repository read access. Hardcoding eight strings would let a renamed CI job drop out of the gate silently. * A required context that was skipped or neutral is not green here, even though GitHub's own evaluation accepts both. The gate may refuse a promotion GitHub would allow; it can never allow one GitHub refuses. Every guard exits cleanly with a notice: nothing to promote, CI still running, a failed or skipped required check, a conflicting or blocked promotion PR. Those states are reported elsewhere already and tomorrow's run looks again. Only two outcomes turn the run red, both of them things nothing else reports: * GitHub refused the merge. The PR is left open and the refusal quoted in the job summary. The likeliest cause is the ruleset's require_extra_approval_for_unattributed_changes, which GITHUB_TOKEN cannot satisfy because a bot may not approve. * The follow-up checks could not be started. A push made with GITHUB_TOKEN starts no `push` workflow run, so this merge does not run ci.yml or the promotion gate on `testing` the way a hand merge does. Left alone the `testing` tip would carry no required contexts at all: the next manual `testing` -> `main` PR would be unmergeable for ever and the release gate would quietly stop running on every landing. The last step starts both through workflow_dispatch, the documented exception to that rule. Neither of those files is modified. A summary is written on every run, including the runs that do nothing, so "nothing to promote" is never indistinguishable from "did not run". Promotion to `main` stays entirely manual. The workflow has no input, variable or code path that can target it, and tests/test_auto_promote.py asserts that against the script, its command line and the YAML. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three reviews of the previous commit found the automation trusted three things it had no right to trust, and asserted a fourth that is not true. **It merged whatever `gh pr list --head staging` returned.** That filter matches a branch called `staging` in any repository -- `gh pr list --help` says outright that `<owner>:<branch>` syntax is not supported -- and this repository is public with 2,871 forks. An outsider opening `theirfork:staging -> testing` became the PR the daily job resumed, had its body rewritten, and was merged: `testing`'s ruleset requires zero approvals and their own `pull_request` run satisfies the eight contexts. The same defect had a no-effort denial-of-service form, a merely conflicting fork PR making every run decide BLOCKED and exit green for ever. **It merged a PR number, and a PR's base branch is mutable by its author.** Repointing the base at `main` starts no `pull_request` run, so the green checks stay put, and `main`'s ruleset is identical to `testing`'s. Every "this can never target main" control -- the env vars, `assert_safe_targets`, the YAML test -- constrained what the job asks for, none constrained what the PR it merges is. Selection now requires the PR to be same-repository, aimed exactly `staging -> testing`, and to carry a new `auto-promotion` label, and `verify` re-reads all of it from the API seconds before the merge. The label also fixes a third case: a promotion PR the maintainer opened by hand and deliberately did not merge used to be indistinguishable, so it was silently merged with its body overwritten first. It is now refused by number, with the reason. **The commit whose checks were read was not the commit that got merged.** The PR head tracks a branch, so a push mid-run moved it. `headRefOid` was fetched and never read. It is compared now, and `gh pr merge` passes `--match-head-commit`, which GitHub enforces. **The premise that a GITHUB_TOKEN PR triggers no `pull_request` run is false.** PR #991, opened by app/github-actions, produced a ci.yml run three seconds later; PR #1040 re-queued all eight already-green contexts on the same SHA and sat BLOCKED for fourteen minutes. The merge step waited sixty seconds and polled `mergeable`, which is only the conflict computation and says MERGEABLE throughout. It now polls `mergeStateStatus` for up to thirty minutes. Every refusal also used to blame the ruleset's require_extra_approval_for_unattributed_changes; that rule has never fired here -- #991 merged under it with zero reviews -- so the refusal quotes GitHub and lists causes in the order they actually occur. Also: the PR number is read from the URL `gh pr create` prints, because re-listing raced GitHub's index and an empty answer skipped the merge, the refusal report and everything else while the run stayed green and the summary said "Promoting"; a failed create names the repository setting that causes it, *Allow GitHub Actions to create and approve pull requests*, which is off today and has already failed the manual Promote workflow; a run stuck on its own PR is red rather than a green warning every morning; a `testing` tip the release gate never ran on is repaired by the next run; a failed gate on `testing` blocks tomorrow's promotion, which is what replaces the maintainer's per-landing decision; and ci.yml is no longer dispatched -- those contexts are satisfied by the manual `testing -> main` PR's own run, and dispatching it switched on a manual-only 45-minute job that is not a required context. uv run pytest tests/ -q: 4171 passed, 842 skipped. ruff and mypy clean. actionlint clean on the workflow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merges origin/staging (PR #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 #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. #913 itself is still untouched -- `get_changed_files`'s default is unchanged and `require_vcs` is staging's own flag, so PR #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 #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 #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>
Records the merge whose resolution the previous commit already carried: a `git stash` run mid-merge had cleared MERGE_HEAD, so that commit landed with one parent and git would have re-conflicted on the next merge. Every conflicted file keeps the resolution verified there; origin/staging's only new content since is tests/determinism_dump.py and tests/test_determinism.py, which merged cleanly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix(mcp): bound every tool call and stop handing the whole working tree to review
Both changelog entries kept: the discovery budget from #262 and the daily promotion. No other file collided. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ci: promote staging to testing automatically, once a day
code-review-graph reviewOverall risk: 0.80 (HIGH) — 248 changed function(s)/class(es), 38 affected flow(s), 98 test gap(s) Risk-scored changes
Affected execution flows
Test gaps
Token savings: this graph-backed report used ~546,337 fewer tokens (~89%) 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. |
pull Bot
pushed a commit
to bhardwajRahul/code-review-graph
that referenced
this pull request
Sep 18, 2026
The Action's gate on promotion PR tirth8205#1047 named _offload, _run_off_loop and 71 other symbols as test gaps while the suite executed every one of them. TESTED_BY is a direct edge from a test to a production symbol, so a private helper that tests reach only by calling the public function above it carries no such edge and was reported as untested. changes.py now classifies each changed symbol in one batched pass before scoring: direct TESTED_BY, else a two-hop upward walk over incoming CALLS edges looking for a tested caller, else nothing. The walk is a new GraphStore.get_caller_test_routes -- the opposite direction from get_transitive_tests, which asks whether a symbol's callees have tests. Indirect coverage is a weaker class, never a suppression. Every gap stays in the report; the indirect ones carry coverage/covered_via/covered_depth/ covered_by, sort after the unreached ones so truncation keeps the real gaps, and get their own heading in the PR comment with a "indirect" Tested column. That distinction is load-bearing on this very delta: incremental.py:: _get_svn_changed_files executes 0 of its 29 statements yet sits one hop under two well-tested callers, so a rule that dropped reachable symbols would have hidden the delta's only genuine gap while fixing six false alarms. Measurements on the 0af6b8b..ca23a73 delta the Action scored (248 changed symbols, 74 gaps on a clean full build): - 74 gaps before, 74 after -- identical set, nothing silenced. - 5 reclassified as reached-through-a-caller: cli.py::_dispatch, incremental.py::_vcs_unavailable, incremental.py::_get_svn_changed_files, main.py::_offload (1 hop), main.py::_run_off_loop (2 hops). - Hop limit 2 is measured, not guessed. Over the 74 gaps the rescue counts are {1: 4, 2: 1}; depths 3, 4 and 5 add exactly zero. Over all 1539 package symbols each further hop re-labels ~8% more of the codebase as covered (27.0% direct, 49.4% at 1, 59.5% at 2, 67.6%, 76.3%, 81.1%). - Risk: an indirectly reached symbol with no direct tests gets a flat 0.03 off the 0.30 untested term -- strictly less than the 0.05 one direct test earns, and never stacked on a symbol that already scored coverage. Two scores moved on the delta (0.65 -> 0.62); overall risk stays 0.80. - Cost: the classification got cheaper. Batching the per-symbol direct check into one query drops it from 5.6 ms to 0.2 ms over 248 symbols, and the two-hop walk over the 227 uncovered ones costs 0.6 ms, so the step goes 5.6 ms -> 0.8 ms. End-to-end analyze_changes is unchanged at 0.25 s. The report payload grows 0.23%. Refs tirth8205#1047, crg-2g8 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pull Bot
pushed a commit
to bhardwajRahul/code-review-graph
that referenced
this pull request
Sep 18, 2026
The first cut of this fix classified a test gap as "indirect" when a tested
caller reached it, then treated that class as weaker evidence of coverage: a
0.03 risk discount, sorted last in the gap list, and copy saying the caller's
test "is execution, not an assertion". Three reviewers showed each of those
three is wrong, and the worst case is the same symbol in all three.
incremental.py::_get_svn_changed_files runs 0 of its 29 statements under the
full suite. It sits one hop under get_changed_files -- the VCS dispatcher's
SVN branch, which no git-repo test takes. Classifying it "indirect" moved it
from position 3 of 74 to 72 of 74 (past detect_changes_tool's default
max_results=25, so gone from the headline MCP tool), dropped its risk 0.65 ->
0.62, and that 0.03 evicted it from the top-10 review_priorities the Action
renders as its whole risk table. The delta's one genuine gap was demoted out
of every surface a maintainer reads, which is the "fix that silences real
gaps" the work was told not to ship.
The premise was the error. Measured against line coverage over this repo's own
4,286-test suite, 32 of the 504 symbols the walk reaches are never executed --
branch-dispatch bodies like parser.py::_parse_svelte, certified via
parse_bytes while "svelte" appears nowhere in tests/. Four rules were tried to
separate them: depth 1, caller fan-out limits from 5 to 50, a same-file
restriction, and combinations. None moves the never-executed share below ~5%
(from 6.5%) and each halves what the walk reaches -- fan-out<=15 drops 494
rescues to 268. The discriminating fact is which branch a test takes, and the
call graph does not record it. So reachability is not scorable, at any tuning.
It is still useful, as a pointer to where a test already runs. That is what it
now reports, and nothing more:
- No risk discount. An indirectly reached symbol scores exactly as untested.
0.03 was also off the 0.05 grid every other term uses, and scores quantize
hard (15 distinct values over 49 symbols, 25 tied at 0.05), so it could
never leave a symbol tied -- it dropped it below its whole tie class.
- Class-blind gap order. Sorting reached ones last made them truncation's
first casualty: at max_results=25 a delta reported 9 indirect gaps and
shipped 0, and a consumer deriving its own Tested column from the
truncated list then labelled those symbols "yes".
- Honest copy. "no test in reach" -> "no tested caller found"; "That is
execution" -> "a path in the call graph, not a record of execution ...
these count as gaps and are scored as untested". On this very delta
auto_promote.py::cmd_verify and ::ForeignPullRequestError are exercised by
tests that load the script through importlib, so no edge records it; the
weaker claim is true of them, the stronger one was not.
Also fixed in the same surfaces:
- render_pr_comment took its headline total from len(test_gaps) and its
split from the untruncated counts, printing "25 test gap(s) (73 ..., 9
...)". Both now come from test_gaps_total.
- A truncated table no longer renders "yes" for a symbol whose gap row was
cut; it renders "?".
- A truncated body that loses every indirect row now says so instead of
letting the section vanish under a headline promising it.
- get_review_context and its "lack test coverage" line used direct edges
only, contradicting detect_changes about the same symbols in the same
session (CLAUDE.md sends reviewers through both). It now splits the same
way, with the same wording.
- The walk credited coverage through callers inside test files, whose
TESTED_BY edges point at tests in their own file -- 20% of this graph's
TESTED_BY edges start at a test-file symbol. Filtered; costs 10 of 504.
- The 2000-caller cap was a budget shared across the change set and applied
after materialising every caller, so one hub in a PR erased the routes of
every other changed symbol (206 of them on a real graph) while making the
walk 3x slower. Replaced by a per-node limit of 500 applied before the
edges are read: hub cost 365ms -> 2ms, cross-contamination 206 -> 0.
Results on the 0af6b8b..ca23a73 delta, scored from a clean tree so all three
runs see the identical diff (248 changed symbols, 74 gaps):
gaps _get_svn_changed_files in 25-window risk top-10
staging 74 pos 3 yes 0.65 yes
first cut 74 pos 72 no 0.62 no
this commit 74 pos 3 yes 0.65 yes
The gap set is byte-identical to staging's in all three; nothing is silenced.
_offload (1 hop) and _run_off_loop (2 hops) are classified indirect as the
issue asked. cmd_verify and ForeignPullRequestError stay unreached -- correct,
since importlib leaves no edge -- and are now described in terms that are true
of them.
Of the 74 gaps, 35 are in code_review_graph/ and measurable against line
coverage: 34 are executed by the suite, 1 (_get_svn_changed_files) never is.
The walk recovers 5 of those 34; the other 29 are executed through fixtures,
CLI entry points and subprocesses that leave no edge, which no hop limit
reaches. The remaining 39 gaps are in scripts/, outside --cov scope.
Cost: analyze_changes 286ms on staging, 283ms here. get_caller_test_routes
1.0ms over 74 seeds. Report payload +0.15% vs staging (was +0.23%); indirect
entries shrank 32% (1492 -> 1012 chars) by naming one crediting test, and all
13 now survive the default 25-row cut instead of none.
Gates: 4286 passed, 844 skipped, 2 xfailed, 2 xpassed. ruff clean. mypy clean
but for one pre-existing google-genai stub error in embeddings.py, present on
staging and untouched here.
Refs tirth8205#1047, crg-2g8
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Linked issue
Closes #
What & why
How it was tested
Checklist
uv run pytest tests/ --tb=short -quv run ruff check code_review_graph/uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optionaldocs/, docstrings)