From 9e8a8b20bfec76cec905a7a4ef5ad371daf8e719 Mon Sep 17 00:00:00 2001 From: ou Date: Tue, 1 Sep 2026 17:02:50 +0300 Subject: [PATCH 1/7] feat(change-summary): resolve the window a digest covers, and the events inside it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The decision log has recorded what the engine decided since it landed, and `usage-report` now aggregates it per method across the whole log. What no reader can answer is "what changed on this branch, and why": there is no git or window logic anywhere in the log's API. This adds the two halves that have no output format — which span of work counts as "the run", and which recorded decisions fall inside it. Rendering and requirement linkage are separate changes. The window comes from the merge-base with the canonical remote rather than from a decision-log `run_id`. A `run_id` is one CLI invocation, while a reviewer's "run" is a branch's worth of work, so `run_id` becomes a grouping key inside the span instead of the span itself. `upstream/*` is preferred over `origin/HEAD` because in a fork-based workflow `origin` is the contributor's fork and lags behind — measured five weeks behind on a real checkout, which would have silently widened every window. Every path returns a value carrying an explicit reason rather than raising or going quiet. An unavailable dimension is named; an event whose timestamp will not parse is excluded *and counted* rather than guessed into or out of the window; unparseable log lines are reported as a lower bound on corruption. A requested base ref is honoured or refused, never silently swapped for a discoverable default. Reason strings carry no filesystem paths, so no home directory or username can reach a rendered digest through them. Git access is a narrow read-only query helper rather than a third general runner: the two existing private helpers in this package have incompatible contracts, so a generic copy would duplicate both. Nothing calls this yet, so the public names are whitelisted for the dead-code scan; the command wrapper follows. `cfs validate` 231/231, 0 errors. `spec-coverage --system studio`: granularity 0.4606 -> 0.4614 against the 0.46 floor, coverage 90.50% -> 90.54% — the module raises the margin rather than consuming it. 45 new tests, 100% line coverage on the new module, full suite 5,173 passed. Signed-off-by: ou --- architecture/features/developer-experience.md | 24 + .../scripts/studio/utils/change_summary.py | 355 +++++++++++++ tests/test_change_summary_core.py | 470 ++++++++++++++++++ vulture_whitelist.py | 20 + 4 files changed, 869 insertions(+) create mode 100644 skills/studio/scripts/studio/utils/change_summary.py create mode 100644 tests/test_change_summary_core.py diff --git a/architecture/features/developer-experience.md b/architecture/features/developer-experience.md index 560db225..87ea3b9d 100644 --- a/architecture/features/developer-experience.md +++ b/architecture/features/developer-experience.md @@ -19,6 +19,7 @@ - [Run Self-Check](#run-self-check) - [Resolve Variables](#resolve-variables-1) - [Pylint Rollout Phase 0](#pylint-rollout-phase-0) + - [Change Summary Window And Events](#change-summary-window-and-events) - [4. States (CDSL)](#4-states-cdsl) - [Developer Experience State](#developer-experience-state) - [5. Definitions of Done](#5-definitions-of-done) @@ -230,6 +231,28 @@ Reduces friction in daily Studio usage. `doctor` catches environment issues befo 3. - `p2` - Keep the remaining backlog deferred for later rollout phases, starting with `R0917`, `R0902`, `C0302`, `C0415`, `R0401`, and `C0301` - `inst-pylint-phase-0-deferred-half` 4. - `p2` - Keep the rollout aligned with `cpt-studio-nfr-zero-harm`: stage advisory cleanup before enabling additional checks - `inst-pylint-phase-0-zero-harm` +### Change Summary Window And Events + +- [x] `p1` - **ID**: `cpt-studio-algo-developer-experience-change-summary` + +**Input**: A project root, plus an optional base ref or explicit lower-bound timestamp + +**Output**: The span of work a change digest covers, and the decision-log events recorded inside it + +**Rules**: +1. [x] - `p1` - Define the window and selection result types, and the reason vocabulary shared by producer and renderer so an unavailable dimension is always named rather than shown as empty - `inst-change-summary-datamodel` +2. [x] - `p1` - Answer read-only git queries as one line of output or nothing, treating git absent, non-zero exit, timeout and empty output identically - `inst-change-summary-git-query` +3. [x] - `p1` - Detect whether the project root sits inside a git work tree - `inst-change-summary-detect-repo` +4. [x] - `p1` - Resolve the base ref, preferring the canonical remote over a fork's lagging default, and honour or refuse an explicitly requested ref rather than substituting a fallback - `inst-change-summary-default-base` +5. [x] - `p1` - Resolve the merge-base between HEAD and the base ref, treating unrelated histories as no window - `inst-change-summary-merge-base` +6. [x] - `p1` - Read the base commit's commit time as the window's lower bound - `inst-change-summary-base-time` +7. [x] - `p1` - Assemble the window, short-circuiting git when the caller supplies an explicit lower bound, and returning a stated reason on every failure path instead of raising - `inst-change-summary-resolve-window` +8. [x] - `p1` - Parse ISO-8601 timestamps to aware datetimes, normalising a trailing Z and refusing naive values rather than assuming an offset that would move events across the boundary - `inst-change-summary-parse-ts` +9. [x] - `p1` - Report why the decision log cannot be read, distinguishing opt-out from absent from unreadable - `inst-change-summary-log-state` +10. [x] - `p1` - Count the log's non-empty lines so the number that failed to parse can be derived and reported as a lower bound on corruption - `inst-change-summary-count-lines` +11. [x] - `p1` - Select events at or after the window boundary, excluding and counting undated events rather than guessing them into or out of the window - `inst-change-summary-select-events` +12. [x] - `p1` - Group selected events by run id in first-seen order, so one invocation is a subdivision of the branch's span and never the whole story - `inst-change-summary-group-runs` + ## 4. States (CDSL) ### Developer Experience State @@ -289,6 +312,7 @@ No feature-specific state machines. Self-check is stateless (run → report). | TOC Command | `skills/.../commands/toc.py` | CLI wrapper for TOC generation | | TOC Utils | `skills/.../utils/toc.py` | Unified TOC generation, anchor slugs, code block awareness | | Resolve Vars Command | `skills/.../commands/resolve_vars.py` | Template variable resolution to absolute paths | +| Change Summary Core | `skills/.../utils/change_summary.py` | Window resolution from git, and decision-log event selection inside it | ## 7. Acceptance Criteria diff --git a/skills/studio/scripts/studio/utils/change_summary.py b/skills/studio/scripts/studio/utils/change_summary.py new file mode 100644 index 00000000..0aa73425 --- /dev/null +++ b/skills/studio/scripts/studio/utils/change_summary.py @@ -0,0 +1,355 @@ +"""Change-summary core — resolve the window a digest covers, and select the +decision-log events recorded inside it. + +The digest answers "what changed on this branch, and why". This module owns the two +halves that have no output format: **which span of work counts as "the run"**, and +**which recorded decisions fall inside it**. Rendering belongs to the command +wrapper; linking changed files to requirements is separate again. + +Three deliberate choices: + +* **The window comes from git, not from a decision-log ``run_id``.** A ``run_id`` is + one CLI invocation, but a reviewer's "run" is a branch's worth of work. The window + is the span since the merge-base with the default branch, so ``run_id`` becomes a + grouping key *inside* that span rather than the span itself. +* **Nothing here raises, and nothing here is silent.** Every path returns a value + carrying an explicit ``reason`` when a dimension is unavailable. A digest that + quietly shows less is the defect this effort exists to remove, so "cannot tell" is + always reported rather than rounded down to "nothing to say". +* **Reason strings carry no filesystem paths**, so no ``$HOME`` or username can + reach a rendered digest through them. + +Git access is a narrow read-only query helper, not a general runner. The two existing +private ``_run_git`` helpers in this package have incompatible contracts — one returns +``(code, stdout, stderr)``, the other returns a string and raises — so a third generic +copy would duplicate both. ``_git_line`` answers only "one line of stdout, or nothing". + +@cpt-algo:cpt-studio-algo-developer-experience-change-summary:p1 +""" + +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-datamodel +from __future__ import annotations + +import logging +import subprocess +from dataclasses import dataclass, field +from datetime import datetime +from pathlib import Path +from typing import Any, Dict, List, Optional + +from . import decision_log + +logger = logging.getLogger(__name__) + +#: Seconds any single git query may take before it is treated as unavailable. +_GIT_TIMEOUT = 10 + +#: Refs tried in order when the caller names no base. +#: +#: ``upstream/*`` comes first deliberately. In a fork-based workflow — which this +#: project mandates, with ``origin`` pointing at the contributor's fork — ``origin/HEAD`` +#: tracks the *fork's* default branch, which lags the canonical one. Measured on this +#: checkout, ``origin/HEAD`` was five weeks behind ``upstream/main``, so preferring it +#: would silently widen every window to include work that shipped long ago. +#: A fresh clone of the canonical repo has no ``upstream`` remote, so it falls through +#: to ``origin/HEAD`` and is still correct. +_DEFAULT_BASE_REFS = ( + "upstream/main", + "upstream/master", + "origin/HEAD", + "origin/main", + "main", + "origin/master", + "master", +) + +# Reasons are module constants so the renderer and the tests share one vocabulary +# instead of matching on prose that can drift. +REASON_OK = "" +REASON_NOT_A_REPO = "not a git repository" +REASON_GIT_UNAVAILABLE = "git unavailable" +#: Kept free of an enumerated candidate list on purpose: the first version of this +#: string named the refs it tried, and went stale the moment the list changed. +REASON_NO_BASE_REF = "no default base ref found" +REASON_BASE_REF_UNKNOWN = "requested base ref not found" +REASON_NO_MERGE_BASE = "no merge base with the base ref" +REASON_NO_BASE_TIME = "base commit has no readable timestamp" +REASON_NOT_A_PROJECT = "not inside a Studio project" +REASON_LOG_DISABLED = "decision log disabled" +REASON_LOG_ABSENT = "no decision log yet" + + +@dataclass +class ChangeWindow: + """The span of work a digest covers. + + ``available`` false means no git-derived window could be established; ``reason`` + then says which of the failure modes applied. ``since`` is the base commit's own + commit time, which is what makes the window "everything after the branch point". + """ + + base_ref: str = "" + base_sha: str = "" + since: str = "" + available: bool = False + reason: str = REASON_NOT_A_REPO + + +@dataclass +class EventSelection: + """Decision-log events falling inside a window. + + ``skipped_lines`` is derived, not observed: :func:`decision_log.read_events` drops + unparseable lines without reporting a count, so this compares the log's non-empty + line count against the events actually returned. It is therefore a lower bound on + corruption, and it is reported rather than hidden. + """ + + events: List[Dict[str, Any]] = field(default_factory=list) + runs: List[str] = field(default_factory=list) + scanned: int = 0 + undated: int = 0 + skipped_lines: int = 0 + available: bool = False + reason: str = REASON_NOT_A_PROJECT +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-datamodel + + +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-git-query +def _git_line(project_root: Path, args: List[str]) -> Optional[str]: + """Run a read-only git query and return its first output line, or ``None``. + + ``None`` covers every failure identically — git absent, non-zero exit, timeout, + empty output — because a caller deciding what to report needs "no answer", not a + diagnosis. Never raises. + """ + try: + result = subprocess.run( + ["git"] + args, + cwd=str(project_root), + capture_output=True, + text=True, + timeout=_GIT_TIMEOUT, + check=False, + ) + except (OSError, subprocess.SubprocessError) as exc: + logger.debug("change-summary git query failed: %s", exc) + return None + if result.returncode: + logger.debug("change-summary git query exited %d", result.returncode) + return None + line = result.stdout.strip().splitlines() + return line[0].strip() if line else None +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-git-query + + +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-detect-repo +def _is_git_repo(project_root: Path) -> bool: + """Report whether ``project_root`` sits inside a git work tree.""" + return _git_line(project_root, ["rev-parse", "--is-inside-work-tree"]) == "true" +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-detect-repo + + +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-default-base +def _resolve_base_ref(project_root: Path, requested: str = "") -> Optional[str]: + """Pick the ref the window is measured from. + + An explicitly requested ref is honoured or refused — never silently swapped for a + fallback, because a digest measured against a different ref than the caller asked + for is worse than one that says it could not comply. + """ + if requested: + resolved = _git_line(project_root, ["rev-parse", "--verify", "--quiet", requested]) + return requested if resolved else None + for candidate in _DEFAULT_BASE_REFS: + if _git_line(project_root, ["rev-parse", "--verify", "--quiet", candidate]): + return candidate + return None +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-default-base + + +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-merge-base +def _merge_base(project_root: Path, base_ref: str) -> Optional[str]: + """Return the merge-base sha between ``HEAD`` and ``base_ref``. + + Unrelated histories and a missing ref both yield ``None``: there is no branch + point, so there is no window to report. + """ + return _git_line(project_root, ["merge-base", "HEAD", base_ref]) +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-merge-base + + +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-base-time +def _commit_time(project_root: Path, sha: str) -> Optional[str]: + """Return a commit's author-independent commit time in strict ISO 8601.""" + return _git_line(project_root, ["show", "-s", "--format=%cI", sha]) +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-base-time + + +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-resolve-window +def resolve_window( + project_root: Path, + *, + base: str = "", + since: str = "", +) -> ChangeWindow: + """Resolve the span of work a digest should cover. + + ``since`` short-circuits git entirely — an explicit lower bound is the caller's + assertion and needs no branch point. Otherwise the window starts at the merge-base + with ``base`` (or the first of :data:`_DEFAULT_BASE_REFS` that exists). + + Every failure returns an unavailable window carrying its reason. Never raises. + """ + if since: + return ChangeWindow(since=since, available=True, reason=REASON_OK) + + if not _is_git_repo(project_root): + reason = REASON_NOT_A_REPO if _git_line(project_root, ["--version"]) else REASON_GIT_UNAVAILABLE + return ChangeWindow(reason=reason) + + base_ref = _resolve_base_ref(project_root, base) + if base_ref is None: + # Two different failures, two different reasons: a ref the caller named and + # git does not have, versus no discoverable default at all. + return ChangeWindow(reason=REASON_BASE_REF_UNKNOWN if base else REASON_NO_BASE_REF) + + base_sha = _merge_base(project_root, base_ref) + if base_sha is None: + return ChangeWindow(base_ref=base_ref, reason=REASON_NO_MERGE_BASE) + + base_time = _commit_time(project_root, base_sha) + if base_time is None: + return ChangeWindow(base_ref=base_ref, base_sha=base_sha, reason=REASON_NO_BASE_TIME) + + return ChangeWindow( + base_ref=base_ref, + base_sha=base_sha, + since=base_time, + available=True, + reason=REASON_OK, + ) +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-resolve-window + + +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-parse-ts +def _parse_ts(value: Any) -> Optional[datetime]: + """Parse an ISO-8601 timestamp to an aware ``datetime``, or ``None``. + + A trailing ``Z`` is normalised because git and the log writer disagree about it. + A naive timestamp is refused rather than assumed to be UTC: guessing an offset + would silently move events across the window boundary. + """ + if not isinstance(value, str) or not value: + return None + text = value[:-1] + "+00:00" if value.endswith("Z") else value + try: + parsed = datetime.fromisoformat(text) + except ValueError as exc: + logger.debug("change-summary could not parse timestamp %r: %s", value, exc) + return None + return parsed if parsed.tzinfo is not None else None +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-parse-ts + + +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-log-state +def _log_unavailable(path: Path) -> str: + """Return the reason the decision log cannot be read, or :data:`REASON_OK`.""" + if not decision_log.is_enabled(): + return REASON_LOG_DISABLED + try: + if not path.is_file(): + return REASON_LOG_ABSENT + except OSError as exc: + logger.debug("change-summary log probe failed: %s", exc) + return REASON_LOG_ABSENT + return REASON_OK +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-log-state + + +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-count-lines +def _count_log_lines(path: Path) -> int: + """Count non-empty lines in the log, for deriving how many failed to parse. + + Returns 0 on any read error: an unreadable log is already reported through the + availability reason, and a wrong skip count must not be invented on top of it. + """ + try: + with path.open("r", encoding="utf-8", errors="replace") as handle: + return sum(1 for line in handle if line.strip()) + except OSError as exc: + logger.debug("change-summary log line count failed: %s", exc) + return 0 +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-count-lines + + +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-select-events +def select_events( + window: ChangeWindow, + *, + path: Optional[Path] = None, +) -> EventSelection: + """Select the decision-log events recorded inside ``window``. + + An event whose timestamp cannot be parsed is **excluded and counted** in + ``undated`` rather than guessed into or out of the window — the caller can then + say so instead of presenting a quietly incomplete list. + + An unavailable window yields an unavailable selection carrying the window's own + reason, so the caller reports one cause rather than two. Never raises. + """ + if not window.available: + return EventSelection(reason=window.reason) + + target = path or decision_log.default_log_path() + if target is None: + return EventSelection(reason=REASON_NOT_A_PROJECT) + reason = _log_unavailable(target) + if reason: + return EventSelection(reason=reason) + + boundary = _parse_ts(window.since) + if boundary is None: + return EventSelection(reason=REASON_NO_BASE_TIME) + + selected, runs, scanned, undated = [], [], 0, 0 + for event in decision_log.read_events(target): + scanned += 1 + stamp = _parse_ts(event.get("ts")) + if stamp is None: + undated += 1 + continue + if stamp < boundary: + continue + selected.append(event) + run_id = str(event.get("run_id", "")) + if run_id and run_id not in runs: + runs.append(run_id) + + return EventSelection( + events=selected, + runs=runs, + scanned=scanned, + undated=undated, + skipped_lines=max(0, _count_log_lines(target) - scanned), + available=True, + reason=REASON_OK, + ) +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-select-events + + +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-group-runs +def group_by_run(selection: EventSelection) -> Dict[str, List[Dict[str, Any]]]: + """Group a selection's events by ``run_id``, preserving first-seen run order. + + This is the role ``run_id`` keeps once the window stops being derived from it: a + subdivision *within* the branch's span, so a digest can say "three invocations" + without treating the last one as the whole story. + """ + grouped: Dict[str, List[Dict[str, Any]]] = {run: [] for run in selection.runs} + for event in selection.events: + run_id = str(event.get("run_id", "")) + if run_id in grouped: + grouped[run_id].append(event) + return grouped +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-group-runs diff --git a/tests/test_change_summary_core.py b/tests/test_change_summary_core.py new file mode 100644 index 00000000..1fbe5900 --- /dev/null +++ b/tests/test_change_summary_core.py @@ -0,0 +1,470 @@ +"""Tests for the change-summary core — window resolution and event selection. + +The module's whole contract is *never raise, never go silent*: every failure path +returns a value carrying a reason. So these tests are weighted toward forcing each +failure rather than confirming the happy path, and several assert on the *reason* +rather than merely on emptiness — an empty result with the wrong explanation is the +defect this feature exists to remove. +""" + +from __future__ import annotations + +import json +import socket +import subprocess +from pathlib import Path + +import pytest + +from studio.utils import change_summary as cs +from studio.utils import decision_log + + +# --------------------------------------------------------------------------- helpers + +def _git(repo: Path, *args: str) -> str: + result = subprocess.run( + ["git", *args], cwd=str(repo), capture_output=True, text=True, check=True, + ) + return result.stdout.strip() + + +def _make_repo(repo: Path) -> Path: + """A minimal repo with one commit and deterministic identity.""" + repo.mkdir(parents=True, exist_ok=True) + _git(repo, "init", "-q") + _git(repo, "config", "user.email", "test@example.com") + _git(repo, "config", "user.name", "Test User") + _git(repo, "config", "commit.gpgsign", "false") + (repo / "a.txt").write_text("one\n", encoding="utf-8") + _git(repo, "add", "a.txt") + _git(repo, "commit", "-q", "-m", "initial") + return repo + + +def _commit(repo: Path, name: str, body: str = "x\n") -> str: + (repo / name).write_text(body, encoding="utf-8") + _git(repo, "add", name) + _git(repo, "commit", "-q", "-m", f"add {name}") + return _git(repo, "rev-parse", "HEAD") + + +def _point_ref(repo: Path, ref: str, sha: str) -> None: + """Create a remote-tracking ref without needing a real remote.""" + _git(repo, "update-ref", ref, sha) + + +def _write_log(path: Path, rows: list, extra: str = "") -> Path: + body = "".join(json.dumps(r) + "\n" for r in rows) + extra + path.write_text(body, encoding="utf-8") + return path + + +def _event(ts: str, run_id: str = "r1", event: str = "validation") -> dict: + return {"schema": 1, "ts": ts, "run_id": run_id, "decision_id": "d", + "event": event, "command": "validate", "payload": {}} + + +# --------------------------------------------------------------------------- window + +class TestTheWindowComesFromGit: + + def test_a_repo_with_a_base_ref_yields_a_window(self, tmp_path): + repo = _make_repo(tmp_path / "r") + base = _git(repo, "rev-parse", "HEAD") + _point_ref(repo, "refs/remotes/upstream/main", base) + _commit(repo, "b.txt") + + window = cs.resolve_window(repo) + + assert window.available is True + assert window.reason == cs.REASON_OK + assert window.base_ref == "upstream/main" + assert window.base_sha == base + assert window.since # the base commit's own time + + def test_an_explicit_since_short_circuits_git_entirely(self): + # A path that is not a repo, and does not exist: proof no git call is needed. + window = cs.resolve_window(Path("/nonexistent-abc"), since="2026-01-01T00:00:00+00:00") + + assert window.available is True + assert window.since == "2026-01-01T00:00:00+00:00" + assert window.base_ref == "" + + def test_an_explicit_base_ref_is_honoured(self, tmp_path): + repo = _make_repo(tmp_path / "r") + base = _git(repo, "rev-parse", "HEAD") + _git(repo, "branch", "release") + _commit(repo, "b.txt") + + window = cs.resolve_window(repo, base="release") + + assert window.base_ref == "release" + assert window.base_sha == base + + def test_an_unknown_explicit_base_ref_is_refused_not_swapped(self, tmp_path): + """The mutation that matters: a default ref exists, so a fallback would 'work'.""" + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + + window = cs.resolve_window(repo, base="no-such-ref") + + assert window.available is False + assert window.reason == cs.REASON_BASE_REF_UNKNOWN + # Must NOT have silently measured against the discoverable default. + assert window.base_ref == "" + + def test_the_canonical_remote_is_preferred_over_a_lagging_fork(self, tmp_path): + """In a fork workflow origin is the contributor's fork and lags upstream. + + Preferring origin/HEAD would widen the window to include long-shipped work. + """ + repo = _make_repo(tmp_path / "r") + old = _git(repo, "rev-parse", "HEAD") + newer = _commit(repo, "b.txt") + _commit(repo, "c.txt") + _point_ref(repo, "refs/remotes/origin/HEAD", old) # the stale fork + _point_ref(repo, "refs/remotes/upstream/main", newer) # the canonical remote + + window = cs.resolve_window(repo) + + assert window.base_ref == "upstream/main" + assert window.base_sha == newer + + +class TestTheWindowSaysWhyItCouldNotBeBuilt: + + def test_a_non_repo_is_reported_not_crashed(self, tmp_path): + window = cs.resolve_window(tmp_path) + + assert window.available is False + assert window.reason == cs.REASON_NOT_A_REPO + + def test_git_unavailable_is_distinguished_from_not_a_repo(self, tmp_path, monkeypatch): + monkeypatch.setattr(cs, "_git_line", lambda *_a, **_k: None) + + window = cs.resolve_window(tmp_path) + + assert window.reason == cs.REASON_GIT_UNAVAILABLE + + def test_no_discoverable_base_ref_is_reported(self, tmp_path): + repo = _make_repo(tmp_path / "r") + # Rename the only branch to something none of the candidates match. + _git(repo, "branch", "-m", "wip-nothing-standard") + + window = cs.resolve_window(repo) + + assert window.available is False + assert window.reason == cs.REASON_NO_BASE_REF + + def test_unrelated_histories_have_no_merge_base(self, tmp_path, monkeypatch): + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + monkeypatch.setattr(cs, "_merge_base", lambda *_a, **_k: None) + + window = cs.resolve_window(repo) + + assert window.available is False + assert window.reason == cs.REASON_NO_MERGE_BASE + assert window.base_ref == "upstream/main" # what we did learn is still reported + + def test_a_base_commit_without_a_readable_time_is_reported(self, tmp_path, monkeypatch): + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + monkeypatch.setattr(cs, "_commit_time", lambda *_a, **_k: None) + + window = cs.resolve_window(repo) + + assert window.reason == cs.REASON_NO_BASE_TIME + assert window.base_sha # still reported + + def test_the_reason_list_has_no_enumerated_ref_names(self): + """The first version of this string listed the refs it tried and went stale.""" + assert "origin" not in cs.REASON_NO_BASE_REF + assert "main" not in cs.REASON_NO_BASE_REF + + +# --------------------------------------------------------------------------- events + +class TestEventSelection: + + def test_events_before_the_boundary_are_excluded(self, tmp_path): + window = cs.ChangeWindow(since="2026-06-01T00:00:00+00:00", available=True) + log = _write_log(tmp_path / "d.jsonl", [ + _event("2026-05-31T23:59:59+00:00", "old"), + _event("2026-06-02T00:00:00+00:00", "new"), + ]) + + selection = cs.select_events(window, path=log) + + assert selection.available is True + assert [e["run_id"] for e in selection.events] == ["new"] + assert selection.scanned == 2 + + def test_an_event_exactly_at_the_boundary_is_included(self, tmp_path): + boundary = "2026-06-01T00:00:00+00:00" + window = cs.ChangeWindow(since=boundary, available=True) + log = _write_log(tmp_path / "d.jsonl", [_event(boundary, "edge")]) + + selection = cs.select_events(window, path=log) + + assert len(selection.events) == 1, "the branch point itself belongs to the window" + + def test_a_z_suffix_and_an_offset_compare_correctly(self, tmp_path): + window = cs.ChangeWindow(since="2026-06-01T00:00:00Z", available=True) + log = _write_log(tmp_path / "d.jsonl", [ + _event("2026-06-01T01:00:00+02:00", "before"), # 23:00 previous day UTC + _event("2026-06-01T03:00:00+02:00", "after"), # 01:00 UTC + ]) + + selection = cs.select_events(window, path=log) + + assert [e["run_id"] for e in selection.events] == ["after"] + + def test_unparseable_lines_are_counted_not_hidden(self, tmp_path): + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + log = _write_log( + tmp_path / "d.jsonl", + [_event("2026-06-01T00:00:00+00:00")], + extra="{ this is not json\n\n[]\n", + ) + + selection = cs.select_events(window, path=log) + + assert selection.skipped_lines >= 1, "corruption must be reported, not swallowed" + + def test_undated_events_are_excluded_and_counted(self, tmp_path): + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + rows = [ + _event("2026-06-01T00:00:00+00:00", "dated"), + {"schema": 1, "run_id": "no-ts", "event": "validation", "payload": {}}, + _event("not-a-timestamp", "bad-ts"), + ] + log = _write_log(tmp_path / "d.jsonl", rows) + + selection = cs.select_events(window, path=log) + + assert [e["run_id"] for e in selection.events] == ["dated"] + assert selection.undated == 2, "cannot-tell is counted, never guessed either way" + + def test_a_naive_timestamp_is_refused_rather_than_assumed_utc(self, tmp_path): + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00", "naive")]) + + selection = cs.select_events(window, path=log) + + assert selection.events == [] + assert selection.undated == 1 + + def test_runs_preserve_first_seen_order(self, tmp_path): + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + log = _write_log(tmp_path / "d.jsonl", [ + _event("2026-06-01T00:00:01+00:00", "second"), + _event("2026-06-01T00:00:02+00:00", "first"), + _event("2026-06-01T00:00:03+00:00", "second"), + ]) + + selection = cs.select_events(window, path=log) + + assert selection.runs == ["second", "first"], "first-seen order, not sorted" + + def test_the_scanned_count_is_reported_even_when_nothing_is_selected(self, tmp_path): + window = cs.ChangeWindow(since="2026-12-01T00:00:00+00:00", available=True) + log = _write_log(tmp_path / "d.jsonl", [ + _event("2026-06-01T00:00:00+00:00"), _event("2026-06-02T00:00:00+00:00"), + ]) + + selection = cs.select_events(window, path=log) + + assert selection.events == [] + assert selection.scanned == 2, "a verdict without its denominator is the defect" + + +class TestEventSelectionSaysWhyItCouldNotRead: + + def test_an_unavailable_window_propagates_exactly_one_reason(self, tmp_path): + window = cs.resolve_window(tmp_path) # not a repo + + selection = cs.select_events(window) + + assert selection.available is False + assert selection.reason == window.reason, "one cause reported, not two" + + def test_a_disabled_log_is_reported(self, tmp_path, monkeypatch): + monkeypatch.setenv("CFS_DECISION_LOG", "0") + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + + selection = cs.select_events(window, path=tmp_path / "d.jsonl") + + assert selection.reason == cs.REASON_LOG_DISABLED + + def test_an_absent_log_is_reported(self, tmp_path): + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + + selection = cs.select_events(window, path=tmp_path / "never-written.jsonl") + + assert selection.reason == cs.REASON_LOG_ABSENT + + def test_outside_a_studio_project_is_reported(self, monkeypatch): + monkeypatch.delenv("CFS_DECISION_LOG", raising=False) + monkeypatch.setattr(decision_log, "default_log_path", lambda: None) + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + + selection = cs.select_events(window) + + assert selection.reason == cs.REASON_NOT_A_PROJECT + + def test_a_window_whose_since_will_not_parse_is_reported(self, tmp_path): + window = cs.ChangeWindow(since="nonsense", available=True) + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) + + selection = cs.select_events(window, path=log) + + assert selection.available is False + assert selection.reason == cs.REASON_NO_BASE_TIME + + +# --------------------------------------------------------------------- fail-safe + +class TestNothingRaises: + + def test_a_git_timeout_degrades(self, tmp_path, monkeypatch): + def _boom(*_a, **_k): + raise subprocess.TimeoutExpired(cmd="git", timeout=1) + monkeypatch.setattr(cs.subprocess, "run", _boom) + + assert cs._git_line(tmp_path, ["status"]) is None + + def test_an_oserror_from_git_degrades(self, tmp_path, monkeypatch): + def _boom(*_a, **_k): + raise OSError("no exec") + monkeypatch.setattr(cs.subprocess, "run", _boom) + + window = cs.resolve_window(tmp_path) + + assert window.available is False + assert window.reason == cs.REASON_GIT_UNAVAILABLE + + def test_an_unreadable_log_is_reported_not_raised(self, tmp_path, monkeypatch): + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) + monkeypatch.setattr(Path, "is_file", lambda _self: (_ for _ in ()).throw(OSError("nope"))) + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + + selection = cs.select_events(window, path=log) + + assert selection.reason == cs.REASON_LOG_ABSENT + + def test_the_line_count_degrades_to_zero_on_a_read_error(self, tmp_path, monkeypatch): + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) + monkeypatch.setattr(Path, "open", lambda *_a, **_k: (_ for _ in ()).throw(OSError("nope"))) + + assert cs._count_log_lines(log) == 0 + + def test_hostile_event_shapes_do_not_raise(self, tmp_path): + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + log = tmp_path / "d.jsonl" + log.write_text( + json.dumps({"ts": 12345, "run_id": None}) + "\n" + + json.dumps({"ts": ["list"], "run_id": {"d": 1}}) + "\n" + + json.dumps("a bare string") + "\n", + encoding="utf-8", + ) + + selection = cs.select_events(window, path=log) + + assert selection.available is True + assert selection.events == [] + + +# ---------------------------------------------------------------------- invariants + +class TestInvariants: + + @pytest.mark.parametrize("reason_name", [ + n for n in dir(cs) if n.startswith("REASON_") and n != "REASON_OK" + ]) + def test_every_reason_is_human_readable_prose(self, reason_name): + value = getattr(cs, reason_name) + assert isinstance(value, str) and value + assert value == value.lower() or value[0].islower(), "reason reads as prose, not a code" + + def test_an_unavailable_result_always_carries_a_reason(self, tmp_path): + for result in (cs.resolve_window(tmp_path), cs.select_events(cs.ChangeWindow())): + assert result.available is False + assert result.reason, "unavailable without a reason is the silent failure" + + def test_an_available_window_carries_no_reason(self, tmp_path): + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + + assert cs.resolve_window(repo).reason == cs.REASON_OK + + +class TestPrivacy: + + def test_no_reason_string_leaks_a_path_home_or_username(self): + import os + home = os.path.expanduser("~") + user = os.environ.get("USER") or os.environ.get("USERNAME") or "" + for name in dir(cs): + if not name.startswith("REASON_"): + continue + value = getattr(cs, name) + assert os.sep not in value, f"{name} contains a path separator" + assert home not in value + if user: + assert user not in value + + def test_no_network_is_used(self, tmp_path, monkeypatch): + """Prove it rather than assert it: make sockets impossible and still work.""" + def _no_sockets(*_a, **_k): + raise AssertionError("change_summary must not open a socket") + monkeypatch.setattr(socket, "socket", _no_sockets) + + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + window = cs.resolve_window(repo) + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) + + assert window.available is True + assert cs.select_events(window, path=log).available is True + + +class TestDeterminism: + + def test_the_same_state_yields_identical_results(self, tmp_path): + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + _commit(repo, "b.txt") + log = _write_log(tmp_path / "d.jsonl", [ + _event("2026-06-01T00:00:00+00:00", "a"), _event("2026-06-02T00:00:00+00:00", "b"), + ]) + + windows, selections = [], [] + for _ in range(5): + window = cs.resolve_window(repo) + windows.append(window) + selections.append(cs.select_events(window, path=log)) + + assert len({(w.base_ref, w.base_sha, w.since) for w in windows}) == 1 + assert len({(tuple(s.runs), s.scanned, s.undated) for s in selections}) == 1 + + +class TestGrouping: + + def test_grouping_preserves_run_order(self, tmp_path): + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + log = _write_log(tmp_path / "d.jsonl", [ + _event("2026-06-01T00:00:01+00:00", "z"), + _event("2026-06-01T00:00:02+00:00", "a"), + _event("2026-06-01T00:00:03+00:00", "z"), + ]) + selection = cs.select_events(window, path=log) + + grouped = cs.group_by_run(selection) + + assert list(grouped) == ["z", "a"], "run order follows the log, not the alphabet" + assert [len(v) for v in grouped.values()] == [2, 1] + + def test_grouping_an_empty_selection_is_empty_not_an_error(self): + assert cs.group_by_run(cs.EventSelection()) == {} diff --git a/vulture_whitelist.py b/vulture_whitelist.py index dbf3375f..0f4f3e4e 100644 --- a/vulture_whitelist.py +++ b/vulture_whitelist.py @@ -17,6 +17,13 @@ from studio.utils.eval_judge import Gold from studio.utils.manifest import ManifestLayerState from studio.utils.okf import write_concept_file +from studio.utils.change_summary import ( + resolve_window, + select_events, + group_by_run, + ChangeWindow, + EventSelection, +) is_json = _UI.is_json # staticmethod alias exposed on the ui singleton @@ -131,3 +138,16 @@ SemanticCalibration.excluded # noqa: B018 SemanticCalibration.judge # noqa: B018 SemanticCalibration.schema_version # noqa: B018 + +# Change-summary core: the window and event-selection API the change-summary +# command will consume. Landed ahead of its CLI wrapper so the pure logic is +# reviewable on its own, so nothing in production calls it yet. Only the fields +# no internal caller reads are listed — the rest are genuinely referenced. +resolve_window # noqa: B018 +select_events # noqa: B018 +group_by_run # noqa: B018 +ChangeWindow.base_ref # noqa: B018 +ChangeWindow.base_sha # noqa: B018 +EventSelection.scanned # noqa: B018 +EventSelection.undated # noqa: B018 +EventSelection.skipped_lines # noqa: B018 From c51d6452cfa45bf5393c01c66479a09618cf1a62 Mon Sep 17 00:00:00 2001 From: ou Date: Wed, 2 Sep 2026 10:39:19 +0300 Subject: [PATCH 2/7] fix(change-summary): never present an unreadable log as an empty window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both real. An existing but unreadable log produced an *available, empty* selection — "no decisions in this window" having read nothing. `Path.is_file` only needs `stat`, so a mode-000 log passes the probe, and `decision_log.read_events` swallows the subsequent open failure and yields nothing. That is the exact failure this module exists to prevent, so readability is now proved by opening the file, and absent is reported separately from unreadable via a new reason. `upstream/HEAD` now leads the candidate refs. It is the canonical remote's own symbolic default, so it is right even when that default is neither `main` nor `master`; guessing branch names first skipped it and fell through to the stale fork ref, which is the same defect the ordering was added to prevent, one level deeper. Both fixes are mutation-checked: reverting either fails exactly one test, and the failing test is the one written for it. Full suite 5,176 passed. 48 tests on this module, 100% line coverage (146 stmts). pylint and vulture clean, `cfs validate` 0 errors, spec-coverage passes at granularity 0.4613. Signed-off-by: ou --- .../scripts/studio/utils/change_summary.py | 28 +++++++++++-- tests/test_change_summary_core.py | 42 ++++++++++++++++++- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/skills/studio/scripts/studio/utils/change_summary.py b/skills/studio/scripts/studio/utils/change_summary.py index 0aa73425..6f3b0b5e 100644 --- a/skills/studio/scripts/studio/utils/change_summary.py +++ b/skills/studio/scripts/studio/utils/change_summary.py @@ -53,7 +53,12 @@ #: would silently widen every window to include work that shipped long ago. #: A fresh clone of the canonical repo has no ``upstream`` remote, so it falls through #: to ``origin/HEAD`` and is still correct. +#: ``upstream/HEAD`` leads because it is the canonical remote's *own* symbolic default +#: — right even when that default is neither ``main`` nor ``master``. Guessing branch +#: names first would skip it and fall through to a stale fork ref, which is the same +#: failure this ordering exists to prevent, one level deeper. _DEFAULT_BASE_REFS = ( + "upstream/HEAD", "upstream/main", "upstream/master", "origin/HEAD", @@ -77,6 +82,7 @@ REASON_NOT_A_PROJECT = "not inside a Studio project" REASON_LOG_DISABLED = "decision log disabled" REASON_LOG_ABSENT = "no decision log yet" +REASON_LOG_UNREADABLE = "decision log unreadable" @dataclass @@ -254,15 +260,31 @@ def _parse_ts(value: Any) -> Optional[datetime]: # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-log-state def _log_unavailable(path: Path) -> str: - """Return the reason the decision log cannot be read, or :data:`REASON_OK`.""" + """Return the reason the decision log cannot be read, or :data:`REASON_OK`. + + Absent and unreadable are reported separately, and readability is proved by + opening the file rather than inferred from :meth:`Path.is_file`. ``is_file`` only + needs ``stat``, so a mode-000 log passes it — and + :func:`decision_log.read_events` swallows the subsequent open failure and yields + nothing. Together those turned an unreadable log into an *available, empty* + selection: "no decisions in this window" when in truth nothing was read. That is + the exact failure this module exists to prevent, so the probe is explicit. + """ if not decision_log.is_enabled(): return REASON_LOG_DISABLED try: - if not path.is_file(): - return REASON_LOG_ABSENT + exists = path.is_file() except OSError as exc: logger.debug("change-summary log probe failed: %s", exc) + return REASON_LOG_UNREADABLE + if not exists: return REASON_LOG_ABSENT + try: + with path.open("r", encoding="utf-8"): + pass + except OSError as exc: + logger.debug("change-summary log is unreadable: %s", exc) + return REASON_LOG_UNREADABLE return REASON_OK # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-log-state diff --git a/tests/test_change_summary_core.py b/tests/test_change_summary_core.py index 1fbe5900..96e43343 100644 --- a/tests/test_change_summary_core.py +++ b/tests/test_change_summary_core.py @@ -10,6 +10,7 @@ from __future__ import annotations import json +import os import socket import subprocess from pathlib import Path @@ -131,6 +132,25 @@ def test_the_canonical_remote_is_preferred_over_a_lagging_fork(self, tmp_path): assert window.base_ref == "upstream/main" assert window.base_sha == newer + def test_the_canonical_remotes_own_default_wins_even_when_unnamed(self, tmp_path): + """A remote whose default is neither main nor master, e.g. trunk. + + Guessing branch names first would skip the symbolic upstream/HEAD and fall + through to the stale fork ref — the same bug one level deeper. + """ + repo = _make_repo(tmp_path / "r") + old = _git(repo, "rev-parse", "HEAD") + newer = _commit(repo, "b.txt") + _commit(repo, "c.txt") + _point_ref(repo, "refs/remotes/origin/HEAD", old) # stale fork + _point_ref(repo, "refs/remotes/upstream/trunk", newer) # unconventional name + _git(repo, "symbolic-ref", "refs/remotes/upstream/HEAD", "refs/remotes/upstream/trunk") + + window = cs.resolve_window(repo) + + assert window.base_ref == "upstream/HEAD" + assert window.base_sha == newer, "must not fall through to the stale fork ref" + class TestTheWindowSaysWhyItCouldNotBeBuilt: @@ -345,14 +365,32 @@ def _boom(*_a, **_k): assert window.available is False assert window.reason == cs.REASON_GIT_UNAVAILABLE - def test_an_unreadable_log_is_reported_not_raised(self, tmp_path, monkeypatch): + def test_a_failing_log_probe_is_reported_as_unreadable_not_absent(self, tmp_path, monkeypatch): log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) monkeypatch.setattr(Path, "is_file", lambda _self: (_ for _ in ()).throw(OSError("nope"))) window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) selection = cs.select_events(window, path=log) - assert selection.reason == cs.REASON_LOG_ABSENT + assert selection.reason == cs.REASON_LOG_UNREADABLE, "a failed probe is not proof of absence" + + def test_an_existing_but_unreadable_log_is_never_an_available_empty_selection(self, tmp_path): + """The false green: is_file() passes on mode 000, and read_events swallows the + open failure and yields nothing — so this reported "no decisions" having read + none. Regression for the exact defect class this module exists to prevent.""" + if os.geteuid() == 0: + pytest.skip("root bypasses file permissions") + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) + log.chmod(0o000) + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + try: + selection = cs.select_events(window, path=log) + finally: + log.chmod(0o644) + + assert selection.available is False, "unreadable must not present as an empty window" + assert selection.reason == cs.REASON_LOG_UNREADABLE + assert selection.events == [] def test_the_line_count_degrades_to_zero_on_a_read_error(self, tmp_path, monkeypatch): log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) From e492d3e19f2a8adaa48176bc275020b903d006e8 Mon Sep 17 00:00:00 2001 From: ou Date: Wed, 2 Sep 2026 17:26:10 +0300 Subject: [PATCH 3/7] fix(change-summary): bind the log to the window's project, and never raise on a bad log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four maintainer review findings, all reproduced before fixing. **The decision log followed the cwd, not the window's project.** `resolve_window` takes an explicit project root; `default_log_path()` derived its location from the current working directory. Those are independent inputs, so a window built for project A while the process sat in project B selected B's decisions — a digest describing one project's changes alongside another project's history. `ChangeWindow` now carries the project it describes, and the log is resolved from it. `default_log_path()` gains an optional start path so path knowledge stays in one place rather than being reconstructed from private constants here. **An undecodable log raised out of `select_events`.** The readability probe opened the file but never decoded it, so `read_events` performed the first strict UTF-8 read and, catching only OSError, let UnicodeDecodeError escape — breaking the never-raises contract outright. The probe now decodes, and the read loop is guarded as well for the case where the file changes between the two. **A git failure after the repository probe was reported as a fact about history.** `_git_line` collapsed timeouts and launch failures into the same `None` as a valid negative, so a transient failure surfaced as "no merge base" or "base commit has no readable timestamp". Queries now return the value alongside a tool-failure flag, and that flag takes precedence. A non-zero exit is deliberately *not* a failure: `merge-base` and `rev-parse --verify` both exit 1 to mean "no", and treating those as breakage would mislead in the other direction. **Selected events with no run id vanished from grouping.** They were kept in `events` but buckets were built only for truthy ids, so a renderer summing groups under-reported without saying so. They now land in an explicit `(unattributed)` bucket and are counted in `runless`. Two knock-on cleanups: promoting the git helpers to the failure-aware form left `_merge_base` and `_commit_time` as dead wrappers, so they became the real implementations rather than being whitelisted; and splitting the base-ref walk out of `resolve_window` keeps pylint's return-count rule satisfied without suppressing a check the project is actively rolling out. Full suite 5,238 passed. 108 tests on this module at 100% line coverage (267 stmts). pylint and vulture clean, `cfs validate` 0 errors, spec-coverage passes at granularity 0.4616. Signed-off-by: ou --- architecture/features/developer-experience.md | 2 + .../scripts/studio/utils/change_summary.py | 190 +++++++++++++----- .../scripts/studio/utils/decision_log.py | 11 +- tests/test_change_summary_core.py | 149 +++++++++++++- vulture_whitelist.py | 4 + 5 files changed, 304 insertions(+), 52 deletions(-) diff --git a/architecture/features/developer-experience.md b/architecture/features/developer-experience.md index 87ea3b9d..bebb3396 100644 --- a/architecture/features/developer-experience.md +++ b/architecture/features/developer-experience.md @@ -252,6 +252,8 @@ Reduces friction in daily Studio usage. `doctor` catches environment issues befo 10. [x] - `p1` - Count the log's non-empty lines so the number that failed to parse can be derived and reported as a lower bound on corruption - `inst-change-summary-count-lines` 11. [x] - `p1` - Select events at or after the window boundary, excluding and counting undated events rather than guessing them into or out of the window - `inst-change-summary-select-events` 12. [x] - `p1` - Group selected events by run id in first-seen order, so one invocation is a subdivision of the branch's span and never the whole story - `inst-change-summary-group-runs` +13. [x] - `p1` - Resolve the decision log belonging to the window's own project rather than to the current working directory, so a digest never reports one project's changes alongside another's decisions - `inst-change-summary-default-log` +14. [x] - `p1` - Walk a known-good base ref down to a window, letting a git tool failure take precedence over a historical reading and keeping whatever was already learned on the returned window - `inst-change-summary-window-from-base` ## 4. States (CDSL) diff --git a/skills/studio/scripts/studio/utils/change_summary.py b/skills/studio/scripts/studio/utils/change_summary.py index 6f3b0b5e..cb85ca07 100644 --- a/skills/studio/scripts/studio/utils/change_summary.py +++ b/skills/studio/scripts/studio/utils/change_summary.py @@ -35,7 +35,7 @@ from dataclasses import dataclass, field from datetime import datetime from pathlib import Path -from typing import Any, Dict, List, Optional +from typing import Any, Dict, List, Optional, Tuple from . import decision_log @@ -84,6 +84,11 @@ REASON_LOG_ABSENT = "no decision log yet" REASON_LOG_UNREADABLE = "decision log unreadable" +#: Bucket name for selected events carrying no ``run_id``. Grouping used to build +#: buckets only for truthy ids, so such events sat in ``events`` and in no group and a +#: renderer under-reported them. Run ids are hex, so this cannot collide with a real one. +RUN_UNATTRIBUTED = "(unattributed)" + @dataclass class ChangeWindow: @@ -94,6 +99,7 @@ class ChangeWindow: commit time, which is what makes the window "everything after the branch point". """ + project_root: str = "" base_ref: str = "" base_sha: str = "" since: str = "" @@ -115,6 +121,7 @@ class EventSelection: runs: List[str] = field(default_factory=list) scanned: int = 0 undated: int = 0 + runless: int = 0 skipped_lines: int = 0 available: bool = False reason: str = REASON_NOT_A_PROJECT @@ -122,12 +129,21 @@ class EventSelection: # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-git-query -def _git_line(project_root: Path, args: List[str]) -> Optional[str]: - """Run a read-only git query and return its first output line, or ``None``. +def _git_query(project_root: Path, args: List[str]) -> Tuple[Optional[str], bool]: + """Run a read-only git query, returning ``(first line or None, tool_failed)``. + + The two halves of "no answer" are kept apart, because conflating them lets a + transient tool failure be reported as a conclusion about history — "no merge base" + when git simply timed out. + + * **Tool failure** is git not launching, or timing out. Nothing was learned. + * **A non-zero exit is a valid negative**, not a failure: ``merge-base`` exits 1 + when two histories genuinely have no common ancestor, and + ``rev-parse --verify --quiet`` exits 1 when a ref genuinely does not exist. Those + are answers, and treating them as breakage would be just as misleading in the + other direction. - ``None`` covers every failure identically — git absent, non-zero exit, timeout, - empty output — because a caller deciding what to report needs "no answer", not a - diagnosis. Never raises. + Never raises. """ try: result = subprocess.run( @@ -139,13 +155,23 @@ def _git_line(project_root: Path, args: List[str]) -> Optional[str]: check=False, ) except (OSError, subprocess.SubprocessError) as exc: - logger.debug("change-summary git query failed: %s", exc) - return None + logger.debug("change-summary git query could not run: %s", exc) + return None, True if result.returncode: logger.debug("change-summary git query exited %d", result.returncode) - return None + return None, False line = result.stdout.strip().splitlines() - return line[0].strip() if line else None + return (line[0].strip() if line else None), False + + +def _git_line(project_root: Path, args: List[str]) -> Optional[str]: + """First output line of a read-only git query, or ``None`` for any non-answer. + + For callers that only need the value; use :func:`_git_query` where a tool failure + must be told apart from a valid negative. + """ + value, _failed = _git_query(project_root, args) + return value # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-git-query @@ -175,20 +201,22 @@ def _resolve_base_ref(project_root: Path, requested: str = "") -> Optional[str]: # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-merge-base -def _merge_base(project_root: Path, base_ref: str) -> Optional[str]: +def _merge_base(project_root: Path, base_ref: str) -> Tuple[Optional[str], bool]: """Return the merge-base sha between ``HEAD`` and ``base_ref``. - Unrelated histories and a missing ref both yield ``None``: there is no branch - point, so there is no window to report. + Returns ``(sha, tool_failed)``. Unrelated histories and a missing ref yield a + ``None`` sha with ``tool_failed`` false — there is genuinely no branch point. A + ``True`` flag means git never answered, which is a different fact and must not be + reported as a finding about history. """ - return _git_line(project_root, ["merge-base", "HEAD", base_ref]) + return _git_query(project_root, ["merge-base", "HEAD", base_ref]) # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-merge-base # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-base-time -def _commit_time(project_root: Path, sha: str) -> Optional[str]: - """Return a commit's author-independent commit time in strict ISO 8601.""" - return _git_line(project_root, ["show", "-s", "--format=%cI", sha]) +def _commit_time(project_root: Path, sha: str) -> Tuple[Optional[str], bool]: + """Commit time in strict ISO 8601, plus whether git itself failed.""" + return _git_query(project_root, ["show", "-s", "--format=%cI", sha]) # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-base-time @@ -207,35 +235,63 @@ def resolve_window( Every failure returns an unavailable window carrying its reason. Never raises. """ + root = str(project_root) if since: - return ChangeWindow(since=since, available=True, reason=REASON_OK) + return ChangeWindow(project_root=root, since=since, available=True, reason=REASON_OK) if not _is_git_repo(project_root): reason = REASON_NOT_A_REPO if _git_line(project_root, ["--version"]) else REASON_GIT_UNAVAILABLE - return ChangeWindow(reason=reason) + return ChangeWindow(project_root=root, reason=reason) base_ref = _resolve_base_ref(project_root, base) if base_ref is None: # Two different failures, two different reasons: a ref the caller named and # git does not have, versus no discoverable default at all. - return ChangeWindow(reason=REASON_BASE_REF_UNKNOWN if base else REASON_NO_BASE_REF) + return ChangeWindow( + project_root=root, + reason=REASON_BASE_REF_UNKNOWN if base else REASON_NO_BASE_REF, + ) + return _window_from_base_ref(project_root, base_ref) +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-resolve-window + - base_sha = _merge_base(project_root, base_ref) +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-window-from-base +def _window_from_base_ref(project_root: Path, base_ref: str) -> ChangeWindow: + """Walk a known-good base ref down to a window, or the reason it could not. + + Split out of :func:`resolve_window` to keep each function's guard clauses legible; + the whole point of this stage is that there are several distinct ways to fail and + each gets its own reported reason rather than a shared shrug. + + Past this point git has already answered once, so a further non-answer is + ambiguous: it may be a genuine negative about history, or the tool falling over. + Reporting "no merge base" for a timeout would be a false conclusion, so the failure + flag takes precedence over the historical reading. Whatever *was* learned — the ref, + then the sha — stays on the returned window so a reason keeps its subject. + """ + root = str(project_root) + base_sha, failed = _merge_base(project_root, base_ref) + if failed: + return ChangeWindow(project_root=root, base_ref=base_ref, reason=REASON_GIT_UNAVAILABLE) if base_sha is None: - return ChangeWindow(base_ref=base_ref, reason=REASON_NO_MERGE_BASE) + return ChangeWindow(project_root=root, base_ref=base_ref, reason=REASON_NO_MERGE_BASE) - base_time = _commit_time(project_root, base_sha) - if base_time is None: - return ChangeWindow(base_ref=base_ref, base_sha=base_sha, reason=REASON_NO_BASE_TIME) + base_time, failed = _commit_time(project_root, base_sha) + if failed or base_time is None: + return ChangeWindow( + project_root=root, base_ref=base_ref, base_sha=base_sha, + reason=REASON_GIT_UNAVAILABLE if failed else REASON_NO_BASE_TIME, + ) return ChangeWindow( + project_root=root, base_ref=base_ref, base_sha=base_sha, since=base_time, available=True, reason=REASON_OK, ) -# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-resolve-window +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-window-from-base # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-parse-ts @@ -280,9 +336,14 @@ def _log_unavailable(path: Path) -> str: if not exists: return REASON_LOG_ABSENT try: - with path.open("r", encoding="utf-8"): - pass - except OSError as exc: + # The bytes are *decoded*, not merely opened. Opening alone proved only that + # the descriptor could be acquired; `read_events` then performed the first + # strict UTF-8 read and catches only OSError, so an invalid byte sequence + # raised UnicodeDecodeError straight out of `select_events` and broke the + # never-raises contract. Decoding here answers the question the probe claims to. + with path.open("r", encoding="utf-8") as handle: + handle.read() + except (OSError, UnicodeDecodeError) as exc: logger.debug("change-summary log is unreadable: %s", exc) return REASON_LOG_UNREADABLE return REASON_OK @@ -305,6 +366,21 @@ def _count_log_lines(path: Path) -> int: # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-count-lines +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-default-log +def _default_log_for(window: ChangeWindow) -> Optional[Path]: + """Resolve the decision log belonging to *the window's* project. + + ``decision_log.default_log_path()`` defaults to the cwd, which is correct for the + writer — it logs whichever project the command runs in. A reader reporting on an + explicitly named project must not inherit that default, or the digest describes one + project's changes alongside another project's decisions. + """ + if not window.project_root: + return decision_log.default_log_path() + return decision_log.default_log_path(Path(window.project_root)) +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-default-log + + # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-select-events def select_events( window: ChangeWindow, @@ -318,12 +394,17 @@ def select_events( say so instead of presenting a quietly incomplete list. An unavailable window yields an unavailable selection carrying the window's own - reason, so the caller reports one cause rather than two. Never raises. + reason, so the caller reports one cause rather than two. + + The default log is resolved from **the window's own project**, not from the current + working directory. Those are independent inputs, so a window built for project A + while the process sits in project B used to select B's decisions — a digest about + one project carrying another's history. Never raises. """ if not window.available: return EventSelection(reason=window.reason) - target = path or decision_log.default_log_path() + target = path or _default_log_for(window) if target is None: return EventSelection(reason=REASON_NOT_A_PROJECT) reason = _log_unavailable(target) @@ -334,25 +415,36 @@ def select_events( if boundary is None: return EventSelection(reason=REASON_NO_BASE_TIME) - selected, runs, scanned, undated = [], [], 0, 0 - for event in decision_log.read_events(target): - scanned += 1 - stamp = _parse_ts(event.get("ts")) - if stamp is None: - undated += 1 - continue - if stamp < boundary: - continue - selected.append(event) - run_id = str(event.get("run_id", "")) - if run_id and run_id not in runs: - runs.append(run_id) + selected, runs, scanned, undated, runless = [], [], 0, 0, 0 + try: + for event in decision_log.read_events(target): + scanned += 1 + stamp = _parse_ts(event.get("ts")) + if stamp is None: + undated += 1 + continue + if stamp < boundary: + continue + selected.append(event) + run_id = str(event.get("run_id") or "") + if not run_id: + runless += 1 + run_id = RUN_UNATTRIBUTED + if run_id not in runs: + runs.append(run_id) + except (OSError, UnicodeDecodeError) as exc: + # Belt and braces: the probe already decoded the file, but it could change + # between probe and read. A read that dies mid-way must not surface as a + # partial selection presented as complete. + logger.debug("change-summary log became unreadable while reading: %s", exc) + return EventSelection(reason=REASON_LOG_UNREADABLE) return EventSelection( events=selected, runs=runs, scanned=scanned, undated=undated, + runless=runless, skipped_lines=max(0, _count_log_lines(target) - scanned), available=True, reason=REASON_OK, @@ -367,11 +459,15 @@ def group_by_run(selection: EventSelection) -> Dict[str, List[Dict[str, Any]]]: This is the role ``run_id`` keeps once the window stops being derived from it: a subdivision *within* the branch's span, so a digest can say "three invocations" without treating the last one as the whole story. + + **Every selected event lands in exactly one bucket.** Events carrying a blank or + missing ``run_id`` go to :data:`RUN_UNATTRIBUTED`; previously buckets were built + only for truthy ids, so such an event sat in ``events`` and in no group at all and + a renderer summing the groups under-reported without saying so. """ grouped: Dict[str, List[Dict[str, Any]]] = {run: [] for run in selection.runs} for event in selection.events: - run_id = str(event.get("run_id", "")) - if run_id in grouped: - grouped[run_id].append(event) + run_id = str(event.get("run_id") or "") or RUN_UNATTRIBUTED + grouped.setdefault(run_id, []).append(event) return grouped # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-group-runs diff --git a/skills/studio/scripts/studio/utils/decision_log.py b/skills/studio/scripts/studio/utils/decision_log.py index 40302695..207fcfd6 100644 --- a/skills/studio/scripts/studio/utils/decision_log.py +++ b/skills/studio/scripts/studio/utils/decision_log.py @@ -113,13 +113,18 @@ def opt_out_sentinel_path() -> Path: return _brand_dir() / _OPT_OUT_SENTINEL -def default_log_path() -> Optional[Path]: +def default_log_path(start: Optional[Path] = None) -> Optional[Path]: """Resolve the log location, or ``None`` when there is nowhere to write. Order: 1. ``$CFS_DECISION_LOG`` if it names a path (an off-value there disables logging). - 2. ``/.cache/decisions.jsonl`` for the project containing the cwd. + 2. ``/.cache/decisions.jsonl`` for the project containing ``start``. 3. ``None`` — outside a project, so the writer no-ops. + + ``start`` defaults to the cwd, which is right for the writer: it logs whatever + project the command is running in. A *reader* working against an explicitly named + project must pass that root, or it can resolve a different project's log than the + one it is reporting on. """ override = os.environ.get(_ENV_PATH, "").strip() if override and override.lower() not in _OFF_VALUES: @@ -127,7 +132,7 @@ def default_log_path() -> Optional[Path]: try: from .files import find_studio_directory - studio_dir = find_studio_directory(Path.cwd()) + studio_dir = find_studio_directory(start or Path.cwd()) except Exception: # pylint: disable=broad-except studio_dir = None if studio_dir is None: diff --git a/tests/test_change_summary_core.py b/tests/test_change_summary_core.py index 96e43343..9ed25df5 100644 --- a/tests/test_change_summary_core.py +++ b/tests/test_change_summary_core.py @@ -180,7 +180,7 @@ def test_no_discoverable_base_ref_is_reported(self, tmp_path): def test_unrelated_histories_have_no_merge_base(self, tmp_path, monkeypatch): repo = _make_repo(tmp_path / "r") _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) - monkeypatch.setattr(cs, "_merge_base", lambda *_a, **_k: None) + monkeypatch.setattr(cs, "_merge_base", lambda *_a, **_k: (None, False)) window = cs.resolve_window(repo) @@ -191,7 +191,7 @@ def test_unrelated_histories_have_no_merge_base(self, tmp_path, monkeypatch): def test_a_base_commit_without_a_readable_time_is_reported(self, tmp_path, monkeypatch): repo = _make_repo(tmp_path / "r") _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) - monkeypatch.setattr(cs, "_commit_time", lambda *_a, **_k: None) + monkeypatch.setattr(cs, "_commit_time", lambda *_a, **_k: (None, False)) window = cs.resolve_window(repo) @@ -488,6 +488,151 @@ def test_the_same_state_yields_identical_results(self, tmp_path): assert len({(tuple(s.runs), s.scanned, s.undated) for s in selections}) == 1 +class TestTheLogIsBoundToTheWindowsProject: + """A window and its events must describe the same project. + + `resolve_window` takes an explicit root; `decision_log.default_log_path()` defaults + to the cwd. Those are independent inputs, so a window for project A resolved while + the process sat in project B used to select B's decisions — one project's changes + reported alongside another's history. + """ + + def test_the_default_log_follows_the_window_not_the_cwd(self, tmp_path, monkeypatch): + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + window = cs.resolve_window(repo) + monkeypatch.delenv("CFS_DECISION_LOG", raising=False) + seen = {} + + def _record(start=None): + seen["start"] = start + return tmp_path / "log.jsonl" + + monkeypatch.setattr(decision_log, "default_log_path", _record) + cs._default_log_for(window) + + assert seen["start"] == Path(window.project_root), "the window's root, not the cwd" + + def test_the_window_records_the_project_it_describes(self, tmp_path): + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + + assert cs.resolve_window(repo).project_root == str(repo) + + def test_even_an_unavailable_window_records_its_project(self, tmp_path): + """Provenance must not depend on success, or a failure reason loses its subject.""" + assert cs.resolve_window(tmp_path).project_root == str(tmp_path) + + def test_a_rootless_window_still_falls_back_to_the_cwd_default(self, monkeypatch): + monkeypatch.setattr(decision_log, "default_log_path", lambda start=None: None) + + assert cs._default_log_for(cs.ChangeWindow(available=True)) is None + + +class TestGitFailureIsNotAConclusionAboutHistory: + """After the repo probe succeeds, a later git failure must not be reported as a + finding about the branch. "No merge base" and "git timed out" are different facts.""" + + def test_a_tool_failure_during_merge_base_reports_git_not_history(self, tmp_path, monkeypatch): + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + monkeypatch.setattr(cs, "_merge_base", lambda *_a, **_k: (None, True)) + + window = cs.resolve_window(repo) + + assert window.reason == cs.REASON_GIT_UNAVAILABLE + assert window.reason != cs.REASON_NO_MERGE_BASE + + def test_a_genuine_absence_of_a_merge_base_still_says_so(self, tmp_path, monkeypatch): + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + monkeypatch.setattr(cs, "_merge_base", lambda *_a, **_k: (None, False)) + + assert cs.resolve_window(repo).reason == cs.REASON_NO_MERGE_BASE + + def test_a_tool_failure_reading_the_base_time_reports_git(self, tmp_path, monkeypatch): + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + monkeypatch.setattr(cs, "_commit_time", lambda *_a, **_k: (None, True)) + window = cs.resolve_window(repo) + + assert window.reason == cs.REASON_GIT_UNAVAILABLE + assert window.base_sha, "what was already learned is still reported" + + def test_a_non_zero_exit_is_a_valid_negative_not_a_tool_failure(self, tmp_path): + """`merge-base` and `rev-parse --verify` exit non-zero to mean "no" — treating + that as breakage would mislead in the opposite direction.""" + repo = _make_repo(tmp_path / "r") + + value, failed = cs._git_query(repo, ["rev-parse", "--verify", "--quiet", "nope"]) + + assert value is None + assert failed is False + + def test_git_not_launching_is_a_tool_failure(self, tmp_path, monkeypatch): + def _boom(*_a, **_k): + raise OSError("no git") + monkeypatch.setattr(cs.subprocess, "run", _boom) + + assert cs._git_query(tmp_path, ["status"]) == (None, True) + + +class TestUndecodableLogs: + + def test_invalid_utf8_returns_unavailable_rather_than_raising(self, tmp_path): + """The probe opened the file but did not decode it, so `read_events` performed + the first strict read and UnicodeDecodeError escaped `select_events`.""" + log = tmp_path / "bad.jsonl" + log.write_bytes(b'{"schema":1,"ts":"2026-06-01T00:00:00+00:00"}\n\xff\xfe bad\n') + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + + selection = cs.select_events(window, path=log) + + assert selection.available is False + assert selection.reason == cs.REASON_LOG_UNREADABLE + + def test_a_log_that_breaks_mid_read_is_not_a_partial_selection(self, tmp_path, monkeypatch): + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) + + def _explode(*_a, **_k): + raise UnicodeDecodeError("utf-8", b"\xff", 0, 1, "boom") + + monkeypatch.setattr(decision_log, "read_events", _explode) + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + + selection = cs.select_events(window, path=log) + + assert selection.available is False + assert selection.reason == cs.REASON_LOG_UNREADABLE + + +class TestEveryEventIsGrouped: + + def test_events_without_a_run_id_are_bucketed_and_counted(self, tmp_path): + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + log = tmp_path / "d.jsonl" + rows = [ + _event("2026-06-01T00:00:00+00:00", "r1"), + _event("2026-06-01T00:00:01+00:00", ""), + {"schema": 1, "ts": "2026-06-01T00:00:02+00:00", "event": "x", "payload": {}}, + {"schema": 1, "ts": "2026-06-01T00:00:03+00:00", "run_id": None, + "event": "x", "payload": {}}, + ] + log.write_text("".join(json.dumps(r) + "\n" for r in rows), encoding="utf-8") + + selection = cs.select_events(window, path=log) + grouped = cs.group_by_run(selection) + + assert selection.runless == 3 + assert sum(len(v) for v in grouped.values()) == len(selection.events), \ + "every selected event lands in exactly one bucket" + assert grouped[cs.RUN_UNATTRIBUTED] and len(grouped[cs.RUN_UNATTRIBUTED]) == 3 + + def test_the_unattributed_bucket_cannot_collide_with_a_real_run_id(self): + """Run ids are hex; a parenthesised label cannot be produced as one.""" + assert not all(c in "0123456789abcdef" for c in cs.RUN_UNATTRIBUTED) + + class TestGrouping: def test_grouping_preserves_run_order(self, tmp_path): diff --git a/vulture_whitelist.py b/vulture_whitelist.py index 0f4f3e4e..0060b8fe 100644 --- a/vulture_whitelist.py +++ b/vulture_whitelist.py @@ -23,6 +23,7 @@ group_by_run, ChangeWindow, EventSelection, + RUN_UNATTRIBUTED, ) is_json = _UI.is_json # staticmethod alias exposed on the ui singleton @@ -151,3 +152,6 @@ EventSelection.scanned # noqa: B018 EventSelection.undated # noqa: B018 EventSelection.skipped_lines # noqa: B018 +ChangeWindow.project_root # noqa: B018 +EventSelection.runless # noqa: B018 +RUN_UNATTRIBUTED # noqa: B018 From 1d45aecf1641c15784f5bb5b2e370bd50e767fa3 Mon Sep 17 00:00:00 2001 From: ou Date: Thu, 3 Sep 2026 16:22:21 +0300 Subject: [PATCH 4/7] fix(change-summary): complete the git diagnosis, validate the bound, canonicalise run ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four more maintainer findings, all reproduced first. **A post-probe read failure was an available empty selection.** `read_events` swallows its own open failure and yields nothing, so a log that vanished between the readability probe and the read reported success having read nothing. Worse, the mitigation I claimed for this yesterday did not work: `_count_log_lines` returned 0 on a read error, so `skipped_lines` was 0 too and the failure left no trace anywhere. That count now returns `None` on failure and is the detector for the race, mapping to `REASON_LOG_UNREADABLE`. **Base-ref lookup discarded the tool-failure signal.** The previous round taught merge-base and commit-time to distinguish a git failure from a valid negative, but left base-ref resolution on the value-only helper — so a timeout there still surfaced as "requested base ref not found". Two of three stages were covered. The default walk also stops at the first launch failure rather than trying eight candidates and then reporting a fact about the repository that was never established. **An explicit `since` was accepted unvalidated**, failing later as a complaint about a base commit that was never consulted. It is parsed up front now, with a reason naming the caller's input. One of my own tests had encoded that behaviour as correct; it is rewritten to cover the direct-construction path it actually guards. **Run ids were used raw as grouping keys**, so case variants split one run, a numeric id merged with its own text, and whitespace formed an attributed group. Ids are now stripped and casefolded, and non-strings are unattributed. On that last point I did not adopt the suggested hexadecimal restriction, and the reasoning is on the PR: it would discard a real distinguishing identifier by folding it into the anonymous bucket, and `decision_log`'s schema is explicit that "readers must ignore unknown event names and unknown payload keys so that newer instrumentation never breaks an older reader". Stripping and casefolding fix all three reported defects without a reader rejecting what it does not recognise. Happy to add the stricter filter if the maintainers want it. Also drops a stale claim that the unattributed label cannot collide with a real id — that rested on the hex assumption. The property now pinned is the consequence: such events merge into one bucket and none is dropped. `select_events` gained a return branch, so log resolution is extracted to keep pylint's return-count rule satisfied without suppressing a check the project is rolling out. Full suite 5,262 passed. 134 tests on this module at 100% line coverage (287 stmts). pylint and vulture clean, `cfs validate` 0 errors, spec-coverage passes at granularity 0.4618. Signed-off-by: ou --- architecture/features/developer-experience.md | 2 + .../scripts/studio/utils/change_summary.py | 133 +++++++++--- tests/test_change_summary_core.py | 189 +++++++++++++++++- 3 files changed, 294 insertions(+), 30 deletions(-) diff --git a/architecture/features/developer-experience.md b/architecture/features/developer-experience.md index bebb3396..59dbe84a 100644 --- a/architecture/features/developer-experience.md +++ b/architecture/features/developer-experience.md @@ -254,6 +254,8 @@ Reduces friction in daily Studio usage. `doctor` catches environment issues befo 12. [x] - `p1` - Group selected events by run id in first-seen order, so one invocation is a subdivision of the branch's span and never the whole story - `inst-change-summary-group-runs` 13. [x] - `p1` - Resolve the decision log belonging to the window's own project rather than to the current working directory, so a digest never reports one project's changes alongside another's decisions - `inst-change-summary-default-log` 14. [x] - `p1` - Walk a known-good base ref down to a window, letting a git tool failure take precedence over a historical reading and keeping whatever was already learned on the returned window - `inst-change-summary-window-from-base` +15. [x] - `p1` - Reduce a run id to a canonical form, casefolding and stripping so one logical run is not split and a non-string does not merge with its own text, while not rejecting an unrecognised-but-real identifier - `inst-change-summary-canonical-run` +16. [x] - `p1` - Resolve and validate the decision log a window should be read from, returning either a usable path or the reason it is unusable - `inst-change-summary-resolve-log` ## 4. States (CDSL) diff --git a/skills/studio/scripts/studio/utils/change_summary.py b/skills/studio/scripts/studio/utils/change_summary.py index cb85ca07..22e84802 100644 --- a/skills/studio/scripts/studio/utils/change_summary.py +++ b/skills/studio/scripts/studio/utils/change_summary.py @@ -83,10 +83,17 @@ REASON_LOG_DISABLED = "decision log disabled" REASON_LOG_ABSENT = "no decision log yet" REASON_LOG_UNREADABLE = "decision log unreadable" +REASON_INVALID_SINCE = "the supplied lower bound is not an absolute timestamp" -#: Bucket name for selected events carrying no ``run_id``. Grouping used to build -#: buckets only for truthy ids, so such events sat in ``events`` and in no group and a -#: renderer under-reported them. Run ids are hex, so this cannot collide with a real one. +#: Bucket name for selected events whose ``run_id`` is missing or unusable. Grouping +#: used to build buckets only for truthy ids, so such events sat in ``events`` and in no +#: group, and a renderer summing the groups under-reported without saying so. +#: +#: The parentheses make it read as a label rather than an identifier, but nothing +#: *enforces* uniqueness: a writer emitting this exact string would share the bucket. +#: That is a deliberate trade — the alternative is rejecting unrecognised ids, which +#: discards real information (see :func:`_canonical_run_id`). Sharing a label is +#: cosmetic; dropping an event is not. RUN_UNATTRIBUTED = "(unattributed)" @@ -151,10 +158,15 @@ def _git_query(project_root: Path, args: List[str]) -> Tuple[Optional[str], bool cwd=str(project_root), capture_output=True, text=True, + # Git refs and paths are bytes and need not be UTF-8, so `text=True`'s + # strict default would raise UnicodeDecodeError past the handler below and + # break the never-raises contract. `surrogateescape` is the handler Python + # uses for filesystem values, so they round-trip to the same bytes. + errors="surrogateescape", timeout=_GIT_TIMEOUT, check=False, ) - except (OSError, subprocess.SubprocessError) as exc: + except (OSError, UnicodeDecodeError, subprocess.SubprocessError) as exc: logger.debug("change-summary git query could not run: %s", exc) return None, True if result.returncode: @@ -183,7 +195,7 @@ def _is_git_repo(project_root: Path) -> bool: # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-default-base -def _resolve_base_ref(project_root: Path, requested: str = "") -> Optional[str]: +def _resolve_base_ref(project_root: Path, requested: str = "") -> Tuple[Optional[str], bool]: """Pick the ref the window is measured from. An explicitly requested ref is honoured or refused — never silently swapped for a @@ -191,12 +203,23 @@ def _resolve_base_ref(project_root: Path, requested: str = "") -> Optional[str]: for is worse than one that says it could not comply. """ if requested: - resolved = _git_line(project_root, ["rev-parse", "--verify", "--quiet", requested]) - return requested if resolved else None + resolved, failed = _git_query( + project_root, ["rev-parse", "--verify", "--quiet", requested], + ) + return (requested if resolved else None), failed for candidate in _DEFAULT_BASE_REFS: - if _git_line(project_root, ["rev-parse", "--verify", "--quiet", candidate]): - return candidate - return None + resolved, failed = _git_query( + project_root, ["rev-parse", "--verify", "--quiet", candidate], + ) + if failed: + # Stop at the first tool failure rather than walking the remaining + # candidates: each would fail the same way, and reporting "no default base + # ref" after eight failed launches states a fact about the repository that + # was never established. + return None, True + if resolved: + return candidate, False + return None, False # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-default-base @@ -237,13 +260,19 @@ def resolve_window( """ root = str(project_root) if since: + # A caller-supplied bound is validated here rather than surfacing later as a + # complaint about a base commit that was never consulted. + if _parse_ts(since) is None: + return ChangeWindow(project_root=root, reason=REASON_INVALID_SINCE) return ChangeWindow(project_root=root, since=since, available=True, reason=REASON_OK) if not _is_git_repo(project_root): reason = REASON_NOT_A_REPO if _git_line(project_root, ["--version"]) else REASON_GIT_UNAVAILABLE return ChangeWindow(project_root=root, reason=reason) - base_ref = _resolve_base_ref(project_root, base) + base_ref, failed = _resolve_base_ref(project_root, base) + if failed: + return ChangeWindow(project_root=root, reason=REASON_GIT_UNAVAILABLE) if base_ref is None: # Two different failures, two different reasons: a ref the caller named and # git does not have, versus no discoverable default at all. @@ -351,18 +380,22 @@ def _log_unavailable(path: Path) -> str: # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-count-lines -def _count_log_lines(path: Path) -> int: - """Count non-empty lines in the log, for deriving how many failed to parse. - - Returns 0 on any read error: an unreadable log is already reported through the - availability reason, and a wrong skip count must not be invented on top of it. +def _count_log_lines(path: Path) -> Optional[int]: + """Count non-empty lines in the log, or ``None`` when it could not be read. + + ``None`` rather than ``0`` is the whole point. This count runs *after* + :func:`decision_log.read_events`, which swallows its own open failure and yields + nothing, so a log that vanished between the readability probe and the read produced + an *available, empty* selection — success reported having read nothing. Returning 0 + made that worse by yielding ``skipped_lines`` of 0 too, so the failure left no trace + anywhere. This read is therefore also the detector for that race. """ try: with path.open("r", encoding="utf-8", errors="replace") as handle: return sum(1 for line in handle if line.strip()) except OSError as exc: logger.debug("change-summary log line count failed: %s", exc) - return 0 + return None # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-count-lines @@ -381,6 +414,24 @@ def _default_log_for(window: ChangeWindow) -> Optional[Path]: # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-default-log +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-resolve-log +def _resolve_log_for(window: ChangeWindow, path: Optional[Path]) -> Tuple[Optional[Path], str]: + """Return ``(log path, reason)`` — exactly one of which is meaningful. + + Split out of :func:`select_events` so each function's guard clauses stay within the + project's return-count budget, and so "which log, and may it be read" is answerable + on its own. + """ + target = path or _default_log_for(window) + if target is None: + return None, REASON_NOT_A_PROJECT + reason = _log_unavailable(target) + if reason: + return None, reason + return target, REASON_OK +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-resolve-log + + # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-select-events def select_events( window: ChangeWindow, @@ -404,11 +455,8 @@ def select_events( if not window.available: return EventSelection(reason=window.reason) - target = path or _default_log_for(window) + target, reason = _resolve_log_for(window, path) if target is None: - return EventSelection(reason=REASON_NOT_A_PROJECT) - reason = _log_unavailable(target) - if reason: return EventSelection(reason=reason) boundary = _parse_ts(window.since) @@ -426,7 +474,7 @@ def select_events( if stamp < boundary: continue selected.append(event) - run_id = str(event.get("run_id") or "") + run_id = _canonical_run_id(event.get("run_id")) if not run_id: runless += 1 run_id = RUN_UNATTRIBUTED @@ -439,19 +487,56 @@ def select_events( logger.debug("change-summary log became unreadable while reading: %s", exc) return EventSelection(reason=REASON_LOG_UNREADABLE) + # Deliberately after the read: `read_events` cannot report its own open failure, + # so this second read is what distinguishes "the log held nothing in the window" + # from "the log was never read". + total_lines = _count_log_lines(target) + if total_lines is None: + return EventSelection(reason=REASON_LOG_UNREADABLE) + return EventSelection( events=selected, runs=runs, scanned=scanned, undated=undated, runless=runless, - skipped_lines=max(0, _count_log_lines(target) - scanned), + skipped_lines=max(0, total_lines - scanned), available=True, reason=REASON_OK, ) # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-select-events +# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-canonical-run +def _canonical_run_id(value: Any) -> str: + """Return the canonical form of a ``run_id``, or ``""`` when it is not usable. + + Raw values were used directly as grouping keys, which fragmented and merged history + in three ways: + + * **case variants split one run** — ``"AB12"`` and ``"ab12"`` became two groups for + something the writer would only ever have emitted once, so casefolding merges them; + * **a non-string merged with its own text** — ``1`` and ``"1"`` both stringified to + ``"1"``, so a numeric field silently joined an unrelated run. Only ``str`` is + accepted, which keeps them apart; + * **whitespace formed an attributed group** — ``" "`` is truthy, so it looked like + a real run. Stripping sends it to :data:`RUN_UNATTRIBUTED` where it belongs. + + What this deliberately does **not** do is require the writer's current shape + (``uuid4().hex[:12]``). Rejecting anything non-hexadecimal would discard a real, + distinguishing identifier by folding it into the anonymous bucket, and + ``decision_log``'s own schema is explicit that "readers must ignore unknown event + names and unknown payload keys so that newer instrumentation never breaks an older + reader". A reader that refuses a run id it does not recognise breaks exactly that. + An unrecognised-but-present id is more honestly reported under its own name than + merged into "unattributed", which is a claim that no id was recorded at all. + """ + if not isinstance(value, str): + return "" + return value.strip().lower() +# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-canonical-run + + # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-group-runs def group_by_run(selection: EventSelection) -> Dict[str, List[Dict[str, Any]]]: """Group a selection's events by ``run_id``, preserving first-seen run order. @@ -467,7 +552,7 @@ def group_by_run(selection: EventSelection) -> Dict[str, List[Dict[str, Any]]]: """ grouped: Dict[str, List[Dict[str, Any]]] = {run: [] for run in selection.runs} for event in selection.events: - run_id = str(event.get("run_id") or "") or RUN_UNATTRIBUTED + run_id = _canonical_run_id(event.get("run_id")) or RUN_UNATTRIBUTED grouped.setdefault(run_id, []).append(event) return grouped # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-group-runs diff --git a/tests/test_change_summary_core.py b/tests/test_change_summary_core.py index 9ed25df5..1c775753 100644 --- a/tests/test_change_summary_core.py +++ b/tests/test_change_summary_core.py @@ -334,7 +334,12 @@ def test_outside_a_studio_project_is_reported(self, monkeypatch): assert selection.reason == cs.REASON_NOT_A_PROJECT - def test_a_window_whose_since_will_not_parse_is_reported(self, tmp_path): + def test_a_hand_built_window_with_an_unparseable_bound_still_degrades(self, tmp_path): + """`resolve_window` now rejects a bad `since` up front, so this can only be + reached by constructing a window directly. The guard stays as the floor beneath + that, but it is no longer the path a caller passing `since` takes — see + `TestAnExplicitSinceIsValidatedUpFront`, which is where that case belongs. + """ window = cs.ChangeWindow(since="nonsense", available=True) log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) @@ -392,11 +397,13 @@ def test_an_existing_but_unreadable_log_is_never_an_available_empty_selection(se assert selection.reason == cs.REASON_LOG_UNREADABLE assert selection.events == [] - def test_the_line_count_degrades_to_zero_on_a_read_error(self, tmp_path, monkeypatch): + def test_the_line_count_reports_a_read_error_rather_than_zero(self, tmp_path, monkeypatch): + """Returning 0 made a failed read indistinguishable from an empty log, which + is what let a vanished log surface as an available empty selection.""" log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) monkeypatch.setattr(Path, "open", lambda *_a, **_k: (_ for _ in ()).throw(OSError("nope"))) - assert cs._count_log_lines(log) == 0 + assert cs._count_log_lines(log) is None def test_hostile_event_shapes_do_not_raise(self, tmp_path): window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) @@ -577,6 +584,163 @@ def _boom(*_a, **_k): assert cs._git_query(tmp_path, ["status"]) == (None, True) +class TestAnExplicitSinceIsValidatedUpFront: + """A caller-supplied bound is the caller's input, so a complaint about it must name + that input. Accepting it and failing later produced a reason about a base commit + that was never consulted.""" + + @pytest.mark.parametrize("bad", [ + "nonsense", "", "2026-13-99T00:00:00+00:00", "not-a-time", + ]) + def test_an_unparseable_bound_never_yields_an_available_window(self, bad, tmp_path): + window = cs.resolve_window(tmp_path, since=bad) + + assert window.available is False + if bad: + assert window.reason == cs.REASON_INVALID_SINCE + + def test_a_naive_bound_is_refused_rather_than_assumed_utc(self, tmp_path): + """Guessing an offset would silently shift the window boundary.""" + window = cs.resolve_window(tmp_path, since="2026-06-01T00:00:00") + + assert window.available is False + assert window.reason == cs.REASON_INVALID_SINCE + + def test_a_valid_bound_still_short_circuits_git(self): + window = cs.resolve_window(Path("/nonexistent-abc"), since="2026-01-01T00:00:00+00:00") + + assert window.available is True + assert window.reason == cs.REASON_OK + + +class TestBaseRefLookupPreservesTheGitDiagnosis: + """The previous round fixed this for merge-base and commit-time but left base-ref + lookup on the value-only helper, so two of three stages were covered.""" + + def test_a_tool_failure_on_an_explicit_ref_reports_git(self, tmp_path, monkeypatch): + repo = _make_repo(tmp_path / "r") + monkeypatch.setattr(cs, "_git_query", lambda *_a, **_k: (None, True)) + monkeypatch.setattr(cs, "_is_git_repo", lambda *_a, **_k: True) + + window = cs.resolve_window(repo, base="release") + + assert window.reason == cs.REASON_GIT_UNAVAILABLE + assert window.reason != cs.REASON_BASE_REF_UNKNOWN + + def test_a_tool_failure_on_the_default_ref_reports_git(self, tmp_path, monkeypatch): + repo = _make_repo(tmp_path / "r") + monkeypatch.setattr(cs, "_git_query", lambda *_a, **_k: (None, True)) + monkeypatch.setattr(cs, "_is_git_repo", lambda *_a, **_k: True) + + window = cs.resolve_window(repo) + + assert window.reason == cs.REASON_GIT_UNAVAILABLE + assert window.reason != cs.REASON_NO_BASE_REF + + def test_a_genuinely_missing_explicit_ref_still_says_so(self, tmp_path): + repo = _make_repo(tmp_path / "r") + + assert cs.resolve_window(repo, base="no-such").reason == cs.REASON_BASE_REF_UNKNOWN + + def test_the_default_walk_stops_at_the_first_tool_failure(self, tmp_path, monkeypatch): + """Walking the remaining candidates after a launch failure would report a fact + about the repository that was never established.""" + repo = _make_repo(tmp_path / "r") + calls = [] + + def _fail(_root, args): + calls.append(args) + return None, True + + monkeypatch.setattr(cs, "_git_query", _fail) + monkeypatch.setattr(cs, "_is_git_repo", lambda *_a, **_k: True) + cs.resolve_window(repo) + + assert len(calls) == 1, "one attempt, not one per candidate ref" + + +class TestAPostProbeReadFailureIsNeverAnEmptySuccess: + """`read_events` swallows its own open failure and yields nothing, so a log that + disappears between the probe and the read looked like an empty window.""" + + def test_a_log_that_vanishes_after_the_probe_is_reported(self, tmp_path, monkeypatch): + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) + real_probe = cs._log_unavailable + + def _probe_then_remove(path): + reason = real_probe(path) + path.unlink() + return reason + + monkeypatch.setattr(cs, "_log_unavailable", _probe_then_remove) + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + + selection = cs.select_events(window, path=log) + + assert selection.available is False, "success must not be reported having read nothing" + assert selection.reason == cs.REASON_LOG_UNREADABLE + + def test_the_line_count_distinguishes_unreadable_from_empty(self, tmp_path): + empty = tmp_path / "empty.jsonl" + empty.write_text("", encoding="utf-8") + + assert cs._count_log_lines(empty) == 0, "an empty log is a count, not a failure" + assert cs._count_log_lines(tmp_path / "gone.jsonl") is None + + +class TestRunIdsAreCanonicalised: + + @pytest.mark.parametrize("raw,expected", [ + ("abcdef012345", "abcdef012345"), + ("ABCDEF012345", "abcdef012345"), # case variants are one run, not two + (" ab12 ", "ab12"), # surrounding whitespace is not identity + (" ", ""), # whitespace is not a run + ("", ""), + (None, ""), + (1, ""), # a non-string must not join its own text + ("not-hex!", "not-hex!"), # unrecognised, but a real identifier + ("Custom-Run-7", "custom-run-7"), # a future writer's shape is not rejected + ]) + def test_the_canonical_form(self, raw, expected): + assert cs._canonical_run_id(raw) == expected + + def test_case_variants_form_one_group(self, tmp_path): + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + log = _write_log(tmp_path / "d.jsonl", [ + _event("2026-06-01T00:00:00+00:00", "ABCDEF012345"), + _event("2026-06-01T00:00:01+00:00", "abcdef012345"), + ]) + + selection = cs.select_events(window, path=log) + + assert selection.runs == ["abcdef012345"], "one logical run, not two" + assert len(cs.group_by_run(selection)["abcdef012345"]) == 2 + + def test_a_numeric_id_does_not_merge_with_its_own_text(self, tmp_path): + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + log = tmp_path / "d.jsonl" + log.write_text( + json.dumps({"schema": 1, "ts": "2026-06-01T00:00:00+00:00", "run_id": 1}) + "\n" + + json.dumps({"schema": 1, "ts": "2026-06-01T00:00:01+00:00", "run_id": "1"}) + "\n", + encoding="utf-8", + ) + + grouped = cs.group_by_run(cs.select_events(window, path=log)) + + assert set(grouped) == {cs.RUN_UNATTRIBUTED, "1"} + assert len(grouped[cs.RUN_UNATTRIBUTED]) == 1 + assert len(grouped["1"]) == 1 + + def test_a_whitespace_id_is_unattributed_not_a_named_run(self, tmp_path): + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00", " ")]) + + selection = cs.select_events(window, path=log) + + assert selection.runless == 1 + assert selection.runs == [cs.RUN_UNATTRIBUTED] + + class TestUndecodableLogs: def test_invalid_utf8_returns_unavailable_rather_than_raising(self, tmp_path): @@ -628,9 +792,22 @@ def test_events_without_a_run_id_are_bucketed_and_counted(self, tmp_path): "every selected event lands in exactly one bucket" assert grouped[cs.RUN_UNATTRIBUTED] and len(grouped[cs.RUN_UNATTRIBUTED]) == 3 - def test_the_unattributed_bucket_cannot_collide_with_a_real_run_id(self): - """Run ids are hex; a parenthesised label cannot be produced as one.""" - assert not all(c in "0123456789abcdef" for c in cs.RUN_UNATTRIBUTED) + def test_a_collision_with_the_unattributed_label_merges_rather_than_loses(self): + """This assertion used to rest on run ids being hexadecimal. That is no longer + enforced — an unrecognised id is kept rather than discarded — so a writer + emitting this exact parenthesised string would land in the same bucket. The + property worth pinning is therefore the *consequence*: such events merge into + one bucket, and none is dropped. Losing an event would be the real defect; + sharing a label with an anonymous one is cosmetic.""" + selection = cs.EventSelection( + events=[{"run_id": cs.RUN_UNATTRIBUTED}, {"run_id": None}], + runs=[cs.RUN_UNATTRIBUTED], + available=True, + ) + + grouped = cs.group_by_run(selection) + + assert sum(len(v) for v in grouped.values()) == 2 class TestGrouping: From aa1f968b562e9307d4c827f44947469b848a786d Mon Sep 17 00:00:00 2001 From: ou Date: Thu, 3 Sep 2026 17:11:56 +0300 Subject: [PATCH 5/7] fix(change-summary): pin the repository and the project the answer describes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven review findings; the eighth is answered on the PR rather than changed. **A relative project root left the cwd dependence in place.** Carrying the root on the window was supposed to stop the log resolving from the current directory, but `resolve_window(Path("."))` recorded `"."` — so a later chdir redirected log resolution again, and `subprocess(cwd=...)` re-resolved the relative path at call time rather than at capture time. The root is resolved now, in both entry points. Verified: the recorded root is absolute and survives a chdir. **An ambient GIT_DIR overrode `cwd=`.** Verified — `GIT_DIR=b/.git git -C a log` reports b's commit, not a's — so every query could silently answer about a different repository than the one named. The git environment is sanitised of the seven variables that redirect repository location. **Caller-controlled refs reached git without an end-of-options separator**, so a ref beginning with a dash was read as an option. All four call sites that interpolate a caller value now pass `--end-of-options`, with a structural test so a new call site without it is caught here rather than in review. Four findings were about verification claiming more than it established, which is the recurring shape of this review: - The log-binding test patched `default_log_path`, so it proved the root was *passed*, not that passing it works. There is now a test driving the real resolver against two genuine Studio projects with the process standing in the wrong one, plus one for the non-empty-root "not a project" branch. - The "no network" test patched `socket` in this process, which cannot observe a child's sockets. It now says so, and a companion test asserts the property that actually holds: every git subcommand issued is a local read, so none has a remote to reach. - The permission test errored rather than skipped on Windows. The neighbouring test was already guarded; this one now is too. - The whitelist entry for `ChangeWindow.project_root` was a false positive, and the comment above it claimed only fields no internal caller reads were listed — which that very commit contradicted. Entry removed, comment rewritten to say that a false positive there suppresses a real dead-code signal. Full suite 5,284 passed. 156 tests on this module at 100% line coverage (323 stmts). pylint and vulture clean, `cfs validate` 0 errors, spec-coverage passes at granularity 0.4613. Signed-off-by: ou --- .../scripts/studio/utils/change_summary.py | 43 ++++- tests/test_change_summary_core.py | 168 +++++++++++++++++- vulture_whitelist.py | 14 +- 3 files changed, 214 insertions(+), 11 deletions(-) diff --git a/skills/studio/scripts/studio/utils/change_summary.py b/skills/studio/scripts/studio/utils/change_summary.py index 22e84802..f47d9888 100644 --- a/skills/studio/scripts/studio/utils/change_summary.py +++ b/skills/studio/scripts/studio/utils/change_summary.py @@ -31,6 +31,7 @@ from __future__ import annotations import logging +import os import subprocess from dataclasses import dataclass, field from datetime import datetime @@ -44,6 +45,22 @@ #: Seconds any single git query may take before it is treated as unavailable. _GIT_TIMEOUT = 10 +#: Environment variables that redirect git away from the repository it was pointed at. +#: +#: ``cwd=`` is *not* sufficient on its own: an ambient ``GIT_DIR`` overrides it, so a +#: query about project A answered from project B's repository. Verified — +#: ``GIT_DIR=b/.git git -C a log`` reports b's commit, not a's. Every one of these is +#: cleared so the answer describes the project the caller named and nothing else. +_GIT_REDIRECT_VARS = ( + "GIT_DIR", + "GIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_OBJECT_DIRECTORY", + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + "GIT_COMMON_DIR", + "GIT_CEILING_DIRECTORIES", +) + #: Refs tried in order when the caller names no base. #: #: ``upstream/*`` comes first deliberately. In a fork-based workflow — which this @@ -136,6 +153,14 @@ class EventSelection: # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-git-query +def _git_env() -> Dict[str, str]: + """The ambient environment with git's repository-redirecting variables removed.""" + env = dict(os.environ) + for name in _GIT_REDIRECT_VARS: + env.pop(name, None) + return env + + def _git_query(project_root: Path, args: List[str]) -> Tuple[Optional[str], bool]: """Run a read-only git query, returning ``(first line or None, tool_failed)``. @@ -156,6 +181,7 @@ def _git_query(project_root: Path, args: List[str]) -> Tuple[Optional[str], bool result = subprocess.run( ["git"] + args, cwd=str(project_root), + env=_git_env(), capture_output=True, text=True, # Git refs and paths are bytes and need not be UTF-8, so `text=True`'s @@ -204,12 +230,15 @@ def _resolve_base_ref(project_root: Path, requested: str = "") -> Tuple[Optional """ if requested: resolved, failed = _git_query( - project_root, ["rev-parse", "--verify", "--quiet", requested], + project_root, + # `--end-of-options` so a caller-supplied ref beginning with a dash is read + # as a ref rather than as a git option. + ["rev-parse", "--verify", "--quiet", "--end-of-options", requested], ) return (requested if resolved else None), failed for candidate in _DEFAULT_BASE_REFS: resolved, failed = _git_query( - project_root, ["rev-parse", "--verify", "--quiet", candidate], + project_root, ["rev-parse", "--verify", "--quiet", "--end-of-options", candidate], ) if failed: # Stop at the first tool failure rather than walking the remaining @@ -232,14 +261,14 @@ def _merge_base(project_root: Path, base_ref: str) -> Tuple[Optional[str], bool] ``True`` flag means git never answered, which is a different fact and must not be reported as a finding about history. """ - return _git_query(project_root, ["merge-base", "HEAD", base_ref]) + return _git_query(project_root, ["merge-base", "--end-of-options", "HEAD", base_ref]) # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-merge-base # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-base-time def _commit_time(project_root: Path, sha: str) -> Tuple[Optional[str], bool]: """Commit time in strict ISO 8601, plus whether git itself failed.""" - return _git_query(project_root, ["show", "-s", "--format=%cI", sha]) + return _git_query(project_root, ["show", "-s", "--format=%cI", "--end-of-options", sha]) # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-base-time @@ -258,6 +287,12 @@ def resolve_window( Every failure returns an unavailable window carrying its reason. Never raises. """ + # Resolved, not merely stored. A relative root left as-is puts the cwd dependence + # straight back: `resolve_window(Path("."))` recorded "." and a later chdir then + # redirected log resolution to a different project — the exact failure carrying the + # root on the window was added to prevent. Resolving also fixes the git cwd, since + # `subprocess(cwd=...)` resolves a relative path at call time, not at capture time. + project_root = Path(project_root).resolve() root = str(project_root) if since: # A caller-supplied bound is validated here rather than surfacing later as a diff --git a/tests/test_change_summary_core.py b/tests/test_change_summary_core.py index 1c775753..fe179653 100644 --- a/tests/test_change_summary_core.py +++ b/tests/test_change_summary_core.py @@ -55,6 +55,20 @@ def _point_ref(repo: Path, ref: str, sha: str) -> None: _git(repo, "update-ref", ref, sha) +def _make_studio_project(root: Path) -> Path: + """A directory the real project/studio resolvers recognise. + + `find_project_root` wants the `@cf:root-agents` marker in AGENTS.md, and + `find_studio_directory` then reads the `studio` key from its TOML block. + """ + (root / ".studio" / "rules").mkdir(parents=True) + (root / "AGENTS.md").write_text( + '\n\n```toml\nstudio = ".studio"\n```\n', encoding="utf-8", + ) + (root / ".studio" / "AGENTS.md").write_text("# studio\n", encoding="utf-8") + return root + + def _write_log(path: Path, rows: list, extra: str = "") -> Path: body = "".join(json.dumps(r) + "\n" for r in rows) + extra path.write_text(body, encoding="utf-8") @@ -383,6 +397,8 @@ def test_an_existing_but_unreadable_log_is_never_an_available_empty_selection(se """The false green: is_file() passes on mode 000, and read_events swallows the open failure and yields nothing — so this reported "no decisions" having read none. Regression for the exact defect class this module exists to prevent.""" + if os.name == "nt": + pytest.skip("POSIX permission bits do not apply on Windows") if os.geteuid() == 0: pytest.skip("root bypasses file permissions") log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) @@ -460,8 +476,10 @@ def test_no_reason_string_leaks_a_path_home_or_username(self): if user: assert user not in value - def test_no_network_is_used(self, tmp_path, monkeypatch): - """Prove it rather than assert it: make sockets impossible and still work.""" + def test_this_process_opens_no_socket(self, tmp_path, monkeypatch): + """Scope stated honestly: patching `socket` covers **this** process only. It + says nothing about what the `git` child does, so the companion test below + checks the git side by a different means.""" def _no_sockets(*_a, **_k): raise AssertionError("change_summary must not open a socket") monkeypatch.setattr(socket, "socket", _no_sockets) @@ -474,6 +492,43 @@ def _no_sockets(*_a, **_k): assert window.available is True assert cs.select_events(window, path=log).available is True + def test_no_git_subcommand_can_reach_a_remote(self, tmp_path, monkeypatch): + """The git side of the no-network claim, which the socket patch above cannot + observe: a child process has its own sockets. Rather than pretending to watch + them, this asserts the only thing that actually matters — every subcommand + issued is a local read, so none of them has a remote to reach.""" + remote_capable = { + "fetch", "pull", "push", "clone", "remote", "ls-remote", + "submodule", "archive", "bundle", "daemon", "send-pack", "fetch-pack", + } + issued: list = [] + + def _capture(args, **_kwargs): + issued.append(list(args)) + raise OSError("not run") + + # The repo is built first: `subprocess` is shared, so patching it before the + # fixture would intercept the fixture's own git calls. + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + window = cs.ChangeWindow( + project_root=str(repo), base_sha=_git(repo, "rev-parse", "HEAD"), + since="2026-01-01T00:00:00+00:00", available=True, + ) + + monkeypatch.setattr(cs.subprocess, "run", _capture) + cs.resolve_window(repo) + # The linkage half lives in a later change; exercise it when present so this + # test covers every git call site on whichever branch it runs. + linker = getattr(cs, "link_changed_files", None) + if linker is not None: + linker(repo, window) + + assert issued, "the helper must actually have been exercised" + for argv in issued: + subcommand = argv[1] if len(argv) > 1 else "" + assert subcommand not in remote_capable, f"remote-capable: {subcommand}" + class TestDeterminism: @@ -535,6 +590,115 @@ def test_a_rootless_window_still_falls_back_to_the_cwd_default(self, monkeypatch assert cs._default_log_for(cs.ChangeWindow(available=True)) is None + def test_the_real_resolver_finds_the_windows_project_from_another_cwd( + self, tmp_path, monkeypatch, + ): + """The earlier test patched `default_log_path`, so it proved the root was + *passed* — not that passing it works. This drives the real resolver against two + genuine Studio projects with the process standing in the wrong one.""" + monkeypatch.delenv("CFS_DECISION_LOG", raising=False) + wanted = _make_studio_project(tmp_path / "wanted") + other = _make_studio_project(tmp_path / "other") + monkeypatch.chdir(other) + + resolved = cs._default_log_for( + cs.ChangeWindow(project_root=str(wanted), available=True), + ) + + assert resolved is not None + assert wanted.resolve() in resolved.parents, "the window's project, not the cwd's" + assert other.resolve() not in resolved.parents + + def test_a_root_that_is_not_a_studio_project_is_reported(self, tmp_path, monkeypatch): + """The non-empty-root branch: a root was recorded, but it is not a project.""" + monkeypatch.delenv("CFS_DECISION_LOG", raising=False) + window = cs.ChangeWindow( + project_root=str(tmp_path), since="2026-01-01T00:00:00+00:00", available=True, + ) + + selection = cs.select_events(window) + + assert selection.available is False + assert selection.reason == cs.REASON_NOT_A_PROJECT + + +class TestTheAnswerDoesNotDependOnWhereTheProcessStands: + """The window carries its project so the log cannot come from the cwd. That only + holds if the recorded root is absolute — a relative one reintroduces the exact + dependence, since both `default_log_path` and `subprocess(cwd=...)` resolve at use + time rather than at capture time.""" + + def test_a_relative_root_is_stored_resolved(self, tmp_path, monkeypatch): + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + monkeypatch.chdir(repo) + + window = cs.resolve_window(Path(".")) + + assert Path(window.project_root).is_absolute() + assert Path(window.project_root) == repo.resolve() + + def test_a_window_survives_a_later_chdir(self, tmp_path, monkeypatch): + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + monkeypatch.chdir(repo) + window = cs.resolve_window(Path(".")) + + monkeypatch.chdir(tmp_path) # the process moves after capture + + assert Path(window.project_root) == repo.resolve(), "the recorded root must not move" + + def test_an_ambient_git_dir_cannot_redirect_the_query(self, tmp_path, monkeypatch): + """`cwd=` alone is not enough: GIT_DIR overrides it, so a query about one + project could be answered from another's repository.""" + here = _make_repo(tmp_path / "here") + elsewhere = _make_repo(tmp_path / "elsewhere") + _point_ref(here, "refs/remotes/upstream/main", _git(here, "rev-parse", "HEAD")) + monkeypatch.setenv("GIT_DIR", str(elsewhere / ".git")) + monkeypatch.setenv("GIT_WORK_TREE", str(elsewhere)) + + window = cs.resolve_window(here) + + assert window.available is True + assert window.base_sha == _git(here, "rev-parse", "HEAD"), "answered from `here`" + + def test_the_sanitised_environment_drops_every_redirect_variable(self, monkeypatch): + for name in cs._GIT_REDIRECT_VARS: + monkeypatch.setenv(name, "/somewhere/else") + + env = cs._git_env() + + assert not [n for n in cs._GIT_REDIRECT_VARS if n in env] + assert "PATH" in env, "the rest of the environment is preserved" + + +class TestCallerValuesCannotBecomeGitOptions: + + def test_a_ref_beginning_with_a_dash_is_treated_as_a_ref(self, tmp_path): + """Without `--end-of-options` git reads this as an option, so the failure is + an argument error rather than an honest "no such ref".""" + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + + window = cs.resolve_window(repo, base="--upload-pack=touch /tmp/pwned") + + assert window.available is False + assert window.reason == cs.REASON_BASE_REF_UNKNOWN, "refused as a ref, not as an option" + + def test_every_option_bearing_call_separates_its_operands(self): + """A structural check, so a new call site that interpolates a caller value + without the separator is caught here rather than in review.""" + import inspect + source = inspect.getsource(cs) + required = ['"--verify", "--quiet", "--end-of-options"', + '["merge-base", "--end-of-options"', + '"--format=%cI", "--end-of-options"'] + if "_git_records" in source: + # Only present once the linkage half lands. + required.append('"--name-status", "-z", "--end-of-options"') + for fragment in required: + assert fragment in source, f"missing separator: {fragment}" + class TestGitFailureIsNotAConclusionAboutHistory: """After the repo probe succeeds, a later git failure must not be reported as a diff --git a/vulture_whitelist.py b/vulture_whitelist.py index 0060b8fe..ecd08a80 100644 --- a/vulture_whitelist.py +++ b/vulture_whitelist.py @@ -140,10 +140,15 @@ SemanticCalibration.judge # noqa: B018 SemanticCalibration.schema_version # noqa: B018 -# Change-summary core: the window and event-selection API the change-summary -# command will consume. Landed ahead of its CLI wrapper so the pure logic is -# reviewable on its own, so nothing in production calls it yet. Only the fields -# no internal caller reads are listed — the rest are genuinely referenced. +# Change-summary core: the window, event-selection and linkage API the +# change-summary command will consume. Landed ahead of its CLI wrapper so the +# pure logic is reviewable on its own, so nothing in production calls it yet. +# +# Listed here are the module's entry points plus the result fields that only an +# external consumer reads. Fields the module reads itself are deliberately absent: +# an entry for one of those is a false positive that suppresses a real dead-code +# signal, so if vulture stops flagging a name here it should be removed rather +# than kept "just in case". resolve_window # noqa: B018 select_events # noqa: B018 group_by_run # noqa: B018 @@ -152,6 +157,5 @@ EventSelection.scanned # noqa: B018 EventSelection.undated # noqa: B018 EventSelection.skipped_lines # noqa: B018 -ChangeWindow.project_root # noqa: B018 EventSelection.runless # noqa: B018 RUN_UNATTRIBUTED # noqa: B018 From 2bf3f3d37663b4a9ab3ff8e7719eb60fc364c21f Mon Sep 17 00:00:00 2001 From: ou Date: Thu, 3 Sep 2026 17:28:15 +0300 Subject: [PATCH 6/7] test(change-summary): make the GIT_DIR redirect test falsifiable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught this and local runs did not, for an instructive reason. `_make_repo` writes identical content with a fixed identity, so two repositories created in the same second produce the *same* commit sha. The test compared the window's sha against a value read from the decoy repository — and since both were the same string, it passed whether or not the environment sanitising worked. An assertion that cannot fail. It surfaced in CI only because the two fixture commits happened to straddle a second boundary there, making the shas differ and the comparison meaningful for the first time. So the red build was the test finally becoming real, not a regression. Two changes: the decoy repository gets a distinct commit so the shas genuinely differ, with a precondition assertion so a future fixture change cannot quietly restore the tautology; and the expected sha is captured before the redirect is installed, since reading it afterwards routes the test's own helper through the mechanism under test. Mutation-checked: removing the environment sanitising now fails this test, which it did not before. Signed-off-by: ou --- tests/test_change_summary_core.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/tests/test_change_summary_core.py b/tests/test_change_summary_core.py index fe179653..187166a4 100644 --- a/tests/test_change_summary_core.py +++ b/tests/test_change_summary_core.py @@ -653,14 +653,29 @@ def test_an_ambient_git_dir_cannot_redirect_the_query(self, tmp_path, monkeypatc project could be answered from another's repository.""" here = _make_repo(tmp_path / "here") elsewhere = _make_repo(tmp_path / "elsewhere") - _point_ref(here, "refs/remotes/upstream/main", _git(here, "rev-parse", "HEAD")) + # The two repositories must be genuinely distinguishable. `_make_repo` writes + # identical content with a fixed identity, so two repos created in the same + # second produce the *same* commit sha — which made the first version of this + # test unfalsifiable: it compared a value against its own twin and passed + # whether or not the redirect had been neutralised. A distinct commit in the + # decoy is what gives the assertion something to fail on. + _commit(elsewhere, "decoy.txt", "different content\n") + + # Both shas are captured *before* the redirect is installed. Reading the + # expected value afterwards would route the test's own helper through the very + # mechanism under test, comparing wrong against wrong. + expected = _git(here, "rev-parse", "HEAD") + decoy = _git(elsewhere, "rev-parse", "HEAD") + assert expected != decoy, "fixture precondition: the repositories must differ" + _point_ref(here, "refs/remotes/upstream/main", expected) monkeypatch.setenv("GIT_DIR", str(elsewhere / ".git")) monkeypatch.setenv("GIT_WORK_TREE", str(elsewhere)) window = cs.resolve_window(here) assert window.available is True - assert window.base_sha == _git(here, "rev-parse", "HEAD"), "answered from `here`" + assert window.base_sha == expected, "answered from `here`" + assert window.base_sha != decoy, "and not from the redirect target" def test_the_sanitised_environment_drops_every_redirect_variable(self, monkeypatch): for name in cs._GIT_REDIRECT_VARS: From 124f7d95c804afbf0efec438cc6db96fee3af9e9 Mon Sep 17 00:00:00 2001 From: ou Date: Thu, 3 Sep 2026 18:10:45 +0300 Subject: [PATCH 7/7] fix(change-summary): one log read, honest repo detection, frozen records MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on the window and event-selection half, each reproduced before being changed: * The log was opened three times — probe, read, count — so a valid line appended between the last two was reported as corruption, and a rotation between the first two swapped the verified file for a fresh one with no trace. `_read_log` now takes one snapshot; readability, the events and the line count all come from it, and `skipped_lines` is exact rather than a bound. Parsing moves to `decision_log.parse_events`, which `read_events` now uses too, so there is one copy of the rules. * `_is_git_repo` dropped the tool-failure flag and guessed with a second `git --version` launch, so a timeout on the real question came back as "not a git repository". `_detect_repo` returns the reason directly and launches git once. A bare repository or the `.git` directory itself now reports the new `REASON_NO_WORK_TREE` rather than denying a repository exists. * `ChangeWindow` and `EventSelection` are frozen, with `events` and `runs` as tuples, so the counts cannot be made wrong through the collections they describe. `group_by_run` deliberately still shares event objects. * A `$CFS_DECISION_LOG` override is followed — that is where the writer wrote — and reported through `EventSelection.log_overridden`, since a shared log cannot be attributed to the window's project. * A NUL byte in a requested ref is refused as a ref that cannot exist, rather than raising `ValueError` out of `subprocess` past the never-raises contract. * `_canonical_run_id` casefolds, as its docstring already promised; `lower()` left "Straße" and "STRASSE" as two runs. * The boundary following the merge-base — and so moving after a rebase — is documented on the module and on `resolve_window`, with `since=` as the remedy and a test pinning both. Widening to the earliest author date was rejected: author dates are arbitrary, so one old commit would pull years of unrelated decisions into the window. Spec: steps reworded to the failure-aware contract, the retired line-count step removed and the list renumbered. Whitelist: `log_overridden`. Tests: 114 on the module (was 95), 100% line coverage (186 stmts). Eleven mutation checks each fail only the tests written for them. Signed-off-by: ou --- architecture/features/developer-experience.md | 23 +- .../scripts/studio/utils/change_summary.py | 289 ++++++------ .../scripts/studio/utils/decision_log.py | 59 ++- tests/test_change_summary_core.py | 414 +++++++++++++++--- vulture_whitelist.py | 1 + 5 files changed, 572 insertions(+), 214 deletions(-) diff --git a/architecture/features/developer-experience.md b/architecture/features/developer-experience.md index 59dbe84a..07e37daf 100644 --- a/architecture/features/developer-experience.md +++ b/architecture/features/developer-experience.md @@ -240,22 +240,21 @@ Reduces friction in daily Studio usage. `doctor` catches environment issues befo **Output**: The span of work a change digest covers, and the decision-log events recorded inside it **Rules**: -1. [x] - `p1` - Define the window and selection result types, and the reason vocabulary shared by producer and renderer so an unavailable dimension is always named rather than shown as empty - `inst-change-summary-datamodel` -2. [x] - `p1` - Answer read-only git queries as one line of output or nothing, treating git absent, non-zero exit, timeout and empty output identically - `inst-change-summary-git-query` -3. [x] - `p1` - Detect whether the project root sits inside a git work tree - `inst-change-summary-detect-repo` +1. [x] - `p1` - Define the window and selection result types as immutable records, and the reason vocabulary shared by producer and renderer so an unavailable dimension is always named rather than shown as empty - `inst-change-summary-datamodel` +2. [x] - `p1` - Answer read-only git queries as one line of output or nothing, keeping a tool failure apart from a valid negative so a timeout is never reported as a conclusion about history - `inst-change-summary-git-query` +3. [x] - `p1` - Detect whether the project root sits inside a git work tree, telling not-a-repository apart from a repository without a working tree and from git itself failing to answer - `inst-change-summary-detect-repo` 4. [x] - `p1` - Resolve the base ref, preferring the canonical remote over a fork's lagging default, and honour or refuse an explicitly requested ref rather than substituting a fallback - `inst-change-summary-default-base` 5. [x] - `p1` - Resolve the merge-base between HEAD and the base ref, treating unrelated histories as no window - `inst-change-summary-merge-base` -6. [x] - `p1` - Read the base commit's commit time as the window's lower bound - `inst-change-summary-base-time` +6. [x] - `p1` - Read the base commit's commit time as the window's lower bound, accepting that the boundary moves with the merge-base and that an explicit lower bound is how a caller pins it - `inst-change-summary-base-time` 7. [x] - `p1` - Assemble the window, short-circuiting git when the caller supplies an explicit lower bound, and returning a stated reason on every failure path instead of raising - `inst-change-summary-resolve-window` 8. [x] - `p1` - Parse ISO-8601 timestamps to aware datetimes, normalising a trailing Z and refusing naive values rather than assuming an offset that would move events across the boundary - `inst-change-summary-parse-ts` -9. [x] - `p1` - Report why the decision log cannot be read, distinguishing opt-out from absent from unreadable - `inst-change-summary-log-state` -10. [x] - `p1` - Count the log's non-empty lines so the number that failed to parse can be derived and reported as a lower bound on corruption - `inst-change-summary-count-lines` -11. [x] - `p1` - Select events at or after the window boundary, excluding and counting undated events rather than guessing them into or out of the window - `inst-change-summary-select-events` -12. [x] - `p1` - Group selected events by run id in first-seen order, so one invocation is a subdivision of the branch's span and never the whole story - `inst-change-summary-group-runs` -13. [x] - `p1` - Resolve the decision log belonging to the window's own project rather than to the current working directory, so a digest never reports one project's changes alongside another's decisions - `inst-change-summary-default-log` -14. [x] - `p1` - Walk a known-good base ref down to a window, letting a git tool failure take precedence over a historical reading and keeping whatever was already learned on the returned window - `inst-change-summary-window-from-base` -15. [x] - `p1` - Reduce a run id to a canonical form, casefolding and stripping so one logical run is not split and a non-string does not merge with its own text, while not rejecting an unrecognised-but-real identifier - `inst-change-summary-canonical-run` -16. [x] - `p1` - Resolve and validate the decision log a window should be read from, returning either a usable path or the reason it is unusable - `inst-change-summary-resolve-log` +9. [x] - `p1` - Read the decision log once and take readability, the events and the corruption count from that single snapshot, keeping absent apart from unreadable, so nothing appended or rotated between separate reads is reported as this window's state - `inst-change-summary-log-state` +10. [x] - `p1` - Select events at or after the window boundary, excluding and counting undated events rather than guessing them into or out of the window - `inst-change-summary-select-events` +11. [x] - `p1` - Group selected events by run id in first-seen order, so one invocation is a subdivision of the branch's span and never the whole story - `inst-change-summary-group-runs` +12. [x] - `p1` - Resolve the decision log belonging to the window's own project rather than to the current working directory, following a process-wide override where the environment sets one but reporting that it did, so a digest never presents another project's decisions as this one's - `inst-change-summary-default-log` +13. [x] - `p1` - Walk a known-good base ref down to a window, letting a git tool failure take precedence over a historical reading and keeping whatever was already learned on the returned window - `inst-change-summary-window-from-base` +14. [x] - `p1` - Reduce a run id to a canonical form, casefolding and stripping so one logical run is not split and a non-string does not merge with its own text, while not rejecting an unrecognised-but-real identifier - `inst-change-summary-canonical-run` +15. [x] - `p1` - Resolve the decision log a window should be read from, returning either a usable path and whether the environment chose it, or the reason no log is usable - `inst-change-summary-resolve-log` ## 4. States (CDSL) diff --git a/skills/studio/scripts/studio/utils/change_summary.py b/skills/studio/scripts/studio/utils/change_summary.py index f47d9888..8dcbd680 100644 --- a/skills/studio/scripts/studio/utils/change_summary.py +++ b/skills/studio/scripts/studio/utils/change_summary.py @@ -11,7 +11,12 @@ * **The window comes from git, not from a decision-log ``run_id``.** A ``run_id`` is one CLI invocation, but a reviewer's "run" is a branch's worth of work. The window is the span since the merge-base with the default branch, so ``run_id`` becomes a - grouping key *inside* that span rather than the span itself. + grouping key *inside* that span rather than the span itself. The boundary is the + merge-base's own commit time, so it moves when the merge-base does — a rebase onto + newer upstream commits advances it, and decisions logged before the new base commit + fall outside the window. Git keeps no record of where a branch *used* to start, so + that is documented (see :func:`resolve_window`) rather than guessed around, and an + explicit ``since`` pins the boundary where the caller says. * **Nothing here raises, and nothing here is silent.** Every path returns a value carrying an explicit ``reason`` when a dimension is unavailable. A digest that quietly shows less is the defect this effort exists to remove, so "cannot tell" is @@ -22,7 +27,8 @@ Git access is a narrow read-only query helper, not a general runner. The two existing private ``_run_git`` helpers in this package have incompatible contracts — one returns ``(code, stdout, stderr)``, the other returns a string and raises — so a third generic -copy would duplicate both. ``_git_line`` answers only "one line of stdout, or nothing". +copy would duplicate both. ``_git_query`` answers only "one line of stdout, or nothing — +and whether git itself failed to answer". @cpt-algo:cpt-studio-algo-developer-experience-change-summary:p1 """ @@ -33,7 +39,7 @@ import logging import os import subprocess -from dataclasses import dataclass, field +from dataclasses import dataclass from datetime import datetime from pathlib import Path from typing import Any, Dict, List, Optional, Tuple @@ -89,6 +95,9 @@ # instead of matching on prose that can drift. REASON_OK = "" REASON_NOT_A_REPO = "not a git repository" +#: A bare repository, or a path inside the ``.git`` directory itself. Both *are* +#: repositories, so reporting them as :data:`REASON_NOT_A_REPO` was a false statement. +REASON_NO_WORK_TREE = "in a git repository but outside any working tree" REASON_GIT_UNAVAILABLE = "git unavailable" #: Kept free of an enumerated candidate list on purpose: the first version of this #: string named the refs it tried, and went stale the moment the list changed. @@ -114,13 +123,16 @@ RUN_UNATTRIBUTED = "(unattributed)" -@dataclass +@dataclass(frozen=True) class ChangeWindow: """The span of work a digest covers. ``available`` false means no git-derived window could be established; ``reason`` then says which of the failure modes applied. ``since`` is the base commit's own commit time, which is what makes the window "everything after the branch point". + + Frozen: a window records what git said at one moment, and nothing here mutates one + after construction, so nothing may. """ project_root: str = "" @@ -131,22 +143,32 @@ class ChangeWindow: reason: str = REASON_NOT_A_REPO -@dataclass +@dataclass(frozen=True) class EventSelection: """Decision-log events falling inside a window. - ``skipped_lines`` is derived, not observed: :func:`decision_log.read_events` drops - unparseable lines without reporting a count, so this compares the log's non-empty - line count against the events actually returned. It is therefore a lower bound on - corruption, and it is reported rather than hidden. + ``skipped_lines`` is the number of non-empty log lines that yielded no event — not + JSON, or JSON that is not an object. It is exact for the snapshot the selection was + read from, because the events and the line count come from one read of the file + (see :func:`_read_log`), and it is reported rather than hidden. + + ``log_overridden`` says the environment named the log (``$CFS_DECISION_LOG``) rather + than the window's project. That log is shared by every project the process ran in, + so its events cannot be attributed to this window's project, and a digest should + say so instead of presenting them as the project's own. + + Frozen, with ``events`` and ``runs`` as tuples: the counts describe those + collections, and a caller able to grow or shrink them would silently make the + counts wrong. """ - events: List[Dict[str, Any]] = field(default_factory=list) - runs: List[str] = field(default_factory=list) + events: Tuple[Dict[str, Any], ...] = () + runs: Tuple[str, ...] = () scanned: int = 0 undated: int = 0 runless: int = 0 skipped_lines: int = 0 + log_overridden: bool = False available: bool = False reason: str = REASON_NOT_A_PROJECT # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-datamodel @@ -200,23 +222,32 @@ def _git_query(project_root: Path, args: List[str]) -> Tuple[Optional[str], bool return None, False line = result.stdout.strip().splitlines() return (line[0].strip() if line else None), False - - -def _git_line(project_root: Path, args: List[str]) -> Optional[str]: - """First output line of a read-only git query, or ``None`` for any non-answer. - - For callers that only need the value; use :func:`_git_query` where a tool failure - must be told apart from a valid negative. - """ - value, _failed = _git_query(project_root, args) - return value # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-git-query # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-detect-repo -def _is_git_repo(project_root: Path) -> bool: - """Report whether ``project_root`` sits inside a git work tree.""" - return _git_line(project_root, ["rev-parse", "--is-inside-work-tree"]) == "true" +def _detect_repo(project_root: Path) -> str: + """Return :data:`REASON_OK` when ``project_root`` is inside a git work tree, else why not. + + One query, three answers, kept apart: + + * a **tool failure** is :data:`REASON_GIT_UNAVAILABLE`. Nothing was learned, so + nothing is claimed about the directory. An earlier version dropped this flag and + asked ``git --version`` separately to guess between "not a repo" and "git broken" + — so a timeout here followed by a healthy second launch was reported as a fact + about the directory that was never established, and every non-repo path cost two + launches instead of one; + * a **non-zero exit** is :data:`REASON_NOT_A_REPO`: git looked, and there is none; + * ``false`` is :data:`REASON_NO_WORK_TREE`: a bare repository, or the ``.git`` + directory itself. Both are repositories, and a digest of working-tree changes + needs a working tree, so the reason names the thing that is actually missing. + """ + answer, failed = _git_query(project_root, ["rev-parse", "--is-inside-work-tree"]) + if failed: + return REASON_GIT_UNAVAILABLE + if answer is None: + return REASON_NOT_A_REPO + return REASON_OK if answer == "true" else REASON_NO_WORK_TREE # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-detect-repo @@ -229,6 +260,11 @@ def _resolve_base_ref(project_root: Path, requested: str = "") -> Tuple[Optional for is worse than one that says it could not comply. """ if requested: + if "\0" in requested: + # No ref can contain NUL, and `subprocess` refuses to pass one — raising + # ValueError before git starts, outside `_git_query`'s handler. Refuse it + # here as what it is: a ref that cannot exist. + return None, False resolved, failed = _git_query( project_root, # `--end-of-options` so a caller-supplied ref beginning with a dash is read @@ -285,6 +321,18 @@ def resolve_window( assertion and needs no branch point. Otherwise the window starts at the merge-base with ``base`` (or the first of :data:`_DEFAULT_BASE_REFS` that exists). + **The boundary moves with the merge-base.** It is the base commit's *commit* time, + so rebasing onto newer upstream commits advances it, and decisions logged before + the new base commit was committed then fall outside the window — while the files + they concern still show as changed, because file changes are measured by tree + content, not by time. Git keeps no record of where the branch used to start, so + the old boundary cannot be recovered from history. The sha and time that *were* + used are carried on the window so a digest can print them, and ``since`` pins the + boundary where the caller says. Widening automatically to the branch's earliest + author date was considered and rejected: author dates are arbitrary (cherry-picks, + ``--date``), so one old commit would silently pull years of unrelated decisions + into the window — the opposite failure, and harder to notice. + Every failure returns an unavailable window carrying its reason. Never raises. """ # Resolved, not merely stored. A relative root left as-is puts the cwd dependence @@ -301,9 +349,9 @@ def resolve_window( return ChangeWindow(project_root=root, reason=REASON_INVALID_SINCE) return ChangeWindow(project_root=root, since=since, available=True, reason=REASON_OK) - if not _is_git_repo(project_root): - reason = REASON_NOT_A_REPO if _git_line(project_root, ["--version"]) else REASON_GIT_UNAVAILABLE - return ChangeWindow(project_root=root, reason=reason) + repo_state = _detect_repo(project_root) + if repo_state: + return ChangeWindow(project_root=root, reason=repo_state) base_ref, failed = _resolve_base_ref(project_root, base) if failed: @@ -379,91 +427,80 @@ def _parse_ts(value: Any) -> Optional[datetime]: # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-log-state -def _log_unavailable(path: Path) -> str: - """Return the reason the decision log cannot be read, or :data:`REASON_OK`. - - Absent and unreadable are reported separately, and readability is proved by - opening the file rather than inferred from :meth:`Path.is_file`. ``is_file`` only - needs ``stat``, so a mode-000 log passes it — and - :func:`decision_log.read_events` swallows the subsequent open failure and yields - nothing. Together those turned an unreadable log into an *available, empty* - selection: "no decisions in this window" when in truth nothing was read. That is - the exact failure this module exists to prevent, so the probe is explicit. +def _read_log(path: Path) -> Tuple[Optional[List[str]], str]: + """Read the whole log once, returning ``(non-empty lines, reason)``. + + One open, one decode, one snapshot. Readability, the events and the line count the + corruption figure is derived from all come out of this single read, so nothing can + change underneath the selection: + + * an earlier design probed readability, let :func:`decision_log.read_events` open + the file again, then opened it a third time to count lines. A writer appending a + valid line between the last two opens made the count exceed the events, so normal + concurrent activity was reported as corruption; a rotation between the first two + swapped the verified file for a fresh near-empty one, and the selection came back + clean and almost empty with no sign that anything had moved; + * readability is proved by reading, not inferred from ``stat``: ``is_file`` passes + a mode-000 file, and only a strict decode catches bytes that are not UTF-8. + + Absent and unreadable stay distinct — "no decision log yet" and "the log could not + be read" call for different actions. Never raises. """ - if not decision_log.is_enabled(): - return REASON_LOG_DISABLED try: - exists = path.is_file() - except OSError as exc: - logger.debug("change-summary log probe failed: %s", exc) - return REASON_LOG_UNREADABLE - if not exists: - return REASON_LOG_ABSENT - try: - # The bytes are *decoded*, not merely opened. Opening alone proved only that - # the descriptor could be acquired; `read_events` then performed the first - # strict UTF-8 read and catches only OSError, so an invalid byte sequence - # raised UnicodeDecodeError straight out of `select_events` and broke the - # never-raises contract. Decoding here answers the question the probe claims to. - with path.open("r", encoding="utf-8") as handle: - handle.read() + text = path.read_text(encoding="utf-8") + except (FileNotFoundError, NotADirectoryError) as exc: + logger.debug("change-summary found no decision log: %s", exc) + return None, REASON_LOG_ABSENT except (OSError, UnicodeDecodeError) as exc: logger.debug("change-summary log is unreadable: %s", exc) - return REASON_LOG_UNREADABLE - return REASON_OK + return None, REASON_LOG_UNREADABLE + return [line for line in text.splitlines() if line.strip()], REASON_OK # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-log-state -# @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-count-lines -def _count_log_lines(path: Path) -> Optional[int]: - """Count non-empty lines in the log, or ``None`` when it could not be read. - - ``None`` rather than ``0`` is the whole point. This count runs *after* - :func:`decision_log.read_events`, which swallows its own open failure and yields - nothing, so a log that vanished between the readability probe and the read produced - an *available, empty* selection — success reported having read nothing. Returning 0 - made that worse by yielding ``skipped_lines`` of 0 too, so the failure left no trace - anywhere. This read is therefore also the detector for that race. - """ - try: - with path.open("r", encoding="utf-8", errors="replace") as handle: - return sum(1 for line in handle if line.strip()) - except OSError as exc: - logger.debug("change-summary log line count failed: %s", exc) - return None -# @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-count-lines - - # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-default-log -def _default_log_for(window: ChangeWindow) -> Optional[Path]: - """Resolve the decision log belonging to *the window's* project. +def _default_log_for(window: ChangeWindow) -> Tuple[Optional[Path], bool]: + """Resolve the decision log for *the window's* project, and whether the environment chose it. ``decision_log.default_log_path()`` defaults to the cwd, which is correct for the writer — it logs whichever project the command runs in. A reader reporting on an explicitly named project must not inherit that default, or the digest describes one project's changes alongside another project's decisions. + + ``$CFS_DECISION_LOG`` names one log for the whole process, and the writer honours it + in every project the process runs in — so the reader follows it too, because that + is where the events *are*. Reading the project-local path instead would report "no + decision log yet" about a log that exists and is being written to. What the reader + cannot do is attribute a shared log's events to this window's project, so the + second value says the environment chose the log, and the selection carries that + rather than presenting a shared log as the project's own. """ + overridden = decision_log.override_log_path() is not None if not window.project_root: - return decision_log.default_log_path() - return decision_log.default_log_path(Path(window.project_root)) + return decision_log.default_log_path(), overridden + return decision_log.default_log_path(Path(window.project_root)), overridden # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-default-log # @cpt-begin:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-resolve-log -def _resolve_log_for(window: ChangeWindow, path: Optional[Path]) -> Tuple[Optional[Path], str]: - """Return ``(log path, reason)`` — exactly one of which is meaningful. +def _resolve_log_for( + window: ChangeWindow, path: Optional[Path], +) -> Tuple[Optional[Path], str, bool]: + """Return ``(log path, reason, overridden)`` — the path and the reason are exclusive. Split out of :func:`select_events` so each function's guard clauses stay within the - project's return-count budget, and so "which log, and may it be read" is answerable - on its own. + project's return-count budget, and so "which log, and is it this project's own" is + answerable on its own. Whether the log can be *read* is answered by reading it — + :func:`_read_log` — not by a separate probe the file could change after. """ - target = path or _default_log_for(window) + if not decision_log.is_enabled(): + return None, REASON_LOG_DISABLED, False + if path is not None: + return path, REASON_OK, False + target, overridden = _default_log_for(window) if target is None: - return None, REASON_NOT_A_PROJECT - reason = _log_unavailable(target) - if reason: - return None, reason - return target, REASON_OK + return None, REASON_NOT_A_PROJECT, False + return target, REASON_OK, overridden # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-resolve-log @@ -485,12 +522,15 @@ def select_events( The default log is resolved from **the window's own project**, not from the current working directory. Those are independent inputs, so a window built for project A while the process sits in project B used to select B's decisions — a digest about - one project carrying another's history. Never raises. + one project carrying another's history. + + The log is read **once**, and everything reported comes from that one snapshot — + see :func:`_read_log` for the two races that separate reads allowed. Never raises. """ if not window.available: return EventSelection(reason=window.reason) - target, reason = _resolve_log_for(window, path) + target, reason, overridden = _resolve_log_for(window, path) if target is None: return EventSelection(reason=reason) @@ -498,44 +538,36 @@ def select_events( if boundary is None: return EventSelection(reason=REASON_NO_BASE_TIME) + lines, reason = _read_log(target) + if lines is None: + return EventSelection(reason=reason) + selected, runs, scanned, undated, runless = [], [], 0, 0, 0 - try: - for event in decision_log.read_events(target): - scanned += 1 - stamp = _parse_ts(event.get("ts")) - if stamp is None: - undated += 1 - continue - if stamp < boundary: - continue - selected.append(event) - run_id = _canonical_run_id(event.get("run_id")) - if not run_id: - runless += 1 - run_id = RUN_UNATTRIBUTED - if run_id not in runs: - runs.append(run_id) - except (OSError, UnicodeDecodeError) as exc: - # Belt and braces: the probe already decoded the file, but it could change - # between probe and read. A read that dies mid-way must not surface as a - # partial selection presented as complete. - logger.debug("change-summary log became unreadable while reading: %s", exc) - return EventSelection(reason=REASON_LOG_UNREADABLE) - - # Deliberately after the read: `read_events` cannot report its own open failure, - # so this second read is what distinguishes "the log held nothing in the window" - # from "the log was never read". - total_lines = _count_log_lines(target) - if total_lines is None: - return EventSelection(reason=REASON_LOG_UNREADABLE) + for event in decision_log.parse_events(lines): + scanned += 1 + stamp = _parse_ts(event.get("ts")) + if stamp is None: + undated += 1 + continue + if stamp < boundary: + continue + selected.append(event) + run_id = _canonical_run_id(event.get("run_id")) + if not run_id: + runless += 1 + run_id = RUN_UNATTRIBUTED + if run_id not in runs: + runs.append(run_id) return EventSelection( - events=selected, - runs=runs, + events=tuple(selected), + runs=tuple(runs), scanned=scanned, undated=undated, runless=runless, - skipped_lines=max(0, total_lines - scanned), + # Exact, not a bound: the lines and the events came from the same snapshot. + skipped_lines=len(lines) - scanned, + log_overridden=overridden, available=True, reason=REASON_OK, ) @@ -565,10 +597,14 @@ def _canonical_run_id(value: Any) -> str: reader". A reader that refuses a run id it does not recognise breaks exactly that. An unrecognised-but-present id is more honestly reported under its own name than merged into "unattributed", which is a claim that no id was recorded at all. + + ``casefold`` rather than ``lower``: the writer's own ids are ASCII hex, where the + two agree, but the contract is casefolding — and ``lower`` leaves ``"Straße"`` and + ``"STRASSE"`` as two runs. """ if not isinstance(value, str): return "" - return value.strip().lower() + return value.strip().casefold() # @cpt-end:cpt-studio-algo-developer-experience-change-summary:p1:inst-change-summary-canonical-run @@ -584,6 +620,11 @@ def group_by_run(selection: EventSelection) -> Dict[str, List[Dict[str, Any]]]: missing ``run_id`` go to :data:`RUN_UNATTRIBUTED`; previously buckets were built only for truthy ids, so such an event sat in ``events`` and in no group at all and a renderer summing the groups under-reported without saying so. + + The buckets hold the selection's own event objects, not copies. One event has one + identity: a copy would let a renderer annotate a group and then read a different + value back from ``events`` — two truths where there was one. The selection itself is + frozen and its collections are tuples, so this view cannot make its counts wrong. """ grouped: Dict[str, List[Dict[str, Any]]] = {run: [] for run in selection.runs} for event in selection.events: diff --git a/skills/studio/scripts/studio/utils/decision_log.py b/skills/studio/scripts/studio/utils/decision_log.py index 207fcfd6..383e8b07 100644 --- a/skills/studio/scripts/studio/utils/decision_log.py +++ b/skills/studio/scripts/studio/utils/decision_log.py @@ -45,7 +45,7 @@ import uuid from datetime import datetime, timezone from pathlib import Path -from typing import Any, Dict, Iterator, List, Optional +from typing import Any, Dict, Iterable, Iterator, List, Optional logger = logging.getLogger(__name__) @@ -113,6 +113,20 @@ def opt_out_sentinel_path() -> Path: return _brand_dir() / _OPT_OUT_SENTINEL +def override_log_path() -> Optional[Path]: + """Return the log named by ``$CFS_DECISION_LOG``, or ``None`` when unset or an off-value. + + The override is process-wide: the writer honours it in every project the process + runs in, so a reader that wants to read what the writer wrote must honour it too — + and may want to know that it did, since one shared log cannot be attributed to any + single project. + """ + override = os.environ.get(_ENV_PATH, "").strip() + if override and override.lower() not in _OFF_VALUES: + return Path(override).expanduser() + return None + + def default_log_path(start: Optional[Path] = None) -> Optional[Path]: """Resolve the log location, or ``None`` when there is nowhere to write. @@ -124,11 +138,12 @@ def default_log_path(start: Optional[Path] = None) -> Optional[Path]: ``start`` defaults to the cwd, which is right for the writer: it logs whatever project the command is running in. A *reader* working against an explicitly named project must pass that root, or it can resolve a different project's log than the - one it is reporting on. + one it is reporting on. The override wins over ``start`` deliberately — see + :func:`override_log_path`. """ - override = os.environ.get(_ENV_PATH, "").strip() - if override and override.lower() not in _OFF_VALUES: - return Path(override).expanduser() + override = override_log_path() + if override is not None: + return override try: from .files import find_studio_directory @@ -385,6 +400,28 @@ def record_read(method: str, target: str, lines: int, tokens: int, source: str = # Reading # --------------------------------------------------------------------------- # @cpt-begin:cpt-studio-algo-core-infra-decision-log:p1:inst-log-read +def parse_events(lines: Iterable[str]) -> Iterator[Dict[str, Any]]: + """Yield the event objects among ``lines``, skipping any line that will not parse. + + The parsing rules of :func:`read_events`, on their own. A reader that has taken its + own snapshot of the file — to count lines and select events from the *same* bytes, + so nothing appended between two reads can be mistaken for corruption — parses that + snapshot the way this module does, rather than growing a second copy of the rules. + """ + for line in lines: + line = line.strip() + if not line: + continue + try: + obj = json.loads(line) + except (ValueError, TypeError) as exc: + logger.debug("decision log: skipping unparseable line: %s", exc) + continue + if not isinstance(obj, dict): + continue + yield obj + + def read_events(path: Optional[Path] = None, *, event: str = "", run_id: str = "", decision_id: str = "", limit: int = 0) -> Iterator[Dict[str, Any]]: @@ -406,17 +443,7 @@ def read_events(path: Optional[Path] = None, *, event: str = "", return matched: List[Dict[str, Any]] = [] - for line in lines: - line = line.strip() - if not line: - continue - try: - obj = json.loads(line) - except (ValueError, TypeError) as exc: - logger.debug("decision log: skipping unparseable line: %s", exc) - continue - if not isinstance(obj, dict): - continue + for obj in parse_events(lines): if event and obj.get("event") != event: continue if run_id and obj.get("run_id") != run_id: diff --git a/tests/test_change_summary_core.py b/tests/test_change_summary_core.py index 187166a4..bc9c76c4 100644 --- a/tests/test_change_summary_core.py +++ b/tests/test_change_summary_core.py @@ -9,6 +9,7 @@ from __future__ import annotations +import dataclasses import json import os import socket @@ -175,7 +176,7 @@ def test_a_non_repo_is_reported_not_crashed(self, tmp_path): assert window.reason == cs.REASON_NOT_A_REPO def test_git_unavailable_is_distinguished_from_not_a_repo(self, tmp_path, monkeypatch): - monkeypatch.setattr(cs, "_git_line", lambda *_a, **_k: None) + monkeypatch.setattr(cs, "_git_query", lambda *_a, **_k: (None, True)) window = cs.resolve_window(tmp_path) @@ -287,7 +288,7 @@ def test_a_naive_timestamp_is_refused_rather_than_assumed_utc(self, tmp_path): selection = cs.select_events(window, path=log) - assert selection.events == [] + assert selection.events == () assert selection.undated == 1 def test_runs_preserve_first_seen_order(self, tmp_path): @@ -300,7 +301,7 @@ def test_runs_preserve_first_seen_order(self, tmp_path): selection = cs.select_events(window, path=log) - assert selection.runs == ["second", "first"], "first-seen order, not sorted" + assert selection.runs == ("second", "first"), "first-seen order, not sorted" def test_the_scanned_count_is_reported_even_when_nothing_is_selected(self, tmp_path): window = cs.ChangeWindow(since="2026-12-01T00:00:00+00:00", available=True) @@ -310,7 +311,7 @@ def test_the_scanned_count_is_reported_even_when_nothing_is_selected(self, tmp_p selection = cs.select_events(window, path=log) - assert selection.events == [] + assert selection.events == () assert selection.scanned == 2, "a verdict without its denominator is the defect" @@ -372,7 +373,7 @@ def _boom(*_a, **_k): raise subprocess.TimeoutExpired(cmd="git", timeout=1) monkeypatch.setattr(cs.subprocess, "run", _boom) - assert cs._git_line(tmp_path, ["status"]) is None + assert cs._git_query(tmp_path, ["status"]) == (None, True) def test_an_oserror_from_git_degrades(self, tmp_path, monkeypatch): def _boom(*_a, **_k): @@ -384,15 +385,6 @@ def _boom(*_a, **_k): assert window.available is False assert window.reason == cs.REASON_GIT_UNAVAILABLE - def test_a_failing_log_probe_is_reported_as_unreadable_not_absent(self, tmp_path, monkeypatch): - log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) - monkeypatch.setattr(Path, "is_file", lambda _self: (_ for _ in ()).throw(OSError("nope"))) - window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) - - selection = cs.select_events(window, path=log) - - assert selection.reason == cs.REASON_LOG_UNREADABLE, "a failed probe is not proof of absence" - def test_an_existing_but_unreadable_log_is_never_an_available_empty_selection(self, tmp_path): """The false green: is_file() passes on mode 000, and read_events swallows the open failure and yields nothing — so this reported "no decisions" having read @@ -411,15 +403,7 @@ def test_an_existing_but_unreadable_log_is_never_an_available_empty_selection(se assert selection.available is False, "unreadable must not present as an empty window" assert selection.reason == cs.REASON_LOG_UNREADABLE - assert selection.events == [] - - def test_the_line_count_reports_a_read_error_rather_than_zero(self, tmp_path, monkeypatch): - """Returning 0 made a failed read indistinguishable from an empty log, which - is what let a vanished log surface as an available empty selection.""" - log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) - monkeypatch.setattr(Path, "open", lambda *_a, **_k: (_ for _ in ()).throw(OSError("nope"))) - - assert cs._count_log_lines(log) is None + assert selection.events == () def test_hostile_event_shapes_do_not_raise(self, tmp_path): window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) @@ -434,7 +418,7 @@ def test_hostile_event_shapes_do_not_raise(self, tmp_path): selection = cs.select_events(window, path=log) assert selection.available is True - assert selection.events == [] + assert selection.events == () # ---------------------------------------------------------------------- invariants @@ -586,9 +570,10 @@ def test_even_an_unavailable_window_records_its_project(self, tmp_path): assert cs.resolve_window(tmp_path).project_root == str(tmp_path) def test_a_rootless_window_still_falls_back_to_the_cwd_default(self, monkeypatch): + monkeypatch.delenv("CFS_DECISION_LOG", raising=False) monkeypatch.setattr(decision_log, "default_log_path", lambda start=None: None) - assert cs._default_log_for(cs.ChangeWindow(available=True)) is None + assert cs._default_log_for(cs.ChangeWindow(available=True)) == (None, False) def test_the_real_resolver_finds_the_windows_project_from_another_cwd( self, tmp_path, monkeypatch, @@ -601,7 +586,7 @@ def test_the_real_resolver_finds_the_windows_project_from_another_cwd( other = _make_studio_project(tmp_path / "other") monkeypatch.chdir(other) - resolved = cs._default_log_for( + resolved, _overridden = cs._default_log_for( cs.ChangeWindow(project_root=str(wanted), available=True), ) @@ -700,6 +685,17 @@ def test_a_ref_beginning_with_a_dash_is_treated_as_a_ref(self, tmp_path): assert window.available is False assert window.reason == cs.REASON_BASE_REF_UNKNOWN, "refused as a ref, not as an option" + def test_a_ref_containing_nul_is_refused_not_raised(self, tmp_path): + """`subprocess` raises ValueError for an embedded NUL before git starts — + outside the git helper's handler, so this escaped the never-raises contract.""" + repo = _make_repo(tmp_path / "r") + _point_ref(repo, "refs/remotes/upstream/main", _git(repo, "rev-parse", "HEAD")) + + window = cs.resolve_window(repo, base="main\x00") + + assert window.available is False + assert window.reason == cs.REASON_BASE_REF_UNKNOWN, "a ref that cannot exist" + def test_every_option_bearing_call_separates_its_operands(self): """A structural check, so a new call site that interpolates a caller value without the separator is caught here rather than in review.""" @@ -799,7 +795,7 @@ class TestBaseRefLookupPreservesTheGitDiagnosis: def test_a_tool_failure_on_an_explicit_ref_reports_git(self, tmp_path, monkeypatch): repo = _make_repo(tmp_path / "r") monkeypatch.setattr(cs, "_git_query", lambda *_a, **_k: (None, True)) - monkeypatch.setattr(cs, "_is_git_repo", lambda *_a, **_k: True) + monkeypatch.setattr(cs, "_detect_repo", lambda *_a, **_k: cs.REASON_OK) window = cs.resolve_window(repo, base="release") @@ -809,7 +805,7 @@ def test_a_tool_failure_on_an_explicit_ref_reports_git(self, tmp_path, monkeypat def test_a_tool_failure_on_the_default_ref_reports_git(self, tmp_path, monkeypatch): repo = _make_repo(tmp_path / "r") monkeypatch.setattr(cs, "_git_query", lambda *_a, **_k: (None, True)) - monkeypatch.setattr(cs, "_is_git_repo", lambda *_a, **_k: True) + monkeypatch.setattr(cs, "_detect_repo", lambda *_a, **_k: cs.REASON_OK) window = cs.resolve_window(repo) @@ -832,39 +828,131 @@ def _fail(_root, args): return None, True monkeypatch.setattr(cs, "_git_query", _fail) - monkeypatch.setattr(cs, "_is_git_repo", lambda *_a, **_k: True) + monkeypatch.setattr(cs, "_detect_repo", lambda *_a, **_k: cs.REASON_OK) cs.resolve_window(repo) assert len(calls) == 1, "one attempt, not one per candidate ref" -class TestAPostProbeReadFailureIsNeverAnEmptySuccess: - """`read_events` swallows its own open failure and yields nothing, so a log that - disappears between the probe and the read looked like an empty window.""" +class TestTheLogIsReadExactlyOnce: + """Readability, the events and the line count come from one read of the file. + + Separate reads let two things go wrong. A valid line appended between the event + read and the line count made the count exceed the events, so ordinary concurrent + logging was reported as corruption. A rotation between the readability probe and + the event read swapped the verified file for a fresh near-empty one, and the + selection came back clean and almost empty with no sign anything had moved. + """ + + def test_the_log_is_opened_exactly_once(self, tmp_path, monkeypatch): + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) + real_open = Path.open + opens = [] + + def _counting(self, *args, **kwargs): + if self == log: + opens.append(args) + return real_open(self, *args, **kwargs) + + monkeypatch.setattr(Path, "open", _counting) + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + + selection = cs.select_events(window, path=log) - def test_a_log_that_vanishes_after_the_probe_is_reported(self, tmp_path, monkeypatch): + assert selection.available is True + assert len(opens) == 1, "a second open is a gap for the file to change in" + + def test_a_line_appended_during_selection_is_not_reported_as_corruption( + self, tmp_path, monkeypatch, + ): + """With separate reads the line count saw the new line and the events did not, + so `skipped_lines` reported normal concurrent activity as corruption.""" log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) - real_probe = cs._log_unavailable + real_parse = decision_log.parse_events - def _probe_then_remove(path): - reason = real_probe(path) - path.unlink() - return reason + def _append_then_parse(lines): + with log.open("a", encoding="utf-8") as handle: + handle.write(json.dumps(_event("2026-06-01T00:00:01+00:00", "late")) + "\n") + return real_parse(lines) - monkeypatch.setattr(cs, "_log_unavailable", _probe_then_remove) + monkeypatch.setattr(decision_log, "parse_events", _append_then_parse) window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) selection = cs.select_events(window, path=log) - assert selection.available is False, "success must not be reported having read nothing" - assert selection.reason == cs.REASON_LOG_UNREADABLE + assert selection.skipped_lines == 0 + assert selection.scanned == 1, "the snapshot, not the file as it is now" + + def test_a_rotation_during_selection_cannot_swap_the_file_under_the_read( + self, tmp_path, monkeypatch, + ): + """The writer rotates by renaming the log aside and starting a fresh one. A + selection that verified the old file and then read the new one reported a + clean, near-empty window; now what was verified is what is read.""" + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) + real_parse = decision_log.parse_events + + def _rotate_then_parse(lines): + os.replace(log, log.with_name(log.name + ".1")) + log.write_text("", encoding="utf-8") + return real_parse(lines) - def test_the_line_count_distinguishes_unreadable_from_empty(self, tmp_path): + monkeypatch.setattr(decision_log, "parse_events", _rotate_then_parse) + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + + selection = cs.select_events(window, path=log) + + assert len(selection.events) == 1, "the file that was read, not its replacement" + assert selection.skipped_lines == 0 + + def test_a_read_error_is_unreadable_not_absent(self, tmp_path, monkeypatch): + """The plain `OSError` arm — neither a missing file nor a permission bit — so a + regression confined to it fails here rather than hiding behind the arms other + tests drive. Line coverage marks a whole `except (A, B)` covered once either + fires, which is how this arm went untested before.""" + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) + + def _io_error(self, *_a, **_k): + raise OSError(5, "Input/output error") + + monkeypatch.setattr(Path, "read_text", _io_error) + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + + selection = cs.select_events(window, path=log) + + assert selection.available is False + assert selection.reason == cs.REASON_LOG_UNREADABLE, "a failed read is not proof of absence" + + def test_a_log_path_under_a_file_is_absent_not_unreadable(self, tmp_path): + """`NotADirectoryError`: a parent component is a regular file, so no log can + exist there. That is absence — the writer's own mkdir fails the same way.""" + (tmp_path / "cache").write_text("", encoding="utf-8") + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + + selection = cs.select_events(window, path=tmp_path / "cache" / "d.jsonl") + + assert selection.reason == cs.REASON_LOG_ABSENT + + def test_an_empty_log_is_a_count_not_a_failure(self, tmp_path): empty = tmp_path / "empty.jsonl" empty.write_text("", encoding="utf-8") - assert cs._count_log_lines(empty) == 0, "an empty log is a count, not a failure" - assert cs._count_log_lines(tmp_path / "gone.jsonl") is None + assert cs._read_log(empty) == ([], cs.REASON_OK) + assert cs._read_log(tmp_path / "gone.jsonl") == (None, cs.REASON_LOG_ABSENT) + + def test_skipped_lines_is_exact_for_the_snapshot(self, tmp_path): + """Two non-empty lines that yield no event — one not JSON, one JSON that is not + an object — and a blank line, which is not a line at all for this purpose.""" + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + log = _write_log( + tmp_path / "d.jsonl", + [_event("2026-06-01T00:00:00+00:00")], + extra="{ this is not json\n\n[]\n", + ) + + selection = cs.select_events(window, path=log) + + assert selection.skipped_lines == 2 class TestRunIdsAreCanonicalised: @@ -879,10 +967,24 @@ class TestRunIdsAreCanonicalised: (1, ""), # a non-string must not join its own text ("not-hex!", "not-hex!"), # unrecognised, but a real identifier ("Custom-Run-7", "custom-run-7"), # a future writer's shape is not rejected + ("STRASSE", "strasse"), + ("Straße", "strasse"), # casefold, not lower: ß folds to ss ]) def test_the_canonical_form(self, raw, expected): assert cs._canonical_run_id(raw) == expected + def test_unicode_case_variants_form_one_group(self, tmp_path): + """`lower()` left these as two runs while the docstring promised casefolding.""" + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + log = _write_log(tmp_path / "d.jsonl", [ + _event("2026-06-01T00:00:00+00:00", "Straße"), + _event("2026-06-01T00:00:01+00:00", "STRASSE"), + ]) + + selection = cs.select_events(window, path=log) + + assert selection.runs == ("strasse",), "one logical run, not two" + def test_case_variants_form_one_group(self, tmp_path): window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) log = _write_log(tmp_path / "d.jsonl", [ @@ -892,7 +994,7 @@ def test_case_variants_form_one_group(self, tmp_path): selection = cs.select_events(window, path=log) - assert selection.runs == ["abcdef012345"], "one logical run, not two" + assert selection.runs == ("abcdef012345",), "one logical run, not two" assert len(cs.group_by_run(selection)["abcdef012345"]) == 2 def test_a_numeric_id_does_not_merge_with_its_own_text(self, tmp_path): @@ -917,7 +1019,7 @@ def test_a_whitespace_id_is_unattributed_not_a_named_run(self, tmp_path): selection = cs.select_events(window, path=log) assert selection.runless == 1 - assert selection.runs == [cs.RUN_UNATTRIBUTED] + assert selection.runs == (cs.RUN_UNATTRIBUTED,) class TestUndecodableLogs: @@ -934,20 +1036,6 @@ def test_invalid_utf8_returns_unavailable_rather_than_raising(self, tmp_path): assert selection.available is False assert selection.reason == cs.REASON_LOG_UNREADABLE - def test_a_log_that_breaks_mid_read_is_not_a_partial_selection(self, tmp_path, monkeypatch): - log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) - - def _explode(*_a, **_k): - raise UnicodeDecodeError("utf-8", b"\xff", 0, 1, "boom") - - monkeypatch.setattr(decision_log, "read_events", _explode) - window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) - - selection = cs.select_events(window, path=log) - - assert selection.available is False - assert selection.reason == cs.REASON_LOG_UNREADABLE - class TestEveryEventIsGrouped: @@ -979,8 +1067,8 @@ def test_a_collision_with_the_unattributed_label_merges_rather_than_loses(self): one bucket, and none is dropped. Losing an event would be the real defect; sharing a label with an anonymous one is cosmetic.""" selection = cs.EventSelection( - events=[{"run_id": cs.RUN_UNATTRIBUTED}, {"run_id": None}], - runs=[cs.RUN_UNATTRIBUTED], + events=({"run_id": cs.RUN_UNATTRIBUTED}, {"run_id": None}), + runs=(cs.RUN_UNATTRIBUTED,), available=True, ) @@ -1007,3 +1095,205 @@ def test_grouping_preserves_run_order(self, tmp_path): def test_grouping_an_empty_selection_is_empty_not_an_error(self): assert cs.group_by_run(cs.EventSelection()) == {} + + +class TestRepoDetectionKeepsItsAnswersApart: + """`rev-parse --is-inside-work-tree` has three outcomes — true, false, and a + non-zero exit — plus the tool not answering at all. Each is a different fact.""" + + def test_a_tool_failure_during_detection_is_not_a_verdict_about_the_directory( + self, tmp_path, monkeypatch, + ): + """Before: the flag was dropped and `git --version` was asked separately, so a + timeout on the real question plus a healthy second launch came back as "not a + git repository" — a claim nothing had established.""" + repo = _make_repo(tmp_path / "r") + real_run = subprocess.run + + def _flaky(cmd, *args, **kwargs): + if "--is-inside-work-tree" in cmd: + raise subprocess.TimeoutExpired(cmd=cmd, timeout=1) + return real_run(cmd, *args, **kwargs) + + monkeypatch.setattr(cs.subprocess, "run", _flaky) + + window = cs.resolve_window(repo) + + assert window.available is False + assert window.reason == cs.REASON_GIT_UNAVAILABLE, "nothing learned, nothing claimed" + + def test_a_bare_repository_is_a_repository_without_a_working_tree(self, tmp_path): + bare = tmp_path / "bare.git" + bare.mkdir() + _git(bare, "init", "-q", "--bare") + + window = cs.resolve_window(bare) + + assert window.available is False + assert window.reason == cs.REASON_NO_WORK_TREE, "a bare repository is a repository" + + def test_the_git_directory_itself_is_outside_the_working_tree(self, tmp_path): + repo = _make_repo(tmp_path / "r") + + assert cs.resolve_window(repo / ".git").reason == cs.REASON_NO_WORK_TREE + + def test_detection_launches_git_exactly_once(self, tmp_path, monkeypatch): + """The second launch was the `--version` guess. It is gone, and with it the + doubled cost on every non-repo path.""" + real_run = subprocess.run + launches = [] + + def _counting(cmd, *args, **kwargs): + launches.append(cmd) + return real_run(cmd, *args, **kwargs) + + monkeypatch.setattr(cs.subprocess, "run", _counting) + + assert cs.resolve_window(tmp_path).reason == cs.REASON_NOT_A_REPO + assert len(launches) == 1 + + +class TestResultsAreImmutableRecords: + """A selection's counts describe its collections. If a caller could grow, shrink or + reassign them, `scanned`, `undated` and `runless` would silently stop being true.""" + + def test_a_window_refuses_reassignment(self, tmp_path): + window = cs.resolve_window(tmp_path) + + with pytest.raises(dataclasses.FrozenInstanceError): + window.since = "2026-01-01T00:00:00+00:00" + + def test_a_selection_refuses_reassignment_and_its_collections_cannot_grow(self, tmp_path): + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) + selection = cs.select_events(window, path=log) + + with pytest.raises(dataclasses.FrozenInstanceError): + selection.scanned = 0 + assert isinstance(selection.events, tuple) + assert isinstance(selection.runs, tuple) + + def test_grouping_shares_the_selections_event_objects_deliberately(self, tmp_path): + """Not a copy: one event, one identity. A copy would let a renderer annotate a + group and then read a different value back from `events`.""" + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00", "r1")]) + selection = cs.select_events(window, path=log) + + grouped = cs.group_by_run(selection) + + assert grouped["r1"][0] is selection.events[0] + + +class TestAnEnvironmentOverrideIsFollowedAndReported: + """`$CFS_DECISION_LOG` is process-wide: the writer honours it in every project the + process runs in. The reader follows it — that is where the events are — and says + so, because one shared log cannot be attributed to the window's project.""" + + def test_the_override_is_read_because_that_is_where_the_writer_wrote( + self, tmp_path, monkeypatch, + ): + """Reading the project-local path instead would report "no decision log yet" + about a log that exists and is being written to.""" + project = _make_studio_project(tmp_path / "project") + shared = _write_log(tmp_path / "shared.jsonl", [_event("2026-06-01T00:00:00+00:00")]) + monkeypatch.setenv("CFS_DECISION_LOG", str(shared)) + window = cs.ChangeWindow( + project_root=str(project), since="2026-01-01T00:00:00+00:00", available=True, + ) + + selection = cs.select_events(window) + + assert selection.available is True + assert len(selection.events) == 1 + assert selection.log_overridden is True, "a shared log is reported as shared" + + def test_the_writer_and_the_reader_agree_on_where_the_log_is(self, tmp_path, monkeypatch): + """End to end: the writer records from inside project B with the override set, + and a window for project A reads that event. Had the reader ignored the + override, it would have found nothing at all.""" + project_a = _make_studio_project(tmp_path / "a") + project_b = _make_studio_project(tmp_path / "b") + shared = tmp_path / "shared.jsonl" + monkeypatch.setenv("CFS_DECISION_LOG", str(shared)) + monkeypatch.setattr(decision_log, "opt_out_sentinel_path", lambda: tmp_path / "absent") + monkeypatch.setattr(decision_log, "_FAILURE_WARNED", False) + monkeypatch.chdir(project_b) + assert decision_log.record("validation", {"check": "x"}, command="validate") is True + window = cs.ChangeWindow( + project_root=str(project_a), since="2000-01-01T00:00:00+00:00", available=True, + ) + + selection = cs.select_events(window) + + assert selection.scanned == 1 + assert selection.log_overridden is True + + def test_a_project_resolved_log_is_not_reported_as_overridden(self, tmp_path, monkeypatch): + monkeypatch.delenv("CFS_DECISION_LOG", raising=False) + project = _make_studio_project(tmp_path / "project") + log = decision_log.default_log_path(project) + assert log is not None + log.parent.mkdir(parents=True, exist_ok=True) + _write_log(log, [_event("2026-06-01T00:00:00+00:00")]) + window = cs.ChangeWindow( + project_root=str(project), since="2026-01-01T00:00:00+00:00", available=True, + ) + + selection = cs.select_events(window) + + assert len(selection.events) == 1 + assert selection.log_overridden is False + + def test_an_explicit_path_is_never_reported_as_overridden(self, tmp_path, monkeypatch): + """A caller that names the log chose it; the environment did not.""" + monkeypatch.setenv("CFS_DECISION_LOG", str(tmp_path / "elsewhere.jsonl")) + log = _write_log(tmp_path / "d.jsonl", [_event("2026-06-01T00:00:00+00:00")]) + window = cs.ChangeWindow(since="2026-01-01T00:00:00+00:00", available=True) + + selection = cs.select_events(window, path=log) + + assert len(selection.events) == 1, "the named log, not the override" + assert selection.log_overridden is False + + +class TestTheBoundaryFollowsTheMergeBase: + """The lower bound is the merge-base's commit time, so it moves when the merge-base + does. This pins that documented behaviour — and its remedy — so a change to the + window's meaning is a deliberate edit here rather than a silent drift.""" + + def test_a_rebase_advances_the_boundary_and_an_explicit_since_restores_it( + self, tmp_path, monkeypatch, + ): + def _at(stamp): + monkeypatch.setenv("GIT_AUTHOR_DATE", stamp) + monkeypatch.setenv("GIT_COMMITTER_DATE", stamp) + + _at("2026-01-01T00:00:00+00:00") + repo = _make_repo(tmp_path / "r") + trunk = _git(repo, "branch", "--show-current") + old_base = _git(repo, "rev-parse", "HEAD") + _git(repo, "checkout", "-q", "-b", "feature") + _at("2026-01-02T00:00:00+00:00") + _commit(repo, "feature.txt") + _git(repo, "checkout", "-q", trunk) + _at("2026-01-05T00:00:00+00:00") + new_base = _commit(repo, "upstream.txt") + _point_ref(repo, "refs/remotes/upstream/main", new_base) + _git(repo, "checkout", "-q", "feature") + log = _write_log(tmp_path / "d.jsonl", [_event("2026-01-03T00:00:00+00:00")]) + + before = cs.resolve_window(repo) + assert before.base_sha == old_base + assert len(cs.select_events(before, path=log).events) == 1 + + _git(repo, "rebase", "-q", "upstream/main") + + after = cs.resolve_window(repo) + assert after.base_sha == new_base, "the merge-base moved, so the boundary moved" + assert after.since > before.since + assert cs.select_events(after, path=log).events == (), \ + "the documented loss: a decision logged before the new base commit" + + pinned = cs.resolve_window(repo, since=before.since) + assert len(cs.select_events(pinned, path=log).events) == 1, "since= is the remedy" diff --git a/vulture_whitelist.py b/vulture_whitelist.py index ecd08a80..61ca10bd 100644 --- a/vulture_whitelist.py +++ b/vulture_whitelist.py @@ -158,4 +158,5 @@ EventSelection.undated # noqa: B018 EventSelection.skipped_lines # noqa: B018 EventSelection.runless # noqa: B018 +EventSelection.log_overridden # noqa: B018 RUN_UNATTRIBUTED # noqa: B018