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
Bound tool calls and optimize change discovery process
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. |
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
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)