diff --git a/.console/log.md b/.console/log.md index dda304dfd..900d54338 100644 --- a/.console/log.md +++ b/.console/log.md @@ -1,3 +1,57 @@ +## 2026-08-19 — CI found what 3.12 could not + +The new `test-rest` job went red on its first run, which is the job doing its +job. Three failures, all in `dependency_drift`, all invisible locally: +**CPython 3.11's `glob()` stats every matched path via `exists()`; 3.12's does +not.** Anything built on those internals behaves differently on the two +interpreters. + +Two of them were assertions counting `Path.stat` calls to prove the collector +does not re-stat after discovery — a fair question, but the counter was also +counting the interpreter's probing. The third was a guard test where one +unreadable run directory has to be skipped while the others are still read. + +My first fix made that worse: wrapping the walk in `list(glob(...))` meant a +single bad entry aborted discovery entirely, turning "skip run1, use run2" into +"not_available". `glob()` is a generator — the first error closes it, so +per-entry recovery inside it is not possible at all. + +`_latest_dependency_report` now walks with `iterdir()`, which stats nothing. +Each entry's failure is isolated, and the interpreter's glob internals stay out +of it. All three pass on 3.11 and 3.12. + +Method note: I reproduced CI's 3.11 in a container to iterate. It also showed 2 +failures CI does not report (`test_resolve_repos_root_falls_back_to_checkout_layout`, +`test_loader_reads_latest_snapshot_with_bounded_history`) — the container mounts +a flat worktree with no sibling checkouts, which is what those two probe. CI is +the authority for CI; the container is a proxy that was right about the 3 that +mattered. + +## 2026-08-19 — the CI gap is closed + +~1,830 tests had no CI job. Fixed the 6 failures that made switching the gate on +a decision, then added `test-rest` (`pytest tests/ --ignore=tests/unit`). + +The 6, and what they actually were: + +* **4 were bad mocks, not bugs.** They raised `FileNotFoundError("msg")` — no + errno. pathlib's predicates swallow only ignorable errnos, so the fabricated + error escaped `is_file()`/`is_dir()`, which a *real* vanished path never does. + Verified on 3.11 and 3.12: deleting a directory mid-scan makes `glob()` return + `[]`. My first fix guarded the walk against deletion — defending against + something that cannot happen — and I reverted it. +* **1 was a real gap the bad mock was hiding.** EACCES/EIO are NOT ignorable, so + a log directory that becomes *unreadable* (not deleted) does raise out of the + walk. The guard is warranted for that, and now says so. +* **1 was a real production bug**: `_emit`'s dry-run branch for zero findings sat + below an early return that already answered, so it was unreachable and a dry + run reported "skipped-zero-findings" — the past tense, for something it had not + done. Two tests asserted opposite labels for the same call; the module's own + `would-` convention and the dead branch settle it. +* **1 was a stale patch target** from the board migration weeks ago + (`proposer.main.PlaneClient`), failing that whole time unnoticed — which is the + gap in miniature. + ## 2026-08-19 — an empty directory is still an importable package After #521 merged, `tests/unit/adapters/test_board_seam.py::test_the_retired_backend_is_actually_gone` diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ab2b3b9aa..23cd1f644 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -174,6 +174,41 @@ jobs: # doc-accuracy suite's collection-count assertions. run: pytest -q tests/integration/reviewer -p no:flaky-detection + test-rest: + name: Test (suites outside tests/unit) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: "3.11" + - name: Install dependencies + run: pip install -e .[dev] + - name: Run every suite the other jobs do not + # The complement of `tests/unit`. Two dedicated jobs above already gate + # single files that had drifted (tests/test_pr_review_watcher.py, + # tests/integration/reviewer) — each added after that specific suite rotted + # unnoticed. This job generalises the fix instead of waiting for the next + # one: ~1,830 tests under tests/maintenance/, tests/observer/, + # tests/verdicts/, the rest of tests/integration/, and the top-level + # tests/test_*.py had NO job at all. + # + # What that cost, concretely: + # #509 shipped a regression that broke tests/maintenance/ — found a week + # later by hand (#513). + # #521 merged a BRAND NEW failing test in tests/test_dependency_check.py. + # CI was green, the reviewer verdict was SUCCESS, and main stayed red + # until #522. + # tests/test_proposer_entrypoint.py had been failing since the board-seam + # migration weeks earlier; nothing noticed. + # + # --ignore=tests/unit keeps this isolated from the unit job, whose + # doc-accuracy suite asserts collection counts and is perturbed by running + # alongside other trees. -p no:flaky-detection for the same reason as the + # jobs above: the plugin's pytest11 entry point imports the observer package + # before coverage instrumentation. + run: pytest -q tests/ --ignore=tests/unit -p no:flaky-detection + performance: name: Performance regression tests runs-on: ubuntu-latest diff --git a/src/operations_center/entrypoints/custodian_sweep/main.py b/src/operations_center/entrypoints/custodian_sweep/main.py index 40c32cfcb..1b4373b7b 100644 --- a/src/operations_center/entrypoints/custodian_sweep/main.py +++ b/src/operations_center/entrypoints/custodian_sweep/main.py @@ -240,12 +240,14 @@ def _emit( ) -> str: """Create-or-comment one Plane task per repo. Returns action label.""" if not sweep.error and sweep.total == 0: - return "skipped-zero-findings" + # dry_run has to be answered HERE. The dry-run-aware version of this + # same condition sat ~5 lines below and was unreachable, so a dry run + # reported "skipped-zero-findings" — the past tense, for something it + # had not done. + return "would-skip-zero-findings" if dry_run else "skipped-zero-findings" title = f"[{sweep.repo_key}] custodian sweep: {sweep.total} findings" body = _render_body(sweep, deltas) existing = existing_tasks.get(sweep.repo_key) - if sweep.error is None and sweep.total == 0: - return "skipped-zero-findings" if not dry_run else "would-skip-zero-findings" if dry_run: return "would-comment" if existing else "would-create" if existing: diff --git a/src/operations_center/observer/collectors/check_signal.py b/src/operations_center/observer/collectors/check_signal.py index 548904ce1..b1faf064f 100644 --- a/src/operations_center/observer/collectors/check_signal.py +++ b/src/operations_center/observer/collectors/check_signal.py @@ -19,8 +19,32 @@ def latest_matching_file(root: Path, pattern: str) -> tuple[Path, float] | None: + """Newest file matching *pattern* under *root*, or None if there is none. + + Two different failures, guarded for two different reasons: + + * The per-file `stat()` is a genuine TOCTOU race — a file can vanish + between the walk yielding it and us stating it, and `stat()` does not + swallow that. + * The walk is guarded for I/O errors, NOT for deletion. `glob()` resolves + the root through `is_dir()`, which swallows only pathlib's ignorable + errnos (ENOENT, ENOTDIR, EBADF, ELOOP) — so a *deleted* root already + yields [] with no help from us. EACCES and EIO are not on that list, so + a log directory that becomes unreadable raises straight out of the walk + and takes the collector with it. This function's contract is "None when + nothing is discoverable"; an unreadable directory is that. + + Verified on 3.11 and 3.12: deleting the root mid-scan returns [], while a + non-ignorable errno propagates. + """ candidates_with_mtime = [] - for path in root.glob(pattern): + try: + discovered = list(root.glob(pattern)) + except OSError: + logger.debug("Log discovery walk failed for %s", root, exc_info=True) + return None + + for path in discovered: try: mtime = path.stat().st_mtime candidates_with_mtime.append((path, mtime)) diff --git a/src/operations_center/observer/collectors/dependency_drift.py b/src/operations_center/observer/collectors/dependency_drift.py index cd3f94200..728b7a38a 100644 --- a/src/operations_center/observer/collectors/dependency_drift.py +++ b/src/operations_center/observer/collectors/dependency_drift.py @@ -76,9 +76,38 @@ def collect(self, context: ObserverContext) -> DependencyDriftSignal: ) def _latest_dependency_report(self, report_root: Path) -> tuple[Path, float] | None: + """Newest dependency report under *report_root*, or None. + + Walks with `iterdir()` rather than `glob("*/dependency_report.json")`, + for two reasons that turned out to be the same reason: + + * **One bad run directory must not hide the others.** A report dir that + is unreadable (EACCES/EIO — pathlib swallows only ENOENT, ENOTDIR, + EBADF and ELOOP) should be skipped, not abort discovery. `glob()` + cannot offer that: it is a generator, so the first error closes it and + the remaining entries are unreachable. + * **`glob()` probes differently across versions.** CPython 3.11 stats + every matched path through `exists()`; 3.12 does not. Anything built + on those internals behaves differently on the two interpreters — which + is precisely how this went green locally on 3.12 and red on CI's 3.11. + + `iterdir()` stats nothing, so each entry's failure is isolated and the + interpreter's internals stay out of it. + """ candidates_with_mtime = [] - for path in report_root.glob("*/dependency_report.json"): + try: + entries = list(report_root.iterdir()) + except OSError: + logger.debug( + "Dependency report discovery walk failed for %s", report_root, exc_info=True + ) + return None + + for entry in entries: + path = entry / "dependency_report.json" try: + if not entry.is_dir(): + continue mtime = path.stat().st_mtime candidates_with_mtime.append((path, mtime)) except (FileNotFoundError, OSError): diff --git a/tests/observer/test_collectors_hardening/test_race_condition_guards.py b/tests/observer/test_collectors_hardening/test_race_condition_guards.py index acf4a6630..0756db13d 100644 --- a/tests/observer/test_collectors_hardening/test_race_condition_guards.py +++ b/tests/observer/test_collectors_hardening/test_race_condition_guards.py @@ -7,6 +7,7 @@ proper error handling. """ +import errno import json import os import threading @@ -69,16 +70,20 @@ def test_file_deleted_during_discovery_skipped(self, tmp_artifact_dir): file2 = tmp_artifact_dir / "deleted.log" file2.write_text("will be deleted") - # Patch stat() to delete file2 on first call, succeed on second + # Patch stat() so file2 vanishes the moment it is stat'd. + # + # This used to gate on `call_count == 1`, which silently never fired: + # `glob()` stats the parent directory internally (via is_dir()), so the + # first call was pathlib's, not ours, and file2 was never deleted. The + # identity check is what the test actually meant. original_stat = Path.stat - call_count = [0] + deleted = [] def stat_with_deletion(self): - call_count[0] += 1 - if self == file2 and call_count[0] == 1: - # Delete the file on first stat attempt + if self == file2 and not deleted: + deleted.append(True) file2.unlink() - raise FileNotFoundError(f"No such file: {self}") + raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), str(self)) return original_stat(self) with patch.object(Path, "stat", stat_with_deletion): @@ -102,7 +107,7 @@ def test_all_files_deleted_during_discovery_returns_none(self, tmp_artifact_dir) def stat_with_deletion(self): if str(self).endswith(".log"): - raise FileNotFoundError(f"No such file: {self}") + raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), str(self)) return original_stat(self) with patch.object(Path, "stat", stat_with_deletion): @@ -363,6 +368,10 @@ def counting_stat(self): stat_call_count[0] += 1 return original_stat(self) + # The collector walks with iterdir(), which stats nothing, so this + # counts only the collector's own calls — on 3.11 and 3.12 alike. It + # used to count glob()'s internal exists() probe as well, which is why + # it passed locally on 3.12 and failed on CI's 3.11. with patch.object(Path, "stat", counting_stat): context = MagicMock() context.settings.report_root = tmp_artifact_dir @@ -392,7 +401,7 @@ def test_all_reports_deleted_during_discovery(self, tmp_artifact_dir): def stat_with_deletion(self): if "dependency_report.json" in str(self): - raise FileNotFoundError(f"File deleted: {self}") + raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), str(self)) return original_stat(self) with patch.object(Path, "stat", stat_with_deletion): @@ -503,7 +512,7 @@ def test_symlink_deleted_during_discovery(self, tmp_artifact_dir): def stat_fail_symlinks(self): if self.name == "link.log": - raise FileNotFoundError(f"Symlink deleted: {self}") + raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), str(self)) return original_stat(self) with patch.object(Path, "stat", stat_fail_symlinks): diff --git a/tests/test_check_signal_collector.py b/tests/test_check_signal_collector.py index 705d44548..4fcbee491 100644 --- a/tests/test_check_signal_collector.py +++ b/tests/test_check_signal_collector.py @@ -2,6 +2,8 @@ # Copyright (C) 2026 ProtocolWarden from __future__ import annotations +import errno +import os import subprocess import sys from datetime import UTC, datetime @@ -249,7 +251,7 @@ def mock_glob(self, pattern): def mock_stat(self): """Raise FileNotFoundError for log1, normal stat for log2.""" if self.name == "unit_test.log": - raise FileNotFoundError(f"File deleted: {self}") + raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), str(self)) return original_stat(self) with patch.object(Path, "glob", mock_glob): @@ -278,7 +280,7 @@ def mock_glob(self, pattern): def mock_stat(self): """Always raise FileNotFoundError.""" - raise FileNotFoundError(f"File deleted: {self}") + raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), str(self)) with patch.object(Path, "glob", mock_glob): with patch.object(Path, "stat", mock_stat): diff --git a/tests/test_custodian_sweep.py b/tests/test_custodian_sweep.py index 339ad1e9d..46da655a1 100644 --- a/tests/test_custodian_sweep.py +++ b/tests/test_custodian_sweep.py @@ -110,7 +110,14 @@ def test_emit_skips_plane_mutation_for_zero_findings() -> None: assert action == "skipped-zero-findings" -def test_emit_still_reports_zero_findings_skip_in_dry_run() -> None: +def test_emit_reports_the_would_form_of_zero_findings_skip_in_dry_run() -> None: + """A dry run must not answer in the past tense. + + This asserted "skipped-zero-findings" and passed only because the dry-run + branch below it was unreachable — the early return fired first. Its twin + (test_emit_dry_run_reports_zero_finding_skip) asserted the opposite and had + always failed. `would-` is what the rest of _emit uses for dry runs. + """ plane = SimpleNamespace( comment_issue=lambda *args, **kwargs: (_ for _ in ()).throw(AssertionError("unexpected")), create_issue=lambda *args, **kwargs: (_ for _ in ()).throw(AssertionError("unexpected")), @@ -119,7 +126,7 @@ def test_emit_still_reports_zero_findings_skip_in_dry_run() -> None: action = _emit(sweep, {}, plane, existing_tasks={}, dry_run=True) - assert action == "skipped-zero-findings" + assert action == "would-skip-zero-findings" def test_index_open_sweep_tasks_maps_repo_key_to_issue() -> None: diff --git a/tests/test_dependency_drift_collector.py b/tests/test_dependency_drift_collector.py index 598578676..55c9e9457 100644 --- a/tests/test_dependency_drift_collector.py +++ b/tests/test_dependency_drift_collector.py @@ -4,6 +4,7 @@ from __future__ import annotations +import errno import json import os from datetime import UTC, datetime @@ -139,7 +140,7 @@ def test_guard_single_file_deleted_during_discovery(self, tmp_path: Path) -> Non def mock_stat(self): """Raise FileNotFoundError for old_report (simulating deletion).""" if "run_old" in str(self): - raise FileNotFoundError(f"File deleted during discovery: {self}") + raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), str(self)) return original_stat(self) with patch.object(PathlibPath, "stat", mock_stat): @@ -164,7 +165,7 @@ def test_guard_all_files_deleted_during_discovery(self, tmp_path: Path) -> None: def mock_stat(self): """Always raise FileNotFoundError.""" - raise FileNotFoundError(f"File deleted during discovery: {self}") + raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), str(self)) with patch.object(PathlibPath, "stat", mock_stat): ctx = _make_context(tmp_path) @@ -212,6 +213,9 @@ def __init__(self, mtime): ) return original_stat(self) + # No glob patching needed: the collector walks with iterdir(), which + # stats nothing, so this counter sees only the collector's own calls on + # any CPython version. with patch.object(PathlibPath, "stat", counting_stat): ctx = _make_context(tmp_path) signal = DependencyDriftCollector().collect(ctx) @@ -285,7 +289,7 @@ def mock_stat(self): """Fail for run0 and run1.""" path_str = str(self) if "run0" in path_str or "run1" in path_str: - raise FileNotFoundError(f"File deleted: {self}") + raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), str(self)) return original_stat(self) with patch.object(PathlibPath, "stat", mock_stat): @@ -312,7 +316,7 @@ def test_guard_read_text_still_fails_after_successful_discovery(self, tmp_path: def mock_read_text(self, **kwargs): """Fail on read after successful stat.""" if "dependency_report.json" in str(self): - raise FileNotFoundError(f"File deleted before read: {self}") + raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), str(self)) return original_read_text(self, **kwargs) with patch.object(PathlibPath, "read_text", mock_read_text): diff --git a/tests/test_proposer_entrypoint.py b/tests/test_proposer_entrypoint.py index 4d7245240..d5c8d3455 100644 --- a/tests/test_proposer_entrypoint.py +++ b/tests/test_proposer_entrypoint.py @@ -115,7 +115,14 @@ def comment_issue(self, task_id: str, comment_markdown: str) -> None: # noqa: A def close(self) -> None: return None - monkeypatch.setattr("operations_center.entrypoints.proposer.main.PlaneClient", FakeClient) + # Patched at the seam: proposer.main has called make_board_client since the + # board migration, and `PlaneClient` stopped existing there entirely. This + # test has been failing since then — invisible because no CI job runs it, + # which is the gap this change closes. + monkeypatch.setattr( + "operations_center.entrypoints.proposer.main.make_board_client", + lambda *a, **k: FakeClient(), + ) monkeypatch.setattr( "sys.argv", ["propose-from-candidates", "--config", str(_write_config(tmp_path)), "--dry-run"],