Repository navigation
Conversation
Untrack docs/superpowers/ and .serena/, drop the private scratch-file list from .gitignore, de-duplicate GEMINI.md, point the CI badge at the default branch, and rename the PR-numbered test modules after the behaviour they pin. - docs/superpowers/specs/: two dated internal design specs were tracked even though .gitignore already lists docs/superpowers/. Untracked with git rm -r --cached; the ignore rule now holds. The older 2026-07-17 plan and spec files under that directory were already removed from the tree. - .serena/: 160 lines of untouched vendor boilerplate for one contributor's tooling. Added .serena/ next to .codex/ under "Local tool configuration" and untracked it, matching how .claude/, .codex/ and .qoder/ are handled. - GEMINI.md was a byte-identical copy of .github/copilot-instructions.md. It is now a short pointer to AGENTS.md and CLAUDE.md. Nothing reads it expecting full content: the GEMINI.md references in skills.py and test_skills.py are about files written into a target repo, under tmp_path. - .gitignore: the "One-off docs" section named about a dozen private scratch files. Replaced with a single scratch/ entry. None of those lines excluded build output, so nothing was kept. - README.md: the CI badge was pinned to ?branch=main while the default branch is staging. Now points at staging. - tests/: 21 modules named after their originating PR are renamed to the behaviour they cover, via git mv, with the PR number moved into the module docstring. Overlapping modules are folded: #831/#845/#866 into test_js_module_resolution.py, #830/#833 into test_go_receiver_resolution.py, #824/#862 into test_kotlin_import_comments.py. The one non-JS test in the #866 module moved to test_affected_flows_limits.py. - Updated the references: CLAUDE.md's "Regressions by pull request" line, and the self-referencing filename in the stale-SDK scan (was test_pr856_edges.py, now test_genai_packaging.py). CI workflow files and tests/test_documentation.py do not name any of the renamed files. Collected tests: 3370 before, 3370 after. Full suite passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pure layout. `_parents` in test_go_receiver_resolution.py and `_parse`/`_import_targets` in test_js_module_resolution.py landed mid-file when the modules were folded together. They now sit with the other helpers above the tests. No test bodies changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lations Every claim below was re-checked against this checkout before it was changed. README.md - Token reduction table: the Reduction column did not divide out from the two columns beside it. It printed the benchmark's average_reduction_ratio, which is the mean of the five per-question ratios, while the table prints the mean of graph_tokens. Replaced with naive_corpus_tokens / avg graph_tokens: 357.6x, 65.4x, 65.5x, 60.3x, 53.5x, 34.6x. Headline figures follow: median 63x, max 358x, range 35x to 358x. - "~2,000 to 3,500 tokens" contradicted the express row (3,936). Now "~2,200 to 3,900". - "On this repository, 208,821 source tokens" was present tense but measured at 84bde35, 981 commits back. Re-measured on staging HEAD with the benchmark's own definition: 1,217,838 tokens over 267 files, and a build gives 6,513 nodes / 57,038 edges against the table's 1,446 / 9,094. The snapshot rows are kept and labelled as a snapshot. - Edge confidence: AMBIGUOUS is never written. graph.py and migrations.py default to EXTRACTED; scoped_resolver.py and event_resolver.py write INFERRED. Now "Two-tier confidence (EXTRACTED/INFERRED)". - Slash commands: `code-review-graph install` writes no .claude/commands and no plugin manifest, so /code-review-graph:* cannot resolve. It writes four skills. Table replaced with the four skills install actually writes, plus the three extra ones Qoder gets from skills/. - SVG export exits 1 with ImportError on a default install; matplotlib is only in the eval extra. Said so in the CLI reference, the feature table and the extras list. - `unregister <id>`: the positional is path_or_alias and no command prints an id. Now `unregister <path|alias>`. - Added `forget` and `dead-code`, both shipped and documented in docs/COMMANDS.md but absent from the README, and said the block is a selection rather than a full reference. - Language list: application.properties / application.yml are promoted to spring_config and parsed into ConfigProperty nodes, so "Other YAML is not treated as source code" was wrong. Listed Spring Boot application config and narrowed the closing sentence. - Added Java framework awareness (spring_resolver, event_resolver, temporal_resolver, Spring Boot config keys), which shipped unmentioned. - Diagram alt text: fixed the flask, funnel and benchmark-board ratios, and added the missing Hermes Agent to the platform list (16 platforms, not 15). - Two links to docs/REPRODUCING.md#calibration-result-committed were dead; the heading is "Calibration table". docs/FAQ.md, docs/USAGE.md, docs/REPRODUCING.md - Same AMBIGUOUS fix, same dead anchor, and a note on REPRODUCING's avg ratio column explaining why it does not divide out and what the README reports. Translations - README.zh-CN.md, README.ja-JP.md, README.ko-KR.md and README.hi-IN.md were last touched on 2026-08-02 and were 337 to 339 lines against 659. Rebuilt from the corrected English: same sections, same tables, same numbers, same commands, same links, 677 lines each. Commands, flags, paths, code blocks and CLI output are verbatim. The stale "this translation is older" notices are gone because the files are now current. The PNGs under diagrams/ still bake in the old ratios and show 15 platform cards. They are exported by hand from Excalidraw, so they are not touched here. Gates: pytest 3359 passed, 10 skipped, 2 xpassed; ruff clean; mypy clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chore: repository-content hygiene
docs: correct the README against the code, and rebuild the four translations
…a test `visualize --format svg` died with a full traceback on a default install because matplotlib only ships in the `eval` extra. export_svg now raises a MissingOptionalDependencyError (an ImportError subclass, so existing callers are unaffected) and the CLI catches exactly that, prints the one-line install hint the README already documents, and exits 1. Catching the subclass rather than bare ImportError keeps a genuinely broken install of a required package visible as a traceback. Every other optional import already degrades cleanly: igraph falls back to file-based communities, tiktoken returns None, jedi skips enrichment, sentence-transformers/google-genai resolve to no provider, watchdog falls back to polling, and tomli is a hard dependency below 3.11. SVG export was the only raw traceback. tests/test_readme_parity.py holds the five READMEs together on the parts a reader depends on and a translator must not change: heading-level sequence, byte-identical fenced code blocks, outbound links and image sources in order, table row counts, and the language switcher. Prose is excluded, as are same-document anchors, whose targets are translated headings. Failures name the file, the check and the first differing item. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The hygiene change moved the English badge to ?branch=staging while the translations still carried ?branch=main. Each change was right on its own; together they drifted, which is exactly what the new parity test is for. It caught this on its first run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix: report missing matplotlib as one line, and pin README parity in a test
get_review_context spent its 800-line source budget first come first
served over the changed-file list, which is the order git handed us. One
alphabetically early 500-line file could take 500 of the 800 lines while
the riskiest changed function in the pull request got nothing.
The budget is now allocated once from the file list ranked by
changes.compute_risk_score, the same score detect_changes reports:
* every served file gets a 20-line floor, so nothing drops to zero
while the budget lasts,
* no file may hold more than 40% of the budget, so one huge file
cannot starve the rest,
* lines a file does not need are handed down the ranking rather than
wasted, and _extract_relevant_lines now reports what it emitted so
the accounting is exact,
* max_files cuts the ranked list, so the riskiest files survive it.
Ranking is bounded (8 nodes per file, 400 nodes overall, round-robin so
every file is scored before any file is scored twice) and falls back to
diff order if scoring fails. Measured on this repository with an
eight-file diff: code_review_graph/search.py, the riskiest file at 0.90,
went from 0 source lines to 200, files served went from 4 to 8, and the
total stayed at exactly 800.
compute_risk_score also has a change-frequency term worth up to 0.15,
but include_churn defaulted to False and only cli.py passed it, so every
agent-driven review scored that term at zero. detect_changes_func and
get_minimal_context now pass it. Because that puts git log --since
--numstat inside a tool call an agent waits on, compute_file_churn is
memoised per (repo, HEAD, window), capped at CRG_CHURN_MAX_COMMITS
commits, given its own CRG_CHURN_TIMEOUT (5s) rather than the 30-second
diff timeout, and caches failures so a slow repository pays the timeout
once and then degrades to the previous zero-churn behaviour.
Measured median of five detect_changes_func runs on this repository:
160ms before, 447ms cold (one 275ms git log walk over 543 commits),
175ms warm. Overall risk score on the same diff moved 0.65 to 0.755.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The whole-repo view stops being useful well before a real repository stops growing. The D3 page falls back to one bubble per community past 3000 nodes or 9000 edges, and a shallow clone of django indexes to 45111 nodes and 397762 edges, 15x and 44x past that ceiling. Every user with a serious codebase gets the bubble view, and bubbles are not reviewable. Seed the view instead. `visualize` grows a payload outward from a symbol, a set of files (including the ones this review changed), or an execution flow, and ships only the nodes within --depth hops and the edges among them. Nodes outside the neighbourhood are absent from the payload, not drawn dimmed, because payload size is the constraint: a depth-2 neighbourhood of django/urls/base.py::resolve is a fraction of the whole-repo page. A hop is a semantic edge (CALLS, IMPORTS_FROM, INHERITS, IMPLEMENTS, TESTED_BY, DEPENDS_ON). CONTAINS is structural and deliberately not a hop: counting it would put a seed's file one hop away and every sibling symbol in that file two hops away, so a depth-2 neighbourhood of one function would swallow its whole module. Containing File nodes are still shipped, at the hop of their closest member, because clustering and collapse are built on CONTAINS. --render-depth (default 1) is how much is drawn on open; clicking a node at the edge of what is drawn reveals its next ring and "+1 hop" reveals a whole ring, both out of the payload already in the page. --path-from and --path-to answer "how does A reach B", walking CALLS, IMPORTS_FROM and INHERITS with edge direction where a directed route exists and undirected otherwise, and highlighting the result in gold. The renderer is not rewritten. D3 stays, and search, node and edge kind filters, community colouring, the flow selector, the detail panel and the keyboard shortcuts all keep working on a seeded page; tests/ test_neighbourhood_browser.py drives each of them in headless Chromium. The default output is still one self-contained HTML file you can email; --sidecar moves the payload to graph.data.js beside it and is opt-in. build_parser() is split out of main() so the flag surface can be tested without running a command; the three subparsers main() still needs afterwards are registered in _SUBCOMMANDS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A persistent graph rots invisibly: nothing errors, caller lists quietly get shorter, and a reviewer gets a confidently incomplete answer. A clean rebuild of the same tree is a free and perfect oracle for that drift and nothing used it. incremental_fidelity copies a repository's tracked files into a throwaway git tree, builds a clean graph, applies one of seven edits (trailing comment, rename a function, delete a file, add a file, move a function between files, change an import, revert a change), commits it, runs incremental_update plus the post-processing build_or_update_graph runs, builds a second clean graph from the same edited tree, and compares the two databases table by table. The comparator keys every projection on something stable (a qualified name, a community name, a flow's path resolved to qualified names) and excludes autoincrement ids and wall-clock columns, so a reported difference is a real difference. nodes_fts is compared through fts5vocab, which reads the FTS index rather than the nodes content table behind it. graph_diff.take_snapshot and diff_snapshots, which nothing called before, supply the headline node/edge summary; they are not sufficient alone, which the module docstring explains. All seven edit kinds diverge on this repository. They are recorded in KNOWN_FAILURES so a run stays green while the report stays honest, and tests/test_incremental_fidelity.py guards against a kind that currently passes starting to diverge. No defect is fixed here; docs/REPRODUCING.md carries the measured per-table numbers and the three root causes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every incremental update called rebuild_fts_index, which drops nodes_fts, recreates it and repopulates it from the whole nodes table. Saving one file reindexed the entire graph on the hook path while the user waited. search.update_fts_index rewrites only the entries that changed. The full rebuild stays for full builds, for the postprocess command, and as the fallback. nodes_fts is an external content table, so removing an entry needs the values that were indexed, and those are gone once the node row is deleted. Deleting with the wrong values leaves a phantom entry: it still matches a query and then resolves to no node, which hybrid_search drops silently. Migration v11 adds nodes_fts_state, a mirror of what the index holds, filled by rebuild_fts_index in the same transaction that rebuilds the index, so the delta always deletes with the values that were indexed. The rewrite set is the union of nodes with no index entry, index entries with no node, and the rows of the files the caller named on both sides. The first two run whatever the caller passes, so a missing hint cannot leave a phantom or an unfindable node. With no hint at all the delta compares the indexed text against the nodes table instead. A delta touching more than 35% of the graph, or a database whose index predates the mirror, falls back to a rebuild. Measured on a build of this repository (6,509 nodes, 515,743 index entries), medians of seven alternating runs per arm: the FTS sync drops from 42.6 ms to 3.8 ms after touching one file, and from 50.7 ms to 9.2 ms after twenty. A delta's index entries are identical to a full rebuild's, entry for entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two defects made text search miss what it should find. 1. `_fts_search` wrapped the whole user query in one pair of double quotes, so every multi-word search was an exact-adjacency phrase match. On a build of this repository `password hashing` returned 0 rows. Queries are now built from tokens: each token is quoted (which is what neutralises AND/OR/NOT/NEAR, `col:`, `^`, parentheses and a typed `*`) and given a prefix term when it carries at least three alphanumeric characters. The tokens are ANDed first and ORed only if the AND finds nothing, so recall widens without costing precision. A phrase the user typed in quotes stays a phrase and suppresses the OR widening. 2. 2,451 of 6,517 nodes carried a docstring that the parser had already extracted and the embedding text builder already used, but that the FTS index could not see: an external-content FTS5 table can only index real columns of its content table. Migration v11 adds `nodes.docstring`, backfills it from `extra` in SQL, and recreates `nodes_fts` with it. Both node write paths populate it, including the batched `_replace_file_data` path a real build takes. `nodes.name_tokens` carries the camelCase splits `unicode61` cannot produce. Measured on this repository, indexing them raises inner-camel segment recall@20 from 24.7% to 54.7% for a 2.7% larger FTS index. BM25 needed help to stay precise once prefix terms and docstrings widened the candidate set: every row carries its file path in two indexed columns, so path tokens dominate the length normalisation and a long test name that repeats the query words outscores the function the query describes. Two boosts fix that, an exact-identifier boost and a symbol-name coverage boost, and the shipped search_quality benchmark goes from MRR 0.667 to 1.000. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CONTRIBUTING.md makes every supported AI tool permanent maintenance surface: config path, schema, install merge, uninstall and tests all have to keep working on every release. Nothing exercised that end to end, so a platform could break in a release without any test noticing. tests/test_platform_lifecycle.py drives the real install and uninstall code paths against a scratch git repository inside a throwaway HOME, HERMES_HOME and CRG_HOME, once for each of the 16 platforms in skills.PLATFORMS, through five steps: 1. first install writes the exact config path, settings key, server entry, instruction block, skills and hooks that platform reads; 2. a second install is byte-identical, with no duplicated entry, hook or instruction block; 3. installing over a pre-existing config preserves unrelated user settings, including another MCP server; 4. uninstall removes the registration, leaves no file it created and leaves the user's own settings intact; 5. reinstalling over an older release's entry (an absolute interpreter path, a previous hook shape) replaces it instead of leaving it beside the new one. Expectations are restated in the test rather than read back out of skills.PLATFORMS, so a moved config path or renamed key fails here. A drift test holds the installer set, the README --platform list and cli._PLATFORM_CHOICES to the same 16 names and reports any that differ, and a canary refuses to pass unless every step really ran against every platform and really compared something. Known-broken behaviour is recorded as xfail(strict=True) rather than fixed, so the suite stays green while a change in either direction fails loudly. 42 of them currently hold, covering: the pre-commit hook left as an unparseable "#!/bin/sh\nfi" after uninstall, which makes every later git commit fail; Qoder skill files and the Gemini CLI settings.json.bak surviving uninstall; every platform left with an empty config husk; stale config entries never replaced (GH #558, all 16 platforms, not one); and stale hook shapes duplicated rather than upgraded on five platforms. The suite is slow and writes real files, so it is gated behind a new platform_lifecycle marker and skipped by an opt-in hook in tests/conftest.py unless the run selects it: uv run --python 3.13 python -m pytest tests/test_platform_lifecycle.py \ -m platform_lifecycle -q Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The composite action at action.yml is the project's most public surface and
only its markdown renderer had tests. tests/test_action_e2e.py drives the
action's own steps, extracted verbatim from action.yml by a YAML parser,
against scratch Git repositories with a real origin, a real branch and a
real diff, then validates the result the way the two workflows and a GitHub
reader would.
Covered: the ordered steps and the report they produce (changed files, risk
scores, test gaps, token-savings honesty); the rendered comment as a
consumer sees it (well-formed tables, no unescaped pipe, no control
characters, links, one sticky marker, hostile symbol names); the inputs that
change behaviour (fail-on-risk at every level, comment false, cache key
versus migrations.LATEST_VERSION); the failure paths (shallow clone with no
merge base, a PR with no changed files, a corrupt cache, a cold cache); and
the security properties the docs claim (unprivileged workflow does not post,
privileged one verifies source event and analysed commit, nothing
interpolated into a shell command, no source code in the report).
Opt-in behind the action_e2e marker: CI's coverage job runs
pytest -m "not browser", which would otherwise select these. Run with
uv run --python 3.13 python -m pytest tests/test_action_e2e.py -m action_e2e -q
Ten checks are xfail(strict=True) and carry the bug they document, so each
flips to a failure the moment it is fixed. No product code is changed here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI installs with `pip install -e .`, which reads every file straight out of the checkout and never exercises the build backend. Nothing in the suite could catch a file that fails to reach the wheel; that is how the shipped skills went missing from the wheel in #909 while `test_qoder_bundled_skills` stayed green, because its editable-install fallback resolved to the checkout's own `skills/` directory. tests/test_packaging.py builds a wheel and an sdist with `python -m build`, installs each into its own throwaway virtual environment, and drives the installed program from a working directory unrelated to this checkout: `--version`, `build` over a small real repository assembled from this project's own sources, `status --json`, `detect-changes`, `visualize` (the only reader of the vendored D3 asset), and the MCP server over stdio spoken to with the real `mcp` client. Each smoke test first asserts the installed package resolves inside the environment's site-packages and not under the repository root, so a pass can never come from the source tree leaking in. The inventory of data files the wheel must carry is derived from the code rather than from the pyproject include list: every packaged module is parsed and its `importlib.resources.files("code_review_graph")` and `Path(__file__).parent` expressions are statically evaluated. `test_discovery_finds_the_known_data_surface` is the canary for that derivation, so a broken evaluator fails loudly instead of leaving the inventory check asserting nothing. Also checked: the wheel carries no tests, dot-directories, agent config files or databases; the sdist's only dot-path is the `.gitignore` hatchling cannot suppress; version agrees across `__init__`, pyproject and both artefacts; the declared Python floor both parses and installs on a real interpreter of that version; entry points import and are callable; and the core path runs with no optional-extra distribution present. The marker is registered in pyproject and skipped by default by a hook in tests/conftest.py. Run it with: uv run --python 3.13 python -m pytest tests/test_packaging.py -m packaging Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing verified that the same input produces the same graph. If a build is not reproducible then every benchmark, every diff and every "what changed" answer is measuring build noise as well as code, and the failure mode is silent. tests/test_determinism.py builds one corpus (this package plus the multi-language fixture tree, committed to a scratch git repo so the file inventory is the sorted `git ls-files` order) nine times, each build in its own subprocess writing its own database, and compares the sorted contents of every table: nodes, edges, metadata, flows, flow_memberships, communities, the per-node community assignment, community_summaries, flow_snapshots, risk_index, the FTS5 index (both what it answers and its on-disk image) and the embedding pipeline. Counts are never compared on their own. tests/determinism_dump.py holds the dump and diff, including the enumerated list of what is excluded and why: wall-clock columns, and autoincrement surrogate keys, which are resolved to the names they point at rather than dropped. It then attacks the likely sources instead of waiting for luck: the parallel executor against CRG_SERIAL_PARSE=1, a process pool against a thread pool with a different worker count, PYTHONHASHSEED 0 against 987654321, the same inventory presented in reverse, and the same tree built at two different absolute paths. Community detection: igraph's Leiden is randomised but communities.py pins the RNG seed before every call, so exact equality is the right property and a canary proves it by showing the partition move when CRG_LEIDEN_SEED changes. Without igraph the file-based fallback has no RNG at all, and a matching canary proves the seed is a no-op there. Five canaries guard the gate itself, because a check that passes having compared nothing is worse than no check: the build did real work; every section cleared a declared row floor and no section may omit one; no table in the database escapes the dump; an injected single-row change is detected and described; and path normalisation is removing a path that is genuinely in the data. Findings are recorded as strict xfails and are deliberately not fixed here: node id assignment and the FTS index image follow inventory order, community_summaries changes with inventory order, the non-git file inventory is unsorted, search ranking depends on the absolute checkout path, and community_summaries.purpose can store the checkout directory name. The gate is opt-in. tests/conftest.py skips anything marked determinism unless the marker is selected, so `pytest tests/` is unaffected and `pytest -m determinism` runs it in roughly 90 seconds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every migration test in this suite builds its database with the code under
test, so a migration is only ever exercised against a database the test just
made. The schema moved to version 10 this week; if that migration is wrong on
a real version 9 database, every existing user's first update after the
release breaks and nothing in CI says so.
tests/test_upgrade_path.py closes that gap against the released artefact
rather than a reconstruction of it. For each of the last three releases it
installs code-review-graph==<version> from PyPI into a throwaway uv
environment, builds a graph with that release over this repository's own tree
exported at the matching tag, then points the current code at the database and
runs what a user runs first: status, an update, and thirteen MCP tool
functions. It asserts the migration lands on LATEST_VERSION, that opening
loses no node or edge, that the v10 symbol backfill covers every pre-existing
row, that the first update keeps every file and the bulk of the graph, and
that no tool raises or returns an error.
Canaries keep it from passing vacuously: the old interpreter must have
imported the wheel and not the working tree, the old build must have produced
a graph of real size, the database it produced must be strictly behind
LATEST_VERSION, and the newest release under test must be the newest tag.
Two strict xfails record the reverse direction, which is broken today: an
older release opens a version 10 database without complaint, and its update
writes rows with a NULL nodes.symbol while leaving schema_version at 10, so
the backfill never runs again and dotted-tail queries answer not_found for
nodes that are present.
Registers the "upgrade" marker, excludes it from the coverage job, and adds a
manual workflow_dispatch job to run it before cutting a release. The module
also skips unless CRG_UPGRADE_TEST is set, since it needs PyPI and minutes:
CRG_UPGRADE_TEST=1 pytest -m upgrade -q
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tests/test_e2e_mcp_client.py drives about seven tools over stdio. This extends that to everything the server publishes, behind an opt-in `surface` marker. What it covers: * All 30 registered tools, called through mcp.ClientSession against a real built graph with realistic arguments, asserting on returned content (names, counts, orderings, files on disk) rather than on the absence of an error. State the tools need is built rather than faked: a git repository with a committed change and a dirty working tree, a populated registry via the CLI, and real embeddings computed against a stub OpenAI-compatible endpoint served from the test process, so the vector path in semantic_search_nodes_tool runs end to end without a network or an optional dependency. * All 5 registered prompts, fetched through prompts/get and asserted to render the arguments they declare. * Argument validation for every tool: a missing required argument, a wrong type, and an out-of-range value must each come back as a clean error rather than a traceback or a hang. * Safety: repo_root outside a project root is refused by every tool that takes one; a node name carrying NUL, ESC and 600 characters of padding is planted in the database and what comes back over the wire is checked; the DEFAULT_BUDGET column of tests/test_token_budget.py is re-measured on the bytes the client receives. * Protocol conformance: the requirements issue #919 names are hand-checked over raw JSON-RPC, and @hasmcp/mcp-spec-test is run when npx is available. Every case carries a canary that fails if it silently did nothing: the tool set is read from tools/list and compared against the cases, the planted hostile name has to reach at least four responses, the budget case asserts the measured payloads were non-empty, and the schemas drive the required/optional split rather than a hand-written list. Known deviations found while writing this are documented in module constants and reported, not fixed here. Run with: uv run --python 3.13 python -m pytest tests/test_mcp_surface.py -m surface -q Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds tests/test_export_formats.py, covering all six `visualize --format` values (html, json, graphml, cypher, obsidian, svg) plus `wiki`. Until now only the HTML path had any test, so the five non-JSON exporters could emit invalid output unnoticed. Every artefact is produced from a real built graph (a real git repo run through full_build plus run_post_processing) and validated with a real consumer, not a substring check: - GraphML: xml.etree, networkx.read_graphml, and the official GraphML 1.0 XSD vendored under tests/fixtures/graphml_xsd/. - Cypher: an openCypher lexer and statement parser in the test file that decodes string literals back to Python and round-trips them against the graph payload. - Obsidian: a vault walker that resolves every [[wikilink]] to a file and parses every YAML frontmatter block with yaml.safe_load. - SVG: xml.etree. - HTML: html.parser, json.loads on the embedded payload, and a SHA-384 check of the written D3 build against the pinned SRI hash. - Wiki: a Markdown table and link walker. Hostile node names are planted in the corpus (</script>, a template expression, a Cypher-breaking quote and backslash and newline, a Markdown link, control characters, a pipe, and a newline/space pair) so escaping is exercised where injection lives. Each export is produced twice and compared byte for byte, with the only tolerated differences named explicitly. Every check records what it actually compared; the recorder refuses a zero-sized comparison and a final canary fails if a whole format never reported in, so a check that silently degraded to a no-op cannot pass. Nine live bugs are recorded as strict xfails, each with a companion test that pins the current wrong behaviour so the pair cannot rot: - GraphML declares xmlns=http://graphml.graphstruct.org/graphml, so no GraphML reader accepts the file (networkx, already a hard dependency, rejects it). - xsi:schemaLocation carries one URI instead of a namespace/location pair. - node/@id is typed NMTOKEN, which no qualified name satisfies. - A newline in a name is written raw into node/@id and XML attribute-value normalisation collapses it onto the space spelling, merging two nodes. - exports._cypher_props tests int/float before bool, so the bool branch is dead and booleans serialise as Python True/False. - exports._cypher_escape leaves newline and tab unescaped. - Obsidian frontmatter values are bare YAML scalars, so a file path with ": " makes the page unparseable. - export_svg leaves matplotlib mathtext on (a name with two "$" aborts the export) and pins neither svg.hashsalt nor SOURCE_DATE_EPOCH (two exports of one graph differ). - The wiki writes names and community names into Markdown tables without escaping "|" or newline, and looks members up by their sanitised qualified name while nodes are stored under the raw one, silently dropping any node whose name contains a control character. These are slow and need two non-default consumers, so they are behind the new `exports` marker and skipped by the ordinary suite with the exact opt-in command in the skip reason: uv run --python 3.13 --with xmlschema --with matplotlib \ python -m pytest tests/test_export_formats.py -m exports -q Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…selines Every parser test in the suite writes a small fixture that exercises one construct. A regression that only shows on real code passes all of them and reaches a release. `pytest -m corpus` clones eight projects at exact commits (Python, Go, TypeScript, Java, C#, Rust, Ruby, PHP), builds the graph over each, and compares twelve measured properties against tests/corpus_baselines.json: the file inventory, files that produced no node, node/edge counts, the share of edges and of imports whose target resolves to a real node, CONTAINS edges whose source is not a node, control characters in node names, a build time budget, and whether the repository's own language survived. fastapi and gin reuse the pins already in code_review_graph/eval/configs rather than forking a second set; a fast guard test fails if the two drift apart. The slow check is opt-in twice over: it carries the new `corpus` marker, and tests/conftest.py skips every corpus-marked item unless the run's -m expression names the marker, so neither `pytest tests/` nor CI's `-m "not browser"` job pays for eight clones. Teeth: tests/test_real_repo_corpus_guard.py runs the comparison against a synthetic measurement and requires each of the twelve properties to fail on its own, and the slow module repeats that against the real measurement it just took, after asserting the build was non-trivial and checked out at the pinned SHA. Verified by hand as well: dropping ".go" from the parser's extension map fails gin on six properties with the exact deltas, and returning None from the Java module resolver fails gson on imports_resolved_share alone. Baselines, band rationale and the measured numbers are in docs/REPRODUCING.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds tests/test_cli_surface.py, an opt-in release gate that 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.
Covers all 39 commands (32 top-level plus 7 daemon subcommands), 150
command/flag pairs, and all 56 environment variables the package reads.
Every command is run in a scratch repository with git history and a built
graph; 14 flags are asserted to actually change the output; every failure
mode listed below is asserted to produce one clear line and a non-zero exit
rather than a traceback, matching the house style set by update/watch and by
the missing-matplotlib branch of visualize --format svg.
Behaviour that does not yet meet that bar is recorded as xfail(strict=True)
rather than fixed here, so the suite stays green, the bug stays visible, and
fixing it forces this file to be updated instead of letting the coverage rot:
- a corrupt graph.db tracebacks with sqlite3.DatabaseError on every command
- an unwritable data directory tracebacks with sqlite3.OperationalError
- detect-changes reports "No changes detected." and exits 0 when git is
absent or git diff times out, even when the tree has changed
- a graph.db built for another repository root is served silently
- a tracked file the OS cannot stat tracebacks with PermissionError
- 20 numeric environment variables raise ValueError at import (#912)
- postprocess creates an empty graph.db and exits 0 when none exists
- embed reports a missing optional dependency as "ERROR:" via logging
rather than the house "Error:" on stderr
test_no_new_unguarded_numeric_env_parse freezes the unguarded int()/float()
environment parses into a baseline, so a new one fails immediately in either
direction.
Registers the cli_surface marker and skips it unless selected by name:
uv run --python 3.13 python -m pytest tests/test_cli_surface.py -m cli_surface -q
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…calls from guessed
CHANGE 1, call sites on the row.
Every edge carries a file and a line, and none carries line 0, but
query_graph put nodes in `results` and edges in a separate `edges` list
with no key joining them. A reading agent could name a caller but not
say where the call is without opening the file, which is the file read
the graph exists to avoid.
callers_of, callees_of and references_to now return one row per call
site, not one per node, each carrying `call_site`. The row's own
`file_path` is not repeated: `file` appears only where the call is
written elsewhere, so a header or generated call is still located while
the common case costs one integer. Call sites are interleaved so every
distinct node appears before any node's second call site, which keeps a
truncated answer as wide as the full one. The response adds
`distinct_nodes` and the summary counts call sites and nodes separately.
get_impact_radius attaches the same `call_site` to an impacted node that
reaches the changed code in one hop, plus `call_site_count`; a node
further out has no single call site and gets none.
CHANGE 2, stored target-resolution certainty.
A CALLS edge either points at an indexed node or carries a bare name
that the read path can only match by name, subject to a receiver
evidence check. On this repository that is 11,442 direct against 21,232
unresolved, and nothing in a response said which kind an answer rested
on. Migration v11 promotes the distinction to `edges.target_resolution`
with `idx_edges_kind_target_resolution`, back-filled for CALLS and
REFERENCES and left NULL for kinds where the question is meaningless.
Every build and update path refreshes it after the resolvers run, since
those rewrite bare targets into qualified ones.
callers_of, callees_of and references_to take `resolution` ("all",
"direct", "unresolved") and report a `resolution_split` of the whole
answer. get_impact_radius takes `resolution` ("all", "direct") and
reports `unresolved_call_sites`, the call sites that name a changed
symbol but were never bound to it, so an empty radius is not read as
proof of absence. list_graph_stats reports the repository-wide split
from a covering-index group-by: 1.0 ms against 26.7 ms for deriving the
same answer, and 466 ms for the COALESCE form the index replaced.
Impact output is unchanged, row for row, on the impact_accuracy
benchmark. A bare target cannot equal a qualified seed, so an unresolved
hop was already unreachable under both traversal engines; the filter is
a guard that keeps that true, not a narrowing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The extension refuses a database newer than the version it was built against. v11 only adds a nullable edges column and an index, which the extension's own queries never read, so raising the ceiling is the whole fix. The schema-sync CI job pins the two constants together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The four release checks built as PRs #1012, #1013, #1014 and #1016 each run on demand and gate nothing. Alongside them, the end-to-end MCP client tests, the browser tests and the coverage matrix already run on every pull request but are not required checks, so nothing they say has ever stopped a promotion. This wires all seven into one workflow that runs when code lands on `testing`, which is the branch a release is cut from. Not on pull requests. Every job here takes minutes and a contributor must not wait for them; ci.yml is untouched. What blocks and what only reports is decided per check and justified in CONTRIBUTING.md. Blocking: upgrade-path (a database a user already has is not ours to lose), packaging (a broken wheel reaches everyone at once), determinism (no network, so a failure is a real difference), the coverage matrix, and the end-to-end tests on Linux and macOS. Reporting: the pinned real-repository corpus (it clones eight repositories that belong to other people, and a failed clone must not stop a release), the browser tests, and the Windows end-to-end leg. Reporting is not ignoring. scripts/promotion_gate.py wraps every check and fails it when the check did not actually run: its test module is not in the checkout, the selection collected almost nothing, or the tests were skipped by an opt-in hook instead of executed. Three of the four new checks are opt-in through a pytest_collection_modifyitems hook in tests/conftest.py and those hooks arrive on four branches that overwrite one another on merge, so a bare `pytest -m corpus` step exiting 0 on twenty skipped tests is a real prospect, not a hypothetical. The result is posted once to the job summary and once to the open issue labelled promotion-gate, naming every check and, for a failure, the assertion that moved. The gate opens no pull request and merges nothing. tests/test_promotion_gate.py asserts the policy, drives each canary to failure with a real junit report, and checks the workflow against the registry so a check cannot be added to one and forgotten in the other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two defects found while verifying the depth-2 neighbourhood of django/urls/base.py::resolve against the graph rather than by eye. An allow-list of six hop edge kinds silently dropped REFERENCES, the fourth-most-common kind in this repository's graph (787 edges) and the fourth in django's (15928). The depth-2 neighbourhood of that symbol came back with 144 nodes where a breadth-first search over the whole graph found 173, so 29 nodes a reviewer would have wanted were simply absent and nothing said so. The rule is now an exclusion: every edge is a hop except CONTAINS, so a parser that starts emitting a new relationship is included the day it does rather than the day someone remembers to add it. export_graph_data de-duplicates on the raw qualified name but emits the _sanitize_name-truncated one, so two nodes whose paths differ only past 256 characters reach the payload sharing a key. That double-counted them against --max-nodes (the django hub seed produced 1506 nodes under a cap of 1500) and would have had D3 bind two data records to one key. Keep the first record per key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds tests/test_concurrency_and_daemon.py (31 tests, opt-in behind a new
`concurrency` marker) covering the four things that only break when more
than one process touches one graph.db, or when one of them dies:
uv run --python 3.13 python -m pytest \
tests/test_concurrency_and_daemon.py -m concurrency -q
1. Cross-process access. Two builds at once, a reader running through a
rebuild, the shipped PostToolUse hook firing while a daemon watcher is
mid-update, plus a hand-run build on top. The protection surface is
pinned directly: WAL, busy_timeout=5000, autocommit, and nothing on
disk. There is no lock file and no advisory lock, so SQLite's
five-second wait is the whole of cross-process safety.
2. Interruption. A real build SIGKILLed at four points: mid-parse, inside
an open per-file transaction, between a file's DELETE and its INSERT,
and mid FTS rebuild. Each asserts integrity plus what the next run
does about it.
3. Daemon lifecycle. start/status/reconcile/stop against the real
crg-daemon, PID-file accuracy at each step, stale and recycled PIDs,
an unexpected child death, and a SIGKILLed daemon.
4. Watcher stress. Rapid create/modify/delete/rename, a watched
directory removed and recreated under a split (non-recursive) root,
and an exhausted OS watch budget.
Eight bugs are pinned as xfail(strict=True) so they flip to failures when
fixed; the worst is that full_build's serial loop files a "database is
locked" from the store call as a parse error, exits 0, writes the VCS
anchor as if complete, and loses those files permanently.
Every test asserts a canary (the lock was really held, the process really
died of a signal, the watcher really saw events, the churn really moved
files) so none can pass by doing nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The PR review comment this project posts on its own pull requests was asking authors to write tests for their tests. On PR #1014 it listed tests/test_upgrade_path.py::_check, _base_env, _venv_python and _venv_script as test gaps. Two causes, fixed at their own level. The parser only ever passed is_test when it built a Function or Test node, so every Class in a test file was stored with is_test = 0, and so was every private helper and fixture whose name did not match a test pattern. parse_bytes now marks each node it returns from a test file, whatever its kind, in one pass after extraction. Running it after extraction leaves TESTED_BY generation, which keys off the test functions alone, unchanged. Test-file detection itself was three regexes in three modules, each with a different idea of what a test file is: flows.py knew nothing about Go, refactor.py knew nothing about tests/ directories, and the parser's directory patterns matched anywhere in the string, so src/latest/ read as test/ and contests/ read as tests/. There is now one definition in parser.is_test_file, anchored on path separators and extended to the conventions it was missing: Ruby spec/ and *_spec.rb, C# *Tests.cs, conftest.py, Java and Kotlin *Tests, and the e2e and test-utils directories refactor.py already knew about. flows.py and refactor.py use it. changes.py and tools/review.py exclude a node whose file is a test file regardless of what is_test says on the row, so a graph built before this change stops producing the bad report without a rebuild. Measured on real diffs from this repository, rebuilding the graph each time: - PR #1014's diff: 20 test gaps, all 20 test-file artefacts, to 0. - detect-changes --base HEAD~6 at that commit, a test-heavy 46-file diff: 55 gaps of which 53 were test-file artefacts, down to 2. The two production gaps, MissingOptionalDependencyError and main, are the same before and after. - PR #997's production files (graph.py, parser.py, tools/query.py): 107 gaps to 98. The 9 dropped are all test-file artefacts; the 98 production gaps are identical sets before and after, including parser.py::_collect_python_receiver_evidence. The eval suite runs all 42 benchmarks over 6 repositories before and after. impact_accuracy, search_quality, agent_baseline and multi_hop_retrieval are unchanged cell for cell. token_efficiency ratios are unchanged. flow_completeness keeps its entry-point recall but reports fewer flows (fastapi 401 to 374, gin 186 to 161) because test-file entry points are no longer traced as production flows, which is what include_tests=False asks for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds tests/test_export_formats.py, covering all six `visualize --format` values (html, json, graphml, cypher, obsidian, svg) plus `wiki`. Until now only the HTML path had any test, so the five non-JSON exporters could emit invalid output unnoticed. Every artefact is produced from a real built graph (a real git repo run through full_build plus run_post_processing) and validated with a real consumer, not a substring check: - GraphML: xml.etree, networkx.read_graphml, and the official GraphML 1.0 XSD vendored under tests/fixtures/graphml_xsd/. - Cypher: an openCypher lexer and statement parser in the test file that decodes string literals back to Python and round-trips them against the graph payload. - Obsidian: a vault walker that resolves every [[wikilink]] to a file and parses every YAML frontmatter block with yaml.safe_load. - SVG: xml.etree. - HTML: html.parser, json.loads on the embedded payload, and a SHA-384 check of the written D3 build against the pinned SRI hash. - Wiki: a Markdown table and link walker. Hostile node names are planted in the corpus (</script>, a template expression, a Cypher-breaking quote and backslash and newline, a Markdown link, control characters, a pipe, and a newline/space pair) so escaping is exercised where injection lives. Each export is produced twice and compared byte for byte, with the only tolerated differences named explicitly. Every check records what it actually compared; the recorder refuses a zero-sized comparison and a final canary fails if a whole format never reported in, so a check that silently degraded to a no-op cannot pass. Nine live bugs are recorded as strict xfails, each with a companion test that pins the current wrong behaviour so the pair cannot rot: - GraphML declares xmlns=http://graphml.graphstruct.org/graphml, so no GraphML reader accepts the file (networkx, already a hard dependency, rejects it). - xsi:schemaLocation carries one URI instead of a namespace/location pair. - node/@id is typed NMTOKEN, which no qualified name satisfies. - A newline in a name is written raw into node/@id and XML attribute-value normalisation collapses it onto the space spelling, merging two nodes. - exports._cypher_props tests int/float before bool, so the bool branch is dead and booleans serialise as Python True/False. - exports._cypher_escape leaves newline and tab unescaped. - Obsidian frontmatter values are bare YAML scalars, so a file path with ": " makes the page unparseable. - export_svg leaves matplotlib mathtext on (a name with two "$" aborts the export) and pins neither svg.hashsalt nor SOURCE_DATE_EPOCH (two exports of one graph differ). - The wiki writes names and community names into Markdown tables without escaping "|" or newline, and looks members up by their sanitised qualified name while nodes are stored under the raw one, silently dropping any node whose name contains a control character. These are slow and need two non-default consumers, so they are behind the new `exports` marker and skipped by the ordinary suite with the exact opt-in command in the skip reason: uv run --python 3.13 --with xmlschema --with matplotlib \ python -m pytest tests/test_export_formats.py -m exports -q Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The directory branch of one relaxation hop must be an index seek on (target_qualified, kind). Left to itself SQLite drove it from the covering index on kind alone and rescanned every IMPORTS_FROM row once per frontier directory: 18 seconds of a 19-second kubernetes traversal, against 1.5 with CROSS JOIN and INDEXED BY. The same shape of mistake filled _impact_frontier_dirs by scanning every File node in the graph instead of the frontier. Neither is visible in a token budget or on a small fixture, so the SQL moves to module level where a test can EXPLAIN the text the traversal actually runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Spend the review source budget on whole changed regions, and make churn degrade once
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix(search): build FTS5 queries from tokens and index node docstrings
Draw a neighbourhood instead of a repository
cli.py collided on the same function: staging split main() into build_parser() plus main() so tests can read the flag surface without running a command, and this branch split main() into a lock-catching main() plus _dispatch(). The two splits compose: main() keeps the sqlite lock wrapper, _dispatch() is staging's parse-and-run body, and build_parser() stays the parser. The seed-flag helpers, _SUBCOMMANDS and the neighbourhood visualize wiring are unchanged from staging; the build-state reporting in status and postprocess is unchanged from this branch. Also fix the v1 seed in the migration-race test. It hand-rolls a nodes table, and parent_name and extra are in the base CREATE TABLE that no migration adds, so a real v1 database has both. Migration v13, which arrived from staging after that test was written, backfills from them, so the seed failed on a missing column instead of exercising the check-then-act window it gates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two conflicts, both resolved by keeping what each side meant. code_review_graph/cli.py: staging split the parser out of main() into build_parser() and added the visualize seed-flag helpers; this branch added _open_graph_store(), the corrupt-cache recovery wrapper. Both are kept: _open_graph_store() stays where it was, staging's helpers and build_parser() follow it, and the stale main() stub the conflict duplicated is gone. The two _open_graph_store() call sites main() already had merged cleanly. tests/test_action_e2e.py: both sides added this suite independently. This branch's copy is staging's plus the four strict-xfail markers it retires (corrupt cache recovery, the truncation budget, git-quoted diff paths, the sticky comment author filter) and the tests that replace them. Kept that copy, and restored staging's assertion that the unescaped markdown link is absent alongside this branch's assertion that the escaped span survives; neither literal is a bare URL, so tests/test_codeql_url_substring.py stays green. The five strict xfails that remain record bugs this branch does not fix: malformed and non-finite risk scores, and line_start escaping. 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>
…this branch An independent re-measurement found the kubernetes build-time row wrong in the direction that favoured this change. It read 149.4s on staging against 129.0s here, which says this branch is faster than staging. Measured isolated and alternating on one idle machine it is not: staging 115.1s and 122.8s against 148.7s and 146.5s here, and 4.41 GB of database against 5.36 GB. The branch is about 24% slower to build and 21.5% larger, never faster. The CHANGELOG now says that plainly, alongside what the cost buys: on cli/cli the CALLS edges bound to an indexed node go from 5503 to 22564 of the same 67503, because a bound import gives the call resolver a file to attribute a cross-file call to. The comparisons that do hold are against the pre-fix revision of this branch, which emitted one edge per member file: 524.6s of build against about 146s, 588,972 IMPORTS_FROM edges against 93,650, 73,512 edges naming an unindexed path against 5, and a 189,163-token get_impact_radius against 23,669. Those are kept and labelled as what they are. Also re-measured and corrected, on clean alternating builds of cli/cli, cobra, gin, jekyll and rubocop against origin/staging: - the grouped-block line defect is 8636 of cli/cli's 8687 import edges, not 8648 of 8690 - jekyll's non-require import edges are 31 of 227 (14%), not 34 (15%) - internal/ is 1317 of the 3495 cli/cli go import edges that bind in-repo, not "3493 of 8687" Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Stop the graph and the daemon from corrupting quietly
graph.py was the only overlap. Staging added a read-transaction helper (contextlib.contextmanager, _active_read_txn, _read_transaction, _get_stats_locked) and migration-safety retries; this branch added the package-scoped import resolver and the pinned impact traversal, which needs collections.abc.Iterable. The import block now carries both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Staging brought the three-state build marker in build_state.py and the state-aware reporting that `status` and `postprocess` print from it, along with the contention wrapper that renamed main() to _dispatch() and put a new main() in front of it to report a locked database instead of raising. cli.py was the only conflict, and only because both sides inserted a different block at the same point above _SUBCOMMANDS. Both are kept: _open_graph_store() stays exactly where it was and is still what both store-opening sites call, so a corrupt CI cache is discarded by `build` and reported as one line by everything else; the lock classifier and the main()/_dispatch() split are taken from staging unchanged, so the two store-opening sites now sit inside _dispatch() and a contended open is reported by the wrapper. The _SUBCOMMANDS and build_parser comments take staging's wording, which names _dispatch. Verified by probe rather than by reading the diff: a not-a-database file and a valid SQLite file holding a foreign `nodes` table each produce one line and exit 1 from a read-only command, and `build` discards and rebuilds both with no traceback; `status` prints nothing extra for complete, the missing-files line for in-progress and the post-processing line for postprocess-pending, with build_incomplete matching in --json; `postprocess` keeps the marker and prints the "cannot add them" line for in-progress, and clears it to complete for postprocess-pending. The branch's own fixes still hold: GraphML carries the graphdrawing namespace with synthetic NMTOKEN ids and names in <data>, _cypher_props emits true rather than True, and get_all_tracked_files reads a C-quoted path back as src/café.py. Gates: pytest 3981 passed, 793 skipped; the opt-in exports (69 passed), action_e2e (88 passed) and concurrency (40 passed) suites; ruff clean; mypy clean over 76 files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
fix: make the GraphML export readable, and a corrupt CI cache recoverable
One conflict, in GraphStore.__init__ of code_review_graph/graph.py. Staging wrapped the schema init and migration run in a guard that turns an unreadable or wrongly shaped database into CorruptGraphDatabaseError; this branch added the crg_parent_dir SQLite function that the directory-scoped import traversal joins on. Both are kept: the guard keeps its original scope, and the function is registered after it, because a failure to register a function is not a corrupt file and must not reach the code path that deletes a database. 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>
fix(parser): resolve Go and Ruby imports to files, not bare strings
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>
fix: report what is wrong instead of a false all-clear or a traceback
code-review-graph reviewOverall risk: 0.85 (CRITICAL) — 500 changed function(s)/class(es), 125 affected flow(s), 387 test gap(s) Risk-scored changes
Affected execution flows
Test gaps
Token savings: this graph-backed report used ~1,249,768 fewer tokens (~88%) 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. |
tirth8205
added a commit
to gowrishacv/code-review-graph
that referenced
this pull request
Sep 18, 2026
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 tirth8205#991, opened by app/github-actions, produced a ci.yml run three seconds later; PR tirth8205#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 -- tirth8205#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>
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)