diff --git a/.agent-plan.md b/.agent-plan.md index 7f0b346..dc83ccb 100644 --- a/.agent-plan.md +++ b/.agent-plan.md @@ -6,10 +6,13 @@ ## Mainline Status -- Last merged PR on main: `#224` (`FIX-CI-GITLEAKS-STATE-JOBS`) — install gitleaks in the - `setup-denbust-state-job` composite action so the fail-closed `scripts/state-run.sh` secret scan +- Last merged PR on main: `#225` (`FIX-DAILY-REVIEW-NOOP`) — make `news-items-daily-review` a clean + no-op (return 0) when there are no `daily-state-run` ingest artifacts to review, instead of failing + the workflow; surfaced by the go-live dispatch (ingest is local-only/gated on #213, so the state + repo has no `news_items/ingest`). Prior: `#224` (`FIX-CI-GITLEAKS-STATE-JOBS`) — install gitleaks in + the `setup-denbust-state-job` composite action so the fail-closed `scripts/state-run.sh` secret scan has its binary on the runner; without it every scheduled state-writing workflow would have failed - at persist on its first scheduled run. The last `UNIFY-PR-06` prerequisite. Prior: `#223` + at persist on its first scheduled run. Earlier: `#223` (`FIX-DISCOVERY-RUNS-SCHEMA`; refs #150/#152/#156/#184/#186, closed on deploy) — add the missing `discovery_runs.skipped_query_count` column whose absence was failing every Supabase run-metadata write and blocking candidate persistence on daily-review runs; a guard test now asserts every @@ -371,6 +374,16 @@ first scheduled run. Pinned to v8.30.1 to match `.gitleaks.toml`. (`state-repo-squash` uses `state-squash.sh`, which does not scan, so it is unaffected.) Prerequisite for the `UNIFY-PR-06` go-live dispatch. +- [done] `FIX-DAILY-REVIEW-NOOP` (#225): make `news-items-daily-review` a clean no-op when there are no + `daily-state-run` ingest artifacts to review, instead of failing the workflow. Surfaced by the + go-live dispatch: daily-review reviews ingest output, but ingest is local/dispatch-only (gated on + #213) and was not seeded, so the state repo has no `news_items/ingest` — a scheduled review with + nothing to review should not be a red run. The finder now raises a **dedicated** + `NoDailyReviewArtifacts` for the benign empty case (caught narrowly by the entrypoint, which emits + a `::warning::` annotation and returns 0 without constructing the Anthropic/GitHub clients), while a + genuinely missing `state_root` (broken checkout / misconfig) still raises `FileNotFoundError` so it + surfaces loudly rather than being swallowed. Tested: benign no-op makes zero client calls; missing + `state_root` propagates. - [next] `UNIFY-PR-06` (operational, go-live): the state repo is **seeded** (key-scrubbed after the incident below) with the recovered core state from local `data/news_items` (27,568 candidates + queues/attempts/verdicts/budget/yield + backfill_batches/runs/metrics; excluded: prefilter models diff --git a/src/denbust/news_items/daily_review.py b/src/denbust/news_items/daily_review.py index 007713c..78b10da 100644 --- a/src/denbust/news_items/daily_review.py +++ b/src/denbust/news_items/daily_review.py @@ -168,6 +168,16 @@ def _render_source_diagnostics_section(source_diagnostics: dict[str, Any] | None ) +class NoDailyReviewArtifacts(Exception): + """No matching daily-ingest run exists yet to review. + + This is a *benign* condition (e.g. ingest runs locally/on-demand and has not + produced state), distinct from a genuine filesystem/configuration failure + such as a missing ``state_root`` — which keeps raising ``FileNotFoundError`` + so it surfaces loudly instead of being silently swallowed. + """ + + def latest_daily_review_artifacts( *, state_root: Path, @@ -176,7 +186,19 @@ def latest_daily_review_artifacts( workflow_name: str = "daily-state-run", source_diagnostics_path: Path | None = None, ) -> ReviewArtifacts: - """Find the latest daily-ingest artifacts from the state repo.""" + """Find the latest daily-ingest artifacts from the state repo. + + Raises: + FileNotFoundError: if ``state_root`` itself is missing or not a directory + (a broken state checkout / misconfigured ``DENBUST_STATE_ROOT`` — a + real failure that must not be mistaken for "nothing to review"). + NoDailyReviewArtifacts: if the state tree is present but holds no + complete run matching ``workflow_name`` (the benign empty case). + """ + if not state_root.is_dir(): + raise FileNotFoundError( + f"state_root {str(state_root)!r} does not exist or is not a directory" + ) runs_dir = state_root / dataset_name / job_name / "runs" logs_dir = state_root / dataset_name / job_name / "logs" candidates = sorted(logs_dir.glob("*.summary.json"), reverse=True) @@ -204,7 +226,7 @@ def latest_daily_review_artifacts( debug_log=_load_json(debug_log_path), ) - raise FileNotFoundError( + raise NoDailyReviewArtifacts( f"No complete {dataset_name}/{job_name} artifacts found for workflow '{workflow_name}'" ) @@ -374,6 +396,15 @@ def create_issue(self, candidate: IssueCandidate, artifacts: ReviewArtifacts) -> response.raise_for_status() +def _emit_review_notice(message: str) -> None: + """Surface a non-fatal review notice. Under GitHub Actions, also emit a + ``::warning::`` annotation so a no-op run is visible at a glance rather than + buried in stdout.""" + print(message) + if os.getenv("GITHUB_ACTIONS") == "true": + print(f"::warning title=daily-review::{message}") + + def review_latest_daily_run( *, state_root: Path, @@ -385,12 +416,27 @@ def review_latest_daily_run( labels: list[str] | None = None, source_diagnostics_path: Path | None = None, ) -> int: - """Review the latest daily run and create any missing issues.""" - artifacts = latest_daily_review_artifacts( - state_root=state_root, - workflow_name=workflow_name, - source_diagnostics_path=source_diagnostics_path, - ) + """Review the latest daily run and create any missing issues. + + Returns the number of issues created. When no matching ``daily-state-run`` + ingest artifacts exist yet (e.g. ingest runs locally/on-demand and has not + produced state, so there is nothing to review), this is a clean no-op that + returns 0 rather than failing the workflow — a scheduled review with nothing + to review should not surface as a red run. + """ + try: + artifacts = latest_daily_review_artifacts( + state_root=state_root, + workflow_name=workflow_name, + source_diagnostics_path=source_diagnostics_path, + ) + except NoDailyReviewArtifacts as exc: + # Benign: nothing to review yet. Make it observable (not a bare stdout + # line) so a scheduled no-op is distinguishable from a healthy review, + # but do not fail the workflow. A genuinely missing state_root raises + # FileNotFoundError instead and is deliberately NOT caught here. + _emit_review_notice(f"No daily ingest artifacts to review: {exc}") + return 0 reviewer = AnthropicDailyReviewer(api_key=anthropic_api_key, model=model) review = reviewer.review(artifacts) if not review.issues: diff --git a/tests/unit/test_daily_review.py b/tests/unit/test_daily_review.py index cc26056..1c982bf 100644 --- a/tests/unit/test_daily_review.py +++ b/tests/unit/test_daily_review.py @@ -13,6 +13,7 @@ AnthropicDailyReviewer, GitHubIssueClient, IssueCandidate, + NoDailyReviewArtifacts, ReviewArtifacts, ReviewResult, _compact_for_prompt, @@ -182,10 +183,11 @@ def test_load_json_rejects_non_object_payload(self, tmp_path: Path) -> None: with pytest.raises(ValueError, match="Expected JSON object"): _load_json(path) - def test_latest_daily_review_artifacts_raises_when_daily_artifacts_missing( + def test_latest_daily_review_artifacts_raises_no_artifacts_when_incomplete( self, tmp_path: Path ) -> None: - """Missing or incomplete daily artifacts should fail clearly.""" + """A present state tree with no complete matching run raises the benign, + narrow NoDailyReviewArtifacts — not a bare FileNotFoundError.""" logs_dir = tmp_path / "news_items" / "ingest" / "logs" _write_json( logs_dir / "2026-03-21T04-00-00-000000Z.summary.json", @@ -195,9 +197,54 @@ def test_latest_daily_review_artifacts_raises_when_daily_artifacts_missing( }, ) - with pytest.raises(FileNotFoundError, match="No complete news_items/ingest artifacts"): + with pytest.raises(NoDailyReviewArtifacts, match="No complete news_items/ingest artifacts"): latest_daily_review_artifacts(state_root=tmp_path) + def test_latest_daily_review_artifacts_raises_filenotfound_when_state_root_missing( + self, tmp_path: Path + ) -> None: + """A missing state_root (broken checkout / misconfig) is NOT the benign + case — it raises FileNotFoundError so it surfaces loudly, and is not a + subclass of NoDailyReviewArtifacts.""" + missing = tmp_path / "does-not-exist" + with pytest.raises(FileNotFoundError) as excinfo: + latest_daily_review_artifacts(state_root=missing) + assert not isinstance(excinfo.value, NoDailyReviewArtifacts) + + def test_review_latest_daily_run_noop_makes_no_client_calls( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The empty case returns 0 and never constructs the Anthropic or GitHub + clients (the no-network guarantee), so a scheduled run cannot fail.""" + + def _boom(*_args: object, **_kwargs: object) -> None: + raise AssertionError("no client should be constructed on the empty path") + + monkeypatch.setattr("denbust.news_items.daily_review.AnthropicDailyReviewer", _boom) + monkeypatch.setattr("denbust.news_items.daily_review.GitHubIssueClient", _boom) + + ingest = tmp_path / "news_items" / "ingest" / "logs" + ingest.mkdir(parents=True) # state tree exists, but holds no runs + + created = review_latest_daily_run( + state_root=tmp_path, + repository="DataHackIL/tfht_enforce_idx", + anthropic_api_key="unused", + github_token="unused", + ) + assert created == 0 + + def test_review_latest_daily_run_propagates_missing_state_root(self, tmp_path: Path) -> None: + """A missing state_root must NOT be silently no-op'd — it propagates so a + broken state checkout fails the workflow loudly.""" + with pytest.raises(FileNotFoundError): + review_latest_daily_run( + state_root=tmp_path / "nope", + repository="DataHackIL/tfht_enforce_idx", + anthropic_api_key="unused", + github_token="unused", + ) + def test_compact_for_prompt_truncates_large_lists_and_strings(self) -> None: """Large debug payloads should be compacted before prompting.""" compact = _compact_for_prompt(