ci: add pytest, ruff, mypy, and Docker build verification to CI#36
Open
LarytheLord wants to merge 1 commit intomainfrom
Open
ci: add pytest, ruff, mypy, and Docker build verification to CI#36LarytheLord wants to merge 1 commit intomainfrom
LarytheLord wants to merge 1 commit intomainfrom
Conversation
- Add test-lint.yml workflow with 4 jobs: - Python Tests: runs all 116 existing pytest tests - Ruff Lint & Format: enforces code style - Mypy Type Check: advisory type checking (non-blocking) - Docker Build Check: builds image and verifies container starts - Add pyproject.toml with ruff and mypy configuration - Fix all ruff violations: - Sort imports, fix duplicate imports in admin.py - Replace nested ifs with combined conditions - Fix implicit Optional in usage_tracker.py - Use contextlib.suppress for ValueError handling - Fix unused variables in tests - Apply ruff formatting across all files - Add E501 and other per-file ignores for admin.py HTML templates
📝 WalkthroughWalkthroughGitHub Actions workflow added for CI/CD. Separately, Python codebase modernized with PEP 604 union-type annotations (Optional[T] → T | None), normalized string quoting (single to double), updated Ruff/Mypy configuration, and refined logic in server routing, admin dashboard, and rate limiting. Test infrastructure updated with new helpers module and all tests adjusted for type/quoting consistency. ChangesCI/CD Infrastructure Setup
Type System Modernization & Code Quality
Sequence DiagramsequenceDiagram
participant Client
participant Auth as Auth Middleware
participant RateLimit as Rate Limiting
participant Proxy as Proxy Handler
participant Upstream
participant Usage as Usage Tracker
Client->>Auth: Request (with API Key or IP)
rect rgba(0, 150, 200, 0.5)
Note over Auth: Skip if /health<br/>or /admin*
Auth-->>Client: Health/Admin response
end
rect rgba(200, 100, 0, 0.5)
Auth->>RateLimit: Check global IP rate limit
RateLimit-->>Auth: Allowed + remaining headers
end
rect rgba(0, 150, 100, 0.5)
alt API Key present
Auth->>Auth: Validate API key
Auth->>RateLimit: Check per-key rate limit
RateLimit-->>Auth: Allowed + limit headers
Auth->>Auth: Store key_id in request
else No API Key
Auth->>Client: 403 Forbidden
end
end
rect rgba(100, 150, 200, 0.5)
Auth->>Proxy: Forward to proxy_request
Proxy->>Upstream: Forward request (inject PrivateMode key)
Upstream-->>Proxy: Response
Proxy->>Proxy: Extract usage data
end
rect rgba(150, 100, 150, 0.5)
alt Status 200 and key_id present
Proxy->>Usage: Record usage
Usage-->>Proxy: Cost calculated
end
end
Proxy->>Client: Add RateLimit headers + response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
auth-proxy/usage_tracker.py (1)
255-286:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn type annotation is incorrect — can return
(None, None).The function signature declares
tuple[float, float]but returns(None, None)for the"all"and unknown periods (lines 281, 284). This will cause mypy type errors.🐛 Proposed fix
-def get_time_range(period: str) -> tuple[float, float]: +def get_time_range(period: str) -> tuple[float | None, float | None]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth-proxy/usage_tracker.py` around lines 255 - 286, The return type of get_time_range is wrong because it can return (None, None); update the function signature to return tuple[Optional[float], Optional[float]] (or Optional[tuple[float, float]]), add the required typing import (from typing import Optional), and keep the existing return logic in get_time_range unchanged so callers and type checkers accept None for the "all" and default cases.
🧹 Nitpick comments (2)
.github/workflows/test-lint.yml (1)
52-53: ⚡ Quick winPin
ruffandmypyversions to prevent non-deterministic CI failures.
pip install ruffandpip install mypywithout version pins will silently pick up new releases. A new ruff version that introduces or modifies rules can break the lint job unexpectedly on an otherwise-clean commit.🔧 Proposed fix
- - name: Install ruff - run: pip install ruff + - name: Install ruff + run: pip install ruff==0.11.9 # pin to known-good version- pip install mypy + pip install mypy==1.15.0 # pin to known-good versionAlso applies to: 76-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-lint.yml around lines 52 - 53, Update the GitHub Actions workflow steps that install linters so they pin versions instead of installing latest: modify the "Install ruff" step (and the "Install mypy" step) to use explicit version pins (e.g., ruff==<version>, mypy==<version>) or a shared pinned requirements file/constraint to ensure deterministic CI; change the pip install commands referenced by the "Install ruff" and "Install mypy" steps to include those exact version specifiers or a requirements file.scripts/scrape_docs.py (1)
159-163: 💤 Low value
link not in visitedis alwaysTrue— dead check.
visitedstores absolute URLs (added viavisited.add(full_url)inget_page), whilelinkis always a relative path such as"/getting-started". A relative path can never equal an absolute URL, solink not in visitedis unconditionallyTrueand adds no guard. The real deduplication isfull_url not in visited.🔧 Proposed fix
for link in extract_nav_links(soup): full_url = urljoin(BASE_URL, link) - if link not in visited and full_url not in visited and link.startswith("/") and not link.startswith("//"): + if full_url not in visited and link.startswith("/") and not link.startswith("//"): to_visit.append(link)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/scrape_docs.py` around lines 159 - 163, The loop that discovers links compares the relative `link` against `visited` (which contains absolute URLs), so `link not in visited` is always true; change the dedupe to use `full_url` and enqueue the absolute URL to keep checks consistent: in the loop that calls `extract_nav_links(soup)` compute `full_url = urljoin(BASE_URL, link)`, replace the condition with `if full_url not in visited and link.startswith("/") and not link.startswith("//"):` and append `full_url` to `to_visit` (and adjust any downstream code that expects paths so it consumes absolute URLs), referencing `extract_nav_links`, the `visited` set, and the loop currently adding to `to_visit`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test-lint.yml:
- Around line 109-120: The workflow's docker run smoke test must supply the env
vars required by auth-proxy/admin.py and avoid auto-removal so startup crashes
aren't hidden; update the docker run in the Verify container starts step to set
PBKDF2_SALT and ADMIN_PASSWORD (use the same safe defaults as
tests/conftest.py), remove the --rm flag and keep the container name, wait, then
explicitly inspect container status and run docker logs test-container before
stopping and removing it so crash logs are preserved for debugging.
In `@auth-proxy/admin.py`:
- Around line 680-690: The code unconditionally trusts X-Forwarded-Proto when
computing is_https for setting the "admin_session" cookie; instead read the
TRUST_PROXY configuration (same flag used in server.py) and only honor
X-Forwarded-Proto when TRUST_PROXY is true, otherwise determine HTTPS from the
actual request (e.g., request.secure or request.scheme == "https"); update the
is_https logic in admin.py where the response.set_cookie call is made so
secure=True is set when the real connection is HTTPS or when TRUST_PROXY allows
the forwarded header to be trusted.
In `@tests/test_endpoints.py`:
- Line 228: The assertion allows any Set-Cookie header to satisfy the test;
instead ensure the fallback specifically checks for "admin_session". Update the
assertion that currently examines resp.cookies and resp.headers so that if the
cookie key set does not contain "admin_session" you explicitly check that
resp.headers.get("Set-Cookie", "") contains "admin_session" (e.g.,
"admin_session=") rather than just testing header truthiness; reference the resp
variable and the "admin_session" cookie name in tests/test_endpoints.py to
locate and fix the failing assertion.
---
Outside diff comments:
In `@auth-proxy/usage_tracker.py`:
- Around line 255-286: The return type of get_time_range is wrong because it can
return (None, None); update the function signature to return
tuple[Optional[float], Optional[float]] (or Optional[tuple[float, float]]), add
the required typing import (from typing import Optional), and keep the existing
return logic in get_time_range unchanged so callers and type checkers accept
None for the "all" and default cases.
---
Nitpick comments:
In @.github/workflows/test-lint.yml:
- Around line 52-53: Update the GitHub Actions workflow steps that install
linters so they pin versions instead of installing latest: modify the "Install
ruff" step (and the "Install mypy" step) to use explicit version pins (e.g.,
ruff==<version>, mypy==<version>) or a shared pinned requirements
file/constraint to ensure deterministic CI; change the pip install commands
referenced by the "Install ruff" and "Install mypy" steps to include those exact
version specifiers or a requirements file.
In `@scripts/scrape_docs.py`:
- Around line 159-163: The loop that discovers links compares the relative
`link` against `visited` (which contains absolute URLs), so `link not in
visited` is always true; change the dedupe to use `full_url` and enqueue the
absolute URL to keep checks consistent: in the loop that calls
`extract_nav_links(soup)` compute `full_url = urljoin(BASE_URL, link)`, replace
the condition with `if full_url not in visited and link.startswith("/") and not
link.startswith("//"):` and append `full_url` to `to_visit` (and adjust any
downstream code that expects paths so it consumes absolute URLs), referencing
`extract_nav_links`, the `visited` set, and the loop currently adding to
`to_visit`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13e687c8-d947-4f38-8b18-c8b0ed7d1ae6
📒 Files selected for processing (19)
.github/workflows/test-lint.ymlauth-proxy/admin.pyauth-proxy/config.pyauth-proxy/key_manager.pyauth-proxy/server.pyauth-proxy/usage_tracker.pyauth-proxy/utils.pypyproject.tomlscripts/manage_keys.pyscripts/scrape_docs.pytests/conftest.pytests/helpers.pytests/test_admin.pytests/test_auth.pytests/test_endpoints.pytests/test_proxy.pytests/test_rate_limiting.pytests/test_usage_tracker.pytests/test_utils.py
💤 Files with no reviewable changes (1)
- tests/helpers.py
Comment on lines
+109
to
+120
| - name: Verify container starts | ||
| run: | | ||
| docker run --rm -d --name test-container \ | ||
| -e API_KEYS_FILE=/app/secrets/keys.json \ | ||
| -e UPSTREAM_URL=http://localhost:8081 \ | ||
| -e PORT=8080 \ | ||
| -e PRIVATEMODE_API_KEY=test-key \ | ||
| -e FORCE_HTTPS=false \ | ||
| privatemode-proxy:test | ||
| sleep 5 | ||
| docker logs test-container | ||
| docker stop test-container |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for PBKDF2_SALT usage and the ValueError it raises
rg -n "PBKDF2_SALT" --type py -A 5 -B 2Repository: Open-Paws/privatemode-proxy
Length of output: 2536
Docker smoke test may silently mask container startup failures.
Two compounding issues:
-
Missing
PBKDF2_SALTandADMIN_PASSWORD:auth-proxy/admin.pyraisesValueErrorat import time ifPBKDF2_SALTis shorter than 16 bytes. The docker run command doesn't set these variables, so the container will crash immediately on startup. The test configuration intests/conftest.pyproperly sets both to valid defaults. -
--rm+ crash = misleading failure: With--rm -d, if the container exits within the 5-second window due to the startup error above, it is automatically removed. Subsequentdocker logs test-containeranddocker stop test-containercommands then fail with "no such container" — the CI step exits non-zero for the wrong reason and the actual crash logs are lost.
🔧 Proposed fix
- name: Verify container starts
run: |
- docker run --rm -d --name test-container \
+ docker run -d --name test-container \
-e API_KEYS_FILE=/app/secrets/keys.json \
-e UPSTREAM_URL=http://localhost:8081 \
-e PORT=8080 \
-e PRIVATEMODE_API_KEY=test-key \
-e FORCE_HTTPS=false \
+ -e PBKDF2_SALT=test-salt-value-1234567890 \
+ -e ADMIN_PASSWORD=test-admin-password \
privatemode-proxy:test
sleep 5
docker logs test-container
- docker stop test-container
+ docker stop test-container || true
+ docker rm -f test-container || true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Verify container starts | |
| run: | | |
| docker run --rm -d --name test-container \ | |
| -e API_KEYS_FILE=/app/secrets/keys.json \ | |
| -e UPSTREAM_URL=http://localhost:8081 \ | |
| -e PORT=8080 \ | |
| -e PRIVATEMODE_API_KEY=test-key \ | |
| -e FORCE_HTTPS=false \ | |
| privatemode-proxy:test | |
| sleep 5 | |
| docker logs test-container | |
| docker stop test-container | |
| - name: Verify container starts | |
| run: | | |
| docker run -d --name test-container \ | |
| -e API_KEYS_FILE=/app/secrets/keys.json \ | |
| -e UPSTREAM_URL=http://localhost:8081 \ | |
| -e PORT=8080 \ | |
| -e PRIVATEMODE_API_KEY=test-key \ | |
| -e FORCE_HTTPS=false \ | |
| -e PBKDF2_SALT=test-salt-value-1234567890 \ | |
| -e ADMIN_PASSWORD=test-admin-password \ | |
| privatemode-proxy:test | |
| sleep 5 | |
| docker logs test-container | |
| docker stop test-container || true | |
| docker rm -f test-container || true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test-lint.yml around lines 109 - 120, The workflow's
docker run smoke test must supply the env vars required by auth-proxy/admin.py
and avoid auto-removal so startup crashes aren't hidden; update the docker run
in the Verify container starts step to set PBKDF2_SALT and ADMIN_PASSWORD (use
the same safe defaults as tests/conftest.py), remove the --rm flag and keep the
container name, wait, then explicitly inspect container status and run docker
logs test-container before stopping and removing it so crash logs are preserved
for debugging.
Comment on lines
+680
to
690
| response = web.HTTPFound("/admin") | ||
| # Detect if running behind HTTPS (Fly.io, nginx, etc.) | ||
| is_https = request.headers.get('X-Forwarded-Proto', '').lower() == 'https' | ||
| is_https = request.headers.get("X-Forwarded-Proto", "").lower() == "https" | ||
| response.set_cookie( | ||
| 'admin_session', | ||
| "admin_session", | ||
| token, | ||
| max_age=86400, # 24 hours | ||
| httponly=True, | ||
| samesite='Strict', | ||
| secure=is_https # Only send cookie over HTTPS in production | ||
| samesite="Strict", | ||
| secure=is_https, # Only send cookie over HTTPS in production | ||
| ) |
There was a problem hiding this comment.
Inconsistent X-Forwarded-Proto handling — should check TRUST_PROXY.
This code unconditionally trusts the X-Forwarded-Proto header to determine HTTPS status for setting the Secure cookie flag. However, server.py (lines 290-310) only trusts this header when the TRUST_PROXY config flag is enabled.
When TRUST_PROXY=false (the default), an attacker could:
- Send a spoofed
X-Forwarded-Proto: httpsheader on an HTTP connection - The cookie would be set with
secure=True, but won't be sent over the HTTP connection - This could cause authentication to fail unexpectedly
More critically, if the app is accessed directly (not behind a proxy) but X-Forwarded-Proto is missing, the cookie may incorrectly be set without secure=True even on HTTPS connections.
🔒 Proposed fix to align with server.py behavior
+from config import TRUST_PROXY
# Then in admin_login_post:
- is_https = request.headers.get("X-Forwarded-Proto", "").lower() == "https"
+ is_https = request.secure # True if connection is TLS
+ if not is_https and TRUST_PROXY:
+ is_https = request.headers.get("X-Forwarded-Proto", "").lower() == "https"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| response = web.HTTPFound("/admin") | |
| # Detect if running behind HTTPS (Fly.io, nginx, etc.) | |
| is_https = request.headers.get('X-Forwarded-Proto', '').lower() == 'https' | |
| is_https = request.headers.get("X-Forwarded-Proto", "").lower() == "https" | |
| response.set_cookie( | |
| 'admin_session', | |
| "admin_session", | |
| token, | |
| max_age=86400, # 24 hours | |
| httponly=True, | |
| samesite='Strict', | |
| secure=is_https # Only send cookie over HTTPS in production | |
| samesite="Strict", | |
| secure=is_https, # Only send cookie over HTTPS in production | |
| ) | |
| response = web.HTTPFound("/admin") | |
| # Detect if running behind HTTPS (Fly.io, nginx, etc.) | |
| is_https = request.secure # True if connection is TLS | |
| if not is_https and TRUST_PROXY: | |
| is_https = request.headers.get("X-Forwarded-Proto", "").lower() == "https" | |
| response.set_cookie( | |
| "admin_session", | |
| token, | |
| max_age=86400, # 24 hours | |
| httponly=True, | |
| samesite="Strict", | |
| secure=is_https, # Only send cookie over HTTPS in production | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@auth-proxy/admin.py` around lines 680 - 690, The code unconditionally trusts
X-Forwarded-Proto when computing is_https for setting the "admin_session"
cookie; instead read the TRUST_PROXY configuration (same flag used in server.py)
and only honor X-Forwarded-Proto when TRUST_PROXY is true, otherwise determine
HTTPS from the actual request (e.g., request.secure or request.scheme ==
"https"); update the is_https logic in admin.py where the response.set_cookie
call is made so secure=True is set when the real connection is HTTPS or when
TRUST_PROXY allows the forwarded header to be trusted.
| cookies = resp.cookies | ||
| assert 'admin_session' in {c.key for c in resp.cookies.values()} or resp.headers.get('Set-Cookie', '') | ||
|
|
||
| assert "admin_session" in {c.key for c in resp.cookies.values()} or resp.headers.get("Set-Cookie", "") |
There was a problem hiding this comment.
or fallback accepts any Set-Cookie header, not just admin_session.
The assertion:
assert "admin_session" in {c.key for c in resp.cookies.values()} or resp.headers.get("Set-Cookie", "")is logically broken. If admin_session is absent but any other Set-Cookie header is present (e.g. a CSRF cookie set on the same response), resp.headers.get("Set-Cookie", "") is truthy and the assertion silently passes — masking a missing session cookie.
The fallback should also check for "admin_session" specifically:
🐛 Proposed fix
- assert "admin_session" in {c.key for c in resp.cookies.values()} or resp.headers.get("Set-Cookie", "")
+ assert "admin_session" in resp.cookies or "admin_session" in resp.headers.get("Set-Cookie", "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_endpoints.py` at line 228, The assertion allows any Set-Cookie
header to satisfy the test; instead ensure the fallback specifically checks for
"admin_session". Update the assertion that currently examines resp.cookies and
resp.headers so that if the cookie key set does not contain "admin_session" you
explicitly check that resp.headers.get("Set-Cookie", "") contains
"admin_session" (e.g., "admin_session=") rather than just testing header
truthiness; reference the resp variable and the "admin_session" cookie name in
tests/test_endpoints.py to locate and fix the failing assertion.
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
1 participant
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
pyproject.tomlwith ruff and mypy configurationWhy this matters
The repo has 116 well-written tests but no CI runs them. A contributor could break everything and the CI would still pass green. This PR fixes that.
Changes
New files
.github/workflows/test-lint.yml— 4 CI jobs: tests, ruff, mypy, dockerpyproject.toml— ruff and mypy configFixed lint violations
contextlib.suppressfor ValueError handling, E501 per-file ignore for HTML templatesOptional(str = None→str | None = None)is_valid()returnkey_obj→_key_obj, etc.), fixed renamed variables that broke assertionsruff formatappliedTest Results
116 passed, 0 failed
Summary by CodeRabbit
Chores
Tests