phase: 135_git_source_branch
All verification complete. No edits were needed by this pass — the phase work and docs were already consistent and green. **Phase 135 final verification pass — report** - Verified all 5 tasks' work present & correct: migration 0024 (reversible), `GitSource.branch` + 4 schema shapes, `validate_branch_name` (full check order per design), `clone_or_pull(…, branch)` (HEAD guard first, `--branch` clone, fetch+ff merge), both call sites pass `row.branch`, UI (add-form field, editor field, row tag), `file://` URL transport. - Completion criteria: (1) branch-mirrored through Sync + CLI — **pass** (E2E + integration); (2) `branch=None` byte-identical — **pass** (`test_branch_none_argv_is_byte_identical_pre_phase_sequence` green); (3) fixed 422s / local-row 422 / tri-state — **pass**; (4) mismatch fails loudly with remove-and-re-add — **pass** (E2E); (5) coverage 99% >90%, E2E isolated green — **pass**; (6) no regressions — **pass**. - Tests: `uv run pytest --cov=app --cov-report=term-missing` → 3223 passed, app/ 99%; `uv run pytest tests/e2e/test_git_source_branch.py -v --no-cov` → 4 passed (16.4 s); `uv run ruff check .` clean; `uv run pyright` → 0 errors, 0 warnings. - Documentation: README.md reviewed in full — phase-135 branch section accurate; the phase's README diff also correctly fixed a pre-existing stale line (shallow `--depth 1` clone vs full-history, phase 107). Routes, env defaults, feature lists verified against code. AGENTS.md — all 10 rules + quick-reference verified (referenced tests, PLAN.md, phases dirs, commands all accurate). **Verified consistent — no changes needed.** - No live-infrastructure changes (dev DB already running and healthy; no live steps in this phase). - Next pending phase: `.agents/phases/todo/136_force_push_reset`.
This commit is contained in:
@@ -58,6 +58,22 @@ injected ``https://x-access-token:<token>@…`` URL with a
|
||||
credential-free checkout path (mock ``clone_or_pull``) and a legacy
|
||||
row's clone still receives its ORIGINAL stored URL (the credential
|
||||
keeps working).
|
||||
|
||||
Phase 135 (task 01 — branch storage + reporting): the output shapes
|
||||
gain ``branch`` (NULL = the repo's default branch) — list DB rows
|
||||
report the stored value, env-fallback rows report ``null`` (no DB row
|
||||
to store one on), and POST 201 / PATCH 200 report the stored value.
|
||||
|
||||
Phase 135 (task 02 — branch validation + editing): POST stores a
|
||||
branch on GIT rows only (absent/empty → NULL = the default branch;
|
||||
non-empty → :func:`app.rag.git_sources.validate_branch_name`, a
|
||||
``ValueError`` → the FIXED-detail 422 ``not a valid branch name`` —
|
||||
the input is never echoed, and the row is not created); a ``branch``
|
||||
on a LOCAL row is 422 ``branch is only valid for git sources`` (POST
|
||||
and PATCH alike — local rows are walked, not cloned); PATCH edits
|
||||
the branch with the token's EXACT tri-state — absent/None = no
|
||||
change, non-empty = set (re-validated), empty string = clear (stored
|
||||
NULL).
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
@@ -161,10 +177,11 @@ def test_get_empty_table_with_env_returns_env_rows(
|
||||
"url": "https://a.example.com/one.git",
|
||||
"path": None,
|
||||
"added_at": None,
|
||||
# Env rows have no DB row to store a list on (phase 89) or
|
||||
# a flag on (phase 105).
|
||||
# Env rows have no DB row to store a list on (phase 89), a
|
||||
# flag on (phase 105) or a branch on (phase 135).
|
||||
"ignore_paths": [],
|
||||
"include_hidden": False,
|
||||
"branch": None,
|
||||
},
|
||||
{
|
||||
"id": None,
|
||||
@@ -174,6 +191,7 @@ def test_get_empty_table_with_env_returns_env_rows(
|
||||
"added_at": None,
|
||||
"ignore_paths": [],
|
||||
"include_hidden": False,
|
||||
"branch": None,
|
||||
},
|
||||
]
|
||||
|
||||
@@ -241,10 +259,11 @@ def test_post_creates_trimmed_and_list_stops_using_env(
|
||||
assert body["added_at"] is not None
|
||||
# Phase 89: the response gains ``ignore_paths`` — absent at create
|
||||
# time → ``[]``; phase 105 adds ``include_hidden`` — absent →
|
||||
# ``False`` (A4).
|
||||
assert set(body) == {"id", "url", "added_at", "ignore_paths", "include_hidden"}
|
||||
# ``False`` (A4); phase 135 adds ``branch`` — absent → ``null``.
|
||||
assert set(body) == {"id", "url", "added_at", "ignore_paths", "include_hidden", "branch"}
|
||||
assert body["ignore_paths"] == []
|
||||
assert body["include_hidden"] is False
|
||||
assert body["branch"] is None
|
||||
|
||||
# The DB row now wins: from_env False, the env URL is gone from the list.
|
||||
body = admin_client.get("/api/git-sources").json()
|
||||
@@ -258,6 +277,10 @@ def test_post_accepts_accepted_url_shapes(admin_client: TestClient) -> None:
|
||||
"http://git.local/repo.git",
|
||||
"ssh://git@example.com/repo.git",
|
||||
"git@github.com:owner/repo.git",
|
||||
# Phase 135 (task 05): the file:// local transport — the E2E
|
||||
# fixture repos the UI adds end to end (the phase-28/32/107
|
||||
# transport-true idiom, promoted from the env fallback).
|
||||
"file:///srv/repos/homelab-notes",
|
||||
):
|
||||
r = admin_client.post("/api/git-sources", json={"url": url})
|
||||
assert r.status_code == 201, f"{url} must be accepted: {r.text}"
|
||||
@@ -356,10 +379,12 @@ def test_post_local_creates_stored_row_with_expanded_path(
|
||||
# The phase-35 response shape is unchanged — the local row reports
|
||||
# its (expanded) path in ``url``; ``kind`` + ``path`` via GET;
|
||||
# phase 89 adds ``ignore_paths`` (absent → ``[]``); phase 105 adds
|
||||
# ``include_hidden`` (absent → ``False``, A4).
|
||||
assert set(body) == {"id", "url", "added_at", "ignore_paths", "include_hidden"}
|
||||
# ``include_hidden`` (absent → ``False``, A4); phase 135 adds
|
||||
# ``branch`` (absent → ``null``).
|
||||
assert set(body) == {"id", "url", "added_at", "ignore_paths", "include_hidden", "branch"}
|
||||
assert body["ignore_paths"] == []
|
||||
assert body["include_hidden"] is False
|
||||
assert body["branch"] is None
|
||||
uuid.UUID(body["id"])
|
||||
assert body["url"] == str(real_dir)
|
||||
assert body["added_at"] is not None
|
||||
@@ -592,7 +617,8 @@ def test_delete_removes_row_and_falls_back_to_env(
|
||||
"path": None,
|
||||
"added_at": None,
|
||||
"ignore_paths": [], # env rows: no DB row to store a list on
|
||||
"include_hidden": False, # … or a flag on (phase 105)
|
||||
"include_hidden": False, # … a flag on (phase 105)
|
||||
"branch": None, # … or a branch on (phase 135)
|
||||
}
|
||||
]
|
||||
db.execute(text("TRUNCATE chunks, documents"))
|
||||
@@ -709,6 +735,7 @@ def test_get_reports_stored_ignore_paths_and_env_rows_empty(
|
||||
"added_at": None,
|
||||
"ignore_paths": [],
|
||||
"include_hidden": False,
|
||||
"branch": None, # env rows: no DB row to store a branch on (135)
|
||||
}
|
||||
]
|
||||
|
||||
@@ -728,9 +755,10 @@ def test_patch_replaces_ignore_paths_including_clear(admin_client: TestClient) -
|
||||
r = admin_client.patch(f"/api/git-sources/{before['id']}", json={"ignore_paths": ["a/", "b"]})
|
||||
assert r.status_code == 200, r.text
|
||||
body = r.json()
|
||||
assert set(body) == {"id", "url", "added_at", "ignore_paths", "include_hidden"}
|
||||
assert set(body) == {"id", "url", "added_at", "ignore_paths", "include_hidden", "branch"}
|
||||
assert body["ignore_paths"] == ["a", "b"] # normalized
|
||||
assert body["include_hidden"] is False # untouched by a list-only PATCH
|
||||
assert body["branch"] is None # untouched by a list-only PATCH
|
||||
for key in ("id", "url", "added_at"):
|
||||
assert body[key] == before[key]
|
||||
# Round-trip through GET.
|
||||
@@ -988,7 +1016,8 @@ def test_post_with_token_stores_column_and_never_echoes(
|
||||
assert r.status_code == 201, r.text
|
||||
body = r.json()
|
||||
assert body["url"] == "https://github.com/owner/private.git"
|
||||
assert set(body) == {"id", "url", "added_at", "ignore_paths", "include_hidden"}
|
||||
assert set(body) == {"id", "url", "added_at", "ignore_paths", "include_hidden", "branch"}
|
||||
assert body["branch"] is None # no branch sent → the default-branch NULL
|
||||
assert TOKEN not in r.text
|
||||
|
||||
row = db.scalars(select(GitSource)).one()
|
||||
@@ -1329,7 +1358,8 @@ def _stub_sync_stack(
|
||||
|
||||
clone_calls: list[tuple[str, Path]] = []
|
||||
|
||||
def fake_clone_or_pull(url: str, dest: Path | str) -> Path:
|
||||
def fake_clone_or_pull(url: str, dest: Path | str, branch: str | None = None) -> Path:
|
||||
# phase 135: the row's branch pin (None for this file's rows)
|
||||
dest = Path(dest)
|
||||
dest.mkdir(parents=True, exist_ok=True)
|
||||
clone_calls.append((url, dest))
|
||||
@@ -1421,3 +1451,229 @@ def test_sync_public_row_clones_with_verbatim_url(
|
||||
assert _poll_sync(sync_admin_client, "success")["error"] is None
|
||||
|
||||
assert clone_calls == [(url, tmp_path / "sources" / "public")]
|
||||
|
||||
|
||||
# --- branch: create + validation (phase 135, task 02) ----------------------
|
||||
|
||||
|
||||
def test_post_git_source_with_branch_stores_and_reports(
|
||||
admin_client: TestClient, db: Session
|
||||
) -> None:
|
||||
"""POST a git source with ``branch: "release"`` → 201 reports the
|
||||
branch; the stored model row carries it; the GET round-trip
|
||||
reports it too (201, list DB row)."""
|
||||
r = admin_client.post(
|
||||
"/api/git-sources",
|
||||
json={"url": "https://example.com/branched.git", "branch": "release"},
|
||||
)
|
||||
assert r.status_code == 201, r.text
|
||||
body = r.json()
|
||||
assert body["branch"] == "release"
|
||||
assert set(body) == {"id", "url", "added_at", "ignore_paths", "include_hidden", "branch"}
|
||||
|
||||
row = db.scalars(select(GitSource)).one()
|
||||
assert row.branch == "release" # the column, not response-only
|
||||
|
||||
listed = admin_client.get("/api/git-sources").json()["sources"][0]
|
||||
assert listed["branch"] == "release"
|
||||
|
||||
|
||||
def test_post_git_source_branch_is_trimmed(admin_client: TestClient) -> None:
|
||||
"""Surrounding whitespace on the branch is trimmed (the schema's
|
||||
``_trim_branch`` + the API's defensive strip) before storage."""
|
||||
r = admin_client.post(
|
||||
"/api/git-sources",
|
||||
json={"url": "https://example.com/trim.git", "branch": " release/1.2\t"},
|
||||
)
|
||||
assert r.status_code == 201, r.text
|
||||
assert r.json()["branch"] == "release/1.2"
|
||||
|
||||
|
||||
def test_post_git_source_empty_branch_stores_null(admin_client: TestClient) -> None:
|
||||
"""Absent/None AND trimmed-EMPTY branch at create → stored NULL
|
||||
(the repo's default branch — the pre-phase-135 behavior): the
|
||||
create-time split from PATCH's clear leg (GitSourceIn docstring)."""
|
||||
for i, branch_value in enumerate((None, "", " ")):
|
||||
payload: dict[str, object] = {
|
||||
"url": f"https://example.com/def-{i}.git"
|
||||
} # distinct URLs: the loop must not 409 on itself
|
||||
if branch_value is not None:
|
||||
payload["branch"] = branch_value
|
||||
r = admin_client.post("/api/git-sources", json=payload)
|
||||
assert r.status_code == 201, r.text
|
||||
assert r.json()["branch"] is None
|
||||
|
||||
|
||||
def test_post_git_source_invalid_branch_422_fixed_detail_no_row(
|
||||
admin_client: TestClient, db: Session
|
||||
) -> None:
|
||||
"""Invalid branch names are 422 with the EXACT fixed detail
|
||||
``not a valid branch name`` — the input is never echoed (the
|
||||
credential-safety discipline, applied for consistency) — and no
|
||||
row is created."""
|
||||
for bad in ("a..b", "a b", "a@b", "-x", ".x", "/x", "x/", "x.", "HEAD", "head"):
|
||||
r = admin_client.post(
|
||||
"/api/git-sources",
|
||||
json={"url": "https://example.com/bad-branch.git", "branch": bad},
|
||||
)
|
||||
assert r.status_code == 422, f"{bad!r} must be rejected: {r.text}"
|
||||
assert r.json()["detail"] == "not a valid branch name" # EXACT
|
||||
assert bad not in r.text, f"422 must not echo the input ({bad!r})"
|
||||
# Nothing stored.
|
||||
assert db.execute(text("SELECT count(*) FROM git_sources")).scalar_one() == 0
|
||||
|
||||
|
||||
def test_post_git_source_oversized_branch_422(admin_client: TestClient, db: Session) -> None:
|
||||
""">200 chars: the schema's ``max_length=200`` rejects it before
|
||||
the handler (FastAPI's standard schema 422 — the same house
|
||||
precedent as the oversized-URL 422, which this router also
|
||||
answers with the framework's standard shape); no row is created.
|
||||
The API-layer branch 422s (charset/sequence/leading/trailing/
|
||||
HEAD) are the FIXED-detail ones — see the adjacent tests."""
|
||||
r = admin_client.post(
|
||||
"/api/git-sources",
|
||||
json={"url": "https://example.com/long-branch.git", "branch": "a" * 201},
|
||||
)
|
||||
assert r.status_code == 422
|
||||
assert db.execute(text("SELECT count(*) FROM git_sources")).scalar_one() == 0
|
||||
|
||||
|
||||
def test_post_local_source_with_branch_422(
|
||||
admin_client: TestClient, tmp_path: Path, db: Session
|
||||
) -> None:
|
||||
"""A ``branch`` on a LOCAL row is 422 with the fixed detail (local
|
||||
rows are walked, not cloned) — for BOTH valid and invalid branch
|
||||
names (the kind check precedes validation); nothing is stored."""
|
||||
real_dir = tmp_path / "local-branch"
|
||||
real_dir.mkdir()
|
||||
for bad in ("main", "a..b"):
|
||||
r = admin_client.post(
|
||||
"/api/git-sources",
|
||||
json={"kind": "local", "path": str(real_dir), "branch": bad},
|
||||
)
|
||||
assert r.status_code == 422, f"{bad!r} must be rejected: {r.text}"
|
||||
assert r.json()["detail"] == "branch is only valid for git sources"
|
||||
assert db.execute(text("SELECT count(*) FROM git_sources")).scalar_one() == 0
|
||||
|
||||
|
||||
# --- branch: PATCH tri-state (phase 135, task 02) ---------------------------
|
||||
|
||||
|
||||
def test_patch_branch_set_change_clear_and_noop(
|
||||
admin_client: TestClient, db: Session
|
||||
) -> None:
|
||||
"""The branch tri-state on a git row — the token's exact
|
||||
semantics field-for-field:
|
||||
|
||||
* set: ``"hotfix/x"`` → 200 reports it (stored + GET round-trip);
|
||||
* change: ``"hotfix/y"`` → 200 reports the new value;
|
||||
* clear: ``""`` → 200 reports ``null`` (stored NULL = the
|
||||
default branch); whitespace-only trims to empty → the same
|
||||
clear leg;
|
||||
* no-op: branch ABSENT (or explicit None) → 200, the stored
|
||||
branch survives (a flag-only PATCH must not touch it).
|
||||
"""
|
||||
created = admin_client.post(
|
||||
"/api/git-sources", json={"url": "https://example.com/patch-branch.git"}
|
||||
)
|
||||
assert created.status_code == 201
|
||||
sid = created.json()["id"]
|
||||
assert created.json()["branch"] is None # started on the default branch
|
||||
|
||||
def stored_branch() -> str | None:
|
||||
# The endpoint commits in its own session — expire this one's
|
||||
# identity map before reading back (the house pattern).
|
||||
db.expire_all()
|
||||
row = db.get(GitSource, uuid.UUID(sid))
|
||||
assert row is not None
|
||||
return row.branch
|
||||
|
||||
# SET — "hotfix/x".
|
||||
r = admin_client.patch(f"/api/git-sources/{sid}", json={"branch": "hotfix/x"})
|
||||
assert r.status_code == 200, r.text
|
||||
assert r.json()["branch"] == "hotfix/x"
|
||||
assert stored_branch() == "hotfix/x"
|
||||
assert (
|
||||
admin_client.get("/api/git-sources").json()["sources"][0]["branch"] == "hotfix/x"
|
||||
)
|
||||
|
||||
# CHANGE — "hotfix/y".
|
||||
r = admin_client.patch(f"/api/git-sources/{sid}", json={"branch": "hotfix/y"})
|
||||
assert r.status_code == 200, r.text
|
||||
assert r.json()["branch"] == "hotfix/y"
|
||||
assert stored_branch() == "hotfix/y"
|
||||
|
||||
# NO-OP — an unrelated field only: the branch is UNTOUCHED.
|
||||
r = admin_client.patch(f"/api/git-sources/{sid}", json={"include_hidden": True})
|
||||
assert r.status_code == 200, r.text
|
||||
assert r.json()["branch"] == "hotfix/y"
|
||||
assert r.json()["include_hidden"] is True
|
||||
assert stored_branch() == "hotfix/y"
|
||||
|
||||
# NO-OP — explicit None is absent too.
|
||||
r = admin_client.patch(f"/api/git-sources/{sid}", json={"branch": None})
|
||||
assert r.status_code == 200, r.text
|
||||
assert r.json()["branch"] == "hotfix/y"
|
||||
assert stored_branch() == "hotfix/y"
|
||||
|
||||
# CLEAR — "" (the tri-state's clear leg; a deliberate empty save).
|
||||
r = admin_client.patch(f"/api/git-sources/{sid}", json={"branch": ""})
|
||||
assert r.status_code == 200, r.text
|
||||
assert r.json()["branch"] is None
|
||||
assert stored_branch() is None
|
||||
|
||||
# Whitespace-only trims to empty → the same clear leg.
|
||||
admin_client.patch(f"/api/git-sources/{sid}", json={"branch": "hotfix/x"})
|
||||
r = admin_client.patch(f"/api/git-sources/{sid}", json={"branch": " "})
|
||||
assert r.status_code == 200, r.text
|
||||
assert r.json()["branch"] is None
|
||||
assert stored_branch() is None
|
||||
|
||||
|
||||
def test_patch_branch_invalid_422_leaves_row_unchanged(
|
||||
admin_client: TestClient, db: Session
|
||||
) -> None:
|
||||
"""A PRESENT invalid branch 422s with the EXACT fixed detail —
|
||||
and the stored branch is untouched (validation raises before the
|
||||
assignment; the row's other fields in the same body never
|
||||
half-apply)."""
|
||||
created = admin_client.post(
|
||||
"/api/git-sources",
|
||||
json={"url": "https://example.com/patch-bad.git", "branch": "stable"},
|
||||
)
|
||||
assert created.status_code == 201
|
||||
sid = created.json()["id"]
|
||||
|
||||
for bad in ("a..b", "HEAD", "x/"):
|
||||
r = admin_client.patch(
|
||||
f"/api/git-sources/{sid}", json={"branch": bad, "include_hidden": True}
|
||||
)
|
||||
assert r.status_code == 422, f"{bad!r}: {r.text}"
|
||||
assert r.json()["detail"] == "not a valid branch name"
|
||||
assert bad not in r.text
|
||||
# Nothing half-applied: the branch AND the flag are still the
|
||||
# originals.
|
||||
row = admin_client.get("/api/git-sources").json()["sources"][0]
|
||||
assert row["branch"] == "stable"
|
||||
assert row["include_hidden"] is False
|
||||
|
||||
|
||||
def test_patch_branch_on_local_row_422(admin_client: TestClient, tmp_path: Path) -> None:
|
||||
"""A PRESENT branch on a LOCAL row is the same fixed 422 as POST
|
||||
(local rows are walked, not cloned) — valid name or not; the row
|
||||
is untouched."""
|
||||
real_dir = tmp_path / "local-patch"
|
||||
real_dir.mkdir()
|
||||
created = admin_client.post(
|
||||
"/api/git-sources", json={"kind": "local", "path": str(real_dir)}
|
||||
)
|
||||
assert created.status_code == 201
|
||||
sid = created.json()["id"]
|
||||
|
||||
for branch_value in ("main", "a..b"):
|
||||
r = admin_client.patch(f"/api/git-sources/{sid}", json={"branch": branch_value})
|
||||
assert r.status_code == 422, f"{branch_value!r}: {r.text}"
|
||||
assert r.json()["detail"] == "branch is only valid for git sources"
|
||||
row = admin_client.get("/api/git-sources").json()["sources"][0]
|
||||
assert row["branch"] is None
|
||||
assert row["kind"] == "local"
|
||||
|
||||
Reference in New Issue
Block a user