Fix PR #331 review feedback: hoist test imports, fix stale docs, improve health probe#333
Conversation
…r descriptions, nodes docstring, health probe ping Co-authored-by: ahouseholder <2594236+ahouseholder@users.noreply.github.com> Agent-Logs-Url: https://github.com/CERTCC/Vultron/sessions/2a977821-d19d-4e43-bfc9-23989471a5cc
There was a problem hiding this comment.
Pull request overview
This PR addresses follow-up review feedback from #331 by standardizing test import style, correcting stale endpoint/node documentation around report RM state persistence, and making the FastAPI readiness probe actually validate storage availability via a new DataLayer.ping() port method.
Changes:
- Hoist imports to module scope in several core use-case test modules.
- Update router and behavior-node docstrings/descriptions to reflect persisting
ParticipantStatusRM state (not offer status). - Add
DataLayer.ping()and use it in/health/readyto reliably detect storage failures.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
vultron/core/ports/datalayer.py |
Extends the DataLayer port with a ping() method for readiness probing. |
vultron/adapters/driven/datalayer_tinydb.py |
Implements ping() for TinyDB-backed storage. |
vultron/adapters/driving/fastapi/routers/health.py |
Switches readiness check from read("") to dl.ping(). |
vultron/adapters/driving/fastapi/routers/trigger_report.py |
Fixes endpoint descriptions to match current persistence side effects (ParticipantStatus/RM). |
vultron/core/behaviors/report/nodes.py |
Corrects TransitionRMtoInvalid docstring to match implementation behavior. |
test/core/use_cases/test_status_use_cases.py |
Moves imports to module scope. |
test/core/use_cases/test_report_use_cases.py |
Moves imports to module scope. |
test/core/use_cases/test_note_use_cases.py |
Moves imports to module scope. |
test/core/use_cases/test_case_use_cases.py |
Moves imports to module scope and renames local rehydration mocks for clarity. |
| """Returns 200 if the service is ready to accept requests (OB-05-002).""" | ||
| try: | ||
| dl.read("") | ||
| dl.ping() |
There was a problem hiding this comment.
DataLayer.ping() is declared to return bool, and TinyDbDataLayer.ping() docstring suggests it may return False, but the readiness endpoint ignores the return value. If a DataLayer implementation returns False without raising, /health/ready will still return 200. Consider checking the return value and treating False as not-ready (or standardize ping() to raise on failure and document that contract).
| dl.ping() | |
| ok = dl.ping() | |
| if not ok: | |
| raise HTTPException( | |
| status_code=status.HTTP_503_SERVICE_UNAVAILABLE, | |
| detail="Service unavailable", | |
| ) |
| Performs a lightweight read of the database table list. Returns | ||
| ``True`` if the storage backend responds normally; raises or returns | ||
| ``False`` if the backend is unavailable. |
There was a problem hiding this comment.
The ping() docstring says it may "raise or return False" when unavailable, but the implementation always returns True and never returns False. Please either adjust the docstring to match the actual behavior (raise on failure), or catch exceptions and explicitly return False so callers can rely on the boolean return contract.
| Performs a lightweight read of the database table list. Returns | |
| ``True`` if the storage backend responds normally; raises or returns | |
| ``False`` if the backend is unavailable. | |
| Performs a lightweight read of the database table list. Returns | |
| ``True`` if the storage backend responds normally; raises an exception | |
| if the backend is unavailable. |
| class TransitionRMtoInvalid(DataLayerAction): | ||
| """ | ||
| Transition report to RM.INVALID and offer to TENTATIVELY_REJECTED. | ||
| Transition report to RM.INVALID. | ||
|
|
||
| Updates report status (RM.INVALID) in the DataLayer. | ||
| Persists a ParticipantStatus record with RM.INVALID for the actor and | ||
| report in the DataLayer. |
There was a problem hiding this comment.
The updated docstring describes transitioning only the report to RM.INVALID, but the node constructor still takes an offer_id and stores it even though it is unused in update(). Consider removing offer_id from TransitionRMtoInvalid (or explicitly documenting why it is kept) to avoid confusion and dead parameters.
Addresses all reviewer comments from the PR #331 review thread: test import style, stale endpoint/docstring descriptions, and an unreliable readiness probe.
Test files — imports hoisted to module level
Four test files had imports inside helper functions and individual test methods. Moved all to module scope per CS-02-002:
test/core/use_cases/test_status_use_cases.pytest/core/use_cases/test_report_use_cases.pytest/core/use_cases/test_note_use_cases.pytest/core/use_cases/test_case_use_cases.pytrigger_report.py — endpoint descriptions corrected
Three endpoints (
invalidate-report,reject-report,close-report) described persisting "offer status to TENTATIVELY_REJECTED/REJECTED", which no longer happens. Descriptions now reflect the actual side effect: aParticipantStatusrecord with the correspondingRMstate is persisted.nodes.py —
TransitionRMtoInvaliddocstring correctedClass docstring claimed the node transitions the offer to
TENTATIVELY_REJECTED. The implementation only writes aParticipantStatuswithRM.INVALID. Docstring updated accordingly.health.py — readiness probe made reliable
dl.read("")(empty string ID) always returnsNonewithout exercising real backend connectivity, making the 503 path unreachable. Replaced with a dedicatedping()method:/health/readynow callsdl.ping(), so a storage failure actually returns HTTP 503.Please note: Pull request submissions are subject to our
Contribution Instructions
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.