Conversation
… `httpx2` as primary dependencies
…gres container is deprecated
* feat: raise a type error if non ecdh keys are loaded via the helper functions * docs: add comments on initialization vectors * build(deps): update dependencies * test(fix): adapt fixtures to the hub's new camel case naming convention * test: close clients on fixture teardown * refactor: do not make the chunk size configurable * refactor: use a kdf instead of the raw shared secret
…in up and save them in the app's state Clients used to be initialized inside dependencies for every request without being properly closed. This commit also addresses the new camel case Hub API.
📝 WalkthroughWalkthroughThe PR simplifies Hub authentication, centralizes initialized services in ChangesService architecture and integration update
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant AppState
participant DependencyProvider
participant StorageRouter
participant Hub
Server->>AppState: initialize configured clients and database
StorageRouter->>DependencyProvider: request storage and Hub clients
DependencyProvider->>AppState: retrieve initialized services
StorageRouter->>StorageRouter: execute blocking storage work in thread pool
StorageRouter->>Hub: upload or retrieve result data
Server->>AppState: close resources during shutdown
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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. 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.env.example (1)
6-7: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftProvide the required Hub client credentials for the test migration service.
Settings()requiresHUB__AUTH__IDandHUB__AUTH__SECRET;tests/docker-compose.ymlonly defines the oldHUB__AUTH__FLOW/HUB__AUTH__USERNAME/HUB__AUTH__PASSWORDvalues. Add the required credentials or use a migration-specific settings/model path that does not require Hub client auth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 6 - 7, Update the test migration configuration represented by Settings() and tests/docker-compose.yml to provide HUB__AUTH__ID and HUB__AUTH__SECRET, replacing or supplementing the obsolete HUB__AUTH__FLOW, HUB__AUTH__USERNAME, and HUB__AUTH__PASSWORD values so the migration service has the required Hub client credentials.Source: MCP tools
project/routers/final.py (1)
66-73: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winBoth final-result uploads still block the event loop.
project/routers/intermediate.pymovedstorage_client.upload_to_bucketintorun_in_threadpool. These two routes did not get the same treatment. Both handlers areasync def, andfind_analysis_bucketsandupload_to_bucketare synchronous network calls.Line 113 passes
file.filedirectly. The whole upload therefore runs on the event loop and stalls every other request for its duration. This contradicts the stated performance goal of the PR.Apply the same
run_in_threadpooltreatment used inproject/routers/intermediate.pylines 90-102.♻️ Proposed change for `submit_final_result_to_hub`
+from fastapi.concurrency import run_in_threadpool + # upload to remote - bucket_file_lst = storage_client.upload_to_bucket( + bucket_file_lst = await run_in_threadpool( + storage_client.upload_to_bucket, analysis_bucket.bucket_id, { "file_name": file.filename, "content": file.file, "content_type": file.content_type or "application/octet-stream", }, )Also applies to: 109-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@project/routers/final.py` around lines 66 - 73, Update both final-result upload paths in the async handlers, including submit_final_result_to_hub, to invoke the synchronous storage_client.upload_to_bucket through run_in_threadpool as done in intermediate.py. Ensure all upload arguments, including file.file in the second path, are passed to the threadpool call so no synchronous network upload runs on the event loop.
🧹 Nitpick comments (6)
tests/test_intermediate.py (1)
37-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the blob spans several chunks.
The patch sets
CHUNK_SIZEto 64 so the test exercises multi-chunk encryption and decryption.AESGCMEncryptingStreamsubtracts the IV and tag overhead, so each record carries roughly 36 plaintext bytes.The test does not check the size of
blob. If theblobfixture later returns a small value, the multi-chunk path stops being covered and the test still passes. Add an explicit assertion to lock the intent.💚 Proposed addition
monkeypatch.setattr(crypto, "CHUNK_SIZE", 64) + assert len(blob) > 64 * 3, "Blob must span several chunks to exercise chunked encryption."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_intermediate.py` around lines 37 - 38, Add an explicit assertion in the test using the blob fixture to verify its length exceeds the effective per-record plaintext capacity, ensuring encryption and decryption exercise multiple chunks. Keep the existing CHUNK_SIZE monkeypatch and assertions unchanged.project/dependencies.py (1)
57-58: 🚀 Performance & Scalability | 🔵 TrivialOffer: cache the JWKS fetch.
The TODO is correct. Every authenticated request now performs an outbound HTTPS round trip to the auth provider before the route runs. This adds latency to each request and couples request availability to the auth provider.
A time-bounded cache with refresh on unknown
kidremoves the per-request fetch. Do you want me to open an issue or draft the cached provider?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@project/dependencies.py` around lines 57 - 58, Update get_auth_jwks to cache the fetched JWKS for a bounded duration instead of requesting it on every authenticated request, while preserving access to current keys. Ensure unknown kid handling triggers a refresh before rejecting the token, and remove the obsolete TODO once caching is implemented.project/server.py (2)
153-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the module logger.
Line 156 calls
logging.info, which writes to the root logger. The rest oflifespanuseslogger. The message can be dropped or formatted differently because of this.♻️ Proposed fix
- logging.info("Enabled OpenDP's 'floating-point' and 'contrib' features.") + logger.info("Enabled OpenDP's 'floating-point' and 'contrib' features.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@project/server.py` around lines 153 - 156, Update the feature-enablement log in lifespan to call the module’s existing logger instead of the root logging module, preserving the current message and surrounding enable_features calls.
53-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueA single proxy setting is applied to both schemes.
If only
s.proxy.http_urlis set, the code also routeshttps://traffic through it, and the reverse case applies too. This is a valid convention, but it is not obvious from the configuration names. A user who sets only an HTTP proxy may not expect HTTPS traffic to use it.Add a short comment that states this fallback, and confirm the README describes it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@project/server.py` around lines 53 - 56, Add a concise comment beside the proxy selection in the http/https transport setup explaining that when only one of s.proxy.http_url or s.proxy.https_url is configured, that proxy is used for both schemes. Update the README’s proxy configuration documentation to explicitly describe this fallback behavior.project/migrations/scripts/router.py (1)
10-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImporting
init_dbfromproject.serverpulls the whole application into the migration entrypoint.
project/server.pyimports FastAPI, the routers, MinIO, truststore, OpenDP, andproject.version.project.versionreadspyproject.tomlat import time. A migration run therefore fails if any of those imports fails, even though a migration needs only the Peewee proxy.Move
init_dbto a neutral module such asproject/crud.pyor a newproject/db.py, and import it from there in bothproject/server.pyand this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@project/migrations/scripts/router.py` around lines 10 - 12, Move the init_db definition out of project.server into a neutral database module such as project.db or project.crud, then update both the server module and the migration entrypoint to import init_db from that module. Ensure the migration script no longer imports project.server or its application dependencies.tests/test_crypto.py (1)
19-24: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGenerate the non-EC keys lazily.
Lines 20-23 run at module import.
dsa.generate_private_key(key_size=2_048)generates new DSA parameters and is slow, often several seconds.rsa.generate_private_keyadds more time. This cost applies to every collection of the test suite, even when no test in this module is selected.Parametrize over factory callables and build the key inside the test instead.
♻️ Proposed refactor
-NON_ECDH_PRIVATE_KEYS = ( - rsa.generate_private_key(public_exponent=65_537, key_size=2_048), - ed448.Ed448PrivateKey.generate(), - dsa.generate_private_key(key_size=2_048), - x448.X448PrivateKey.generate(), -) +NON_ECDH_PRIVATE_KEY_FACTORIES = ( + pytest.param(lambda: rsa.generate_private_key(public_exponent=65_537, key_size=2_048), id="rsa"), + pytest.param(ed448.Ed448PrivateKey.generate, id="ed448"), + pytest.param(lambda: dsa.generate_private_key(key_size=2_048), id="dsa"), + pytest.param(x448.X448PrivateKey.generate, id="x448"), +)Then update both parametrized tests:
-@pytest.mark.parametrize("private_key", NON_ECDH_PRIVATE_KEYS) -def test_load_ecdh_private_key_type_error(private_key): +@pytest.mark.parametrize("private_key_factory", NON_ECDH_PRIVATE_KEY_FACTORIES) +def test_load_ecdh_private_key_type_error(private_key_factory): with pytest.raises(TypeError) as e: - load_ecdh_private_key(_get_private_key_in_pem_format(private_key)) + load_ecdh_private_key(_get_private_key_in_pem_format(private_key_factory())) assert str(e.value) == "Expected an EC private key."This also produces readable test ids instead of
private_key0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_crypto.py` around lines 19 - 24, Replace eager key instances in NON_ECDH_PRIVATE_KEYS with factory callables that generate each private key lazily. Update both parametrized tests to invoke the selected factory inside the test, and assign descriptive parameter IDs so test output names identify the key type instead of using default indices.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 28-29: Remove the duplicate PYTEST__USE_TESTCONTAINERS setup path
by keeping the setting in .env.example and updating the README setup
instructions to stop appending the same key after copying the file.
In @.github/actions/setup-poetry/action.yml:
- Line 12: Update the Poetry setup configuration around the default version
value so the documented poetry shell workflow remains supported: retain or
install poetry-plugin-shell alongside Poetry, or revise the README instructions
to use a supported poetry run flame-storage command. Keep the setup and
documentation behavior consistent.
In `@project/crypto.py`:
- Around line 87-94: Make the crypto-format change backward-compatible across
exchange_ecdh_shared_secret(), encrypt_default(), decrypt_default(), and
AESGCMEncryptingStream: either require coordinated upgrades for every encrypting
and decrypting node, or add a format/version marker to encrypted streams so
readers select the legacy raw ECDH key material versus the new HKDF-derived key
material. Ensure both one-shot and streaming paths preserve interoperability
with existing ciphertext.
In `@project/dependencies.py`:
- Around line 72-75: Update the HTTPException raise in the dependency error
handler to explicitly chain from None, hiding the transport exception from the
client response while preserving the traceback captured by logger.exception.
- Around line 65-68: Update get_auth_jwks to construct the JWKS httpx client
through the shared client-building helper, passing settings so it applies
_build_ssl_context()/extra_ca_certs and _build_proxy_mounts()/settings.proxy.
Configure an explicit timeout on the client or request, while preserving the
existing JWKS fetch and error handling.
- Around line 25-26: Update get_app_state to read the application-level
app_state assigned by the server through request.app instead of
request.state.app_state, preserving the AppState return contract for dependent
routes.
In `@project/migrations/scripts/router.py`:
- Around line 32-43: Move the init_db(db) call inside the try block in the
router setup so db.close() in the finally clause runs even when initialization
raises. Keep the existing Router construction and migration configuration
unchanged.
In `@project/routers/final.py`:
- Line 56: The Hub bucket filters use the wrong camelCase field; update the
filter dictionaries in project/routers/final.py lines 56-56 and 98-98 and
project/routers/intermediate.py line 77-77 to use analysis_id for the analysis
identifier, preserving the existing client_id values and RESULT type filter.
In `@project/routers/intermediate.py`:
- Around line 150-151: Update the async handler around get_bucket_file,
stream_bucket_file, and get_remote_node_public_key to execute all synchronous
network calls via run_in_threadpool. For the stream_bucket_file probe, create
and consume the generator within the worker and explicitly close it after
retrieving the first chunk, including cleanup when retrieval raises.
- Around line 163-164: Update the chunked download/decryption loop in the
intermediate router to buffer data from stream_bucket_file until a complete
crypto.CHUNK_SIZE encryption record is assembled before calling
crypto.decrypt_default. Preserve leftover bytes across iterations, decrypt the
final partial record separately after the stream ends, and yield decrypted
output without truncating valid ciphertext.
In `@project/routers/local.py`:
- Around line 466-479: Move the try/finally in the surrounding request handler
to begin immediately after _get_object_from_s3 returns, keeping the filename
lookup and UploadFile creation inside the try block. Preserve the existing
submit_intermediate_result_to_hub call, and ensure s3_response.close() and
s3_response.release_conn() execute for failures in result.count(), result.get(),
or subsequent processing.
- Around line 392-413: Update the _iter_response generator to call response.read
with a fixed chunk size, yielding each bounded chunk until the stream is
exhausted; retain the existing cleanup of close and release_conn. Add coverage
using an object larger than the chosen chunk size to verify multiple
StreamingResponse chunks are produced without reading the full body at once.
In `@project/server.py`:
- Around line 91-96: Update the application lifespan resource management around
auth_flow_client and the existing shutdown block to use a single ExitStack,
registering auth_flow_client, postgres, core_client, and storage_client for
cleanup. Wrap startup work before yield in try/finally so resources are closed
both on normal shutdown and when minio.bucket_exists, init_db,
_get_ecdh_private_key, or _get_node_id raises; preserve the existing yielded
application behavior.
In `@project/version.py`:
- Around line 4-5: Remove the __version_info__ assignment unless it is required
by existing callers; otherwise replace its integer-splitting logic with a PEP
440-aware version parser so valid local and dev versions loaded by __version__
do not raise ValueError.
---
Outside diff comments:
In @.env.example:
- Around line 6-7: Update the test migration configuration represented by
Settings() and tests/docker-compose.yml to provide HUB__AUTH__ID and
HUB__AUTH__SECRET, replacing or supplementing the obsolete HUB__AUTH__FLOW,
HUB__AUTH__USERNAME, and HUB__AUTH__PASSWORD values so the migration service has
the required Hub client credentials.
In `@project/routers/final.py`:
- Around line 66-73: Update both final-result upload paths in the async
handlers, including submit_final_result_to_hub, to invoke the synchronous
storage_client.upload_to_bucket through run_in_threadpool as done in
intermediate.py. Ensure all upload arguments, including file.file in the second
path, are passed to the threadpool call so no synchronous network upload runs on
the event loop.
---
Nitpick comments:
In `@project/dependencies.py`:
- Around line 57-58: Update get_auth_jwks to cache the fetched JWKS for a
bounded duration instead of requesting it on every authenticated request, while
preserving access to current keys. Ensure unknown kid handling triggers a
refresh before rejecting the token, and remove the obsolete TODO once caching is
implemented.
In `@project/migrations/scripts/router.py`:
- Around line 10-12: Move the init_db definition out of project.server into a
neutral database module such as project.db or project.crud, then update both the
server module and the migration entrypoint to import init_db from that module.
Ensure the migration script no longer imports project.server or its application
dependencies.
In `@project/server.py`:
- Around line 153-156: Update the feature-enablement log in lifespan to call the
module’s existing logger instead of the root logging module, preserving the
current message and surrounding enable_features calls.
- Around line 53-56: Add a concise comment beside the proxy selection in the
http/https transport setup explaining that when only one of s.proxy.http_url or
s.proxy.https_url is configured, that proxy is used for both schemes. Update the
README’s proxy configuration documentation to explicitly describe this fallback
behavior.
In `@tests/test_crypto.py`:
- Around line 19-24: Replace eager key instances in NON_ECDH_PRIVATE_KEYS with
factory callables that generate each private key lazily. Update both
parametrized tests to invoke the selected factory inside the test, and assign
descriptive parameter IDs so test output names identify the key type instead of
using default indices.
In `@tests/test_intermediate.py`:
- Around line 37-38: Add an explicit assertion in the test using the blob
fixture to verify its length exceeds the effective per-record plaintext
capacity, ensuring encryption and decryption exercise multiple chunks. Keep the
existing CHUNK_SIZE monkeypatch and assertions unchanged.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 21b35dd9-cf32-4fc4-b052-9ef5ff492441
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
.env.example.github/actions/setup-poetry/action.ymlDockerfileREADME.mddocker-compose.ymlproject/config.pyproject/crypto.pyproject/dependencies.pyproject/main.pyproject/migrations/scripts/create_migration.pyproject/migrations/scripts/migrate.pyproject/migrations/scripts/router.pyproject/models.pyproject/routers/final.pyproject/routers/intermediate.pyproject/routers/local.pyproject/server.pyproject/utils.pyproject/version.pypyproject.tomltests/common/auth.pytests/common/env.pytests/common/helpers.pytests/common/rest.pytests/conftest.pytests/test_config.pytests/test_crypto.pytests/test_final.pytests/test_intermediate.pytests/test_local.pytests/test_local_tagged.pytests/test_main.pytests/test_version.py
💤 Files with no reviewable changes (3)
- tests/common/env.py
- tests/test_config.py
- docker-compose.yml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
80-94: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFix the non-Testcontainers test setup before merging.
When
PYTEST__USE_TESTCONTAINERS=0, the README directs users to the compose-based setup below. Themigrateservice intests/docker-compose.ymlstill suppliesHUB__AUTH__FLOW,HUB__AUTH__USERNAME, andHUB__AUTH__PASSWORD, but the currentSettingsmodel requiresHUB__AUTH__IDandHUB__AUTH__SECRET.init_router()createsSettings()before migrations run, so this documented path fails with missing Hub authentication settings. Updatetests/docker-compose.ymlor make migrations independent of Hub authentication. (raw.githubusercontent.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 80 - 94, Fix the non-Testcontainers setup by updating the migrate service configuration in tests/docker-compose.yml to provide the HUB__AUTH__ID and HUB__AUTH__SECRET values required by Settings during init_router(), or change the migration initialization path to avoid requiring Hub authentication. Ensure the documented PYTEST__USE_TESTCONTAINERS=0 flow completes migrations without missing Hub settings.Source: MCP tools
🧹 Nitpick comments (1)
README.md (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClear the MD014 warning in the command block.
Line 26 uses a
$prompt, but the block shows no command output. Remove the$prompts from the commands, or add representative output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 26, Remove the "$ " shell prompts from the command block containing "poetry run flame-storage" in README.md, since no command output is shown.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@README.md`:
- Around line 80-94: Fix the non-Testcontainers setup by updating the migrate
service configuration in tests/docker-compose.yml to provide the HUB__AUTH__ID
and HUB__AUTH__SECRET values required by Settings during init_router(), or
change the migration initialization path to avoid requiring Hub authentication.
Ensure the documented PYTEST__USE_TESTCONTAINERS=0 flow completes migrations
without missing Hub settings.
---
Nitpick comments:
In `@README.md`:
- Line 26: Remove the "$ " shell prompts from the command block containing
"poetry run flame-storage" in README.md, since no command output is shown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d82064e8-2579-4eba-8303-d38a93f98ded
📒 Files selected for processing (6)
README.mdproject/dependencies.pyproject/migrations/scripts/router.pyproject/routers/local.pyproject/server.pyproject/version.py
💤 Files with no reviewable changes (1)
- project/version.py
🚧 Files skipped from review as they are similar to previous changes (4)
- project/migrations/scripts/router.py
- project/server.py
- project/routers/local.py
- project/dependencies.py
1 participant
Summary by CodeRabbit