phase: 136_force_push_reset
Everything is verified green and consistent. Final report: **Phase 136 — final verification pass (all tasks already complete in `complete/136_force_push_reset/`):** - **Verified implemented:** unified ff-or-reset update sequence in `scripts/git_sync.py::clone_or_pull` (phase-135 HEAD guard → shallow self-heal → `fetch origin <ref>` → `merge --ff-only` → on non-ff: SHA capture + `reset --hard FETCH_HEAD` + one WARNING line, returns normally); detached-HEAD fail-loud; `branch=None` legacy `git pull` deliberately gone - **Completion criteria — all met:** 1. ✅ Force push no longer fails sync (integration `test_clone_or_pull_force_push_resets_to_the_new_head` + API-seam `test_sync_pipeline_survives_a_force_push` + E2E run 2 asserts `success` and KB serves the V2 sentinel) 2. ✅ Unrelated-history force push handled by the same reset (`test_clone_or_pull_unrelated_history_force_push_resets`) 3. ✅ Broken fetch still fails loud (fetch-failure unit pins raise with no merge/reset argv; `test_main_git_failure_aborts_before_import`) 4. ✅ WARNING line pinned: one line, 7-char SHAs, checkout path, `origin/<ref>`, no URL (unit exact-string pin + integration caplog pin) 5. ✅ `uv run pytest --cov=app --cov-report=term-missing` → **3231 passed, app/ 99%**; `uv run pytest tests/e2e/test_force_push_resync.py -v --no-cov` → **2 passed in isolation** (10.6 s) 6. ✅ Phase-135 pins preserved (mismatch guard, `--branch` clone argv, `branch=None` fresh-clone argv all green in the 53-test run) - **Test/lint results:** `uv run pytest tests/unit/test_git_sync.py tests/integration/test_import_docs_git.py -v` → 53 passed; full suite 3231 passed (72 s); `uv run ruff check . && uv run pyright` → clean (0 errors) - **Documentation:** README.md git-sources section already updated this phase to the ff-or-reset contract; all `.env.example` vars documented; pages table matches router; AGENTS.md rules and quick-reference commands verified accurate (rule 5 correctly documents the phase-100 §7.1 supersession) — **no changes needed** - **Notable:** E2E "teeth" verification (fails on pre-phase code) is recorded in the task report; nothing was committed per harness protocol - **Next pending phase:** `todo/137_pdf_image_context`
This commit is contained in:
@@ -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