phase: 89_source_ignore_paths
All verification complete — TODO.md was already cleared in the roadmap commit; the two extra unit-test diffs are necessary fake-signature adaptations for the new keywords. Everything is green, no fixes were needed. ## Phase 89 — final verification pass: ALL GREEN **Verified (all 6 task files present in `complete/`):** - `git_sources.ignore_paths` JSONB column + migration 0013; `alembic downgrade -1 && upgrade head` round-trips (head `0013`) - Importer: `normalize_ignore_path`/`is_ignored`/`_ignore_for_root`, `ignore` in walk + progress pre-walk, `ignore_by_root` in `import_sources` - API: GET/POST carry list; admin-only `PATCH` (replace, 404/422 fixed details, anonymous 403) - Pipelines wired: `_run_sync`, `_run_upload` re-upload, `scripts/import_docs.py` - Sources-page box: dialog, §7.4 save lifecycle, `N ignored` tag, a11y; env rows get no box **Test/lint results:** - `uv run pytest` → 1808 passed - `uv run pytest --cov=app --cov-report=term-missing` → TOTAL **99%** (>90%) - `uv run pytest tests/e2e/test_source_ignore_paths.py -v --no-cov` → 6 passed (isolated, DB up) - Regressions in isolation: `test_git_sources_admin` 6, `test_archive_upload_sources` 5, `test_sync_button` 3, `test_smoke` 3 — all passed - `uv run ruff check . && uv run pyright` → clean (0 errors) **Completion criteria:** box→PATCH 200→count+GET round-trip ✅ · sync excludes `ignore/` (no docs/chunks/embeddings/summaries) + prunes newly-ignored (pruned==2) ✅ · no-mid-path rule E2E ✅ · PATCH 404/422/replace/clear/403 ✅ · full gate green ✅ · commit + phase move left to harness per rules. **Deviations:** none blocking — E2E pins `files == 4` (overview's "5" was an off-by-one vs its own 6-file tree, documented in-test); `tests/unit/test_importer.py` + `test_sync_button.py` test-double fakes extended for the new keywords (needed for the suite to stay green). **Next pending phase:** none — `todo/` holds only this phase.
This commit is contained in:
+115
-17
@@ -14,10 +14,18 @@ fallback is active).
|
||||
|
||||
Routes: ``GET`` (DB rows oldest-first, or the env list with
|
||||
``from_env: true`` while the table is empty; rows carry ``kind`` +
|
||||
``path``, git rows — and env rows — report ``path: null``), ``POST``
|
||||
``path``, git rows — and env rows — report ``path: null``; every row
|
||||
reports ``ignore_paths`` — DB rows their stored normalized list, env
|
||||
rows ``[]``, phase 89), ``POST``
|
||||
(201, validated create; ``kind`` selects the validation: git → exactly
|
||||
the phase-35 URL contract, local → an existing absolute directory, else
|
||||
422 naming the path), ``POST /upload`` (phase 49, backgrounded in phase
|
||||
422 naming the path; both kinds accept ``ignore_paths`` — optional,
|
||||
absent → ``[]``, stored normalized, phase 89),
|
||||
``PATCH /{source_id}`` (phase 89, A5 — replace one source's ignore
|
||||
list: 404 unknown id, the required body list is normalized + A4-
|
||||
validated with fixed-detail 422s and REPLACES the row's list wholesale
|
||||
— an empty list clears all; 200 → the ``GitSourceOut`` shape),
|
||||
``POST /upload`` (phase 49, backgrounded in phase
|
||||
64 task 03 — admin archive upload: the ``.tar``/``.tar.gz``/``.tgz``/
|
||||
``.zip`` name/format gate + the 1 MiB-chunk receive with the
|
||||
``upload_max_mb`` cap run **inline** and answered 202 the moment the
|
||||
@@ -90,7 +98,7 @@ from app.rag.archive_upload import (
|
||||
swap_in,
|
||||
unpack_archive,
|
||||
)
|
||||
from app.rag.importer import import_sources
|
||||
from app.rag.importer import import_sources, normalize_ignore_path
|
||||
from app.rag.llm import LLMClient, check_models
|
||||
from app.rag.overview import regenerate_overview
|
||||
from app.rag.source_removal import (
|
||||
@@ -101,6 +109,7 @@ from app.rag.source_removal import (
|
||||
)
|
||||
from app.rag.sources_meta import bump_sources_version
|
||||
from app.schemas import (
|
||||
GitSourceIgnoreIn,
|
||||
GitSourceIn,
|
||||
GitSourceList,
|
||||
GitSourceOut,
|
||||
@@ -172,6 +181,14 @@ _upload_status = UploadStatus()
|
||||
#: accepted shapes are exactly these four prefixes.
|
||||
URL_RE = re.compile(r"^(https?://|ssh://|git@)")
|
||||
|
||||
#: Phase 89, A4 — the per-source ignore-list limits, enforced in
|
||||
#: :func:`_validate_ignore_paths` (shared by POST and PATCH): at most
|
||||
#: 200 entries, each ≤500 chars after normalization. The 422 details
|
||||
#: are fixed strings that never echo the input (the router's
|
||||
#: credential-safety discipline, applied for consistency).
|
||||
MAX_IGNORE_PATHS = 200
|
||||
MAX_IGNORE_PATH_LENGTH = 500
|
||||
|
||||
|
||||
@router.get("", response_model=GitSourceList)
|
||||
def list_git_sources(
|
||||
@@ -181,10 +198,13 @@ def list_git_sources(
|
||||
|
||||
DB rows ordered by ``(added_at, id)`` (oldest first, id tie-break for
|
||||
same-timestamp inserts) with ``from_env: false`` — each row carries
|
||||
its ``kind`` and, for local rows, the stored ``path`` (git rows and
|
||||
env rows report ``path: null``); while the table is empty, the
|
||||
its ``kind``, its ``ignore_paths`` (phase 89 — the stored,
|
||||
normalized list; ``or []`` guards a row that predated the column),
|
||||
and, for local rows, the stored ``path`` (git rows and env rows
|
||||
report ``path: null``); while the table is empty, the
|
||||
``BOR_GIT_SOURCES`` env URLs as git rows (the env fallback is
|
||||
git-only) with null ``id``/``added_at`` and ``from_env: true``.
|
||||
git-only) with null ``id``/``added_at``, ``ignore_paths: []`` (no
|
||||
DB row to store a list on), and ``from_env: true``.
|
||||
"""
|
||||
rows = db.scalars(
|
||||
select(GitSource).order_by(GitSource.added_at.asc(), GitSource.id.asc())
|
||||
@@ -200,6 +220,7 @@ def list_git_sources(
|
||||
url=row.url,
|
||||
path=row.path,
|
||||
added_at=row.added_at,
|
||||
ignore_paths=row.ignore_paths or [],
|
||||
)
|
||||
for row in rows
|
||||
],
|
||||
@@ -207,7 +228,9 @@ def list_git_sources(
|
||||
)
|
||||
return GitSourceList(
|
||||
sources=[
|
||||
GitSourceRow(id=None, kind="git", url=url, path=None, added_at=None)
|
||||
GitSourceRow(
|
||||
id=None, kind="git", url=url, path=None, added_at=None, ignore_paths=[]
|
||||
)
|
||||
for url in get_settings().git_source_list
|
||||
],
|
||||
from_env=True,
|
||||
@@ -236,9 +259,35 @@ def create_git_source(
|
||||
|
||||
Wrong field combinations (git without url, local without path, both
|
||||
kinds' fields) are 422 with fixed, input-free details.
|
||||
|
||||
``ignore_paths`` (phase 89) — optional, both kinds: the raw box
|
||||
lines are normalized + A4-validated (``_validate_ignore_paths`` —
|
||||
the fixed-detail 422s) and the normalized list is what is stored and
|
||||
reported.
|
||||
"""
|
||||
row = _create_git_row(payload, db) if payload.kind == "git" else _create_local_row(payload, db)
|
||||
return GitSourceOut(id=row.id, url=row.url, added_at=row.added_at)
|
||||
return GitSourceOut(
|
||||
id=row.id, url=row.url, added_at=row.added_at, ignore_paths=row.ignore_paths
|
||||
)
|
||||
|
||||
|
||||
def _validate_ignore_paths(raw: list[str] | None) -> list[str]:
|
||||
"""Normalize + enforce the A4 limits (phase 89); the fixed 422
|
||||
details never echo the input (the credential-safety discipline,
|
||||
applied for consistency).
|
||||
|
||||
Shared by POST and PATCH. The empty check runs FIRST: a
|
||||
whitespace-only entry must 422, not be silently dropped (the UI
|
||||
drops blank lines client-side; the API stays defensive).
|
||||
"""
|
||||
entries = [normalize_ignore_path(e) for e in (raw or [])]
|
||||
if any(not e for e in entries):
|
||||
raise HTTPException(status_code=422, detail="ignore paths must be non-empty")
|
||||
if len(entries) > MAX_IGNORE_PATHS:
|
||||
raise HTTPException(status_code=422, detail="a source has at most 200 ignore paths")
|
||||
if any(len(e) > MAX_IGNORE_PATH_LENGTH for e in entries):
|
||||
raise HTTPException(status_code=422, detail="an ignore path exceeds 500 characters")
|
||||
return entries
|
||||
|
||||
|
||||
def _commit_new(row: GitSource, duplicate_detail: str, db: Session) -> GitSource:
|
||||
@@ -270,7 +319,13 @@ def _create_git_row(payload: GitSourceIn, db: Session) -> GitSource:
|
||||
if db.scalar(select(GitSource).where(GitSource.url == url)) is not None:
|
||||
raise HTTPException(status_code=409, detail="a git source with this URL already exists")
|
||||
return _commit_new(
|
||||
GitSource(url=url, kind="git"), "a git source with this URL already exists", db
|
||||
GitSource(
|
||||
url=url,
|
||||
kind="git",
|
||||
ignore_paths=_validate_ignore_paths(payload.ignore_paths),
|
||||
),
|
||||
"a git source with this URL already exists",
|
||||
db,
|
||||
)
|
||||
|
||||
|
||||
@@ -296,12 +351,41 @@ def _create_local_row(payload: GitSourceIn, db: Session) -> GitSource:
|
||||
# rows carry the expanded path there too — git URL shapes and absolute
|
||||
# paths cannot collide).
|
||||
return _commit_new(
|
||||
GitSource(url=path, kind="local", path=path),
|
||||
GitSource(
|
||||
url=path,
|
||||
kind="local",
|
||||
path=path,
|
||||
ignore_paths=_validate_ignore_paths(payload.ignore_paths),
|
||||
),
|
||||
f"a local source with this path already exists: {path}",
|
||||
db,
|
||||
)
|
||||
|
||||
|
||||
@router.patch("/{source_id}", response_model=GitSourceOut)
|
||||
def patch_git_source(
|
||||
source_id: uuid.UUID,
|
||||
payload: GitSourceIgnoreIn,
|
||||
db: Session = Depends(get_db), # noqa: B008
|
||||
) -> GitSourceOut:
|
||||
"""Replace one source's ignore list (phase 89, A5).
|
||||
|
||||
404 unknown id. The body list (required) is normalized +
|
||||
A4-validated (fixed 422 details) and REPLACES the row's list
|
||||
wholesale — an empty list clears all. Returns the updated
|
||||
row's public shape (id, url, added_at, ignore_paths).
|
||||
"""
|
||||
row = db.get(GitSource, source_id)
|
||||
if row is None:
|
||||
raise HTTPException(status_code=404, detail="git source not found")
|
||||
row.ignore_paths = _validate_ignore_paths(payload.ignore_paths)
|
||||
db.commit()
|
||||
db.refresh(row)
|
||||
return GitSourceOut(
|
||||
id=row.id, url=row.url, added_at=row.added_at, ignore_paths=row.ignore_paths
|
||||
)
|
||||
|
||||
|
||||
@router.post("/upload", response_model=UploadAccepted, status_code=202)
|
||||
async def upload_archive(
|
||||
file: UploadFile = File(...), # noqa: B008
|
||||
@@ -339,14 +423,18 @@ async def upload_archive(
|
||||
6. upsert the row by ``path`` (``kind='local'``; an existing row is
|
||||
left as-is — ``added_at`` preserved — and the unique index is
|
||||
the backstop: a concurrent insert lands ``failed`` with
|
||||
``a local source with this path already exists: <path>``);
|
||||
``a local source with this path already exists: <path>``); the
|
||||
row's saved ``ignore_paths`` are captured for the scan (phase
|
||||
89: a re-upload of an existing source honors the list the owner
|
||||
already saved);
|
||||
7. fail-fast ``check_models`` — ``ModelUnavailableError`` →
|
||||
``failed`` with the sanitized message (the phase-49 503 becomes
|
||||
a status state, A5); the folder/row are already committed, so
|
||||
the next sync/re-upload retries idempotently;
|
||||
8. ``import_sources([folder], llm, prune=True, progress=<hook>)``
|
||||
+ the change-gated ``regenerate_overview`` — the hook feeds the
|
||||
status ``current_file`` / ``files_done`` / ``files_total``;
|
||||
8. ``import_sources([folder], llm, prune=True, progress=<hook>,
|
||||
ignore_by_root={folder: row's list})`` (phase 89) + the
|
||||
change-gated ``regenerate_overview`` — the hook feeds the status
|
||||
``current_file`` / ``files_done`` / ``files_total``;
|
||||
9. one INFO log line (PLAN §9 / AGENTS.md rule 10 — ``total_ms`` is
|
||||
the background run's duration);
|
||||
10. ``success`` — ``detail`` = the ``UploadOut`` fields.
|
||||
@@ -502,8 +590,10 @@ async def _run_upload(
|
||||
path = str(final_dir)
|
||||
db = SessionLocal()
|
||||
try:
|
||||
if db.scalar(select(GitSource).where(GitSource.path == path)) is None:
|
||||
db.add(GitSource(url=path, kind="local", path=path))
|
||||
row = db.scalar(select(GitSource).where(GitSource.path == path))
|
||||
if row is None:
|
||||
row = GitSource(url=path, kind="local", path=path)
|
||||
db.add(row)
|
||||
try:
|
||||
db.commit()
|
||||
except IntegrityError:
|
||||
@@ -511,6 +601,11 @@ async def _run_upload(
|
||||
raise ValueError(
|
||||
f"a local source with this path already exists: {path}"
|
||||
) from None
|
||||
# Phase 89: the row's saved ignore list, copied to plain
|
||||
# values while the row is still usable in this session — a
|
||||
# re-upload of an existing source honors the list the owner
|
||||
# already saved; a fresh row has no list yet.
|
||||
ignore_paths = list(row.ignore_paths or [])
|
||||
finally:
|
||||
db.close()
|
||||
# Step 7 — fail-fast models (phase 41): ``ModelUnavailableError``
|
||||
@@ -530,7 +625,10 @@ async def _run_upload(
|
||||
_upload_status.files_done = done
|
||||
_upload_status.files_total = total
|
||||
|
||||
summary = await import_sources([final_dir], llm, prune=True, progress=_hook)
|
||||
summary = await import_sources(
|
||||
[final_dir], llm, prune=True, progress=_hook,
|
||||
ignore_by_root={str(final_dir): ignore_paths},
|
||||
)
|
||||
overview = False
|
||||
if summary.added + summary.updated > 0:
|
||||
overview = await regenerate_overview(llm)
|
||||
|
||||
+20
-9
@@ -34,9 +34,11 @@ decisions):
|
||||
<path>``; a failing clone or a missing local dir aborts before any
|
||||
import;
|
||||
4. ``import_sources(..., prune=True)`` over the single combined list
|
||||
(git checkouts + local dirs) — prune so files deleted upstream or
|
||||
out of a local dir leave the index (pruning covers the union; the
|
||||
CLI's no-prune default is unchanged);
|
||||
(git checkouts + local dirs), honoring each row's ``ignore_paths``
|
||||
(phase 89 — the per-root ignore map is built in the same per-row
|
||||
loop as the source list) — prune so files deleted upstream, out of
|
||||
a local dir, or newly matching an ignore pattern leave the index
|
||||
(pruning covers the union; the CLI's no-prune default is unchanged);
|
||||
5. when the import changed the KB (added + updated > 0),
|
||||
``regenerate_overview`` refreshes the single ``kb_overview`` row
|
||||
(phase 31 trigger, best-effort inside);
|
||||
@@ -214,19 +216,26 @@ async def _run_sync() -> None:
|
||||
)
|
||||
sources_root = Path(settings.sources_dir).expanduser()
|
||||
sources: list[Path] = []
|
||||
ignore_by_root: dict[str, list[str]] = {}
|
||||
for row in rows:
|
||||
if row.kind == "git":
|
||||
sources.append(clone_or_pull(row.url, sources_root / repo_name(row.url)))
|
||||
root = clone_or_pull(row.url, sources_root / repo_name(row.url))
|
||||
else:
|
||||
# kind=local — the stored expanded path (phase 38 also
|
||||
# mirrors it in the NOT-NULL ``url`` location column, the
|
||||
# ``or`` keeps the type checker honest); re-verified at
|
||||
# sync time because the directory may have moved or been
|
||||
# deleted since add-time.
|
||||
path = Path(row.path or row.url).expanduser()
|
||||
if not path.is_dir():
|
||||
raise GitSyncError(f"local source missing: {path}")
|
||||
sources.append(path)
|
||||
root = Path(row.path or row.url).expanduser()
|
||||
if not root.is_dir():
|
||||
raise GitSyncError(f"local source missing: {root}")
|
||||
sources.append(root)
|
||||
# Phase 89: the row's ignore list, keyed by the SAME root
|
||||
# string the importer sees; two rows sharing a root string
|
||||
# get the union (extend, not replace) — the sibling/repo-name
|
||||
# edge.
|
||||
if row.ignore_paths:
|
||||
ignore_by_root.setdefault(str(root), []).extend(row.ignore_paths)
|
||||
# Phase 64 (task 02): the per-file progress hook — the status
|
||||
# endpoint reports the file being processed right now. The
|
||||
# closure captures the module ``_status`` exactly like the state
|
||||
@@ -236,7 +245,9 @@ async def _run_sync() -> None:
|
||||
_status.files_done = done
|
||||
_status.files_total = total
|
||||
|
||||
summary: ImportSummary = await import_sources(sources, llm, prune=True, progress=_hook)
|
||||
summary: ImportSummary = await import_sources(
|
||||
sources, llm, prune=True, progress=_hook, ignore_by_root=ignore_by_root
|
||||
)
|
||||
overview = False
|
||||
if summary.added + summary.updated > 0:
|
||||
overview = await regenerate_overview(llm)
|
||||
|
||||
Reference in New Issue
Block a user