Skip to content

fix: report a skipped embedding refresh on a graph with no embeddings - #1071

Merged
1 commit merged into
tirth8205:stagingfrom
alex-feel:fix/warn-embedding-refresh-without-index
Oct 15, 2026
Merged

1 commit merged into
tirth8205:stagingfrom
alex-feel:fix/warn-embedding-refresh-without-index

Conversation

@alex-feel

Copy link
Copy Markdown
Contributor

Linked issue

Closes #1070

What & why

An explicit refresh (embedding_provider + embedding_model, or --embedding-provider + --embedding-model) on a graph with no vectors returned ok with no embedding field and no warning, the same result as a build that never asked for a refresh.

  • refresh_embeddings keeps returning None before provider resolution, so the feat: add documentation-aware, opt-in embedding refresh #653 guarantee holds: no model load and no network call. I moved its table-and-row check into has_embeddings() in embeddings.py so the no-change path below can reuse it.
  • build_or_update_graph, run_postprocess (behind the MCP tools and the build, update and postprocess commands) and the shared run_post_processing now turn that None into embeddings_refresh_skipped: true plus REFRESH_SKIPPED_WARNING, which tells the caller to run code-review-graph embed / embed_graph_tool once.
  • An update that finds no changes returns before post-processing. With both values passed, it now calls has_embeddings() and reports the same notice if the graph has no index. On an embedded graph it does nothing, as before.
  • watch treats any post-processing warning as a failed update, so the notice alone would end watch --embedding-provider ... --embedding-model ... on its first update. _raise_watch_postprocess_warnings now exempts that one warning: watch logs it after each update and keeps running. Any other warning fails the update as before, including a refresh that fails against an existing index.

Builds without both values behave as before. A watch that starts with nothing to reconcile stays quiet until the first file changes, because no refresh runs before then.

I updated the docstrings, docs/USAGE.md and the changelog.

How it was tested

Linux, Python 3.12 (python:3.12 container), on a clone of the branch with full history and tags, checked out at the path CI uses (/home/runner/work/code-review-graph/code-review-graph):

uv run pytest tests/ --tb=short -q --cov=code_review_graph --cov-report=term-missing --cov-fail-under=65
# 4280 passed, 855 skipped, 2 xfailed, 2 xpassed; total coverage 85.85%
uv run ruff check code_review_graph/
# All checks passed!
uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional
# Success: no issues found in 77 source files
bandit -r code_review_graph/ -c pyproject.toml
# no issues

tests/test_e2e_real_repo.py::test_hybrid_search_ranks_impact_radius_nodes_first depends on where the checkout lives. With the clone at /work/repo it fails on this branch and on unchanged staging (6b12d11) alike, and at the CI path it passes on both. The new and changed test modules also pass on Windows with Python 3.14.

New tests:

  • tests/test_watch_refresh_without_index.py: watch startup reconciliation and a watch batch, each with an explicit pair on a never-embedded graph, keep running and never resolve a provider. Any other warning, alone or next to the notice, fails the update.
  • TestRefreshWithoutVectors in tests/test_embedding_refresh.py: every build post-process level, shared post-processing, run_postprocess, a full build, and a no-change update with and without an index. The post-process cases run with no embeddings table and with an empty one, and each case asserts that get_provider is not called.

Checklist

  • Tests added for new functionality
  • All tests pass: uv run pytest tests/ --tb=short -q
  • Linting passes: uv run ruff check code_review_graph/
  • Type checking passes: uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional
  • Lines are at most 100 characters
  • Docs updated where behavior changed (README, docs/, docstrings)

An explicit embedding refresh (provider and model supplied) on a graph that was never embedded returned before provider resolution and left no trace in the result, so it looked the same as a build that never asked for a refresh.

build, update and postprocess now set embeddings_refresh_skipped and add a warning that names the embed command.
An update that finds no changes returns before post-processing, so it checks for an index directly and reports the same notice.
No model is loaded and no provider is contacted in either case; has_embeddings() is the check refresh_embeddings() already made, now shared.

watch fails closed on post-processing warnings, so the notice would have ended it on the first update.
The watch boundary exempts that one notice: watch logs it and keeps running, and every other warning, including a refresh that fails against an existing index, still fails the update.
@tirth8205 tirth8205 closed this pull request by merging all changes into tirth8205:staging in 03a5c7d Oct 15, 2026
@tirth8205

Copy link
Copy Markdown
Owner

Thank you! I added tests for watch logging the notice once per update, the real CLI watch and build runs, an empty embeddings table left by graph stats, and half a provider/model pair. The tests and your change are merged into staging in #1115.

azizur100389 pushed a commit to azizur100389/code-review-graph that referenced this pull request Oct 15, 2026
- incremental update that re-parses files reports the skip, including a
  repo path with spaces and non-ASCII characters
- repeated build/update requests report every time and never create the
  embeddings table
- no-change update reports the skip at postprocess none/minimal/full
  (guards the tirth8205#1105 no-op rewrite in tools/build.py)
- interrupted-build repair reports the skip
- postprocess with an empty embeddings table keeps status ok
- skip, then embed, then refresh reports embeddings_refreshed and no notice
- has_embeddings: no table, empty table, current and legacy rows
- legacy index without provider identity fails the refresh and stops watch;
  an empty legacy table is a skip
- watch exempts only the exact notice (duplicates ok; half-pair, altered
  text still fail); None/non-dict results are ignored
- real MCP tools and CLI build/update/postprocess/--quiet report the
  notice, exit cleanly and never resolve a provider

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
azizur100389 pushed a commit to azizur100389/code-review-graph that referenced this pull request Oct 15, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Explicit embedding refresh on a never-embedded graph returns ok and reports nothing

2 participants