fix(review): daily-review no-op when there are no ingest artifacts#225
Open
shaypal5 wants to merge 2 commits into
Open
fix(review): daily-review no-op when there are no ingest artifacts#225shaypal5 wants to merge 2 commits into
shaypal5 wants to merge 2 commits into
Conversation
…acts 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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adjusts the news-items-daily-review workflow entrypoint to treat “no ingest artifacts exist yet” as a clean no-op (return 0) instead of failing, which prevents scheduled runs from going red when there is nothing to review.
Changes:
- Catch
FileNotFoundErrorfromlatest_daily_review_artifacts()inreview_latest_daily_run()and return0after logging a skip message. - Add a unit test asserting the orchestrator returns
0when the state root has no ingest artifacts at all. - Record the completed work item in
.agent-plan.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/denbust/news_items/daily_review.py |
Makes review_latest_daily_run() tolerant of missing ingest artifacts by catching FileNotFoundError and returning 0. |
tests/unit/test_daily_review.py |
Adds a regression test for the “no artifacts → no-op” behavior. |
.agent-plan.md |
Marks the daily-review no-op fix as [done] in the task ledger. |
Comment on lines
+396
to
+404
| 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 |
Comment on lines
+201
to
+213
| 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 |
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #225 +/- ##
=======================================
Coverage 92.84% 92.84%
=======================================
Files 84 84
Lines 12337 12341 +4
=======================================
+ Hits 11454 11458 +4
Misses 883 883
🚀 New features to boost your workflow:
|
|
pr-agent-context report: This run includes unresolved review comments on PR #225.
For each unresolved review comment, recommend one of: resolve as irrelevant, accept and implement
the recommended solution, open a separate issue and resolve as out-of-scope for this PR, accept and
implement a different solution, or resolve as already treated by the code.
After I reply with my decision per item, implement the accepted actions, resolve the corresponding
PR comments, and push all of these changes in a single commit.
# Copilot Comments
## COPILOT-1
Location: src/denbust/news_items/daily_review.py:404
URL: https://github.com/DataHackIL/tfht_enforce_idx/pull/225#discussion_r3438181322
Root author: copilot-pull-request-reviewer
Comment:
`review_latest_daily_run()` currently swallows *all* `FileNotFoundError` from `latest_daily_review_artifacts()`, which means it will also silently no-op when ingest artifacts exist but are incomplete (e.g. `.summary.json` present but companion `runs/*.json`/`logs/*.json` missing). That can hide a real regression in the ingest workflow/state production. Consider only no-op'ing when the ingest artifact root is entirely absent/empty, and re-raise otherwise so incomplete/corrupt artifacts still fail loudly.
## COPILOT-2
Location: tests/unit/test_daily_review.py:213
URL: https://github.com/DataHackIL/tfht_enforce_idx/pull/225#discussion_r3438181348
Root author: copilot-pull-request-reviewer
Comment:
This test's docstring claims the no-op path "never reaches the Anthropic or GitHub clients", but the assertions don't currently enforce that. Adding a `monkeypatch` that makes those constructors raise would prevent future refactors from accidentally reintroducing network/client construction while still returning 0.Run metadata: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Surfaced by the
UNIFY-PR-06go-live verification dispatch.news-items-daily-reviewreviews the output ofdaily-state-run(ingest): it looks fornews_items/ingest/logs/*.summary.jsontaggedworkflow_name: daily-state-run. But:news_items/ingestdirectory at all (confirmed: top-level dirs arediscover, backfill_discover, backup, monthly_report, operational, release, scrape_candidates).So
review_latest_daily_run()raisedFileNotFoundError: No complete news_items/ingest artifacts found for workflow 'daily-state-run'and failed the workflow. A scheduled review job with nothing to review should not be a red run (it's noise, and would ironically spawn its own daily-ai-review ticket).What
review_latest_daily_run()now catches thatFileNotFoundError, logsNo daily ingest artifacts to review (...); skipping., and returns0— before constructing the Anthropic or GitHub clients, so a no-op makes zero API/network calls.The low-level
latest_daily_review_artifacts()finder still raises (its contract and thetest_..._raises_when_daily_artifacts_missingtest are unchanged) — only the workflow entrypoint tolerates the empty case. Adds a regression test asserting the no-op returns 0 with an empty state root.Verification
pytest -q tests/unit/test_daily_review.py— 28 passed (incl. the new no-op test and the still-raising finder test)ruff format --check/ruff check/mypy— cleannews-items-daily-reviewdispatch after merge to confirm it goes green (no-op) against the real seeded state.Context
Last open structural blocker from the go-live dispatch. With this, the scheduled go-live set (discover, daily-review, monthly-report, release, backup) can all run without a spurious failure; daily-review becomes useful automatically once ingest output exists in the state repo.