From 36647c72b1e776ce88ffc4f5068f713c6333bd54 Mon Sep 17 00:00:00 2001 From: Shay Palachy Date: Thu, 18 Jun 2026 21:44:57 +0300 Subject: [PATCH 1/3] fix(review): make daily-review a no-op when there are no ingest artifacts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Surfaced by the UNIFY-PR-06 go-live dispatch: news-items-daily-review reviews the output of daily-state-run (ingest), but ingest runs locally/on-demand (gated on #213) and was not seeded, so the state repo has no news_items/ingest. review_latest_daily_run() raised FileNotFoundError and failed the workflow — a scheduled review with nothing to review should not be a red run. Catch FileNotFoundError in review_latest_daily_run(), log, and return 0 (no issues created, no Anthropic/GitHub calls). The low-level latest_daily_review_ artifacts() finder still raises (its contract and tests are unchanged); only the workflow entrypoint tolerates the empty case. Adds a no-op regression test. Co-Authored-By: Claude Opus 4.8 --- .agent-plan.md | 7 +++++++ src/denbust/news_items/daily_review.py | 23 +++++++++++++++++------ tests/unit/test_daily_review.py | 14 ++++++++++++++ 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/.agent-plan.md b/.agent-plan.md index 7f0b346..86a86f8 100644 --- a/.agent-plan.md +++ b/.agent-plan.md @@ -371,6 +371,13 @@ 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`: make `news-items-daily-review` a clean no-op when there are no + `daily-state-run` ingest artifacts to review. `review_latest_daily_run()` now catches the + `FileNotFoundError` from `latest_daily_review_artifacts()`, logs, and returns 0 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 low-level finder still + raises (its contract/tests are unchanged); only the workflow entrypoint tolerates the empty case. - [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..01a3936 100644 --- a/src/denbust/news_items/daily_review.py +++ b/src/denbust/news_items/daily_review.py @@ -385,12 +385,23 @@ 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 FileNotFoundError as exc: + print(f"No daily ingest artifacts to review ({exc}); skipping.") + 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..9de4993 100644 --- a/tests/unit/test_daily_review.py +++ b/tests/unit/test_daily_review.py @@ -198,6 +198,20 @@ def test_latest_daily_review_artifacts_raises_when_daily_artifacts_missing( with pytest.raises(FileNotFoundError, match="No complete news_items/ingest artifacts"): latest_daily_review_artifacts(state_root=tmp_path) + def test_review_latest_daily_run_is_a_noop_when_no_ingest_artifacts( + self, tmp_path: Path + ) -> None: + """With no ingest artifacts to review (ingest runs locally/on-demand), + the review is a clean no-op: it returns 0 and never reaches the Anthropic + or GitHub clients, so a scheduled run does not fail.""" + created = review_latest_daily_run( + state_root=tmp_path, # empty: no news_items/ingest at all + repository="DataHackIL/tfht_enforce_idx", + anthropic_api_key="unused", + github_token="unused", + ) + assert created == 0 + def test_compact_for_prompt_truncates_large_lists_and_strings(self) -> None: """Large debug payloads should be compacted before prompting.""" compact = _compact_for_prompt( From d1d547ebc56d83149fbd5c6eb5b165b95b22bd0b Mon Sep 17 00:00:00 2001 From: Shay Palachy Date: Thu, 18 Jun 2026 21:46:47 +0300 Subject: [PATCH 2/3] docs(plan): record PR #225 for the daily-review no-op fix Co-Authored-By: Claude Opus 4.8 --- .agent-plan.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/.agent-plan.md b/.agent-plan.md index 86a86f8..589720c 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,7 +374,7 @@ 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`: make `news-items-daily-review` a clean no-op when there are no +- [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. `review_latest_daily_run()` now catches the `FileNotFoundError` from `latest_daily_review_artifacts()`, logs, and returns 0 instead of failing the workflow. Surfaced by the go-live dispatch: daily-review reviews ingest output, but ingest is From 23638ff29cc860e329327fbaee2bfaf81b48724a Mon Sep 17 00:00:00 2001 From: Shay Palachy Date: Sat, 20 Jun 2026 07:59:16 +0300 Subject: [PATCH 3/3] fix(review): narrow the daily-review no-op to a dedicated exception + make it observable Self-review hardening of the no-op: - Catching the broad FileNotFoundError swallowed genuine fs/config errors, turning a monitoring job silent. Introduce NoDailyReviewArtifacts and raise it for the benign "no matching run" case; the entrypoint catches only that. - Distinguish idle-ingest from a broken checkout: latest_daily_review_artifacts now raises FileNotFoundError when state_root itself is missing/not a dir (a real misconfig that must surface), and NoDailyReviewArtifacts when the tree is present but empty. - Make the no-op observable: emit a ::warning:: GitHub Actions annotation, not a bare stdout print, so "nothing to review" is distinguishable from a healthy run. - Tests now assert the no-network guarantee (Anthropic/GitHub clients are never constructed on the empty path) and that a missing state_root propagates. Co-Authored-By: Claude Opus 4.8 --- .agent-plan.md | 15 ++++---- src/denbust/news_items/daily_review.py | 43 +++++++++++++++++++--- tests/unit/test_daily_review.py | 49 +++++++++++++++++++++----- 3 files changed, 89 insertions(+), 18 deletions(-) diff --git a/.agent-plan.md b/.agent-plan.md index 589720c..dc83ccb 100644 --- a/.agent-plan.md +++ b/.agent-plan.md @@ -375,12 +375,15 @@ `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. `review_latest_daily_run()` now catches the - `FileNotFoundError` from `latest_daily_review_artifacts()`, logs, and returns 0 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 low-level finder still - raises (its contract/tests are unchanged); only the workflow entrypoint tolerates the empty case. + `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 01a3936..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, @@ -399,8 +430,12 @@ def review_latest_daily_run( workflow_name=workflow_name, source_diagnostics_path=source_diagnostics_path, ) - except FileNotFoundError as exc: - print(f"No daily ingest artifacts to review ({exc}); skipping.") + 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) diff --git a/tests/unit/test_daily_review.py b/tests/unit/test_daily_review.py index 9de4993..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,23 +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_review_latest_daily_run_is_a_noop_when_no_ingest_artifacts( + def test_latest_daily_review_artifacts_raises_filenotfound_when_state_root_missing( self, tmp_path: Path ) -> None: - """With no ingest artifacts to review (ingest runs locally/on-demand), - the review is a clean no-op: it returns 0 and never reaches the Anthropic - or GitHub clients, so a scheduled run does not fail.""" + """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, # empty: no news_items/ingest at all + 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(