fix: resolve P1 bugs from codebase audit - #1
Merged
Merged
Conversation
The CBS API returns a single Title per column. dutch_name was always set to the same value as name, making display_name fallback dead code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Client now tracks whether it owns the httpx.Client. close() and __exit__ only close the HTTP client if Client created it internally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Template leftover referencing non-existent cbspy/foo.py. This is a library — no application entrypoint to containerise. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ODataClient now retries once with a 1s delay on 5xx status codes and httpx.RequestError (timeouts, connection errors). 404 and 4xx errors are not retried. Also removes unused http_client fixture from conftest. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Summary
Fixes the four Priority 1 bugs identified in the codebase audit. These are correctness issues and resource leaks that should be resolved before any feature work.
1. Remove dead
dutch_name/display_namefrom Column modelThe
ColumnPydantic model had adutch_namefield and adisplay_namecomputed property intended for bilingual support. However, the CBS OData API returns a singleTitlefield per column — there is no separate Dutch vs English title in one response._parse_columnwas setting bothnameanddutch_namefrom the same"Title"value, makingdisplay_name's fallback logic dead code.Changes:
dutch_name: strfield anddisplay_namecomputed property fromColumncomputed_fieldimport from pydantic_parse_columninclient.pyto drop the redundantdutch_name=argumenttest_models.py— removed tests for the non-functional bilingual behavior2. Make
Clienta context manager to prevent connection leaksWhen no
http_clientis passed,Client.__init__creates anhttpx.Client()internally but never closes it. This leaks TCP connections and file descriptors, and httpx emitsResourceWarningon garbage collection.Changes:
_owns_httpflag to track whetherClientcreated the httpx instanceclose()method that only closes the httpx client ifClientowns it__enter__/__exit__forwithstatement supportclose()and context manager3. Remove broken Dockerfile
The Dockerfile was a cookiecutter template leftover with
CMD ["python", "cbspy/foo.py"]— a file that doesn't exist. The source layout issrc/cbspy/, notcbspy/, and this is a library with no application entrypoint.Changes:
Dockerfile4. Add retry logic for transient 5xx and network errors
The MVP design doc specifies "retry once with backoff" for 5xx errors, but the implementation had zero retry logic. The CBS API occasionally returns 503s under load.
Changes:
_check_responsewith_request_with_retryinODataClienthttpx.RequestError(connection errors, timeouts)http_clientfixture fromconftest.pyTest plan
uv run python -m pytest tests/ -v)uv run ruff check src tests)uv run ty check)dutch_nameordisplay_name🤖 Generated with Claude Code