fix(forecast,docs): address CodeRabbit feedback on PR #266 (3 quick wins) (#267)#272
Conversation
) CodeRabbit #3270767550 on PR #266: the truthy-only check on `job.result["model_path"]` accepted any truthy JSONB value (ints, floats, lists, dicts, booleans). A non-string truthy value would bypass the guard and surface later as an unhandled `TypeError` inside `load_model_bundle`. Strengthen the guard to `isinstance(str) and stripped non-empty` and keep the existing 422 path. Adds a parametrized regression test covering int/float/bool/list/dict/empty/whitespace inputs.
CodeRabbit #3270767557 on PR #266: lines 17-18 said "one new backend endpoint" but the PRP defines two (`/runs/{run_id}/feature-metadata` and `/jobs/{job_id}/feature-metadata`). Match the rest of the doc and name both paths inline so the scope is unambiguous.
CodeRabbit #3270767563 on PR #266: line 1210 listed `500 — bundle file missing on disk` but the rest of the PRP and the implementation (`forecasting/service.py:832-839`) raise UnprocessableEntityError → 422 for the deleted-artifact case. Fold the "missing on disk" bullet into the 422 line so the documented contract matches the code.
There was a problem hiding this comment.
Sorry @w7-mgfcode, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Closes #267.
Three quick-win CodeRabbit comments from PR #266 (the v0.2.17 dev → main release prep). All flagged in code/docs that landed in the release; tackled here as a follow-up so the release window stayed clean.
Commits (one per fix, individually revertable)
79db58ffix(forecast)bundle_path_strtype in feature-metadata loader1ea196fdocs(docs)655007adocs(docs)What each fix does
1.
fix(forecast)— CodeRabbit #3270767550app/features/forecasting/service.py:809-812only checked truthiness onjob.result["model_path"](an untyped JSONB column). A non-string truthy value (int/float/bool/list/dict) would bypass the guard and surface later as an unhandledTypeErrorinsideload_model_bundle. Strengthened the guard toisinstance(bundle_path_str, str) and bundle_path_str.strip()and added a parametrized regression test (7 cases) covering int, float, bool, list, dict, empty string, and whitespace.2.
docs(docs)— CodeRabbit #3270767557PRP-31 lines 17-18 said "one new backend endpoint" but the PRP defines two (
/forecasting/runs/{run_id}/feature-metadataand/forecasting/jobs/{job_id}/feature-metadata). Rephrased the scope statement to name both paths inline so it matches the detailed spec at lines 97-99, 149, and 567.3.
docs(docs)— CodeRabbit #3270767563PRP-31 line 1210 listed
500 — bundle file missing on diskbut the rest of the PRP and the implementation (forecasting/service.py:832-839) raiseUnprocessableEntityError→ 422 for the deleted-artifact case. Folded the "missing on disk" bullet into the 422 line so the documented contract matches both the code andtest_routes_feature_metadata.py::test_get_run_metadata_returns_422_for_deleted_artifact.Out of scope
CodeRabbit's fourth comment (#3270767554) flagged the registry↔forecasting
ModelFamilyslice cycle. That's a heavy-lift refactor needing its own PRP and is tracked separately in #268.Test plan
uv run ruff check .— cleanuv run ruff format --check .— clean (299 files)uv run mypy app/features/forecasting/—Success: no issues found in 21 source filesuv run pyright app/features/forecasting/service.py app/features/forecasting/tests/test_routes_feature_metadata.py— 0 errors / 0 warningsuv run pytest -v -m "not integration" app/features/forecasting/— 187 passed