phase: 127_sync_incremental_folder_summaries
All verified green — the previous rejection was a harness interruption (no code failure), and the working tree + doc fixes are intact. **Phase 127 — final verification pass: 100% green** (all 4 tasks already complete; no code changes needed this pass) - **Re-verified implementation:** `ImportSummary.changed_paths` at all 5 increment sites + `_prune`; `affected_folder_keys` + `generate_folder_summaries(changed_docs=…)` (restricted keys, prune pass both modes, hook total = affected count); sync changed-KB branch passes `changed_docs=summary.changed_paths`; CLI keeps full-regen contract - **Tests:** `uv run pytest --cov=app --cov-report=term-missing` → **3022 passed**, `app/` coverage **99%**; `uv run pytest tests/e2e/test_incremental_folder_summaries.py -v --no-cov` → **3 passed in isolation**; `uv run ruff check . && uv run pyright` → clean, **0 errors** - **Completion criteria:** one-file change rewrites only its folder chain (E2E pins siblings byte-identical, text AND `updated_at`) ✅; coverage >90% ✅; isolated E2E green ✅; no completed-phase behavior change (full suite green) ✅; atomic commit — delegated to the harness per pass rules (no git commands run by me) - **Docs:** README.md re-checked — all 7 previous-pass fixes still in place (Theme page row, drill-down `ls` + capped `read`, phase-124 answer images section + `BOR_ANSWER_IMAGES`, markdown-included summaries, 17-color palette, folder summaries in `BOR_LLM_SUMMARY_MODEL`, CLI regen note); AGENTS.md verified consistent — no changes needed - **Deviations:** none; untracked owner file `NOTES.md` left untouched (rides in the harness commit) - **Next pending phase:** `128_sync_label_mobile`
This commit is contained in:
@@ -64,6 +64,17 @@ background sync task lives on the app's event loop, so the loop must
|
||||
survive across requests — exactly how the app runs under uvicorn.
|
||||
(A TestClient without the context manager starts a fresh loop per
|
||||
request and would cancel the task on request exit.)
|
||||
|
||||
Phase 127 (task 03): the changed-KB branch passes the import's change
|
||||
set to the generator (``changed_docs=summary.changed_paths`` —
|
||||
incremental regeneration over the changed set's folder keys only),
|
||||
the ``changed=N`` log line names the change count, and the status's
|
||||
``summaries_total`` is the affected count (the hook's total) rather
|
||||
than the table's candidate count. The unchanged-walk gap probe is
|
||||
orthogonal: it still calls the generator exactly as before
|
||||
(``only_missing=True``, no change set). The fakes above record the
|
||||
``changed_docs=`` kwarg (``FakeFolderSummaries.changed_docs_sets``) so
|
||||
the wiring is asserted, not inferred.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
@@ -71,7 +82,7 @@ import asyncio
|
||||
import logging
|
||||
import threading
|
||||
import time
|
||||
from collections.abc import Callable, Iterator
|
||||
from collections.abc import Callable, Iterator, Sequence
|
||||
from datetime import UTC, datetime
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
@@ -85,8 +96,9 @@ from app.api import sync as sync_api
|
||||
from app.config import Settings
|
||||
from app.db import SessionLocal, db_available
|
||||
from app.main import app as fastapi_app
|
||||
from app.models import GitSource
|
||||
from app.models import Document, GitSource
|
||||
from app.rag import git_sources as git_sources_resolver
|
||||
from app.rag.folder_summaries import affected_folder_keys
|
||||
from app.rag.importer import ImportSummary
|
||||
from app.rag.llm import EmbeddingError, LLMClient, ModelUnavailableError
|
||||
from app.rag.sources_meta import current_sources_version
|
||||
@@ -330,6 +342,11 @@ class FakeFolderSummaries:
|
||||
hook when given — the same way the import's file counter is driven
|
||||
through its hook, so the status's summary counters are testable
|
||||
deterministically.
|
||||
Phase 127 (task 03): records the ``changed_docs=`` change set the
|
||||
runner passes on the CHANGED-KB branch (the import's
|
||||
``changed_paths`` — None when the runner regresses to the
|
||||
change-set-less full regeneration; the unchanged-walk gap-fill
|
||||
branch never passes it).
|
||||
"""
|
||||
|
||||
ZERO = {"generated": 0, "failed": 0, "pruned": 0}
|
||||
@@ -346,6 +363,7 @@ class FakeFolderSummaries:
|
||||
self.skip_flags: list[bool] = []
|
||||
self.only_missing_flags: list[bool] = []
|
||||
self.progress_hooks: list[Callable[[int, int, str, str], None] | None] = []
|
||||
self.changed_docs_sets: list[Sequence[tuple[str, str]] | None] = []
|
||||
|
||||
async def __call__(
|
||||
self,
|
||||
@@ -354,10 +372,12 @@ class FakeFolderSummaries:
|
||||
*,
|
||||
skip: bool = False,
|
||||
only_missing: bool = False,
|
||||
changed_docs: Sequence[tuple[str, str]] | None = None,
|
||||
on_progress: Callable[[int, int, str, str], None] | None = None,
|
||||
) -> dict[str, int]:
|
||||
self.skip_flags.append(skip)
|
||||
self.only_missing_flags.append(only_missing)
|
||||
self.changed_docs_sets.append(changed_docs)
|
||||
self.progress_hooks.append(on_progress)
|
||||
if skip:
|
||||
return dict(self.ZERO)
|
||||
@@ -663,6 +683,7 @@ class _GatedFolderSummaries:
|
||||
self.steps_before = steps_before
|
||||
self.steps_after = steps_after
|
||||
self.hook: Callable[[int, int, str, str], None] | None = None
|
||||
self.changed_docs: Sequence[tuple[str, str]] | None = None
|
||||
|
||||
async def __call__(
|
||||
self,
|
||||
@@ -671,10 +692,12 @@ class _GatedFolderSummaries:
|
||||
*,
|
||||
skip: bool = False,
|
||||
only_missing: bool = False,
|
||||
changed_docs: Sequence[tuple[str, str]] | None = None,
|
||||
on_progress: Callable[[int, int, str, str], None] | None = None,
|
||||
) -> dict[str, int]:
|
||||
assert not skip, "these runs take a generation branch, never --limit"
|
||||
self.hook = on_progress
|
||||
self.changed_docs = changed_docs
|
||||
for done, total, source, folder_path in self.steps_before:
|
||||
if on_progress is not None:
|
||||
on_progress(done, total, source, folder_path)
|
||||
@@ -822,6 +845,7 @@ def test_failed_terminal_after_the_summary_hook_clears_phase_keeps_counts(
|
||||
*,
|
||||
skip: bool = False,
|
||||
only_missing: bool = False,
|
||||
changed_docs: Sequence[tuple[str, str]] | None = None, # phase 127
|
||||
on_progress: Callable[[int, int, str, str], None] | None = None,
|
||||
) -> dict[str, int]:
|
||||
self.hook = on_progress
|
||||
@@ -845,6 +869,255 @@ def test_failed_terminal_after_the_summary_hook_clears_phase_keeps_counts(
|
||||
assert failing.hook is not None # the wiring held
|
||||
|
||||
|
||||
# --- phase 127 (task 03): the changed-KB branch's incremental wiring ----
|
||||
|
||||
|
||||
def test_changed_sync_passes_the_change_set_to_the_generator(
|
||||
sync_client: TestClient,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
db: Session,
|
||||
tmp_path: Path,
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""Phase 127 (task 03): a changed-KB sync passes the import's
|
||||
change set into the generator (``changed_docs=summary.
|
||||
changed_paths`` — the incremental-regression restriction), NOT the
|
||||
gap-fill flag, the success ``detail`` keeps its exact pre-phase key
|
||||
set (the stats stay log-only), and the log line names the change
|
||||
count (``changed=N``)."""
|
||||
repo_url = f"file://{tmp_path / 'repo.git'}"
|
||||
_seed(db, repo_url)
|
||||
_stub_env(monkeypatch)
|
||||
monkeypatch.setattr(
|
||||
sync_api,
|
||||
"get_settings",
|
||||
lambda: _settings(sources_dir=str(tmp_path / "bor")),
|
||||
)
|
||||
_, fake_clone = _fake_clone()
|
||||
monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone)
|
||||
_stub_probe(monkeypatch)
|
||||
# One updated document — the import's change set carries exactly
|
||||
# its (source, path) identity (task 01's contract).
|
||||
change_set = [("repo", "notes/deep.md")]
|
||||
summary = ImportSummary(
|
||||
files=4, added=0, updated=1, unchanged=3, changed_paths=change_set
|
||||
)
|
||||
fake_import = FakeImportSources(summary)
|
||||
monkeypatch.setattr(sync_api, "import_sources", fake_import)
|
||||
monkeypatch.setattr(sync_api, "regenerate_overview", FakeOverview(ok=True))
|
||||
fake_folders = FakeFolderSummaries()
|
||||
monkeypatch.setattr(sync_api, "generate_folder_summaries", fake_folders)
|
||||
|
||||
_login(sync_client)
|
||||
with caplog.at_level(logging.INFO, logger="app.api.sync"):
|
||||
assert sync_client.post("/api/sync").status_code == 202
|
||||
body = _poll(sync_client, "success")
|
||||
|
||||
# The CHANGED-KB branch fired the generator exactly once ... with
|
||||
# the change set, NOT the gap-fill flag (the two restriction
|
||||
# kwargs are orthogonal — no caller combines them).
|
||||
assert len(fake_folders.changed_docs_sets) == 1
|
||||
assert fake_folders.changed_docs_sets == [change_set]
|
||||
assert fake_folders.only_missing_flags == [False]
|
||||
assert fake_folders.skip_flags == [False]
|
||||
# The success detail is byte-identical in shape to pre-phase 127 —
|
||||
# no new key (the stats stay log-only, the phase-98 convention).
|
||||
assert set(body["detail"]) == {
|
||||
"files", "added", "updated", "unchanged", "pruned", "errors",
|
||||
"chunks", "summaries", "summary_errors", "dates_updated",
|
||||
"overview", "sources_version",
|
||||
}
|
||||
assert body["detail"]["added"] == 0 and body["detail"]["updated"] == 1
|
||||
assert body["detail"]["overview"] is True
|
||||
assert body["detail"]["sources_version"] == 1 # changed KB → one bump
|
||||
# The ample-logging line names the change count (PLAN §9).
|
||||
assert any(
|
||||
r.getMessage() == (
|
||||
"sync: folder_summaries changed=1 "
|
||||
"stats={'generated': 0, 'failed': 0, 'pruned': 0}"
|
||||
)
|
||||
for r in caplog.records
|
||||
)
|
||||
|
||||
|
||||
def test_unchanged_walk_gap_probe_passes_no_change_set(
|
||||
sync_client: TestClient,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
db: Session,
|
||||
tmp_path: Path,
|
||||
clean_documents: None,
|
||||
) -> None:
|
||||
"""Phase 127 (task 03): the unchanged-walk gap probe is
|
||||
ORTHOGONAL to the change set — a walk that changed nothing with a
|
||||
real gap (seeded catalogue, empty ``folder_summaries`` table) still
|
||||
calls the generator exactly as before: ``only_missing=True`` and
|
||||
NO ``changed_docs`` (the gap probe's total is the missing count,
|
||||
as before)."""
|
||||
repo_url = f"file://{tmp_path / 'repo.git'}"
|
||||
_seed(db, repo_url)
|
||||
_stub_env(monkeypatch)
|
||||
monkeypatch.setattr(
|
||||
sync_api,
|
||||
"get_settings",
|
||||
lambda: _settings(sources_dir=str(tmp_path / "bor")),
|
||||
)
|
||||
_, fake_clone = _fake_clone()
|
||||
monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone)
|
||||
_stub_probe(monkeypatch)
|
||||
# The gap: two docs under notes/ (candidates root + notes), the
|
||||
# folder_summaries table empty (the autouse fixture truncated it).
|
||||
db.add_all([
|
||||
Document(
|
||||
source="repo", path="notes/one.md", full_path="/tmp/one.md",
|
||||
title="One", content="# One\nfirst\n", content_hash="a" * 64,
|
||||
),
|
||||
Document(
|
||||
source="repo", path="notes/two.md", full_path="/tmp/two.md",
|
||||
title="Two", content="# Two\nsecond\n", content_hash="b" * 64,
|
||||
),
|
||||
])
|
||||
db.commit()
|
||||
summary = ImportSummary(files=2, added=0, updated=0, unchanged=2)
|
||||
fake_import = FakeImportSources(summary)
|
||||
monkeypatch.setattr(sync_api, "import_sources", fake_import)
|
||||
monkeypatch.setattr(sync_api, "regenerate_overview", FakeOverview(ok=True))
|
||||
fake_folders = FakeFolderSummaries()
|
||||
monkeypatch.setattr(sync_api, "generate_folder_summaries", fake_folders)
|
||||
|
||||
_login(sync_client)
|
||||
assert sync_client.post("/api/sync").status_code == 202
|
||||
body = _poll(sync_client, "success")
|
||||
|
||||
# The gap probe fired the generator exactly once — the gap-fill
|
||||
# shape, byte-identical to pre-phase 127: only_missing=True, and
|
||||
# NO change set (the gap is a stored-rows gap, not a document
|
||||
# change — passing changed_paths here would restrict the fill to
|
||||
# nothing).
|
||||
assert len(fake_folders.changed_docs_sets) == 1
|
||||
assert fake_folders.changed_docs_sets == [None]
|
||||
assert fake_folders.only_missing_flags == [True]
|
||||
assert body["detail"]["overview"] is False # unchanged → no overview
|
||||
assert body["detail"]["sources_version"] == 0 # unchanged → no bump
|
||||
|
||||
|
||||
class _AffectedKeysFolderSummaries:
|
||||
"""A folder-summary step that mirrors the REAL generator's hook
|
||||
contract on a changed set (phase 127, task 02): the loop runs over
|
||||
the sorted AFFECTED keys (``affected_folder_keys(changed_docs)``),
|
||||
the hook fires once per key with ``total == len(keys)``, and the
|
||||
step parks mid-span after the first key — the (long) phase the
|
||||
user reported. The test reads the status mid-span and asserts the
|
||||
status's ``summaries_total`` IS the affected count (the hook's
|
||||
total), not the table's candidate count."""
|
||||
|
||||
def __init__(
|
||||
self,
|
||||
started: threading.Event,
|
||||
release: threading.Event,
|
||||
) -> None:
|
||||
self.started = started
|
||||
self.release = release
|
||||
self.changed_docs: Sequence[tuple[str, str]] | None = None
|
||||
self.keys: list[tuple[str, str]] = []
|
||||
self.hook: Callable[[int, int, str, str], None] | None = None
|
||||
|
||||
async def __call__(
|
||||
self,
|
||||
db: Session,
|
||||
llm: LLMClient,
|
||||
*,
|
||||
skip: bool = False,
|
||||
only_missing: bool = False,
|
||||
changed_docs: Sequence[tuple[str, str]] | None = None,
|
||||
on_progress: Callable[[int, int, str, str], None] | None = None,
|
||||
) -> dict[str, int]:
|
||||
assert not skip, "these runs take a generation branch, never --limit"
|
||||
self.hook = on_progress
|
||||
self.changed_docs = changed_docs
|
||||
assert on_progress is not None, "the runner must wire the hook"
|
||||
# The real generator's step 2: sorted(candidates & affected) —
|
||||
# the fake takes the affected side (the candidates are a
|
||||
# superset here: every affected folder holds a doc).
|
||||
self.keys = sorted(affected_folder_keys(changed_docs or []))
|
||||
assert self.keys, "the change set must affect at least the root"
|
||||
on_progress(1, len(self.keys), self.keys[0][0], self.keys[0][1])
|
||||
self.started.set()
|
||||
await asyncio.to_thread(self.release.wait)
|
||||
for i, (source, folder_path) in enumerate(self.keys[1:], start=2):
|
||||
on_progress(i, len(self.keys), source, folder_path)
|
||||
return {"generated": len(self.keys), "failed": 0, "pruned": 0}
|
||||
|
||||
|
||||
def test_summaries_phase_reports_the_affected_count_as_total(
|
||||
sync_client: TestClient,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
db: Session,
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""Phase 127 (task 03): mid-span, the status's ``summaries_total``
|
||||
is the AFFECTED count — ``len(affected_folder_keys(changed set))``
|
||||
(the hook's total), not the table's candidate count — and the
|
||||
terminal keeps that final count (the phase-64 convention, now over
|
||||
the affected span). The change set: one nested-folder doc (a
|
||||
three-key chain) + one root-level doc (shares the root key) →
|
||||
exactly 3 affected keys."""
|
||||
repo_url = f"file://{tmp_path / 'repo.git'}"
|
||||
_seed(db, repo_url)
|
||||
_stub_env(monkeypatch)
|
||||
monkeypatch.setattr(
|
||||
sync_api,
|
||||
"get_settings",
|
||||
lambda: _settings(sources_dir=str(tmp_path / "bor")),
|
||||
)
|
||||
_, fake_clone = _fake_clone()
|
||||
monkeypatch.setattr(sync_api, "clone_or_pull", fake_clone)
|
||||
_stub_probe(monkeypatch)
|
||||
change_set = [("repo", "notes/a/one.md"), ("repo", "top.md")]
|
||||
fake_import = FakeImportSources(
|
||||
ImportSummary(
|
||||
files=6, added=1, updated=2, unchanged=3, changed_paths=change_set
|
||||
)
|
||||
)
|
||||
monkeypatch.setattr(sync_api, "import_sources", fake_import)
|
||||
monkeypatch.setattr(sync_api, "regenerate_overview", FakeOverview(ok=True))
|
||||
gated_folders = _AffectedKeysFolderSummaries(
|
||||
threading.Event(), threading.Event()
|
||||
)
|
||||
monkeypatch.setattr(sync_api, "generate_folder_summaries", gated_folders)
|
||||
|
||||
_login(sync_client)
|
||||
assert sync_client.post("/api/sync").status_code == 202
|
||||
try:
|
||||
assert gated_folders.started.wait(5.0), "the run never reached the summaries"
|
||||
s = sync_client.get("/api/sync/status").json()
|
||||
assert s["state"] == "running"
|
||||
assert s["phase"] == "summaries"
|
||||
# The hook's first key: the source root (the bare source name —
|
||||
# folder_path "" never gets a slash) ...
|
||||
assert s["current_summary"] == "repo"
|
||||
assert s["summaries_done"] == 1
|
||||
# ... and the total IS the affected count (the task's pin):
|
||||
# notes/a/one.md → ("repo", ""), ("repo", "notes"),
|
||||
# ("repo", "notes/a"); top.md → ("repo", "") — 3 keys.
|
||||
expected = sorted(affected_folder_keys(change_set))
|
||||
assert expected == [("repo", ""), ("repo", "notes"), ("repo", "notes/a")]
|
||||
assert s["summaries_total"] == len(expected)
|
||||
assert s["summaries_total"] == 3
|
||||
gated_folders.release.set()
|
||||
finally:
|
||||
gated_folders.release.set()
|
||||
|
||||
body = _poll(sync_client, "success")
|
||||
# Terminal: phase + current_summary cleared, the hook's FINAL
|
||||
# (affected) counts kept (the phase-64 keep-final-counts rule, now
|
||||
# over the affected span).
|
||||
assert body["phase"] is None and body["current_summary"] is None
|
||||
assert body["summaries_done"] == 3 and body["summaries_total"] == 3
|
||||
# Wiring: the runner passed the change set, not the gap-fill flag.
|
||||
assert gated_folders.changed_docs == change_set
|
||||
assert gated_folders.hook is not None
|
||||
|
||||
|
||||
# --- admin: failures -------------------------------------------------------
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user