Repository navigation
fix: report what is wrong instead of a false all-clear or a traceback - #1029
Merged
Merged
Conversation
Brings in tests/test_cli_surface.py from test/cli-and-failure-modes (PR #1018) unchanged, with the pytest marker and conftest gate it needs. It enumerates the command tree from the live argparse parser and the environment variables from the package source, so neither list can drift away from the code, and it records the failure modes that do not yet meet the house style as strict xfails. The fixes in the following commits flip those markers off. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five failures where the tool lied about its own state or crashed rather
than saying what it could not do. All five are recorded as strict xfails in
tests/test_cli_surface.py; every marker is flipped off here.
detect-changes reported a clean tree it never looked at. With no git on
PATH, or a `git diff` that timed out, every discovery call returned an
empty list and the command printed "No changes detected." and exited 0
while the working tree had changed. A CI review gate keyed on that exit
code passed pull requests nobody reviewed.
Error: could not determine the changes: git could not be run
([Errno 2] No such file or directory: 'git'). Install git and make
sure it is on PATH.
get_changed_files, get_staged_and_unstaged and the diff-range parsers take
require_vcs: a VCS that cannot be run at all raises ChangeDiscoveryError
instead of returning empty. A base ref that simply does not resolve keeps
the documented --cached fallback, so a repository with no commits still
works. build and update stay lenient on purpose: they recover by
re-parsing the tree and reconciling by content hash, which is not a lie.
Every caller propagates the distinction rather than flattening it. The CLI
turns it into one Error line and exit 1; detect_changes_tool returns
{"status": "error"} instead of its "No changed files detected" all-clear;
render_pr_comment.py now recognises only detect-changes' own
"No changes detected." line as a clean tree and exits 4 for anything else,
and the composite action posts a comment saying the pull request was not
analyzed and fails the job. Exit 4 is outside fail-on-risk: an unknown
risk is not a low one. When only the line-level diff is unreadable while
the file list is not, analyze_changes degrades to whole-file scoring and
records it, rather than failing or pretending to be line-precise.
A corrupt graph.db tracebacked out of GraphStore.__init__ on every
command. Four kinds of damage now report one actionable line: garbage
bytes and a truncated file (SQLite's own read errors), a valid SQLite file
belonging to something else (CREATE TABLE IF NOT EXISTS used to graft our
schema onto it), and a database from a newer schema version
(run_migrations is a no-op above LATEST_VERSION, so the mismatch only
surfaced later as a missing column).
An unwritable data directory did the same. It is told apart from a corrupt
file and gets the opposite advice:
Error: cannot open the graph database for writing at <path> (attempt
to write a readonly database). Check the permissions on <dir>, or set
CRG_DATA_DIR to a writable directory.
A graph.db built for another repository was accepted in silence, so
dead-code in one repository listed the other's symbols with the other's
absolute paths and exited 0. The root check from #909 only ever ran on the
write side, during incremental reconciliation, where a wrong root would
purge files; nothing asked on the read side. assert_graph_serves_root asks
at every store open, in the CLI and in the MCP tools' _get_store. It is
narrower than the write-side check because refusing to answer is not
destructive: it fires only when no file marker lies under the root, and
lets through a graph with no markers, a graph of relative paths, an
equivalent spelling of the same root (/var against /private/var, a
symlinked checkout), and a graph whose files are not on this machine.
cli.main now wraps the dispatch and reports any CodeReviewGraphError in
the house style already set by update and watch: one `Error: ...` line on
stderr, exit 1. Anything else really is a bug and keeps its traceback.
Two existing tests pinned the old call signature of get_changed_files and
are updated for require_vcs=True. tests/test_failure_reporting.py and
tests/test_env_config.py carry the same contracts in the ordinary suite,
which does not collect the opt-in cli_surface gate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six files collided. Resolutions: pyproject.toml: the pytest markers list. Union of both sides; the cli_surface entry was identical on each. tests/conftest.py: three pytest_collection_modifyitems definitions would have met here (staging already carried two, so its own packaging gate was dead: pytest resolves the hook by name and only the last definition runs). Folded into staging's single _OPT_IN_MARKERS hook, with packaging added to the set and its CRG_RUN_PACKAGING_TESTS escape hatch preserved as a per-marker override. tests/test_conftest_gates.py now fails if a second hook is ever added. code_review_graph/changes.py: both sides kept. The require_vcs path and the diff_ranges_unavailable summary line sit alongside staging's churn cache, churn_status and is_test_file-based test-gap detection. Staging's two new numeric settings (CRG_CHURN_TIMEOUT, CRG_CHURN_MAX_COMMITS) were bare int()/float() of os.environ.get, which is exactly what #912 removed, so they now read through constants.env_float / env_int and are covered by the numeric env var table in tests/test_cli_surface.py. code_review_graph/graph.py: both sides kept. The GraphStore.__init__ guard (_assert_usable_database plus the GraphStoreError wrapper) wraps staging's schema, which now reaches v13; `import os` is gone because env_int replaced its only use, and `import re` came in for staging's identifier tokenizer. code_review_graph/cli.py: staging split main() into build_parser() + main(); this branch split it into main() + _run(). Merged as build_parser() + main() (the CodeReviewGraphError reporter) + _run() (the dispatch body), so the parser is still importable for tests and every self-explaining failure still becomes one Error: line and exit 1. tests/test_cli_surface.py: add/add. Staging's copy is byte-identical to this branch's own first commit, so this branch's later version supersedes it. All five fixes re-probed end to end against the merged tree: absent git and CRG_GIT_TIMEOUT=0 both exit 1 with a named cause instead of "No changes detected"; four kinds of unusable graph.db each report one line; an unwritable data directory reports one line for status, detect-changes, wiki and build; eight numeric variables set to "" and "abc" exit 0 with a warning naming the variable; a donor repository's graph.db is refused by both the CLI and _get_store. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
code-review-graph reviewOverall risk: 0.55 (MEDIUM) — 116 changed function(s)/class(es), 5 affected flow(s), 34 test gap(s) Risk-scored changes
Affected execution flows
Test gaps
Token savings: this graph-backed report used ~231,713 fewer tokens (~91%) 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. |
Both sides wrapped the CLI's dispatch function to report a class of failure as one line instead of a traceback, and both named their wrapper main(). cli.py: one main() catching both. A CodeReviewGraphError is printed in the house style (one Error line, exit 1); lock contention keeps its own message and its own advice. The dispatch function keeps staging's _dispatch name, so the branch's _run is gone and its tests follow. graph.py: the two failures meet inside GraphStore.__init__, where the branch turns any sqlite3.Error into a GraphStoreError. run_migrations re-raises a held write lock once its retries are spent, so a contended open was being described as an unreadable database and answered with "delete it and rebuild". Lock errors are now re-raised untouched and reach main()'s contention report. The classifier moved to errors.py so cli.py and graph.py cannot drift apart on what counts as contention. incremental.py: the import conflict keeps both sides. Staging's new CRG_MAX_UNWATCHED_TRACKED was parsed with a bare int(os.environ.get(...)), which is the import-time crash the branch removed from 22 other settings; it reads through env_int now and joins the coverage table. Also fixed while probing the branch's five cases: a data directory that cannot be created still tracebacked out of pathlib.mkdir, because get_data_dir creates it while the path is being resolved, before any GraphStore exists. It is now the same one-line GraphStoreError as a directory that cannot be written to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both sides handled a corrupt graph.db and they had to become one thing. This branch turned every failure the tool understands about itself into a single `Error: ...` line: a corrupt or foreign database, a data directory it may not write to, a graph built for another repository root, a git it could not run, and 22 numeric environment variables that used to raise at import. Staging added `_open_graph_store`, which lets `build` discard a database it cannot open and rebuild it, so a restored CI cache stops failing every run. Resolution: * `CorruptGraphDatabaseError` now subclasses `GraphStoreError` instead of `sqlite3.DatabaseError`. It keeps staging's job (naming the one unusable shape a rebuild can fix by itself) and gains this branch's contract (nothing the tool can explain reaches a caller as a raw SQLite error). Its message is the house line, so `main` reports it and no second message in `_open_graph_store` can drift from it. * `GraphStore.__init__` keeps this branch's structure (mkdir inside the try, the connection closed on every failure path, `_assert_usable_database` refusing a foreign file or a newer schema before any DDL runs) and gains staging's classification step, in an order that protects a healthy graph: an already-classified `GraphStoreError` first, contention second, corruption only then, and the described failure last. * A foreign SQLite file and a newer-schema graph stay plain `GraphStoreError`s, so `build` never deletes somebody else's data or a graph this build is merely too old to read. `_assert_usable_database` catches the foreign file before staging's classifiers see it; a readable graph.db whose own tables are the wrong shape still reaches them and is still recovered. * cli.py drops staging's second copy of the lock classifier; `is_lock_error` in errors.py is the single one. * Docs say what `build` now does by itself, and that contention is never in the discardable list. Also fixes a canary that was stale on this branch: action.yml's "Fail when the analysis did not run" step was missing from the expected step list. Probed, not just read: detect-changes with no git on PATH exits 1 naming the cause; a read command reports a corrupt database in one line while `build` discards and rebuilds it; a wrong-shape schema recovers the same way; an invoices database survives `build` untouched; an unwritable data directory gives one line; an invalid numeric env var warns and falls back; a donor repository's graph is refused; and a held write lock is reported as contention with the database present and byte-identical afterwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One conflict, in GraphStore.__init__. Staging (via #1027) registers the SQLite function crg_parent_dir after the open/migrate guard; this branch ends that guard with a described GraphStoreError fallback. Kept both: the fallback stays the last arm of the except, the registration stays after it, so a failure registering the helper is still not mistaken for a corrupt database. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tirth8205
added a commit
to gowrishacv/code-review-graph
that referenced
this pull request
Sep 18, 2026
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 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.
Five failures where the tool lied about its own state or crashed instead of saying what it could not do. Each one is recorded as a
xfail(strict=True)intests/test_cli_surface.py(from #1018, brought in unchanged in the first commit); every marker is flipped off in the second.Before:
218 passed, 119 xfailed. After:350 passed, 3 xfailed— the three left are the bugs this branch does not claim (postprocesswith no graph, an unstatable file duringbuild,embed's non-house-style dependency message).1.
detect-changesreported a clean tree it never looked atBug. With no
giton PATH, or agit diffthat timed out, every change-discovery call returned an empty list and the command printedNo changes detected.and exited 0 while the working tree had changed. A CI review gate keyed on that exit code passed pull requests nobody reviewed. The same false all-clear reacheddetect_changes_tool("No changed files detected",status: ok) and the GitHub Action, whose renderer turned any non-JSON input into "No analyzable code changes detected".Cause.
get_changed_files,get_staged_and_unstaged,_get_svn_changed_filesandparse_*_diff_rangesall caughtFileNotFoundError/TimeoutExpiredand returned[]/{}. Their callers could not tell "nothing changed" from "could not look", because the type system gave them no way to.Fix. A
require_vcsflag on the discovery path. A VCS that cannot be run at all raisesChangeDiscoveryError; a base ref that simply does not resolve keeps the documented--cachedfallback, so a repository with no commits still works.buildandupdatestay lenient on purpose — they recover by re-parsing the tree and reconciling by content hash, which is not a lie.Every caller propagates the distinction rather than flattening it:
detect-changesNo changes detected., exit 0Error: could not determine the changes: git could not be run (...). Install git and make sure it is on PATH., exit 1detect_changes_tool(MCP){"status": "ok", "summary": "No changed files detected."}{"status": "error", "error": "could not determine the changes: ..."}scripts/render_pr_comment.pyNo changes detected.is a clean tree; anything else renders "this pull request has not been reviewed" and exits 4action.ymlfail-on-risk, because an unknown risk is not a low oneA partial failure degrades instead of failing: when the changed-file list is known but the line-level diff is not,
analyze_changesscores whole files and recordsdiff_ranges_unavailableplus a summary line, rather than presenting a file-level answer as a line-level one.Proof.
test_absent_git_binary_is_not_a_false_all_clear,test_git_timeout_is_not_a_false_all_clear(both xfail flipped, both now assert exit 1,Error:on stderr, the cause named, and an exit code different from the genuine no-changes run);test_detect_changes_tool_reports_an_undiscoverable_diff_as_an_error,test_detect_changes_tool_asks_for_a_working_vcs;test_main_not_analyzed_returns_4_and_says_so,test_not_analyzed_beats_fail_on_risk_none,test_empty_detect_changes_output_is_not_an_all_clear;test_require_vcs_turns_an_unrunnable_git_into_an_error,test_a_base_ref_that_does_not_resolve_is_not_an_unrunnable_git.One existing assertion changed with the behaviour:
test_absent_git_binary_does_not_tracebacklooped overstatus,detect-changes,updateandbuildasserting exit 0 for each.detect-changesnow owes exit 1 there — that is the fix, and the loop carries a table saying which commands owe which code and why.status,updateandbuildstill exit 0 with no git.2. A corrupt
graph.dbtracebacked on every commandBug.
sqlite3.DatabaseError('file is not a database')straight out ofGraphStore.__init__, onstatus,detect-changes,visualize,wiki,dead-code,update,searchandqueryalike.Cause.
__init__ranconnect,PRAGMA journal_mode=WALand_init_schemawith nothing catchingsqlite3.Error, and no command had a handler for it either. Two further kinds of bad database were not detected at all: a valid SQLite file belonging to something else (CREATE TABLE IF NOT EXISTSgrafted our schema onto it and every query then answered "nothing found" about a graph that was never there), and one from a newer schema version (run_migrationsis a no-op at or aboveLATEST_VERSION, so the mismatch surfaced much later as a missing column in an unrelated query).Fix.
GraphStore.__init__raisesGraphStoreErrorwith one actionable line, andcli.mainreports it in the house style. All four kinds tested:A zero-byte file is deliberately still a new database — that is SQLite's own rule and the one
buildrelies on.Proof.
test_corrupt_database_is_reported_cleanly(xfail flipped, 8 commands) and a newtest_every_kind_of_corrupt_database_is_reported_cleanlycovering garbage bytes, truncation, a foreign SQLite file and a newer schema. In the ordinary suite:test_garbage_bytes_are_reported_not_raised,test_a_truncated_database_is_reported,test_a_foreign_sqlite_file_is_not_adopted(which also asserts the foreign file is left exactly as found),test_a_newer_schema_is_refused_with_both_versions,test_an_empty_file_is_still_a_new_database,test_a_failed_open_leaves_no_connection_behind.3. An unwritable data directory tracebacked on every command
Bug.
sqlite3.OperationalError('attempt to write a readonly database')from the samePRAGMA, including onbuild.Fix. Same handler, opposite message, because it needs the opposite action:
Proof.
test_read_only_data_dir_is_reported_cleanly(xfail flipped, 5 commands includingbuild), now also asserting the message names the directory;test_unwritable_data_directory_names_the_directory.4. Twenty-two numeric environment variables aborted the import (#912)
Bug.
CRG_MAX_IMPACT_NODES=killedstatus,buildanddaemon statuswithValueError: invalid literal for int() with base 10: ''— a traceback that never named the variable at fault, from commands that do not use the setting.Cause. Twenty-two bare
int(os.environ.get("CRG_...", "..."))at module scope acrossconstants.py,incremental.py,changes.py,daemon.py,graph.py,main.pyandcommunities.py.Fix. One shared helper, not twenty-two try blocks.
constants.env_int/env_floatreturn the documented default and warn once, naming the variable and the value.env_floatalso rejectsnanandinf, whichfloat()accepts and which then poison every comparison they reach.Proof.
UNGUARDED_AT_RUNTIMEandUNGUARDED_BASELINEare now empty, which flips 80test_invalid_numeric_env_var_does_not_tracebackcases and 20test_every_scanned_env_var_survives_a_nonsense_valuecases off xfail, and makestest_no_new_unguarded_numeric_env_parsefail on the first new bare parse.tests/test_env_config.pypins the in-process half: which values count as invalid, whitespace tolerance, the warning naming the variable, and warn-once.Two test-side updates go with this. The source scan and the AST walk in
test_cli_surface.pylearned the helper spelling, or the module would have gone blind to every setting the fix moved behind it — that surfacedCRG_IMPACT_DEPTH_DECAYandCRG_IMPACT_SCORE_FLOOR, already guarded but never covered, which are now in the table. Andtest_canary_harness_detects_a_tracebackusedCRG_MAX_IMPACT_NODES=to prove the traceback detector fires; with that fixed it needs a crash of its own, so it shadowssqlite3with a module that raises. Without that canary everyassert not result.tracebackin the file would be vacuous.5. A
graph.dbbuilt for another repository was accepted in silenceBug. Dropping one repository's
graph.dbinto another (a copied file, a restored CI cache) madedead-codelist the donor's symbols with the donor's absolute paths, and exit 0.Cause. The root check from #909 exists —
_assert_graph_matches_root— but only ever ran on the write side, during incremental reconciliation, where a wrong root would purge files. No read-only consumer asked.Fix.
assert_graph_serves_rootasks at every store open: incli._runand in the MCP tools'_get_store. It is deliberately narrower than the write-side check, because refusing to answer is not destructive the way purging is. It fires only when no file marker lies under the requested root, and four things pass: a graph with no markers, a graph of repo-relative paths (which carry no root identity), an equivalent spelling of the same root (/varagainst/private/var, a symlinked checkout — resolved before comparing), and a graph whose files are not on this machine at all, which is stale rather than another live checkout being served.Proof.
test_foreign_repository_database_is_rejected(xfail flipped), now also asserting the house-style failure and that the message says "different repository root";test_a_foreign_repository_graph_is_refusedplus the four negatives —test_the_graph_of_this_repository_is_served,test_an_equivalent_spelling_of_the_same_root_is_served,test_a_relative_path_graph_carries_no_root_identity,test_a_graph_of_files_not_on_this_machine_is_stale_not_foreign.Gates
docs/TROUBLESHOOTING.mdgains sections for each new message anddocs/GITHUB_ACTION.mddocuments exit 4.🤖 Generated with Claude Code