chore(agent): phase roadmap from TODO.md, 3 phases (134-136)
Protocol B append: the TODO's three items became phases 134 (cancel a running sync), 135 (per-source git branch), and 136 (force-push reset). TODO.md cleared; each phase carries a dedicated Playwright E2E task. LLM-Generated: true
This commit is contained in:
@@ -0,0 +1,36 @@
|
||||
# Phase 134 — The owner can cancel a running sync from the Sources page
|
||||
|
||||
**Source:** `TODO.md` L3 — "Need to be able to cancel a sync after I've clicked the button."
|
||||
**Story:** n/a (TODO-derived phase, Protocol B append).
|
||||
**Context:** `app/api/sync.py` — `SyncStatus` (the four-state dataclass: `idle` / `running` / `success` / `failed`, phase-64 progress fields, phase-98 phase-machine fields), the module-level `_status` + `_task`, `POST /api/sync` (202 / 409 one-sync-at-a-time), `GET /api/sync/status`, and `_run_sync` — the in-process `asyncio` background task: the blocking git prelude (`scripts.git_sync.clone_or_pull` → `subprocess.run`, no await inside), then `await import_sources(...)` (per-file LLM awaits — a cancellation point between files), then `await regenerate_overview(...)` and `await generate_folder_summaries(...)` (both only on a changed KB / gap). The module docstring's shutdown contract: `CancelledError` is deliberately NOT swallowed — app shutdown cancels the task, and swallowing would mask a real stop. `import_sources` closes its own private session in a `finally` (leak-safe under a mid-import cancel). `frontend/assets/sources.js` — the sync-button lifecycle: `enterSyncRunningState` (live click + re-attach + every running tick), `settleSyncButton` (every terminal), `applySyncSuccess` / `applySyncFailure` / `applySyncIdle`, `startSyncPolling` (the 2 s two-job decision tree, 7 branches), `initSyncButton` (load-time re-attach). `frontend/index.html` — the `#sync-btn` markup (the Sources view section). E2E patterns: `tests/e2e/test_sync_button.py` (per-module app on a dedicated port, the `file://` fixture repo, the `_wait_sync_done` status-poll helper), `tests/e2e/slow_llm.py` (the delay-injecting proxy that makes a run's duration deterministic), `tests/e2e/mock_llm.py`. Unit: `tests/unit/test_sync_button.py`; integration: `tests/integration/test_sync_api.py`.
|
||||
|
||||
## Objective
|
||||
A sync that is already running can be stopped by the owner: while a run is in flight the Sources page shows a **Cancel** button; one click requests the cancel, the pipeline stops at the next boundary, the run lands in a new `cancelled` terminal state (reported by the status endpoint and rendered by the UI as a neutral result — cancel is intent, not failure), and the controls settle back to the retry-ready idle state. A subsequent sync starts cleanly (the partial run leaves a consistent KB — the importer's content-hash delta makes the next run idempotent).
|
||||
|
||||
## Dependencies
|
||||
- `133_pdf_screenshot_fallback` (todo) — pipeline order only; no code dependency.
|
||||
|
||||
## Design (shared by all tasks — the executor reads this, not the chat)
|
||||
- **The fifth state — `cancelled`** (ASSUMPTION A1): `SyncStatus.state` becomes `Literal["idle", "running", "success", "failed", "cancelled"]`. It is TERMINAL like `success` / `failed`: carries `finished_at`, clears `current_file` / `phase` / `current_summary` but keeps the run's final `files_done` / `files_total` and summary counts (the phase-64/98 keep-final-counts convention), `detail = {}`, and **`error = None`** (no error string — the UI never treats it as a failure: no error banner, no error modal, just the neutral "Sync cancelled" result line). `GET /api/sync/status` gains nothing structurally — the `state` key simply reports `"cancelled"`.
|
||||
- **`POST /api/sync/cancel`** (same `router`, so `require_admin` — 403 anonymous): while a run is in flight → set the module-level `_cancel_event` (an `asyncio.Event`, next to `_status` / `_task`) AND call `_task.cancel()` (unblocks whatever `await` the pipeline is parked on — e.g. a per-file LLM call inside the import) → **202** `{"detail": "cancel requested"}`. No run in flight (`_task` None or done) → **409** `{"detail": "no sync is running"}`. `start_sync` resets `_cancel_event` (clear) when it starts a run.
|
||||
- **Cancellation semantics — cooperative, best-effort** (ASSUMPTION A2): `_run_sync` gains (a) a private `_check_cancelled()` helper that raises `asyncio.CancelledError()` when `_cancel_event.is_set()`, called at the pipeline boundaries — the top of the per-source loop (before each clone/pull or local-dir check), immediately before `import_sources(...)`, and immediately before the `if summary.added + summary.updated > 0:` block (covers the overview + summaries spans); and (b) an `except asyncio.CancelledError:` clause (placed before the existing `except Exception`): if `_cancel_event.is_set()` → land the run in the `cancelled` terminal state + one INFO log line (PLAN §9: `sync: cancelled by user state=cancelled`), then `raise` (the task ends cancelled — `_task.done()` is True, the status object stays authoritative); otherwise (no user cancel — the app-shutdown path) → `raise` unchanged (the module docstring's shutdown contract holds: an unrequested cancellation is never swallowed). Consequence the UI/tests may rely on: a git subprocess already running at the cancel moment runs to completion (the blocking `subprocess.run` has no await inside) — the cancel takes effect at the NEXT boundary (after that source's git ops, between files, or before overview/summaries). During the import span the `_task.cancel()` from the endpoint interrupts at the next per-file await.
|
||||
- **The UI** (ASSUMPTION A3): a `#sync-cancel-btn` button in the same sync-control area as `#sync-btn` (`frontend/index.html`), `hidden` by default; secondary button styling consistent with the theme (contrast ≥4.5:1, `:focus-visible` ring — house UI rules). `sources.js` rules: SHOWN + ENABLED only while a SYNC run is running (`enterSyncRunningState` with `kind === "sync"` — the upload job is NOT cancellable in this phase, so the cancel button stays hidden while only the upload run drives the button); HIDDEN + DISABLED by `settleSyncButton` (every terminal/idle settle) and by every `applySync*` path. Click → `requestCancel()`: the cancel button disables IMMEDIATELY (never stale, no double request), `POST /api/sync/cancel`; 403 → the whoami-backstop hide-everything branch (same as the other endpoints); 409 → a harmless race (the run ended between ticks — the poll settles on the next tick, no error copy); 202 → the button stays disabled until the terminal (the poll's new `cancelled` branch settles everything); any other non-2xx → the phase-55 neutral retry copy in `#sync-result` ("Couldn't cancel the sync — try again.") + the cancel button re-enables. New poll-tree branch (after the `failed` branch, before the idle branch): `syncStatus.state === "cancelled"` → stop polling, settle the main button to "Sync sources", `#sync-result` = "Sync cancelled" (the aria-live announcer reads it), hide the error banner, hide the cancel button, `emitSyncStatus`. `initSyncButton` re-attach: `state === "cancelled"` → the same neutral settle (a page reload during/right after a cancel shows "Sync cancelled", never a stuck running state).
|
||||
|
||||
## Tasks
|
||||
1. `01_backend_cancel_state.md` — the `cancelled` state, `POST /api/sync/cancel`, and the cooperative cancel mechanics in `_run_sync`.
|
||||
2. `02_frontend_cancel_button.md` — the `#sync-cancel-btn` UI: show/hide rules, the cancel request lifecycle, the `cancelled` terminal branch in the poll tree + re-attach.
|
||||
3. `03_e2e_sync_cancel.md` — the dedicated Playwright E2E (the slow-LLM cancel window, the API contract, the full UI lifecycle, the clean re-sync after cancel).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: the cancel endpoint's contract (202/409/403), `_check_cancelled`, the `CancelledError` conversion (event set → `cancelled` state + re-raise; event unset → re-raise with the state untouched), the `SyncStatus` reset on a fresh start (a stale cancel event cannot bleed into the next run).
|
||||
- Integration: a real run started through the API is cancelled mid-run → the status reports `cancelled` (with `error: None`); a second run after the cancel completes successfully (the restart is clean).
|
||||
- E2E: one dedicated Playwright file, run in isolation (`tests/e2e/test_sync_cancel.py`) — the mock-only app with the `slow_llm` proxy for a deterministic cancel window.
|
||||
- Coverage: **>90%** on new/modified code (`app/`).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `POST /api/sync/cancel` (admin): 202 while a run is in flight, 409 `no sync is running` otherwise, 403 anonymous
|
||||
- [ ] a cancelled run lands in the `cancelled` terminal state (`error: None`, final counts kept) and the status endpoint reports it
|
||||
- [ ] the app-shutdown cancel contract is preserved (an unrequested `CancelledError` is never swallowed — a unit test pins it)
|
||||
- [ ] the UI: the Cancel button is visible exactly while a sync run is running, the `cancelled` terminal settles to "Sync cancelled" with no error banner/modal, and a re-sync after a cancel succeeds
|
||||
- [ ] `uv run pytest --cov=app --cov-report=term-missing` green, `app/` coverage >90%; `uv run pytest tests/e2e/test_sync_cancel.py -v --no-cov` green in isolation
|
||||
- [ ] no behavior change in completed phases (the uncancelled run is byte-identical: same states, same labels, same 409 double-trigger)
|
||||
@@ -0,0 +1,63 @@
|
||||
# Task 01 — Backend: the `cancelled` state, `POST /api/sync/cancel`, and the cooperative cancel in `_run_sync`
|
||||
|
||||
**Phase:** `134_sync_cancel` · **Source:** `TODO.md:3` — "Need to be able to cancel a sync after I've clicked the button."
|
||||
|
||||
## Objective
|
||||
The sync API can stop its own in-flight run: a new admin-only `POST /api/sync/cancel` requests the cancel; `_run_sync` converts a user-requested cancellation into a new `cancelled` terminal state (never an error); the shutdown-cancel contract is untouched.
|
||||
|
||||
## Work
|
||||
1. `app/api/sync.py` — the state machine:
|
||||
- `SyncStatus.state`: `Literal["idle", "running", "success", "failed", "cancelled"]` (the docstring's four-state paragraph becomes five; `cancelled` is terminal like `success` / `failed`).
|
||||
- Module level, next to `_status` / `_task`: `_cancel_event = asyncio.Event()`.
|
||||
- `start_sync`: reset the run's cancellation — `_cancel_event.clear()` before `_task = asyncio.create_task(_run_sync())` (a stale event from a previous cancelled run must never bleed into the next run).
|
||||
2. `app/api/sync.py` — the endpoint (same `router` → `require_admin` applies, 403 anonymous):
|
||||
```python
|
||||
@router.post("/cancel", status_code=202)
|
||||
async def cancel_sync() -> dict[str, str]:
|
||||
if _task is None or _task.done():
|
||||
raise HTTPException(status_code=409, detail="no sync is running")
|
||||
_cancel_event.set()
|
||||
_task.cancel()
|
||||
return {"detail": "cancel requested"}
|
||||
```
|
||||
202 is answered the moment the request is REGISTERED (the run may already be terminal — the UI's poll then settles on the terminal state; a 202-then-immediate-404-style race is impossible because the status object, not the task, is authoritative).
|
||||
3. `app/api/sync.py` — `_run_sync` mechanics:
|
||||
- A private helper: `def _check_cancelled() -> None:` — `if _cancel_event.is_set(): raise asyncio.CancelledError()`.
|
||||
- Call it at the boundaries: (a) the top of the per-source loop (before the `kind == "git"` / `kind == "local"` split), (b) immediately before `await import_sources(...)`, (c) immediately before the `if summary.added + summary.updated > 0:` block (covers both the overview and the summaries spans).
|
||||
- New `except asyncio.CancelledError:` clause placed BEFORE the existing `except Exception`:
|
||||
```python
|
||||
except asyncio.CancelledError:
|
||||
if _cancel_event.is_set():
|
||||
_status.state = "cancelled"
|
||||
_status.finished_at = datetime.now(UTC)
|
||||
_status.error = None
|
||||
_status.current_file = None
|
||||
_status.phase = None
|
||||
_status.current_summary = None
|
||||
logger.info(
|
||||
"sync: cancelled by user state=cancelled files_done=%d files_total=%d",
|
||||
_status.files_done, _status.files_total,
|
||||
)
|
||||
raise
|
||||
raise # app shutdown — the module-docstring contract: never swallow an unrequested cancel
|
||||
```
|
||||
The terminal keeps `files_done` / `files_total` and the summary counts (the phase-64/98 keep-final-counts convention); `detail` stays `{}` (a partial run reports no counts — ASSUMPTION A1: `error` is None, `detail` is empty).
|
||||
- Update the module docstring: the state machine is five-state; the cancel semantics are cooperative (a git subprocess in flight at the cancel moment runs to completion — the cancel lands at the next boundary); the `CancelledError` paragraph now names BOTH conversion paths (user-requested → `cancelled` state + re-raise; unrequested → re-raise).
|
||||
4. `GET /api/sync/status` — no shape change; update its docstring (the `state` value set now includes `cancelled`; `error` is null in that state).
|
||||
5. Unit tests — `tests/unit/test_sync_button.py` (extend) or a new `tests/unit/test_sync_cancel.py` (your call — the existing file owns the endpoint's contract, so extending it is the default):
|
||||
- `POST /api/sync/cancel` anonymous → 403; idle (never started, and after a terminal run) → 409 with the exact detail `no sync is running`.
|
||||
- A started run: the endpoint returns 202 `{"detail": "cancel requested"}`, sets the event, and cancels the task (drive `_run_sync` with a controllable await — e.g. a monkeypatched `import_sources` that awaits a `asyncio.Event` the test controls — then cancel via the endpoint and assert the task finished + `_status.state == "cancelled"`, `error is None`, `finished_at` set, final counts kept, `detail == {}`).
|
||||
- The shutdown contract pin: cancel the task WITHOUT setting the event (bare `task.cancel()`) → `_status.state` is NOT `cancelled` (stays `running` — the task died mid-run without a terminal state, exactly as today's shutdown path does) and the `CancelledError` propagated.
|
||||
- The stale-event pin: a run that was cancelled, then a FRESH run starts → `_cancel_event` is cleared (the new run completes to `success` with no cancel request).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: the behaviors in Work step 5 (the suite above IS the task's unit layer).
|
||||
- Integration: `tests/integration/test_sync_api.py` (extend) — start a real run against the `file://` fixture pattern the file already uses (a slow/many-file fixture or a monkeypatched delay at the per-file hook seam — follow the file's existing fakes), `POST /api/sync/cancel` while running → poll `GET /api/sync/status` → `state == "cancelled"` (and `error is None`); then a second full run → `state == "success"` (the restart after a partial cancel is clean — the importer's hash delta re-imports what the cancel skipped).
|
||||
- Coverage: **>90%** on new/modified code.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/unit/test_sync_button.py tests/integration/test_sync_api.py -v` green (plus the new test file, if split)
|
||||
- [ ] the five-state machine is exercised in tests: `cancelled` is reachable ONLY through a user-requested cancel (the unrequested-cancel pin is green)
|
||||
- [ ] the module docstring names the cooperative boundary semantics (a future reader sees why a long clone can outlive the click)
|
||||
- [ ] `uv run ruff check . && uv run pyright` clean
|
||||
- [ ] no behavior change for the uncancelled run (the existing sync tests stay green untouched)
|
||||
@@ -0,0 +1,45 @@
|
||||
# Task 02 — Frontend: the `#sync-cancel-btn` and the `cancelled` terminal branch in the poll tree
|
||||
|
||||
**Phase:** `134_sync_cancel` · **Source:** `TODO.md:3` — "Need to be able to cancel a sync after I've clicked the button."
|
||||
|
||||
## Objective
|
||||
The Sources page shows a Cancel button exactly while a sync run is in flight (live click, re-attached run, every running poll tick); clicking it requests the cancel and the UI settles into the neutral "Sync cancelled" terminal — no error banner, no modal, never stale.
|
||||
|
||||
## Work
|
||||
1. `frontend/index.html` (the Sources view section, next to `#sync-btn`): add the cancel control —
|
||||
```html
|
||||
<button type="button" id="sync-cancel-btn" class="sync-cancel-btn" hidden>Cancel</button>
|
||||
```
|
||||
- `hidden` by default (anonymous-safe + the idle state; the module reveals it, the same convention as `#sync-btn`).
|
||||
- A visible "Cancel" label (no icon-only — WCAG 2.4 name/role; the accessible name is the text).
|
||||
- `frontend/assets/styles.css`: `.sync-cancel-btn` — a secondary button style consistent with the theme (the house tokens, contrast ≥4.5:1 on `--surface`, `:focus-visible` ring like the other buttons, disabled state). No layout regression on the sync-control row (mobile included — the row already wraps; verify the 360 px case).
|
||||
2. `frontend/assets/sources.js` — the lifecycle (all inside the existing sync section; the §7.4 never-stale contract applies exactly as to the main button):
|
||||
- Element lookup: `const syncCancelBtn = root.querySelector("#sync-cancel-btn");`
|
||||
- Show/hide helpers: `showSyncCancelBtn()` (unhide + enable + `aria-label="Cancel sync"`) / `hideSyncCancelBtn()` (hide + disable).
|
||||
- `enterSyncRunningState(kind, …, status)`: when `kind === "sync"` → `showSyncCancelBtn()`; when `kind === "upload"` → `hideSyncCancelBtn()` (the upload job is NOT cancellable in this phase — the cancel button belongs to the sync run only).
|
||||
- `settleSyncButton(label)`: always `hideSyncCancelBtn()` (every terminal/idle settle — sync success/failed/cancelled, upload success/failed, both idle).
|
||||
- `requestCancel()` + the click listener (armed next to `syncBtn.addEventListener("click", startSync)`):
|
||||
- `syncCancelBtn.disabled = true` FIRST (never stale, one request at a time), `#sync-result` cleared.
|
||||
- `POST /api/sync/cancel`: 403 → the whoami-backstop branch (stop polling, hide the main button, `applySyncIdle()`) — the same depth as every other 403 in this module; 409 → NO-OP (the run ended between the tick and the click — the poll settles on the next tick; the cancel button is hidden by that settle; no error copy); 202 → nothing (the button stays disabled — the poll's `cancelled` branch settles everything); any other non-2xx / network failure → `#sync-result` = "Couldn't cancel the sync — try again." (the phase-55 neutral retry copy) + `syncCancelBtn.disabled = false` (the run is still going — the owner can retry).
|
||||
- New terminal branch in `startSyncPolling`'s tick — placed AFTER the `failed` branch and BEFORE the upload-success branch:
|
||||
```js
|
||||
if (syncStatus.state === "cancelled") {
|
||||
stopSyncPolling();
|
||||
applySyncCancelled(syncStatus);
|
||||
return;
|
||||
}
|
||||
```
|
||||
- `applySyncCancelled(status)`: `settleSyncButton("Sync sources")` (re-enables the main button, hides the cancel button), `syncResult.textContent = "Sync cancelled"`, `hideSyncError()`, `emitSyncStatus(status)`. No modal, no error styling (ASSUMPTION A1 — cancel is intent).
|
||||
- `initSyncButton` re-attach: `status.state === "cancelled"` → `applySyncCancelled(status)` (a reload during/right after a cancel shows the neutral result, never a stuck running state).
|
||||
- Update the section-header docstring (the decision tree gains the `cancelled` branch — the tree's "7" becomes "8" — and the cancel button's show/hide contract is documented there, the house pattern).
|
||||
3. No change to the upload job, the main sync button's existing labels, or the `bor:sync-status` event shape (the `cancelled` state simply flows through `emitSyncStatus` — the event's `detail` is the status object, as for every other state).
|
||||
|
||||
## Testing & Quality
|
||||
- No JS unit layer in this repo — the UI contract is pinned by the phase's E2E (task 03) and by the never-stale rules above.
|
||||
- Coverage: **>90%** on new/modified code (test-only-for-`app/` task — the bar is carried by the suite run; the JS is covered by the E2E).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] the Cancel button is visible + enabled ONLY while a sync run is running (hidden on boot/idle, hidden while only the upload run drives the button, hidden on every terminal)
|
||||
- [ ] clicking Cancel settles to "Sync cancelled" in `#sync-result` with the main button re-enabled, the cancel button hidden, no error banner and no modal
|
||||
- [ ] a 409 cancel (race) is silent; a failed cancel request shows the neutral retry copy and re-enables the button (the run keeps going)
|
||||
- [ ] `uv run ruff check . && uv run pyright` clean; the page renders with no console errors (verified in the E2E, task 03)
|
||||
@@ -0,0 +1,36 @@
|
||||
# Task 03 — E2E: `tests/e2e/test_sync_cancel.py` (the dedicated Playwright suite, run in isolation)
|
||||
|
||||
**Phase:** `134_sync_cancel` · **Source:** `TODO.md:3` — "Need to be able to cancel a sync after I've clicked the button."
|
||||
|
||||
## Objective
|
||||
The owner's cancel path is pinned end to end against the REAL pipeline: a real `file://` fixture sync whose duration is long enough to cancel (the `slow_llm` proxy), the API contract (403/409/202), the UI lifecycle (Cancel button visible → click → "Sync cancelled" settle), and the clean re-sync after a partial run.
|
||||
|
||||
## Work
|
||||
1. `tests/e2e/test_sync_cancel.py` (new) — the `test_sync_button.py` module-app pattern, adapted:
|
||||
- **Ports/env:** per-module app on its own port — `APP_PORT = int(os.environ.get("E2E_APP_PORT_SYCNCEL", "8133"))`; `BOR_STATIC_DIR`, `BOR_RELEVANCE_THRESHOLD=0.30` (mock-calibrated, the conftest pattern), `BOR_ADMIN_PASSWORD` / `BOR_SESSION_SECRET` from `e2e.conftest`, `BOR_GIT_SOURCES=file://<fixture repo>`, a dedicated `BOR_SOURCES_DIR`.
|
||||
- **The slow-LLM proxy:** spawn `tests/e2e/slow_llm.py` the conftest way (the `SLOW_LLM_DELAY_S` env — 0.3 s is a good default: ~40 fixture files → ~12+ s of import — a wide, deterministic cancel window against the UI's 2 s poll) in front of the mock LLM (the `mock_llm` fixture from `e2e.conftest`); `BOR_LLM_BASE_URL` points at the proxy.
|
||||
- **The fixture repo** (module-scoped, `tmp_path_factory`, real `git` subprocess — the `sync_git_repo` pattern): ~40 small A9-format `.md` files under a `notes/` folder (each one unique enough to import independently; ONE sentinel file — `Marker: RESE-CANCEL-SENTINEL-4f1e` — the assertion target for the re-sync).
|
||||
- **`_wait_sync_state(page, app_url, states, timeout_s)`** — the `test_sync_button._wait_sync_done` helper generalized to a set of terminal states (here `{"cancelled"}` and `{"success", "failed"}` for the re-sync).
|
||||
2. Tests (Playwright Mapping Rule — each test names the behavior it pins):
|
||||
- `test_anonymous_cancel_403` — anonymous: `POST /api/sync/cancel` → 403 (the admin-only surface, same as the other two endpoints); `#sync-cancel-btn` is never revealed (the whoami gate).
|
||||
- `test_cancel_when_idle_409` — logged-in, no run in flight: `POST /api/sync/cancel` → 409 with the exact detail `no sync is running`; the Cancel button is hidden in the UI (boot + idle).
|
||||
- `test_admin_cancel_lifecycle` — the owner's exact flow:
|
||||
1. login (lands on `/sources.html`);
|
||||
2. click `#sync-btn` → `#sync-label` "Syncing…", the main button disabled, **`#sync-cancel-btn` visible + enabled** (within the 2 s poll tick — the running state is already entered synchronously by the 202);
|
||||
3. confirm via the status endpoint that the run is `running` (the deterministic layer — `page.request` poll at ~100 ms, the `slow_llm` module's convention);
|
||||
4. click `#sync-cancel-btn` → the cancel button disables immediately (never stale);
|
||||
5. `_wait_sync_state(…, {"cancelled"})` → the status endpoint reports `state == "cancelled"` with `error is None`;
|
||||
6. the UI settles (timeout-bound): main button ENABLED + label "Sync sources", `#sync-cancel-btn` hidden, `#sync-result` text "Sync cancelled", the error banner hidden, no error modal in the DOM;
|
||||
7. **the re-sync is clean:** click `#sync-btn` again → "Synced HH:MM" (the `SYNCED_LABEL` regex) + `#sync-result` includes "40 added" (or the fixture's file count — the first FULL pass; the cancelled run's partial import is idempotent under the hash delta — assert the sentinel row is present in the catalog via the drill-down tree, the `_expect_fixture_row` pattern) → the cancel never corrupted the KB.
|
||||
- Wall-time guard: the whole module < 120 s (40 files × 0.3 s proxy delay + the re-sync's 40 × 0.3 s — if it drifts, cut the file count, not the guard).
|
||||
3. Header docstring: the story ("the owner's TODO item, L3"), the run command (`uv run pytest tests/e2e/test_sync_cancel.py -v --no-cov`), the isolation note (per-module app, dedicated port — the combined-run convention), the slow-LLM provenance (the proxy is the only thing that makes the cancel window deterministic).
|
||||
|
||||
## Testing & Quality
|
||||
- E2E: this file IS the task's suite; it runs in ISOLATION (`--no-cov`, the AGENTS.md rule 9 gate) with the DB up (`podman compose up -d db`) and git on PATH.
|
||||
- Coverage: **>90%** on new/modified code (test-only task — the bar is carried by the suite run).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/e2e/test_sync_cancel.py -v --no-cov` green in isolation
|
||||
- [ ] the cancel window is deterministic (the test does not race the 2 s poll — the `slow_llm` delay is the gate; a rerun on a quiet machine passes)
|
||||
- [ ] the re-sync after a cancel imports the FULL fixture (the sentinel row present — a partial cancel never leaves a corrupted or permanently-incomplete KB)
|
||||
- [ ] the `cancelled` state's `error` is null end to end (the UI can never render it as a failure)
|
||||
@@ -0,0 +1,40 @@
|
||||
# Phase 135 — A git source can pin the branch it mirrors
|
||||
|
||||
**Source:** `TODO.md` L4 — "Need to be able to specify the branch for a git source"
|
||||
**Story:** n/a (TODO-derived phase, Protocol B append).
|
||||
**Context:** `app/models.py::GitSource` (id, url, kind, path, ignore_paths, include_hidden, token, added_at — no branch column; the phase-121 `token` column is the nullable-write-only precedent); `alembic/versions/` (0023 is the newest; every migration is reversible — the house rule); `app/schemas.py` — `GitSourceIn` / `GitSourceOut` / `GitSourceRow` / `GitSourcePatchIn` (the token field is the write-trimmed precedent; the output shapes use `extra="forbid"`); `app/api/git_sources.py` — `POST` (the `_create_git_row` / `_create_local_row` split, the fixed-detail 422s, the token normalization), `PATCH /{source_id}` (the per-field present-wins tri-state — the token's absent/None = no change, `""` = clear, non-empty = replace), `GET` (DB rows vs the env-fallback synthetic rows); `scripts/git_sync.py::clone_or_pull(url, dest)` (fresh clone with full history; the shallow probe + one-time `git fetch --unshallow` self-heal (phase 107 D11); `git pull --ff-only` — the fail-loud D12 contract); the TWO call sites that resolve effective sources and clone git rows: `app/api/sync.py::_run_sync` and `scripts/import_docs.py::_resolve_sources` (both already pass `clone_url_for(row)` + `repo_name(row.url)`); `file_commit_dates` (walks the checkout — untouched); the UI: `frontend/index.html` — the add form `#git-source-form` (the url + token inputs) and the per-row editor dialog `#ignore-editor-dialog` (the source line, the ignore textarea, the token field); `frontend/assets/git-sources.js` — `loadSources` / `makeRow` (the "hidden on" text-tag idiom in the source cell), `wireAddForm` (`opts.body(value, token)` builds the POST body), the dialog open/save handlers (the PATCH body builder), the `toggleHidden` never-stale lifecycle as the per-row control precedent.
|
||||
|
||||
## Objective
|
||||
A git source can specify the branch it mirrors: the owner gives a branch when adding a git source (or edits it per-row later), the clone/pull honors it (a fresh clone checks the branch out; an existing checkout tracks it), BOTH entry points — the Sync button and the `import_docs` CLI — honor it, and every row without a branch (all existing rows + the env-fallback rows) behaves byte-identically to today (default branch, `git pull --ff-only`).
|
||||
|
||||
## Dependencies
|
||||
- `134_sync_cancel` (todo) — pipeline order only; no code dependency (independent surfaces).
|
||||
|
||||
## Design (shared by all tasks — the executor reads this, not the chat)
|
||||
- **The column** (ASSUMPTION B1): `git_sources.branch` — `TEXT NULL`, server default NULL; alembic **0024** (reversible — `add_column` / `drop_column`, no data migration: NULL = "the repo's default branch", so every existing row is unchanged by definition). `GitSource.branch: Mapped[str | None] = mapped_column(Text, nullable=True)`.
|
||||
- **Validation** (ASSUMPTION B2): a pure helper `validate_branch_name(raw: str) -> str` in `app/rag/git_sources.py` (next to `normalize_credential` — the write-path helper home; the API layer maps its `ValueError` to the fixed-detail 422). Contract: trim → reject empty; reject length > 200; reject anything not matching `^[A-Za-z0-9._/-]+$` (alphanumerics + dot, underscore, hyphen, slash — whitespace/`@`/`~`/`^`/`:`/`?`/`*`/`[`/backslash are all out); reject a name containing `..` or `//`; reject a leading `-`, `.`, or `/`; reject a trailing `/` or `.`; reject `HEAD` (case-insensitive — git's refname; a branch literally named `head` is a documented cost of the simple rule). Returns the trimmed value. The API's 422 detail is the FIXED string `not a valid branch name` (never echoes the input — the house credential-safety discipline, applied for consistency).
|
||||
- **The write paths** (ASSUMPTION B3): `GitSourceIn.branch: str | None` (optional at create — absent/empty → NULL; trimmed in the schema before the length constraint, the `_trim_url` precedent). `GitSourcePatchIn.branch: str | None = None` — TRI-STATE exactly like the token: absent/None = no change; `""` = clear (stored NULL); non-empty = set (re-validated). A `branch` on a LOCAL row (POST or PATCH) → 422 fixed detail `branch is only valid for git sources` (local rows are walked, not cloned). `GitSourceOut.branch: str | None` + `GitSourceRow.branch: str | None` (env-fallback synthetic rows report `None` — no DB row to store one on, the `ignore_paths: []` precedent).
|
||||
- **`clone_or_pull(url, dest, branch: str | None = None)`** (ASSUMPTION B4): `branch=None` → the EXISTING code path byte-identical (same argv: clone without `--branch`; probe; unshallow-if-shallow; `git pull --ff-only`). A fresh clone with a branch → `git clone --branch <b> url dest`. An existing checkout with a branch → (1) the HEAD GUARD first: `git rev-parse --abbrev-ref HEAD` — if the result is not `<b>` → `GitSyncError("checkout <dest> is on branch '<current>' but the source specifies branch '<b>' — remove and re-add the source to switch branches")` (ASSUMPTION B5 — fail loud, the D12 convention; NO silent checkout switch in this phase: the checkout may serve other rows' documents, and a switch would silently re-map the source's file set; phase 136's explicit-ref reset makes the ref unambiguous later without changing this guard); (2) the shallow probe + one-time unshallow (unchanged); (3) `git fetch origin <b>`; (4) `git merge --ff-only FETCH_HEAD` (the single-fetch ff pull — the sequence phase 136 extends with its reset fallback).
|
||||
- **Wiring:** both call sites pass `row.branch` — `app/api/sync.py::_run_sync` and `scripts/import_docs.py::_resolve_sources`: `clone_or_pull(clone_url_for(row), sources_root / repo_name(row.url), row.branch)`. Env-fallback synthetic rows carry `branch=None` (the `GitSource(url=…, kind="git")` constructor default). `file_commit_dates`, the checkout naming (`repo_name` stays on the BARE url), and the ignore/hidden/date maps are untouched.
|
||||
- **The UI** (ASSUMPTION B6): the add form gains an optional `#git-source-branch` text input (maxlength 200, `autocomplete="off"`, hint "optional — default: the repo's default branch") between the URL and Token fields; the submit body becomes `(url, token, branch) => ({ url, ...(branch ? { branch } : {}), ...(token ? { token } : {}) })` — a blank branch OMITS the key (NULL). The per-row editor dialog gains a `#ignore-editor-branch` field (visible for GIT rows only — local rows' dialogs hide it; it opens prefilled from `s.branch ?? ""`; the PATCH body for a git row ALWAYS includes `branch: <trimmed value>` — empty = the tri-state's clear leg, so clearing a branch is a deliberate empty save, unlike the token's blank = keep). Every STORED git row with a branch shows a `branch: <name>` text tag in the source cell (the "hidden on" tag idiom — text + background, WCAG: the text IS the signal, never color alone).
|
||||
|
||||
## Tasks
|
||||
1. `01_branch_column.md` — the alembic 0024 migration, the `GitSource.branch` column, the schema fields, the migration round-trip test.
|
||||
2. `02_branch_api.md` — `validate_branch_name` + POST/PATCH/GET wiring (the tri-state, the local-row 422, the env rows).
|
||||
3. `03_clone_pull_branch.md` — `clone_or_pull(…, branch)` (the `--branch` clone, the HEAD guard, the fetch+ff sequence) + both call sites.
|
||||
4. `04_branch_ui.md` — the add-form field, the per-row editor field, the row tag.
|
||||
5. `05_e2e_git_source_branch.md` — the dedicated Playwright E2E (the two-branch `file://` fixture, the UI add-with-branch → sync → branch content, the fail-loud mismatch, the API contracts).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: `validate_branch_name` (the full accept/reject matrix); `clone_or_pull`'s argv sequences with mocked `subprocess` (the `tests/unit/test_git_sync.py` pattern — fresh-clone `--branch`, the existing-checkout guard → probe → fetch → merge, the mismatch raise, `branch=None` byte-identical).
|
||||
- Integration: the migration round-trip (0024); the API round-trips (create/patch/list, the tri-state, the 422s); a real `file://` two-branch clone through `clone_or_pull` (the `tests/integration/test_import_docs_git.py` pattern) + the sync pipeline importing the branch's content.
|
||||
- E2E: one dedicated Playwright file, run in isolation.
|
||||
- Coverage: **>90%** on new/modified code (`app/`).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] a git source added with a branch mirrors THAT branch (fresh clone + subsequent pulls) through both the Sync button and the CLI — pinned by the E2E + the integration test
|
||||
- [ ] a row with no branch (all existing rows + env rows) behaves byte-identically to pre-phase (the `branch=None` argv pin is green)
|
||||
- [ ] invalid branch names are rejected with the fixed 422 (never echoing input); a branch on a local row is a 422; the PATCH tri-state (set/change/clear) round-trips
|
||||
- [ ] setting a branch on an existing checkout on a different branch fails the sync LOUDLY with the remove-and-re-add message (no silent switch)
|
||||
- [ ] `uv run pytest --cov=app --cov-report=term-missing` green, `app/` coverage >90%; `uv run pytest tests/e2e/test_git_source_branch.py -v --no-cov` green in isolation
|
||||
- [ ] no behavior change in completed phases (the token/ignore/hidden round-trips, the sync pipeline, the CLI — all existing tests green untouched)
|
||||
@@ -0,0 +1,38 @@
|
||||
# Task 01 — The `git_sources.branch` column: migration 0024, the model field, the schema fields
|
||||
|
||||
**Phase:** `135_git_source_branch` · **Source:** `TODO.md:4` — "Need to be able to specify the branch for a git source"
|
||||
|
||||
## Objective
|
||||
The registry can store a per-source branch: a reversible alembic 0024 adds the nullable `branch` column, the ORM model carries it, and the four API schema shapes report/accept it — with NULL meaning "the repo's default branch" so every existing row is unchanged.
|
||||
|
||||
## Work
|
||||
1. `alembic/versions/0024_git_source_branch.py` (new) — follow the shape of `0021_git_source_token.py` (read it first):
|
||||
- `revision = "0024"`, `down_revision = "0023"` (verify the actual latest revision id in `alembic/versions/` at execution time — the file numbering is the convention, the `down_revision` chain is the truth).
|
||||
- `upgrade`: `op.add_column("git_sources", sa.Column("branch", sa.Text(), nullable=True))`.
|
||||
- `downgrade`: `op.drop_column("git_sources", "branch")` (reversible — the house rule; NULL = default branch means no data migration in either direction).
|
||||
2. `app/models.py::GitSource` — add:
|
||||
```python
|
||||
#: The branch to mirror (phase 135): NULL = the repo's default branch
|
||||
#: (every pre-phase-135 row and every env-fallback row — byte-identical
|
||||
#: clone/pull behavior). A non-NULL value is validated at the API
|
||||
#: boundary (app.rag.git_sources.validate_branch_name) — the column
|
||||
#: itself is plain Text.
|
||||
branch: Mapped[str | None] = mapped_column(Text, nullable=True)
|
||||
```
|
||||
3. `app/schemas.py` (the four shapes — each docstring gains its phase-135 paragraph):
|
||||
- `GitSourceIn.branch: str | None = Field(default=None, max_length=200)` — optional at create (absent → NULL); a `field_validator` trims before the length constraint runs (the `_trim_url` precedent) — note the API layer treats a trimmed-EMPTY value as absent at create and as the tri-state's clear leg at PATCH (document that split in the docstring).
|
||||
- `GitSourceOut.branch: str | None` — the stored value (NULL → null in the response); the shape keeps `extra="forbid"` (no new omission risk — the field is explicit).
|
||||
- `GitSourceRow.branch: str | None` — DB rows report the stored value; env-fallback synthetic rows report `None` (the `ignore_paths: []` / `include_hidden: False` precedent — no DB row to store one on).
|
||||
- `GitSourcePatchIn.branch: str | None = None` — the tri-state (absent/None = no change; `""` = clear; non-empty = set — the token's precedent, documented in the docstring).
|
||||
4. `uv run alembic upgrade head` against the dev DB (and `downgrade -1` + `upgrade head` once — the reversibility proof).
|
||||
5. `tests/integration/test_migration_0024.py` (new — the `test_migration_0021.py` pattern): the upgrade adds the nullable column to a table WITH pre-existing rows (all report NULL — the byte-identical guarantee), a row can take a branch value, the downgrade drops the column.
|
||||
|
||||
## Testing & Quality
|
||||
- Integration: the migration round-trip (the suite above).
|
||||
- Coverage: **>90%** on new/modified code (the model/schema surface is exercised by task 02's API tests — this task's own gate is the migration test + the suite run).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run alembic upgrade head && uv run alembic downgrade -1 && uv run alembic upgrade head` succeeds on a scratch DB (the reversibility proof)
|
||||
- [ ] pre-existing rows read `branch: None` (the integration test asserts on seeded rows — no backfill, no default)
|
||||
- [ ] `uv run pytest tests/integration/test_migration_0024.py -v` green
|
||||
- [ ] `uv run ruff check . && uv run pyright` clean
|
||||
@@ -0,0 +1,52 @@
|
||||
# Task 02 — API: branch validation, POST/PATCH/GET wiring (the tri-state, the local-row 422, the env rows)
|
||||
|
||||
**Phase:** `135_git_source_branch` · **Source:** `TODO.md:4` — "Need to be able to specify the branch for a git source"
|
||||
|
||||
## Objective
|
||||
The admin API accepts, validates, stores, and reports a branch per git source: a pure `validate_branch_name` helper is the single choke point, POST stores it (git rows only), PATCH edits it with the token's tri-state, and GET/POST-201/PATCH-200 report it — invalid names get a fixed-detail 422 that never echoes the input.
|
||||
|
||||
## Work
|
||||
1. `app/rag/git_sources.py` — the pure validator (next to `normalize_credential`; no DB, no logging of the raw value beyond the fixed reason):
|
||||
```python
|
||||
_BRANCH_RE = re.compile(r"^[A-Za-z0-9._/-]+$")
|
||||
|
||||
def validate_branch_name(raw: str) -> str:
|
||||
"""Trim + validate a branch name (phase 135, ASSUMPTION B2).
|
||||
|
||||
Returns the trimmed name; raises ValueError (fixed reason strings,
|
||||
for logs only — the API maps to the fixed-detail 422) when the
|
||||
name is empty, > 200 chars, outside the safe charset
|
||||
([A-Za-z0-9._/-]+), contains ".." or "//", starts with "-"/"."/"",
|
||||
ends with "/" or ".", or is HEAD (case-insensitive).
|
||||
"""
|
||||
```
|
||||
(The leading-slash check is `startswith("/")`; write the helper with the checks in this order: trim → empty → length → charset → `..`/`//` → leading → trailing → HEAD.)
|
||||
2. `app/api/git_sources.py`:
|
||||
- `_create_git_row`: after the URL contract (and the token normalization — order: URL shape → credential normalization → duplicate check → branch): `branch = (payload.branch or "").strip()`; if `branch`: `validate_branch_name` → store; else store `None`. A `ValueError` → `HTTPException(422, detail="not a valid branch name")` (FIXED detail — the input is never echoed).
|
||||
- `_create_local_row`: `payload.branch` present and non-empty after strip → `HTTPException(422, detail="branch is only valid for git sources")` (local rows are walked, not cloned — the fixed detail, the kind-field rules' precedent).
|
||||
- `patch_git_source`: a new independent present-wins field — `if payload.branch is not None:` the row must be `kind == "git"` (local → the same fixed 422); `""` → `row.branch = None` (the tri-state's clear leg); non-empty → `validate_branch_name` → set. No Integrity interaction (branch never touches a unique index) — no 409 path. Update the docstring's field list (ignore_paths / include_hidden / token / branch).
|
||||
- `list_git_sources`: DB rows report `branch=row.branch`; env-fallback rows report `branch=None`. Update the docstring.
|
||||
- `create_git_source` / `patch_git_source` responses: include `branch=row.branch` in the `GitSourceOut` construction.
|
||||
- The module docstring's route summary gains the branch clauses (the house one-paragraph-per-route contract).
|
||||
3. Unit tests — `tests/unit/test_git_sources_branch.py` (new; the validator has no existing home):
|
||||
- Accept: `main`, `release/1.2`, `feature_x`, `a.b/c`, `v2-rc.1` (trim applied: `" main "` → `main`).
|
||||
- Reject (each → `ValueError`): `""`, `" "`, 201 chars, `a b`, `a@b`, `a~b`, `a:b`, `a?b`, `a*b`, `a[b`, `a\b`, `a..b`, `a//b`, `-x`, `.x`, `/x`, `x/`, `x.`, `HEAD`, `head`, `HeAd`.
|
||||
4. Integration tests — `tests/integration/test_git_sources_api.py` (extend — the file already owns this router's contract):
|
||||
- POST a git source with `branch: "release"` → 201 reports `branch: "release"`; GET reports it.
|
||||
- POST with an invalid branch (`"a..b"`) → 422 `not a valid branch name` (the detail is EXACT — no input echo); the row is not created.
|
||||
- POST a LOCAL source with a branch → 422 `branch is only valid for git sources`.
|
||||
- PATCH the tri-state: set (`"hotfix/x"`) → 200 reports it; change (`"hotfix/y"`); clear (`""`) → 200 reports `null`; absent (PATCH `{"include_hidden": true}` only) → the branch is untouched.
|
||||
- PATCH a local row's branch → 422.
|
||||
- Env-fallback rows (empty table + `BOR_GIT_SOURCES` env): GET reports `branch: null` for every synthetic row.
|
||||
- The existing token/ignore/hidden round-trips stay green (no regression).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: the validator matrix (step 3).
|
||||
- Integration: the API round-trips (step 4).
|
||||
- Coverage: **>90%** on new/modified code.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/unit/test_git_sources_branch.py tests/integration/test_git_sources_api.py -v` green
|
||||
- [ ] every 422 detail is a fixed string (a grep of the new code shows no f-string echoing the branch input)
|
||||
- [ ] the tri-state matches the token's semantics field-for-field (the PATCH docstring documents both)
|
||||
- [ ] `uv run ruff check . && uv run pyright` clean
|
||||
@@ -0,0 +1,48 @@
|
||||
# Task 03 — `clone_or_pull(…, branch)` and both call sites (sync pipeline + CLI)
|
||||
|
||||
**Phase:** `135_git_source_branch` · **Source:** `TODO.md:4` — "Need to be able to specify the branch for a git source"
|
||||
|
||||
## Objective
|
||||
The git plumbing honors a row's branch: a fresh clone checks it out (`--branch`), an existing checkout is tracked (fetch + ff-merge of that ref) with a fail-loud guard when the checkout is on a different branch, and `branch=None` keeps the pre-phase argv byte-identical — wired through BOTH entry points (the Sync button and the `import_docs` CLI).
|
||||
|
||||
## Work
|
||||
1. `scripts/git_sync.py::clone_or_pull` — the new optional parameter (the module docstring's clone/pull paragraph + the function docstring gain the branch contract):
|
||||
```python
|
||||
def clone_or_pull(url: str, dest: Path | str, branch: str | None = None) -> Path:
|
||||
```
|
||||
- **`branch is None`** — the EXISTING code path, byte-identical: clone without `--branch`; `rev-parse --is-shallow-repository` probe; `fetch --unshallow` while shallow; `git pull --ff-only`. (The argv pin in the unit tests is the proof.)
|
||||
- **Fresh clone + branch** (`dest` absent or no `.git`): `git clone --branch <b> url dest`.
|
||||
- **Existing checkout + branch**, in order:
|
||||
1. HEAD GUARD: `git rev-parse --abbrev-ref HEAD` → let `current` be the result. If `current != branch` → `GitSyncError(f"checkout {dest} is on branch '{current}' but the source specifies branch '{branch}' — remove and re-add the source to switch branches")` (ASSUMPTION B5 — fail loud per D12; the message names the checkout and both branch names — no URL, so no credential exposure).
|
||||
2. the shallow probe + one-time `git fetch --unshallow` self-heal (UNCHANGED from today — the phase-107 D11 contract).
|
||||
3. `git fetch origin <b>` → `git merge --ff-only FETCH_HEAD` (the single-fetch ff pull of the specified ref — the sequence phase 136 extends with its reset fallback; a non-ff merge still fails loud HERE — the force-push fix is phase 136, not this one).
|
||||
- All commands go through `run_git` (the A11 single invocation site — unchanged).
|
||||
2. The call sites — pass `row.branch`:
|
||||
- `app/api/sync.py::_run_sync` (the per-source loop, the `kind == "git"` arm): `root = clone_or_pull(clone_url_for(row), sources_root / repo_name(row.url), row.branch)`.
|
||||
- `scripts/import_docs.py::_resolve_sources` (the same shape): `root = clone_or_pull(clone_url_for(row), sources_root / repo_name(row.url), row.branch)`.
|
||||
- Both files' docstrings/comments name the branch argument (the phase-106/121 comment style). Env-fallback synthetic rows carry `branch=None` (the `GitSource(url=…, kind="git")` constructor — no argument needed, the model default is None — verified by the env-fallback integration path).
|
||||
- `file_commit_dates(root)` is untouched (it walks whatever the checkout holds — for a branch checkout that is the branch's tree, exactly the head-of-branch semantics the owner asked for).
|
||||
3. Unit tests — `tests/unit/test_git_sync.py` (extend — the mocked-`subprocess` argv pattern the file already uses):
|
||||
- `branch=None` fresh + existing: the argv sequence is byte-identical to the pre-phase expectations (the existing tests stay green + add an explicit argv-equality pin if the file doesn't already pin it).
|
||||
- Fresh clone + `branch="release"`: `["git", "clone", "--branch", "release", url, dest]`.
|
||||
- Existing + matching HEAD (`rev-parse --abbrev-ref HEAD` → `"release"`): the sequence `rev-parse --abbrev-ref HEAD` → `rev-parse --is-shallow-repository` → (`fetch --unshallow` when the probe says shallow) → `fetch origin release` → `merge --ff-only FETCH_HEAD`.
|
||||
- Existing + mismatched HEAD (`"main"` vs `branch="release"`): raises `GitSyncError` whose message contains both branch names and the checkout path — and NO fetch/merge argv was invoked (the guard is first).
|
||||
- The mismatch message never contains the URL (credential-safety — even for a plain URL the message is URL-free).
|
||||
4. Integration tests — `tests/integration/test_import_docs_git.py` (extend — the real-`git`-`file://` pattern the file already uses):
|
||||
- Build a two-branch fixture repo (real git: `main` with file A, `release` with file B — different content per branch).
|
||||
- `clone_or_pull(url, dest, "release")` fresh → the checkout's HEAD branch is `release` and file B is present (file A absent).
|
||||
- A second `clone_or_pull` after the origin gains a commit on `release` → ff-merged (file B updated, no error).
|
||||
- The mismatch case: clone fresh with no branch (HEAD = `main`), then `clone_or_pull(url, dest, "release")` → `GitSyncError` (the remove-and-re-add message), the checkout untouched (still on `main`).
|
||||
- The full sync pipeline with a branch row: a `git_sources` row (`kind="git"`, `branch="release"`) → `_run_sync`'s source resolution imports file B (the `tests/integration/test_sync_api.py` fixture shape) — or at minimum `_resolve_sources`-level for the CLI path (follow whichever seam the file already exercises; the E2E, task 05, pins the button path end to end).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: the argv matrix (step 3) — the `branch=None` byte-identical pin is the regression gate for every existing deployment.
|
||||
- Integration: the real-git two-branch scenarios (step 4).
|
||||
- Coverage: **>90%** on new/modified code.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/unit/test_git_sync.py tests/integration/test_import_docs_git.py -v` green
|
||||
- [ ] `branch=None` produces the exact pre-phase argv (the pin is in the suite — a future refactor that rewrites the default path breaks it)
|
||||
- [ ] the mismatch fails loud BEFORE any fetch (the unit test asserts no fetch argv after the guard)
|
||||
- [ ] both entry points pass `row.branch` (a grep of the two call sites + the integration test covering each)
|
||||
- [ ] `uv run ruff check . && uv run pyright` clean
|
||||
@@ -0,0 +1,43 @@
|
||||
# Task 04 — UI: the add-form field, the per-row editor field, and the row tag
|
||||
|
||||
**Phase:** `135_git_source_branch` · **Source:** `TODO.md:4` — "Need to be able to specify the branch for a git source"
|
||||
|
||||
## Objective
|
||||
The Sources manager (git-sources view) lets the owner see, set, change, and clear a source's branch: an optional Branch input in the add form, a Branch field in the per-row editor dialog (git rows only, prefilled, empty-save = clear), and a `branch: <name>` text tag on every stored git row that has one — all on the §7.4 never-stale lifecycle.
|
||||
|
||||
## Work
|
||||
1. `frontend/index.html` (the `#view-git-sources` section):
|
||||
- **The add form** (`#git-source-form`) — between the URL field and the Token field:
|
||||
```html
|
||||
<label for="git-source-branch">Branch <span class="field-hint">optional — default: the repo's default branch</span></label>
|
||||
<input id="git-source-branch" name="branch" type="text" maxlength="200" autocomplete="off" placeholder="main, release/1.2, …">
|
||||
```
|
||||
(The visible `<label for=…>` is the WCAG input-label rule, same as the url/token fields.)
|
||||
- **The per-row editor dialog** (`#ignore-editor-dialog`) — after the source line, before the ignore textarea:
|
||||
```html
|
||||
<label for="ignore-editor-branch">Branch</label>
|
||||
<input id="ignore-editor-branch" type="text" maxlength="200" autocomplete="off" placeholder="(default branch)">
|
||||
<p class="ignore-editor-branch-hint">Leave empty for the repo's default branch.</p>
|
||||
```
|
||||
The hint uses the existing `.field-hint` / muted-copy idiom (match the token field's hint styling — no new CSS class unless the existing one doesn't fit).
|
||||
2. `frontend/assets/git-sources.js`:
|
||||
- **Element lookups:** `const branchInput = root.querySelector("#git-source-branch");` and `const ignoreBranchInput = root.querySelector("#ignore-editor-branch");` (next to the existing token lookups).
|
||||
- **Add form:** thread the branch through `wireAddForm` — the opts object gains `branchInput`, and the git-kind body builder becomes `body: (url, token, branch) => ({ url, ...(branch ? { branch } : {}), ...(token ? { token } : {}) })` (a blank branch OMITS the key — the token's blank-omits precedent; the submit handler reads `branchInput.value.trim()` and clears it on 201 alongside the url/token — the credential-free inputs clear together).
|
||||
- **Per-row editor dialog:**
|
||||
- Open: for a git row (`s.kind === "git"`) — prefill `ignoreBranchInput.value = s.branch ?? ""` and show the field; for a local row — hide the field (label + input + hint) and the save body OMITS `branch` (local rows can't take one — the API's 422 is the backstop, the UI simply doesn't offer it).
|
||||
- Save: the PATCH body for a git row ALWAYS includes `branch: ignoreBranchInput.value.trim()` (empty = the tri-state's clear leg — a deliberate empty save clears the branch, unlike the token's blank = keep; the docstring/section comment documents the asymmetry), composed with the existing `ignore_paths` + conditional `token` (the token stays the blank = keep tri-state — untouched).
|
||||
- The never-stale lifecycle mirrors the existing save handler: the Save button disables in flight, 200 → close the dialog + `loadSources()` (the row re-renders from the server — the tag appears/disappears), failure → the dialog's error line (the existing pattern) + the inputs keep the user's text.
|
||||
- **The row tag** (`makeRow` / the source-cell builder): when `s.id && s.branch` — append a text tag `branch: <name>` in the source cell, next to the existing "hidden on" tag (the SAME tag idiom: a span with the tag class — text + background, never color alone; the tag text is the signal). Env-fallback rows (no `s.id`) and NULL-branch rows show no tag. Update the source cell's `aria-label`/`title` to include the branch (the hover precision idiom).
|
||||
- The module's section-header docstring gains the phase-135 paragraph (the house pattern — the field, the tri-state asymmetry, the tag rule).
|
||||
3. No new API surface, no new fetches — the branch rides the existing POST/PATCH/GET payloads (task 02's shapes).
|
||||
|
||||
## Testing & Quality
|
||||
- No JS unit layer in this repo — the UI contract is pinned by the phase's E2E (task 05).
|
||||
- Coverage: **>90%** on new/modified code (frontend-only task — the bar is carried by the suite run + the E2E).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] adding a git source with a branch sends `branch` (non-blank) / omits it (blank) — pinned by the E2E's request assertions
|
||||
- [ ] the dialog prefills the stored branch; saving an empty value clears it (the row's tag disappears after `loadSources`); local rows' dialogs never show the field
|
||||
- [ ] a stored git row with a branch shows the `branch: <name>` text tag (visible, text-based, in the source cell)
|
||||
- [ ] the 360 px case renders (the form + dialog don't overflow — verified in the E2E's viewport assertions where applicable)
|
||||
- [ ] no console errors on the view (the E2E pins it)
|
||||
@@ -0,0 +1,44 @@
|
||||
# Task 05 — E2E: `tests/e2e/test_git_source_branch.py` (the dedicated Playwright suite, run in isolation)
|
||||
|
||||
**Phase:** `135_git_source_branch` · **Source:** `TODO.md:4` — "Need to be able to specify the branch for a git source"
|
||||
|
||||
## Objective
|
||||
The branch feature is pinned end to end against the REAL pipeline: a two-branch `file://` fixture repo, the owner adding a source WITH a branch through the UI (→ sync → the branch's content is indexed, not the default branch's), the row tag, the per-row editor's branch field (change + the fail-loud mismatch), and the API contracts (the tri-state, the fixed 422s, the local-row rejection).
|
||||
|
||||
## Work
|
||||
1. `tests/e2e/test_git_source_branch.py` (new) — the per-module app pattern (`test_git_sources_admin.py` + `test_sync_button.py` combined):
|
||||
- **Ports/env:** `APP_PORT = int(os.environ.get("E2E_APP_PORT_GITBR", "8134"))`; the standard module env (`BOR_STATIC_DIR`, the mock LLM base URL, `BOR_RELEVANCE_THRESHOLD=0.30`, `BOR_ADMIN_PASSWORD` / `BOR_SESSION_SECRET`, a dedicated `BOR_SOURCES_DIR`). **No `BOR_GIT_SOURCES` env** — the sources under test are DB rows (the UI adds them), so the env fallback stays empty.
|
||||
- **The fixture repo** (module-scoped, `tmp_path_factory`, real `git`): two branches — `main` (file `notes/main-only.md` with `Marker: RESE-BRANCH-MAIN-7a31`) and `release` (file `notes/release-only.md` with `Marker: RESE-BRANCH-REL-9c2d`, plus a shared `notes/common.md` whose content DIFFERS between the branches — `main` says `VERSION: 1`, `release` says `VERSION: 2`). Both branches pushed to the repo (a local bare-ish fixture: the test builds `main`, commits, then `git checkout -b release`, edits, commits — a normal two-branch repo; the app clones it via `file://`).
|
||||
- **Helpers:** `add_git_source(page, url, branch=None, token=None)` (fills the UI form — the branch field when given), `sync_and_wait(page, app_url)` (clicks `#sync-btn` and polls the status endpoint to a terminal state — the `_wait_sync_done` pattern), `catalog_has_doc(page, doc_name)` (the drill-down tree assertion, the `_expect_fixture_row` pattern).
|
||||
2. Tests:
|
||||
- `test_add_with_branch_syncs_that_branch` — the owner's exact flow:
|
||||
1. login (lands on the git-sources view — the `login` helper's `next` param or a nav click, per the existing suites);
|
||||
2. add a source: url = `file://<fixture>`, branch = `release` → 201; the row appears with the `branch: release` tag (the text is in the source cell);
|
||||
3. switch to the Sources/RAG view, `sync_and_wait` → `state == "success"`;
|
||||
4. the catalog contains `release-only.md` (the branch's doc) AND `common.md` shows `VERSION: 2` (drill in and assert on the document's stored content via the document modal or the `GET /api/docs` row — follow the assertion idiom the dates suite uses for content checks); `main-only.md` is ABSENT (the default branch's doc was never indexed);
|
||||
5. re-sync (idempotent): "0 added · 2 unchanged" (release's two files).
|
||||
- `test_default_branch_source_unaffected` — a SECOND source row for the SAME url is impossible (the unique URL 409) — so this test uses a CLONE of the fixture (a second `file://` repo, identical history, `main` only): add it with NO branch → sync → `main-only.md` present, `release-only.md` absent (the `branch=None` path mirrors the pre-phase behavior — the byte-identical guarantee, observable through the UI).
|
||||
- `test_row_editor_branch_change_fails_loud` — the fail-loud mismatch (ASSUMPTION B5, end to end):
|
||||
1. take the `release` source from test 1's state (or a fresh module state — order the tests so the fixture state is explicit; the KB-truncate autouse fixture keeps them independent: re-add the source fresh);
|
||||
2. open the per-row editor, change the branch field to `main`, save → the dialog closes, the row re-renders with the `branch: main` tag (the PATCH landed — the registry is updated);
|
||||
3. sync → `state == "failed"`; the sanitized error banner/modal shows the fixed mismatch copy (assert on the user-visible fragment `remove and re-add the source` — the message is a fixed string, safe to pin);
|
||||
4. restore: editor → branch field `release` → save → sync → success again (the guard is symmetric — back to the matching branch, the ff path works).
|
||||
- `test_api_branch_contracts` — the API layer through `page.request` (admin cookie):
|
||||
- POST a git source with `branch: "a..b"` → 422, detail EXACTLY `not a valid branch name` (no echo); the list is unchanged;
|
||||
- POST a git source with `branch: "HEAD"` → 422 (same detail);
|
||||
- PATCH the tri-state on a stored row: set `hotfix/x` → 200 reports it; clear with `""` → 200 reports `null`; a PATCH of `{"include_hidden": true}` leaves the branch untouched;
|
||||
- POST a LOCAL source (an existing tmp dir) with `branch: "x"` → 422 `branch is only valid for git sources`;
|
||||
- the token field is unaffected by all of the above (the existing token tests stay green — no regression pin needed here).
|
||||
- Wall-time guard: the whole module < 180 s (three full syncs of a 3-file repo against the mock LLM — the `slow_llm` proxy is NOT needed: the branch assertions are about WHICH files land, not about a running window).
|
||||
3. Header docstring: the story (the TODO item, L4), the run command (`uv run pytest tests/e2e/test_git_source_branch.py -v --no-cov`), the isolation note (per-module app, dedicated port, DB rows not env), the fixture's two-branch shape.
|
||||
|
||||
## Testing & Quality
|
||||
- E2E: this file IS the task's suite; it runs in ISOLATION (`--no-cov`, the AGENTS.md rule 9 gate) with the DB up (`podman compose up -d db`) and git on PATH.
|
||||
- Coverage: **>90%** on new/modified code (test-only task — the bar is carried by the suite run).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/e2e/test_git_source_branch.py -v --no-cov` green in isolation
|
||||
- [ ] the branch's content is what gets indexed (the sentinel docs on the right branch present, the wrong branch's doc absent — the feature's core promise)
|
||||
- [ ] the `branch=None` path imports the default branch exactly as pre-phase (the unaffected-source test)
|
||||
- [ ] the mismatch is LOUD (the failed sync names the remove-and-re-add fix — the owner is never left with a silently-wrong checkout)
|
||||
- [ ] every 422 detail is the fixed string (the test asserts on the exact detail, not a substring of the input)
|
||||
@@ -0,0 +1,39 @@
|
||||
# Phase 136 — A force push no longer breaks the sync: the checkout resets to the head of the tracked branch
|
||||
|
||||
**Source:** `TODO.md` L5 — "Force pushes break brain of reese git pull. Brain of reese should always have the head of the branch specified, so if pull fails due to force pushes it should just reset origin/<branch>"
|
||||
**Story:** n/a (TODO-derived phase, Protocol B append).
|
||||
**Context:** `scripts/git_sync.py::clone_or_pull` — today's existing-checkout path: the shallow probe + one-time `git fetch --unshallow` self-heal (phase 107 D11), then `git pull --ff-only` — a force push is a NON-fast-forward, so `--ff-only` refuses → `GitSyncError` → the whole sync lands `failed` (the owner's reported break; this is the bug). Phase 135 (the immediately preceding todo phase) lands the `branch` parameter: for a branched row the sequence is already `git fetch origin <b>` + `git merge --ff-only FETCH_HEAD` behind the HEAD guard; for a `NULL`-branch row it is still the legacy `git pull --ff-only`. `run_git` is the A11 single git invocation site; the two call sites (`app/api/sync.py::_run_sync`, `scripts/import_docs.py::_resolve_sources`) are untouched in shape — `clone_or_pull`'s signature is theirs to satisfy; `file_commit_dates` walks the post-update checkout (a reset checkout yields the NEW head's per-file dates — exactly the head-of-branch semantics the owner asked for). The sync's failure path (`_run_sync`'s `except Exception` → the `failed` state + the sanitized error) is what a force push hits today — after this phase it no longer does.
|
||||
|
||||
## Objective
|
||||
A force push (or any divergent remote head) never fails the sync: the existing-checkout update tries the fast-forward and, when that is impossible, **resets the checkout to the remote head of the tracked ref** — for a branched row `origin/<branch>` (the owner's verbatim "reset origin/<branch>"), for a `NULL`-branch row the head of the branch the checkout is on — with one loud log line. The checkout is a cache of the remote head (the app owns it, the KB is rebuilt from it on the same run), so "always the head of the branch" is the invariant this phase enforces.
|
||||
|
||||
## Dependencies
|
||||
- `135_git_source_branch` (todo) — the `branch` parameter of `clone_or_pull` (the reset's target ref for branched rows) and the phase-135 HEAD guard, which stays in front of the update sequence.
|
||||
|
||||
## Design (shared by all tasks — the executor reads this, not the chat)
|
||||
- **The existing-checkout update sequence** (replaces BOTH today's `git pull --ff-only` path and phase 135's branch path — one unified sequence):
|
||||
1. ref resolution: `branch` given → `ref = branch` (the HEAD guard from phase 135 runs FIRST, unchanged: checkout on a different branch → the remove-and-re-add `GitSyncError`); `branch` None → `ref = git rev-parse --abbrev-ref HEAD` (the checkout's own branch — the pre-phase default-branch behavior; if the result is `"HEAD"` (detached) → `GitSyncError` naming the checkout: the app never CREATES a detached checkout — a fresh clone always lands on a branch — so this is a genuinely broken checkout and fails loud per D12, not a force-push case).
|
||||
2. the shallow probe + one-time `git fetch --unshallow` self-heal (UNCHANGED — phase 107 D11).
|
||||
3. `git fetch origin <ref>` — a fetch FAILURE (network/auth, the branch gone from the remote, git missing) STILL raises `GitSyncError` (a loud failure — a force push is NOT a fetch failure; the owner's "if pull fails due to force pushes" scopes the reset to the merge step only).
|
||||
4. try `git merge --ff-only FETCH_HEAD`.
|
||||
5. **on merge failure — the reset fallback (this phase, ASSUMPTION C1):** capture `old = git rev-parse HEAD` and `new = git rev-parse FETCH_HEAD`, run `git reset --hard FETCH_HEAD`, log one WARNING line (PLAN §9): `git sync: divergent remote head on <dest> (force push) — reset <old[:7]> → <new[:7]> to origin/<ref>`, and RETURN the dest normally (the sync proceeds — a force push never fails a run). The merge's stderr is included in the log (diagnosability) but never in a user-facing error (there is no error — the run continues).
|
||||
- **Unchanged:** the fresh-clone path (with and without `--branch`); the `run_git` discipline; the two call sites; `file_commit_dates`; the sync's status/progress surface. The `branch=None` argv CHANGES by design (the legacy `git pull --ff-only` becomes the explicit fetch + ff-or-reset sequence — that IS the fix, and it applies to every row, branched or not).
|
||||
- **ASSUMPTION C2 — the dirty checkout:** the checkout is app-managed (a cache of the remote head; the KB is rebuilt from it by the same run's import), so local modifications are not expected and are not defended against: the `reset --hard` discards any by definition. No `git status` precheck (one fewer git call per source per sync; the house minimal-invocation discipline) — a dirty checkout simply resets, which is the desired end state either way.
|
||||
|
||||
## Tasks
|
||||
1. `01_ff_or_reset.md` — the unified update sequence in `clone_or_pull` (ff-or-reset, the detached-HEAD fail-loud, the loud log) + the unit/integration matrix.
|
||||
2. `02_e2e_force_push_resync.md` — the dedicated Playwright E2E (a real force push of the fixture repo between two UI sync runs — the pre-phase behavior, a failed sync, is the regression pin).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: the mocked-`subprocess` argv matrix in `tests/unit/test_git_sync.py` (ff success — no reset argv; ff failure — the rev-parse pair + `reset --hard FETCH_HEAD` + return, no raise; fetch failure — still raises; detached `NULL`-branch — raises; the phase-135 mismatch guard still fires before the update sequence).
|
||||
- Integration: real-`git` `file://` scenarios in `tests/integration/test_import_docs_git.py` (normal ff; a force push — the origin's history rewritten; an UNRELATED-history force push — different root commit; a branched row's force push — the reset targets `origin/<branch>`; `file_commit_dates` on the post-reset checkout reflects the new head).
|
||||
- E2E: one dedicated Playwright file, run in isolation (the owner's exact reproduction: sync → force push the origin → sync again → success + the new head's content).
|
||||
- Coverage: **>90%** on new/modified code (`app/` — the change lives in `scripts/`, exercised through the `app/` sync pipeline tests; the coverage bar is carried by the suite run).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] a force push of a source's branch no longer fails the sync — the next sync succeeds and the checkout (and therefore the KB) holds the NEW head (pinned by the integration + E2E)
|
||||
- [ ] an unrelated-history force push (a rewritten root) is handled by the same reset (the E2E or integration covers it — pick the integration, the E2E stays on the owner's exact repro)
|
||||
- [ ] a genuinely broken fetch (git missing / unreachable origin / branch deleted upstream) STILL fails loud (the `GitSyncError` pins are green — the reset is scoped to the merge step only)
|
||||
- [ ] the WARNING log line lands on a reset (one line, the SHAs, the checkout path, `origin/<ref>` — no URL in it)
|
||||
- [ ] `uv run pytest --cov=app --cov-report=term-missing` green, `app/` coverage >90%; `uv run pytest tests/e2e/test_force_push_resync.py -v --no-cov` green in isolation
|
||||
- [ ] the phase-135 behavior is preserved (the `--branch` clone, the mismatch guard, the `branch=None` fresh-clone argv — the pins from phase 135 stay green)
|
||||
@@ -0,0 +1,71 @@
|
||||
# Task 01 — `clone_or_pull`: ff-or-reset (the force push no longer fails the sync)
|
||||
|
||||
**Phase:** `136_force_push_reset` · **Source:** `TODO.md:5` — "Force pushes break brain of reese git pull. Brain of reese should always have the head of the branch specified, so if pull fails due to force pushes it should just reset origin/<branch>"
|
||||
|
||||
## Objective
|
||||
The existing-checkout update becomes: fetch the tracked ref → try the fast-forward → on a non-ff failure, reset the checkout hard to that ref's head and continue — with one loud WARNING line. A force push is now a logged, self-healing event, never a failed sync; genuinely broken fetches still fail loud.
|
||||
|
||||
## Work
|
||||
1. `scripts/git_sync.py::clone_or_pull` — replace the existing-checkout tail (today: probe → unshallow-if-shallow → `git pull --ff-only`; phase 135's branch tail: HEAD guard → probe → unshallow-if-shallow → `fetch origin <b>` → `merge --ff-only FETCH_HEAD`) with the unified sequence from the phase overview:
|
||||
```python
|
||||
# existing checkout (the fresh-clone path above is untouched):
|
||||
if branch is not None:
|
||||
# phase-135 HEAD guard, UNCHANGED (runs first, before any update):
|
||||
current = run_git(["git", "rev-parse", "--abbrev-ref", "HEAD"], cwd=dest).strip()
|
||||
if current != branch:
|
||||
raise GitSyncError(
|
||||
f"checkout {dest} is on branch '{current}' but the source specifies "
|
||||
f"branch '{branch}' — remove and re-add the source to switch branches"
|
||||
)
|
||||
ref = branch
|
||||
else:
|
||||
ref = run_git(["git", "rev-parse", "--abbrev-ref", "HEAD"], cwd=dest).strip()
|
||||
if ref == "HEAD": # detached — the app never creates one (D12 fail-loud)
|
||||
raise GitSyncError(
|
||||
f"checkout {dest} is in a detached-HEAD state — remove and re-add the "
|
||||
f"source to rebuild it"
|
||||
)
|
||||
shallow = run_git(["git", "rev-parse", "--is-shallow-repository"], cwd=dest).strip() == "true"
|
||||
if shallow:
|
||||
run_git(["git", "fetch", "--unshallow"], cwd=dest)
|
||||
run_git(["git", "fetch", "origin", ref], cwd=dest) # fetch failure STILL raises
|
||||
try:
|
||||
run_git(["git", "merge", "--ff-only", "FETCH_HEAD"], cwd=dest)
|
||||
except GitSyncError as ff:
|
||||
old = run_git(["git", "rev-parse", "HEAD"], cwd=dest).strip()
|
||||
new = run_git(["git", "rev-parse", "FETCH_HEAD"], cwd=dest).strip()
|
||||
run_git(["git", "reset", "--hard", "FETCH_HEAD"], cwd=dest)
|
||||
logger.warning(
|
||||
"git sync: divergent remote head on %s (force push) — reset %s → %s to "
|
||||
"origin/%s (%s)", dest, old[:7], new[:7], ref, ff,
|
||||
)
|
||||
return dest
|
||||
```
|
||||
Notes: the `branch=None` path deliberately LOSES the legacy `git pull --ff-only` (that is the fix — every row gets the ff-or-reset update); the `dest` in the log is the checkout path (NEVER the URL — credential-safety, the phase-121 discipline); the merge stderr rides the log only (the run continues — there is no user-facing error). Update the module + function docstrings (the D11/D12 paragraphs gain the phase-136 force-push contract: "a non-ff merge resets to the ref's head instead of failing — the checkout is a cache of the remote head").
|
||||
2. Unit tests — `tests/unit/test_git_sync.py` (extend — the mocked-`subprocess` argv pattern; the mock needs per-argv responses: `rev-parse --abbrev-ref HEAD` → the branch, `rev-parse --is-shallow-repository` → `false`, the ff merge → non-zero on the failure case, the `rev-parse HEAD` / `FETCH_HEAD` → SHAs, the reset → zero):
|
||||
- `branch=None`, ff SUCCESS: argv sequence `rev-parse --abbrev-ref HEAD` → `rev-parse --is-shallow-repository` → `fetch origin <branch-from-HEAD>` → `merge --ff-only FETCH_HEAD` — and NO `reset` argv, NO `rev-parse HEAD/FETCH_HEAD` pair (the capture is only on failure).
|
||||
- `branch=None`, ff FAILURE (merge exit ≠ 0): the sequence gains `rev-parse HEAD` → `rev-parse FETCH_HEAD` → `reset --hard FETCH_HEAD` — and the function RETURNS the dest (no raise).
|
||||
- `branch="release"` (matching HEAD), ff failure: same reset tail, `fetch origin release` — the reset targets the branch's ref (the owner's verbatim "reset origin/<branch>").
|
||||
- the phase-135 mismatch (`branch="release"`, HEAD `main`): STILL raises before any fetch (the guard is first — the phase-135 pin stays green).
|
||||
- `branch=None` + detached HEAD (`rev-parse --abbrev-ref HEAD` → `"HEAD"`): raises the detached `GitSyncError` (no fetch, no reset).
|
||||
- fetch FAILURE (non-zero): raises `GitSyncError` (NOT the reset path — the reset is scoped to the merge step; a deleted upstream branch / dead network still fails loud).
|
||||
- the shallow self-heal still fires in front of the fetch (the `--unshallow` argv pin, unchanged).
|
||||
3. Integration tests — `tests/integration/test_import_docs_git.py` (extend — the real-`git` `file://` pattern):
|
||||
- **Normal ff:** a fixture repo, `clone_or_pull` ×2 with a normal second commit → ff-merged, no reset (assert via `git -C dest log` — the second commit's message is the HEAD, no merge commit).
|
||||
- **Force push (the owner's repro):** after the first clone, rewrite the origin's history (a NEW root commit — `git checkout --orphan` or `git reset --soft` + a fresh commit with different content — the origin's branch now diverges); `clone_or_pull` again → NO exception, the checkout's HEAD == the origin's new head (`git -C origin rev-parse <ref>` == `git -C dest rev-parse HEAD`), the working tree holds the NEW content.
|
||||
- **Unrelated-history force push:** the rewritten origin has a DIFFERENT root commit (a fully unrelated tree) → the same reset path handles it (the ff merge of an unrelated history is exactly what `--ff-only` refuses).
|
||||
- **Branched force push:** a two-branch fixture (the phase-135 shape), a row-shaped call `clone_or_pull(url, dest, "release")` after a force push of ONLY `release` → the reset lands `origin/release`'s head and `main`'s content is not involved.
|
||||
- **`file_commit_dates` after a reset:** the dates map reflects the NEW head (the new root commit's date is what the walk returns for its files — the head-of-branch dates the owner asked for).
|
||||
- **The sync pipeline end-to-end (the seam `tests/integration/test_sync_api.py` already exercises):** a forced-push fixture + a sync → `state == "success"` (the pre-phase code lands `failed` — this test is the regression pin at the API seam; if the file's existing fixtures make the seam awkward, the E2E, task 02, carries the API-seam pin and this step stays at the `clone_or_pull` level — document the choice in the test's docstring).
|
||||
|
||||
## Testing & Quality
|
||||
- Unit: the argv matrix (step 2) — the ff-success pin (no reset argv) and the fetch-failure pin (still raises) are the two regression gates.
|
||||
- Integration: the real-git matrix (step 3).
|
||||
- Coverage: **>90%** on new/modified code.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/unit/test_git_sync.py tests/integration/test_import_docs_git.py -v` green
|
||||
- [ ] a force push (normal divergence AND unrelated history) resets instead of failing — pinned by real-git integration tests
|
||||
- [ ] the phase-135 pins (the mismatch guard, the `--branch` clone argv, the `branch=None` fresh-clone argv) stay green untouched
|
||||
- [ ] the WARNING line is the ONLY observable side effect of a reset besides the checkout state (no user-facing error, no status change — the run continues)
|
||||
- [ ] `uv run ruff check . && uv run pyright` clean
|
||||
@@ -0,0 +1,42 @@
|
||||
# Task 02 — E2E: `tests/e2e/test_force_push_resync.py` (the dedicated Playwright suite, run in isolation)
|
||||
|
||||
**Phase:** `136_force_push_reset` · **Source:** `TODO.md:5` — "Force pushes break brain of reese git pull. Brain of reese should always have the head of the branch specified, so if pull fails due to force pushes it should just reset origin/<branch>"
|
||||
|
||||
## Objective
|
||||
The owner's exact reproduction is pinned end to end: sync (clone + import) → force-push the origin (its history rewritten) → sync again → SUCCESS, and the KB holds the NEW head's content. Pre-phase, that second sync landed `failed` — this test is the regression pin for the fix.
|
||||
|
||||
## Work
|
||||
1. `tests/e2e/test_force_push_resync.py` (new) — the `test_sync_button.py` module-app pattern:
|
||||
- **Ports/env:** `APP_PORT = int(os.environ.get("E2E_APP_PORT_FPPUSH", "8135"))`; the standard module env (`BOR_STATIC_DIR`, the mock LLM, `BOR_RELEVANCE_THRESHOLD=0.30`, the admin/session secrets, `BOR_GIT_SOURCES=file://<fixture repo>`, a dedicated `BOR_SOURCES_DIR`).
|
||||
- **The fixture repo** (module-scoped, `tmp_path_factory`, real `git` — the `sync_git_repo` pattern): `notes/doc.md` with `Marker: RESE-FPPUSH-V1-5e8a`, one commit, one branch.
|
||||
- **The force-push helper:**
|
||||
```python
|
||||
def _force_push_new_head(repo: Path, content: str) -> None:
|
||||
"""Rewrite the origin's history to a NEW root commit (a full
|
||||
divergence — not just a new commit on top): delete the branch
|
||||
ref and recreate it at a new orphan commit with `content`."""
|
||||
```
|
||||
(Implementation at execution time: `git checkout --orphan newroot` → replace `notes/doc.md`'s content → commit → `git branch -M <branch>` / `git push --force` is not applicable to a local fixture — the test rewrites the repo IN PLACE: `git update-ref -d <branch>`, commit on the orphan, `git update-ref <branch> HEAD`. The app's next `git fetch` sees the divergent head. The exact ref mechanics are the executor's call — the OBSERVABLE contract is: the origin's single branch now points at a commit with UNRELATED history and the new content.)
|
||||
- **`_wait_sync_state`** — the `test_sync_button._wait_sync_done` pattern (terminal `{"success", "failed"}`), plus a UI-level settle wait on `#sync-label`.
|
||||
2. Tests:
|
||||
- `test_sync_survives_a_force_push` — the owner's exact flow:
|
||||
1. login (lands on `/sources.html` — the button's home);
|
||||
2. **run 1:** click `#sync-btn` → "Syncing…" → "Synced HH:MM" + "1 added"; the catalog shows `doc.md` (the drill-down assertion, the `_expect_fixture_row` pattern);
|
||||
3. **the force push** (server-side, via the fixture helper — the test owns the origin repo): `_force_push_new_head(repo, "Marker: RESE-FPPUSH-V2-2b7f …")`;
|
||||
4. **run 2:** click `#sync-btn` again → the status endpoint reaches `state == "success"` (NOT `failed` — the pre-phase regression: the `--ff-only` pull refused the divergent head and the run died) within the timeout; the UI settles to "Synced HH:MM" (the error banner is HIDDEN, no error modal);
|
||||
5. **the KB holds the NEW head:** the document modal / `GET /api/docs` content for `notes/doc.md` contains `RESE-FPPUSH-V2-2b7f` (the reset's content flowed through the import — the digest delta re-indexed the file as `updated`; `#sync-result` shows "1 updated" or "0 added · 1 updated" — assert on the content, the counts are the secondary signal);
|
||||
6. **run 3 (idempotent):** "0 added · 1 unchanged" (the checkout is stably at the new head — the reset was not a one-off fluke).
|
||||
- `test_failed_pull_is_not_the_outcome` (the negative pin, cheap while the fixture exists): assert on the APP LOG or skip if the seam is awkward — PREFERRED: a direct API-level assertion instead — after the force push, `POST /api/sync` + poll → `state == "success"` (the same contract as run 2 without the UI; keeps the UI test focused). If the module app's fixture state makes two full UI+API passes heavy, fold it into the main test as step 4b (the API trigger of run 2) — document the choice in the docstring.
|
||||
- Wall-time guard: the whole module < 120 s (three small syncs against the mock LLM — no `slow_llm` proxy needed: the point is the TERMINAL state, not a running window).
|
||||
3. Header docstring: the story (the TODO item, L5 — the owner's verbatim repro), the run command (`uv run pytest tests/e2e/test_force_push_resync.py -v --no-cov`), the isolation note (per-module app, dedicated port), and the pre-phase behavior it pins against (run 2 used to land `failed` — the test fails on the pre-phase code, which is the point).
|
||||
|
||||
## Testing & Quality
|
||||
- E2E: this file IS the task's suite; it runs in ISOLATION (`--no-cov`, the AGENTS.md rule 9 gate) with the DB up (`podman compose up -d db`) and git on PATH.
|
||||
- Coverage: **>90%** on new/modified code (test-only task — the bar is carried by the suite run).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/e2e/test_force_push_resync.py -v --no-cov` green in isolation
|
||||
- [ ] the test FAILS on the pre-phase code (run 2 lands `failed` — verified by temporarily reverting the `clone_or_pull` change in a scratch branch, then restoring — the pin has teeth)
|
||||
- [ ] the NEW head's content is what the KB serves (the sentinel V2 in the document — the reset's observable end-to-end effect)
|
||||
- [ ] the third run is idempotent (the checkout is stably at the new head — no drift, no second reset)
|
||||
- [ ] no error banner / modal on a force-push resync (a reset is a logged event, not a user-facing failure)
|
||||
Reference in New Issue
Block a user