phase: 186_git_log_streaming_dates
## Final report — phase 186 `git_log_streaming_dates`
**State found:** all three task files were already implemented and in `.agents/phases/complete/186_git_log_streaming_dates/`, so this run was the final verification + documentation pass.
### Verification (all green)
| Gate | Result |
|---|---|
| `pytest --cov=app --cov-report=term-missing` | **4531 passed, exit 0** · coverage `TOTAL 6872 stmts → 99%` (gate >90%) |
| `tests/e2e/test_git_log_stream_dates.py -v --no-cov` (isolation) | **5 passed** |
| `ruff check .` | All checks passed |
| `pyright` | 0 errors |
Implementation shape confirmed against the phase contracts: `_parse_commit_dates(str | Iterable[str])` (existing string pins untouched — only 3 non-test lines removed from `test_git_sync.py`), `stream_git_lines` (`Popen` + daemon stderr drain + per-line stdout + terminate-on-abandon, `run_git`-shaped `GitSyncError`), `file_commit_dates` on the stream with byte-identical dates and the fail-soft funnel unchanged, `async_sync` propagating the `to_thread` errors to the existing `except`. A11 holds: two `subprocess.` sites, both in `scripts/git_sync.py`, both off-loop.
### Documentation-consistency pass
Verified (clean): all ~100 `BOR_*` README defaults against a bare `Settings()` (the local `.env` produced false positives — none real), `.env.example` completeness, every module/route/page/test path referenced, `compose.yaml`/`Containerfile`/`entrypoint.sh` claims, E2E port conventions (8901 / 8123), palette count (17), chip cap (90), token format, payload limits, retrieval battery + audit commands.
Fixed (both files, prose only):
- **Page-naming drift** — README/AGENTS called the sync/catalog page "the Sources page" in 5 places, contradicting the README's own convention (and `frontend/index.html`): the **Sync sources** button and the document catalogue are the **RAG** page (`/sources.html`); "Sources" is `/git-sources.html`. Now `RAG page (/sources.html)` in README:186, 376, 418, 682, 713, 745, 1006, 1057 and AGENTS:19, 38, 183.
- **Phase-186 AST-pin wording** — "fails if a third spawn appears anywhere in `app/` or `scripts/`" overstated it (the pin *enumerates* the spawns, including the non-git Valkey probe in `app/core/caching.py`); rewritten to describe the enumeration.
- Moved `00_phase.md` into `complete/186_git_log_streaming_dates/` (AGENTS rule 2 moves the whole directory); `todo/` now starts at 187.
⚠️ **One mistake to disclose:** while tidying line wraps I ran `git checkout README.md`, which discarded task 03's uncommitted phase-186 README bullet. I restored it verbatim from the full `git diff` captured earlier in this session (plus the two corrections above) and re-verified the section renders as intended — no other README content was uncommitted, and AGENTS.md was untouched by the checkout. A blanket line-length rewrap also reflowed 4 unrelated lines; all four were restored to their exact original text (`git diff` now shows only intended changes).
Nothing was committed or staged — the working tree is left for the harness: modified `scripts/git_sync.py`, `app/api/sync.py`, `app/rag/git_sources.py`, `tests/unit/test_git_sync.py`, `tests/integration/test_git_file_dates.py`, `README.md`, `AGENTS.md`; new `tests/e2e/test_git_log_stream_dates.py`, `.agents/phases/complete/186_git_log_streaming_dates/`, `.agents/reports/186_git_log_streaming_dates/`.
This commit is contained in:
+161
-23
@@ -28,20 +28,38 @@ No credentials are stored here; whatever the URL/SSH config supplies is
|
||||
used.
|
||||
|
||||
This module is the only place the ``git`` CLI is invoked (A11: stdlib
|
||||
``subprocess`` only, no new packages) — every git command goes through
|
||||
:func:`run_git`: the clone (with ``--branch`` when a branch is
|
||||
pinned, phase 135), the branch guard's / ref-resolution's ``git
|
||||
rev-parse --abbrev-ref HEAD`` probe, the shallow probe, the one-time
|
||||
``git fetch --unshallow``, the unified update's ``git fetch origin
|
||||
<ref>`` + ``git merge --ff-only FETCH_HEAD`` pair, and the phase-136
|
||||
force-push fallback's ``git rev-parse HEAD`` / ``git rev-parse
|
||||
FETCH_HEAD`` capture + ``git reset --hard FETCH_HEAD`` in
|
||||
:func:`clone_or_pull`; the per-file last-commit-date walk in
|
||||
:func:`file_commit_dates` (phase 106 — it walks the checkout's own
|
||||
log, so a branch checkout yields the BRANCH's history: the
|
||||
head-of-branch semantics phase 135 pins, untouched — a post-reset
|
||||
checkout yields the NEW head's dates, the same semantics); and the
|
||||
docs-push sequence in :mod:`app.core.docs_push` (phase 59).
|
||||
``subprocess`` only, no new packages) — and since phase 186 there are
|
||||
exactly TWO spawn sites here, one per shape, both owning the same
|
||||
``argv`` → :class:`GitSyncError` contract:
|
||||
|
||||
* :func:`run_git` — the BUFFERED site (``subprocess.run``, the whole
|
||||
output returned as one string): the clone (with ``--branch`` when a
|
||||
branch is pinned, phase 135), the branch guard's / ref-resolution's
|
||||
``git rev-parse --abbrev-ref HEAD`` probe, the shallow probe, the
|
||||
one-time ``git fetch --unshallow``, the unified update's ``git fetch
|
||||
origin <ref>`` + ``git merge --ff-only FETCH_HEAD`` pair, and the
|
||||
phase-136 force-push fallback's ``git rev-parse HEAD`` / ``git
|
||||
rev-parse FETCH_HEAD`` capture + ``git reset --hard FETCH_HEAD`` in
|
||||
:func:`clone_or_pull`, and the docs-push sequence in
|
||||
:mod:`app.core.docs_push` (phase 59) — every caller but the commit-date
|
||||
walk, which moved to the streaming sibling below in phase 186 (A4:
|
||||
``run_git`` keeps its shape; migrating the others is churn without a
|
||||
measured cost).
|
||||
* :func:`stream_git_lines` — the STREAMING site added by phase 186
|
||||
(``subprocess.Popen`` + line-at-a-time iteration) — used ONLY by the
|
||||
commit-date walk, because a full-history ``git log`` scales with a
|
||||
repository's ENTIRE history, not its working tree, and holding it as
|
||||
one string (plus the parser's ``splitlines()`` copy) cost tens of MB
|
||||
of transient peak per source on the 2026-10-15 incident box.
|
||||
|
||||
Both sites are reached from async code ONLY through
|
||||
:func:`asyncio.to_thread` (phase 179 / ``AGENTS.md`` rule 11) — neither
|
||||
may ever run on the event loop. The ONE enumerated spawn outside these
|
||||
two functions in the whole app is the boot-time asset-version probe in
|
||||
:mod:`app.core.caching` (a 5-second-timeouted ``git rev-parse --short
|
||||
HEAD`` that never raises and is not part of any sync path); the unit
|
||||
pin ``test_only_two_git_spawn_sites`` enumerates it, so a NEW spawn
|
||||
site anywhere in ``app/`` or ``scripts/`` fails the suite.
|
||||
|
||||
Per-file last-commit dates (phase 106 D2; phase 107 D11 supersedes
|
||||
phase 106 D10) — behavior verified against scratch and live repos
|
||||
@@ -79,14 +97,23 @@ gone; ``--branch`` clones, phase 135, included), and an existing
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import contextlib
|
||||
import logging
|
||||
import subprocess
|
||||
import threading
|
||||
from collections.abc import Iterable, Iterator
|
||||
from datetime import datetime
|
||||
from pathlib import Path
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
__all__ = ["GitSyncError", "clone_or_pull", "file_commit_dates", "run_git"]
|
||||
__all__ = [
|
||||
"GitSyncError",
|
||||
"clone_or_pull",
|
||||
"file_commit_dates",
|
||||
"run_git",
|
||||
"stream_git_lines",
|
||||
]
|
||||
|
||||
|
||||
class GitSyncError(RuntimeError):
|
||||
@@ -237,9 +264,13 @@ def _unshallow_if_shallow(dest: Path) -> None:
|
||||
run_git(["git", "fetch", "--unshallow"], cwd=dest)
|
||||
|
||||
|
||||
def _parse_commit_dates(output: str) -> dict[str, datetime]:
|
||||
def _parse_commit_dates(lines: str | Iterable[str]) -> dict[str, datetime]:
|
||||
"""Parse ``git log --name-only --format=@@%cI`` output (newest first).
|
||||
|
||||
*lines* may be a string or any iterable of lines (phase 186 —
|
||||
:func:`stream_git_lines` feeds it a live pipe); a string input
|
||||
behaves exactly as before.
|
||||
|
||||
A ``@@<ISO-8601>`` line starts a commit (``%cI`` is ISO-strict, so
|
||||
the date is always aware — parsed with ``datetime.fromisoformat``);
|
||||
the following non-empty, non-``@@`` lines are repo-relative paths.
|
||||
@@ -251,7 +282,9 @@ def _parse_commit_dates(output: str) -> dict[str, datetime]:
|
||||
"""
|
||||
dates: dict[str, datetime] = {}
|
||||
commit: datetime | None = None
|
||||
for line in output.splitlines():
|
||||
if isinstance(lines, str):
|
||||
lines = lines.splitlines()
|
||||
for line in lines:
|
||||
line = line.strip()
|
||||
if not line:
|
||||
continue
|
||||
@@ -271,9 +304,11 @@ def file_commit_dates(dest: Path | str) -> dict[str, datetime]:
|
||||
"""Per-file last-commit dates for one checkout (phase 106, D2).
|
||||
|
||||
ONE ``git log --name-only --format=@@%cI`` walk through
|
||||
:func:`run_git` (the A11 single-invocation site, one git call per
|
||||
source per sync) → ``{repo-relative POSIX path: last-commit
|
||||
datetime}``, newest-first so the first sighting of a path wins.
|
||||
:func:`stream_git_lines` (one git call per source per sync — since
|
||||
phase 186 the STREAMING of the two A11 sites, parsed straight off
|
||||
the pipe; :func:`run_git` is its buffered sibling) →
|
||||
``{repo-relative POSIX path: last-commit datetime}``, newest-first
|
||||
so the first sighting of a path wins.
|
||||
|
||||
Every ``clone_or_pull`` checkout is FULL history (fresh: no
|
||||
``--depth``; an existing shallow checkout is unshallowed on its
|
||||
@@ -286,6 +321,21 @@ def file_commit_dates(dest: Path | str) -> dict[str, datetime]:
|
||||
(:class:`GitSyncError`), or ANY parse anomaly logs a warning and
|
||||
returns ``{}`` — the importer falls back to file mtimes; a date
|
||||
walk must never break a sync.
|
||||
|
||||
Phase 186 — the walk STREAMS (the owner incident of 2026-10-15).
|
||||
The output arrives line by line from :func:`stream_git_lines`
|
||||
instead of as one ``run_git`` string, so a full-history ``git log``
|
||||
is never held in RAM (the incident's transient peak: 346,829 lines
|
||||
/ 15 MB buffered TWICE — the string plus the parser's
|
||||
``splitlines()`` list — for one source). Two consequences stated,
|
||||
not hidden: (1) a git failure that happens MID-stream now surfaces
|
||||
AFTER some lines were parsed, and the method still returns ``{}``
|
||||
with the same warning — the phase-106 D2 fail-soft contract is
|
||||
about the RESULT (``{}`` + the mtime fallback), and the result is
|
||||
unchanged; (2) the dict that IS returned — one entry per path in
|
||||
the repository's history — stays retained by ``_run_sync`` in
|
||||
``doc_dates_by_root`` for the whole run (phase 186 A3, OUT OF
|
||||
SCOPE: bounding it changes document DATES, not just memory).
|
||||
"""
|
||||
dest = Path(dest)
|
||||
if not dest.is_dir():
|
||||
@@ -296,7 +346,10 @@ def file_commit_dates(dest: Path | str) -> dict[str, datetime]:
|
||||
)
|
||||
return {}
|
||||
try:
|
||||
output = run_git(["git", "log", "--name-only", "--format=@@%cI"], cwd=dest)
|
||||
# Phase 186: the streaming site — no whole ``git log`` in RAM.
|
||||
return _parse_commit_dates(
|
||||
stream_git_lines(["git", "log", "--name-only", "--format=@@%cI"], cwd=dest)
|
||||
)
|
||||
except GitSyncError as exc:
|
||||
logger.warning(
|
||||
"file_commit_dates: git log failed for %s: %s — the importer "
|
||||
@@ -305,8 +358,6 @@ def file_commit_dates(dest: Path | str) -> dict[str, datetime]:
|
||||
exc,
|
||||
)
|
||||
return {}
|
||||
try:
|
||||
return _parse_commit_dates(output)
|
||||
except ValueError as exc:
|
||||
logger.warning(
|
||||
"file_commit_dates: unparseable git log output for %s (%s) — the "
|
||||
@@ -348,3 +399,90 @@ def run_git(
|
||||
f"{proc.stderr.strip()}"
|
||||
)
|
||||
return proc.stdout
|
||||
|
||||
|
||||
def stream_git_lines(argv: list[str], cwd: Path) -> Iterator[str]:
|
||||
"""Stream one git command's stdout, line by line (phase 186, A11 sibling).
|
||||
|
||||
The streaming counterpart of :func:`run_git` and the SECOND of the
|
||||
module's exactly-two ``subprocess`` spawn sites (the unit pin
|
||||
:func:`test_only_two_git_spawn_sites` enforces it): the commit-date
|
||||
walk uses it so a repository's whole history never exists as one
|
||||
``str`` — each line is yielded straight off the pipe and can be
|
||||
dropped afterwards.
|
||||
|
||||
Shape (locked A2 — the deadlock-free form):
|
||||
|
||||
* ``Popen`` with BOTH ``stdout`` and ``stderr`` piped and
|
||||
``text=True``; ``env`` stays ``None``, i.e. the parent environment
|
||||
is inherited — the same semantics as :func:`run_git` (the
|
||||
subprocess env-replacement trap documented there does not apply
|
||||
here because no env dict is ever built);
|
||||
* ``stderr`` is drained by a DAEMON THREAD from before stdout is
|
||||
consumed. A second pipe nobody reads fills up (~64 KB) and
|
||||
blocks the child while the parent blocks reading ``stdout`` — the
|
||||
classic two-pipe deadlock. ``stderr=STDOUT`` would corrupt the
|
||||
parse, ``stderr=DEVNULL`` would lose the text
|
||||
:class:`GitSyncError` carries, and ``communicate()`` after the
|
||||
fact is impossible once stdout has been consumed: the thread is
|
||||
the only correct option;
|
||||
* on a non-zero exit, :class:`GitSyncError` with the message shape
|
||||
copied from :func:`run_git` verbatim (``git <argv[1:]> failed
|
||||
(exit N): <stderr>``) so every caller's fail-soft handling and
|
||||
log line is byte-identical to the buffered path;
|
||||
* ``finally``-safe: a consumer that abandons the generator (early
|
||||
``break``, an exception in the parser, ``GeneratorExit`` from
|
||||
``close()``) terminates the child instead of leaking it, then
|
||||
closes both pipes, joins the drain thread, and reaps.
|
||||
|
||||
Because the error surfaces while the generator is being consumed,
|
||||
a mid-stream failure raises AFTER some lines were yielded — the
|
||||
only caller (:func:`file_commit_dates`) turns that into its usual
|
||||
``{}`` + mtime fallback, so the observable result is unchanged.
|
||||
"""
|
||||
try:
|
||||
proc = subprocess.Popen(
|
||||
argv,
|
||||
cwd=cwd,
|
||||
stdout=subprocess.PIPE,
|
||||
stderr=subprocess.PIPE,
|
||||
text=True,
|
||||
env=None,
|
||||
)
|
||||
except FileNotFoundError:
|
||||
# The same contract as run_git: a missing git binary is a
|
||||
# GitSyncError, so file_commit_dates keeps failing soft to mtimes.
|
||||
raise GitSyncError("git was not found on PATH — install git and retry") from None
|
||||
stderr_lines: list[str] = []
|
||||
|
||||
def _drain_stderr() -> None:
|
||||
"""Keep git's stderr pipe empty for the whole life of the child."""
|
||||
if proc.stderr is not None:
|
||||
stderr_lines.extend(proc.stderr)
|
||||
|
||||
err_thread = threading.Thread(target=_drain_stderr, daemon=True, name="git-stderr-drain")
|
||||
err_thread.start()
|
||||
try:
|
||||
# `yield from` keeps the delegate closed on any exit path (the
|
||||
# GeneratorExit machinery closes it), and the `or ()` guards the
|
||||
# Optional type of ``Popen.stdout`` for the type checker.
|
||||
yield from proc.stdout or ()
|
||||
except BaseException:
|
||||
# The consumer walked away (GeneratorExit on close(), a parse
|
||||
# error, a cancelled worker): never leave a `git` child running.
|
||||
# An already-exited child just makes terminate() a no-op.
|
||||
with contextlib.suppress(OSError):
|
||||
proc.terminate()
|
||||
raise
|
||||
finally:
|
||||
if proc.stdout is not None:
|
||||
proc.stdout.close()
|
||||
err_thread.join()
|
||||
if proc.stderr is not None:
|
||||
proc.stderr.close()
|
||||
proc.wait()
|
||||
if proc.returncode != 0:
|
||||
raise GitSyncError(
|
||||
f"git {' '.join(argv[1:])} failed (exit {proc.returncode}): "
|
||||
f"{''.join(stderr_lines).strip()}"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user