Repository navigation
Integrate the release gates: 11 checks covering upgrade, packaging, determinism, the MCP surface, the CLI, exports, concurrency, the Action and the corpus - #1024
Merged
Conversation
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>
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>
…/gates # Conflicts: # pyproject.toml
…on/gates # Conflicts: # pyproject.toml # tests/conftest.py
…ion/gates # Conflicts: # pyproject.toml
…on/gates # Conflicts: # pyproject.toml
…gration/gates # Conflicts: # pyproject.toml # tests/conftest.py
…ation/gates # Conflicts: # pyproject.toml
… integration/gates # Conflicts: # pyproject.toml # tests/conftest.py
…o integration/gates # Conflicts: # pyproject.toml
…integration/gates
code-review-graph reviewOverall risk: 0.85 (CRITICAL) — 500 changed function(s)/class(es), 65 affected flow(s), 174 test gap(s) Risk-scored changes
Affected execution flows
Test gaps
Token savings: this graph-backed report used ~197,145 fewer tokens (~65%) than reading every changed file in full (estimated, chars/4 approximation).
Powered by code-review-graph — local-first analysis; no code leaves the CI runner. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Eleven pull requests merged into one branch, because every one of them added a marker to the same two lines and they could not land separately without conflicting.
What is in it
testingwith a blocking policyThe one thing resolved by hand
All eleven collided on
[tool.pytest.ini_options] markersand onpytest_collection_modifyitemsintests/conftest.py. Three of them had written their own bespoke skip hook.Resolved into a single generic gate: a suite is opt-in if its marker is in
_OPT_IN_MARKERS, and it is skipped unless the run's-mexpression names it. Adding another slow suite is now one marker inpyproject.tomland one name in that set.What these found
The checks are recorded as strict xfails where they fail, so each one flips to green the moment someone fixes the bug. On the first run they found 9 release-blocking and 23 serious defects, the worst being that uninstalling leaves a broken
pre-commithook that makes every subsequentgit commitfail. Those fixes are in flight separately.Gates
The 685 skipped are the opt-in suites, which is the intended default. Each runs on demand with its own marker, and the promotion gate runs them on
testing.