Modernize file storage and transport using Django Storage API - #3733
Open
cesarbenjamindotnet wants to merge 28 commits into
Open
Modernize file storage and transport using Django Storage API#3733cesarbenjamindotnet wants to merge 28 commits into
cesarbenjamindotnet wants to merge 28 commits into
Conversation
Phase 0 inventory of CARE's runtime, verified against the code at 6a2976d. Documentation only; no application behavior changed. Adds docs/xii/architecture/inventory/: storage-call-sites.md 19 object-storage call sites task-call-sites.md 8 task definitions, 4 async dispatch sites cache-and-redis.md 69 cache/Redis call sites, 3 hard couplings frontend-file-flow.md file API contract and required changes runtime-and-deployment.md process/image/CI facts, baseline blockers plugin-impact.md plugin loading; no plugins bundled unresolved-items.md 12 defects, 10 open questions, 8 unknowns Corrections to 01-current-runtime.md: - remove stray ````markdown fence that made the frontmatter and sections 1-2 render as literal code - section 38: patient files do not use FILE_UPLOAD_REGION/KEY/SECRET; get_patient_bucket_config reads FACILITY_S3_* instead, leaving those three settings dead - section 40: note reports inherit the same FACILITY_S3_* credentials - section 26: add the three task definitions outside care/emr/tasks/ - fix two wrong paths to 02-target-runtime.md - record verified_against_commit Baseline commands could not run: Docker daemon down, make absent, no Python 3.13 environment, Redis not running. Blockers documented in runtime-and-deployment.md section 11. All 84 cited paths and 46 cited symbols verified to exist. Co-Authored-By: Claude <noreply@anthropic.com>
Record the Phase 0 green baseline for the current upstream-compatible CARE runtime. Documentation only; no runtime, settings, model, migration or dependency change. runtime-and-deployment.md - Replace the BLOCKED section 11 with the recorded baseline: environment, environment files, translated Makefile commands, build result, service health, startup sequence, migration and sync results, fixtures, tests. - 312 migrations applied; makemigrations --check clean; 115 permissions, 10 roles, 546 role-permissions, 30 valuesets; fixtures loaded. - 1912 tests, 0 skipped, green on 2 of 3 --parallel --shuffle runs. unresolved-items.md - Mark Part E resolved and record the disposition of E1-E6. - Withdraw E6: no root .env is required. docker/.local.env and docker/.prebuilt.env are tracked in git and have no .example variants. - Add E7 (flaky test_password_request_rate_limiting under --parallel) and E8 (transient BuildKit pip cache wheel corruption). - Resolve D8. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0001 / ES-01 (IS-01) milestone 1. Adds django-storages 1.14.6 with the s3 and google extras so that object persistence can move onto Django's Storage API. No application code uses it yet. Resolution notes: - A full `pipenv lock` moved 59 unrelated packages, which ES-01 section 8 forbids. Used `pipenv upgrade` instead, which merges only the resolved subtree into Pipfile.lock. - All 44 direct [packages] pins are unchanged, including boto3 ==1.43.6 (retained: SNS and the legacy signed-URL path still need it) and django ==6.0. - dev-packages and docs categories are untouched. - 12 packages added: django-storages plus the google-cloud-storage tree. - 8 transitive packages moved as an unavoidable consequence of resolving the google extra: asgiref, botocore, certifi, cffi, charset-normalizer, cryptography, idna, s3transfer. None is a direct dependency. django-storages 1.14.6 advertises Django support only through 5.1, so compatibility with Django 6.0 was verified by rebuilding the image and importing both S3Storage and GoogleCloudStorage. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0001 / ES-01 (IS-01) milestone 2. Configuration only; no application code is routed through the new aliases yet. Adds CARE_STORAGE_BACKEND (s3 default, gcs supported). An unsupported value raises ImproperlyConfigured naming the supported values. Defines the logical aliases patient, facility and report in STORAGES, built by config/storage.py. staticfiles is untouched and stays on WhiteNoise. `default` is deliberately still absent: Django 6.0 does not merge STORAGES with its global defaults and nothing in CARE resolves that alias, so adding it would change behaviour rather than preserve it. Bucket names come from provider-neutral CARE_*_STORAGE_BUCKET variables, each defaulting to the existing setting, so no local or deployed configuration has to be rewritten. report shares the patient bucket, as before, but is a separate alias so it can be repointed without touching application code. Behaviour change, per ES-01 section 26 (do not preserve a verified settings bug): the patient and report aliases now read FILE_UPLOAD_REGION, FILE_UPLOAD_KEY and FILE_UPLOAD_SECRET. get_patient_bucket_config and get_report_bucket_config read the FACILITY_S3_* credentials instead, leaving those three settings dead. Locally all of them resolve to the same MinIO values, so there is no local change; a deployment that sets FACILITY_S3_KEY to something other than BUCKET_KEY without also setting FILE_UPLOAD_KEY will need to set it. file_overwrite is set explicitly because CARE generates a unique internal_name and the boto3 put_object being replaced overwrote unconditionally; Django must not rename on collision. Verified against MinIO: re-saving returns the same name and replaces the content. BUCKET_HAS_FINE_ACL is honoured at the facility alias rather than per object. The facility bucket's only writer is cover_image.py, so this is equivalent. It is ignored under GCS, which requires uniform bucket-level access. Verified: all four aliases resolve; a MinIO save/exists/size/open/delete round trip succeeds; open of a missing object raises FileNotFoundError; GCS aliases construct with no credentials present; collectstatic and manage.py check pass. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0001 / ES-01 (IS-01) milestone 3. Ordinary object persistence now goes through Django Storage aliases. No provider client is constructed for persistence anywhere in CARE. file_manager.py replaces S3FilesManager with FilesManager, a transitional wrapper that resolves a logical alias and delegates to Django Storage. It imports no provider SDK and holds no provider branch. Retained rather than removed because files_manager is a class attribute on FileUpload and ReportUpload and is used by viewsets, tasks and report generation; rewriting every caller was wider than IS-01 warrants. get_storage_name() is the provider-neutral name helper. It preserves the verified <file_type>/<internal_name> convention exactly, returns a relative name with no bucket, URL or endpoint, and rejects traversal. Both components are already server-controlled (FileTypeChoices, the report-type registry, and a UUID internal_name), so the guard is defence in depth. Signed URLs move to the new legacy_signed_urls.py, isolated from ordinary CRUD per ES-01 section 21 and listing its exact callers. It is the only remaining provider SDK use for files and is S3-only; IS-02 removes it. Migrated call sites: - file_upload viewset put_object -> Storage.save (unchanged signature) - report generation -> Storage.save, wrapping the already-materialised bytes in ContentFile - cleanup task -> Storage.delete - cover_image.py -> storages[facility] directly, keeping its own <folder>/<id>_<token>.<ext> key convention, and now streaming the uploaded file instead of passing raw bytes delete_object no longer takes `quiet`. Django Storage delete is idempotent on both backends, and so was the boto3 call it replaces: S3 delete_object does not raise NoSuchKey, so the old quiet=False branch never fired. report generation keeps an explicit content type. S3Storage prefers content.content_type; GoogleCloudStorage derives it from the name, so the extension in internal_name stays the portable signal. The only registered generator is html/.html, which guesses identically. test_file_upload_api no longer asserts botocore ClientError/NoSuchKey. It now asserts the object does not exist and that opening it raises FileNotFoundError -- provider-neutral and one assertion stronger. care/emr/tasks/report_generation.py keeps autoretry_for=(ClientError,) deliberately. It imports an exception class for retry configuration and never instantiates boto3; under the default s3 profile django-storages raises ClientError from inside Storage.save, so retry behaviour is unchanged. It would not retry under gcs; recorded as a known gap rather than changing Celery retry semantics, which ES-01 section 31 excludes. Tests: 102 pass across file upload, facility, user and diagnostic report suites, including real MinIO round trips. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0001 / ES-01 (IS-01) milestone 4. 50 new tests. care/utils/tests/test_storage_config.py -- construction only, no provider contact and no Google credentials: - s3 is the default backend - patient, facility and report resolve to S3Storage under s3 - staticfiles stays on WhiteNoise - an invalid backend raises ImproperlyConfigured naming s3 and gcs - report shares the patient bucket while staying a distinct alias - alias names contain no provider name - GCS variables are not required under s3, and S3 options are not leaked into a GCS alias - GCS never uses object ACLs, and GoogleCloudStorage constructs with no credentials present - the endpoint is omitted for AWS and included for S3-compatible care/emr/tests/test_storage.py: - the <file_type>/<internal_name> convention, including Unicode, multi-part extensions and unusual names, all preserved byte-for-byte - names are relative: no bucket, URL or endpoint - traversal, absolute paths and empty components raise SuspiciousFileOperation - models are bound to logical aliases, cover images to facility - delegation proven by substituting InMemoryStorage at the Django Storage boundary; no provider SDK is mocked - MinIO integration on all three aliases: save, exists, size, open, delete, missing-object FileNotFoundError, idempotent delete, and overwrite keeping the name Writing the delegation tests surfaced a real distinction worth recording: overwrite-on-collision comes from the file_overwrite backend option, not from Django Storage. InMemoryStorage renames instead. So overwrite is asserted against the configured backends in the MinIO tests and the option itself in the config tests, while the delegation test asserts only that FilesManager reports back what the backend did. Integration test names are UUID-based because the 16 parallel workers share one MinIO. Co-Authored-By: Claude <noreply@anthropic.com>
…rence 07-configuration-reference.md section 12.4 specifies GCS_PROJECT_ID. The initial implementation used CARE_GCS_PROJECT_ID. The documentation is authoritative, so the code is aligned to it rather than the reverse. Optional either way: when unset, GoogleCloudStorage uses Application Default Credentials and no project_id option is emitted. Co-Authored-By: Claude <noreply@anthropic.com>
Documentation only. inventory/storage-call-sites.md - New section 11 marks the migration status of all 19 call sites: 9 migrated_to_django_storage, 7 legacy_signed_url_only, 2 temporary_wrapper, 1 removed, 1 not_storage_persistence, 0 blocked. - Records the alias table, the object-name helper, the corrected credential defect from section 2.1 and its behaviour change, the two remaining provider SDK uses, and the whole-file reads that remain. - Sections 1-8 are left as the Phase 0 snapshot; section 11 is current where they disagree. inventory/frontend-file-flow.md - New section 11: no route, request field or response field changed. - Maps the file and line references IS-01 relocated. - Records which persistence now uses Django Storage, and which signed upload, signed download and base64 flows remain for IS-02. - Marks D3 partly done and C3 resolved at the settings layer; the rest of the section 9 change list is unchanged. - Records that legacy_signed_urls is S3-only, so IS-02 is a prerequisite for a real GCS deployment. inventory/unresolved-items.md - New Part B2 with S1 (GCS cannot serve files end to end), S2 (report retry does not fire under GCS), S3 (overwrite safety depends on a backend option) and S4 (items unchanged by IS-01). - B10 resolved: delete_objects removed. - B1 partly resolved: the aliases read the correct credentials; the two csp/config.py resolvers still carry the defect but are now reached only by the legacy signed-URL path. - E7 scope corrected. It is not one flaky test but a defect class: six tests across rate limiting and favorites, all caused by a single Redis cache shared by 16 parallel workers while three modules call cache.clear() in setUp. Measured: full suite serial 1962/1962 OK; full suite parallel 1 of 6 green; the three cache-touching modules alone fail 4 of 4 in parallel with no storage code involved. Includes the recommended per-worker KEY_PREFIX fix. 02-target-runtime.md section 11 - States the four required claims explicitly: Django Storage API is the architecture; MinIO through S3Storage is the default local profile; generic S3-compatible storage remains supported; GCS is the initial GCP profile, not the only supported provider. - Adds the post-IS-01 status of the three SHALL NOT clauses, naming the two transitional exceptions. 07-configuration-reference.md - 11.2: records CARE_STORAGE_BACKEND as implemented, s3 default, and that filesystem is not a supported value per ES-01 section 9. - 12.8: corrects the suggestion that GCS_FILE_OVERWRITE=false may suit unique immutable names. false does not reject duplicates, it silently renames, which is exactly the database-reference hazard the section warns about. Overwrite is enabled on every alias, with the tests that demonstrate both behaviours cited. Co-Authored-By: Claude <noreply@anthropic.com>
… Django
ADR-0001 completion. Object transport is now entirely mediated by CARE:
no application code generates a storage-provider URL, and no client ever
receives one.
Removed:
- care/emr/utils/legacy_signed_urls.py (presigned PUT and GET generation)
- care/utils/csp/ entirely: BucketType, CSProvider, ClientConfig and
get_client_config existed only to resolve provider credentials and
external endpoints for those signed URLs
- the signed_url response field on FileUploadRetrieveSpec and
ReportUploadRetrieveSpec, and the _just_created write branch
- FACILITY_CDN, BUCKET_HAS_FINE_ACL, BUCKET_EXTERNAL_ENDPOINT,
FILE_UPLOAD_BUCKET_EXTERNAL_ENDPOINT and
FACILITY_S3_BUCKET_EXTERNAL_ENDPOINT, which had no consumer left
- the public-read ACL on the facility alias; every bucket is now private
Added care/emr/utils/file_download.py: a provider-neutral FileResponse
helper that streams through Django Storage. It carries the
inline-vs-attachment decision the presigned ResponseContentDisposition
used to make, so browser handling of patient documents is unchanged.
Download routes:
- GET /api/v1/files/{external_id}/download/ -- authorized by the existing
get_queryset file_authorizer, which runs for every detail action
- GET /api/v1/template_reports/{external_id}/download/ -- authorizes
explicitly, because ReportUploadViewSet.get_queryset only guards list
- GET /api/v1/assets/facility/{external_id}/cover_image/
- GET /api/v1/assets/user/{username}/profile_picture/
read_signed_url is replaced by download_url, a CARE route.
The two asset routes are unauthenticated, and deliberately so. Cover
images and avatars are already world-readable: AllFacilityViewSet and
FacilitySchedulableUsersViewSet are anonymous and expose them. Who can
see an image is unchanged; what changes is that CARE serves the bytes,
so the bucket becomes private. They are separate views because
FacilityViewSet and UserViewSet filter their querysets by request.user
and cannot serve an anonymous request.
Facility.read_cover_image_url and User.read_profile_picture_url now
reverse to those routes instead of concatenating a bucket URL, which also
removes the bucket-name divergence between persistence and URL building
that the review found.
BUCKET_PROVIDER survives as the credential-source switch, now compared
against a literal in config/storage.py rather than the deleted enum.
Tests: the presigned PUT round trip in test_upload_patient_file is
replaced by the Django upload endpoint plus a download through CARE,
asserting the route is a CARE URL and that Content-Type and
Content-Disposition are preserved. test_cleanup_incomplete_file_uploads
writes its object through Django Storage. The
FILE_UPLOAD_BUCKET_EXTERNAL_ENDPOINT override, which existed only for
signed URLs, is gone. 12 file API tests pass.
Co-Authored-By: Claude <noreply@anthropic.com>
plugin-impact.md records that S3FilesManager is importable from care.emr.utils.file_manager and that any plugin may use it. Renaming it to FilesManager in the previous commit was therefore a silent breaking change for external plugins. The name is restored as a thin subclass. Despite the name it is not S3-specific: it is FilesManager, delegating to whichever provider the alias is configured for. It emits a DeprecationWarning, accepts the historical positional argument that used to be a BucketType, and maps PATIENT/FACILITY/REPORT onto the logical aliases. An unrecognised value raises rather than silently writing to the wrong bucket. It deliberately exposes no signed_url or read_signed_url method. Those were removed with the provider-specific transport, and a plugin must not be able to mint a bucket URL that bypasses CARE. Also updates the module docstring, which still pointed at the deleted legacy_signed_urls module. Co-Authored-By: Claude <noreply@anthropic.com>
New care/emr/tests/test_storage_transport.py (25 tests) asserts the property the architecture depends on rather than the mechanics: - the base64 upload endpoint still works and returns no provider URL - no response offers signed_url or read_signed_url - download_url is a CARE route, matched against reverse() - detail and list responses are scanned for provider markers (minio, amazonaws, storage.googleapis, X-Amz-Signature, GoogleAccessId, :9100) - downloading through Django returns the stored bytes - download enforces authorization; a different user gets 403 - a missing object yields 404, not a provider error - facility cover image and user avatar URLs are CARE routes that serve the stored bytes - the cover-image route stays anonymous, matching the world-readable bucket objects it replaces, and 404s when no image is set - every alias round-trips through Django Storage - no manager exposes a signed-URL helper Plugin compatibility: S3FilesManager imports, warns, delegates to Django Storage, maps each legacy bucket name, rejects unknown aliases, and exposes no signed-URL method. Updated test_storage_config for the removed default_acl parameter and added a test that no configured alias can be public. 68 storage tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0001 - Implementation checklist: IS-01 complete, legacy storage removed, legacy signed URL flows removed. - Records what IS-01 delivered and that only the base64 upload transport remains for IS-02. storage-call-sites.md - Final per-call-site status: 11 migrated, 2 temporary wrapper, 6 removed. Nothing is legacy_signed_url_only; nothing is blocked. - New 11.1a: the four CARE routes that now mediate transport, their authorization, and why the two asset routes are anonymous. - New 11.3a: one source of truth per logical bucket. Records that this closes a defect introduced earlier in IS-01, where setting CARE_PATIENT_STORAGE_BUCKET moved persistence but not the signed URLs, so uploads and downloads silently addressed different buckets on the default s3 profile. - 11.5 rewritten: no storage module imports a provider SDK. Lists the deleted settings and the csp package. - New 11.8: the two remaining compatibility layers. frontend-file-flow.md - 11.3 rewritten: signed upload, signed download and unsigned bucket URLs are all gone; base64 upload is all that remains for IS-02. - 11.4: 13 of the 18 section 9 change items are now done. - 11.5: contract impact as delivered, including that the four URL fields are now relative paths where they used to be absolute. - 11.6: GCS is no longer blocked on transport. plugin-impact.md - Corrects the row claiming a plugin can mint presigned URLs through S3FilesManager. - New section 9: deprecation notice with a before/after table of every behavioural change, the DeprecationWarning, the migration target, and the fact that the no-signed-URL guarantee is CARE's rather than enforced against plugin code. 07-configuration-reference.md - Section 13 corrected. It specified S3_ACCESS_KEY, S3_SECRET_KEY, S3_ENDPOINT_URL, S3_REGION_NAME, S3_ADDRESSING_STYLE and S3_SIGNATURE_VERSION; none exists. Replaced with the actual per-alias credential and endpoint variables, BUCKET_PROVIDER as the credential source, and the bucket variables as the single point of resolution. - Records that addressing_style and signature_version are not configurable, and what that rules out. 02-target-runtime.md - Section 11 status: all three SHALL NOT clauses now hold for persistence and transport. Records that transport is entirely CARE-mediated and that csp/ is deleted. unresolved-items.md - S1 resolved: the GCS profile serves files end to end; IS-02 is no longer a prerequisite for a GCS deployment. - S2 is now the only item blocking that profile. - B9 largely defused: no client can write to the bucket, so an unverified mark_upload_completed is bookkeeping rather than an unverified external write. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0002 / ES-02. The last remaining transport issue: the Django-mediated upload still carried complete file contents as base64 inside JSON. POST /api/v1/files/upload-file/ now accepts multipart/form-data. The binary part is `file`; the base64 `file_data` field is removed with no fallback, per ES-02 section 23 (greenfield, no production client). FileUploadMultipartSerializer declares the transport contract and the binary field. It deliberately does not restate the domain rules: FileUploadCreateSpec remains the authoritative validator for file type, category and filename, so there is no second source of truth. `original_name` becomes optional and defaults to the uploaded part's filename, which multipart supplies and base64 could not. The file is never materialised: - Django's upload handlers decide InMemoryUploadedFile vs TemporaryUploadedFile before the view runs; - size is read from UploadedFile.size, not by reading content; - MIME sniffing reads only the leading 2048 bytes and seeks back, as before, so a browser-declared Content-Type is still not trusted; - the UploadedFile is handed straight to Storage.save() with no ContentFile round trip. Upload limits are now explicit rather than implicit, per ADR-0002. The added FILE_UPLOAD_MAX_MEMORY_SIZE and DATA_UPLOAD_MAX_MEMORY_SIZE are set to Django's own defaults, so behaviour is unchanged: anything over 2.5 MB is temp-file backed, and MAX_FILE_UPLOAD_SIZE still caps the total at 5 MB. DATA_UPLOAD_MAX_MEMORY_SIZE does not bound the file, because multipart file parts are exempt from it. Worth noting: base64 inflated a 5 MB file to roughly 6.7 MB of JSON body, which exceeds Django's 2.5 MB DATA_UPLOAD_MAX_MEMORY_SIZE. The old endpoint could not actually accept a file near its own documented limit over JSON. Multipart removes that. Persistence, object naming, alias selection and authorization are untouched: still Storage.save() through the patient alias, still get_storage_name, still authorize_create -> file_authorizer. The DB row is still written first inside the transaction so a storage failure rolls it back. Existing tests are updated to send multipart. test_upload_patient_file also stops conflating two rows: it previously created a row via POST /files/, uploaded a different row, then marked the first complete. 30 upload and transport tests pass. Co-Authored-By: Claude <noreply@anthropic.com>
39 focused tests in care/emr/tests/test_file_upload_multipart.py, covering ES-02 sections 31, 33 and 34. Successful upload: record contents, patient alias, the ES-01 object name unchanged, stored bytes match, download works, response carries no provider marker and no bucket or endpoint key. Rejection: missing file, missing metadata, oversized file (and that an oversized file persists nothing), and that the old base64 `file_data` body no longer works even by accident. Validation: the declared part Content-Type is not trusted. A shell script sent as image/jpeg is rejected on sniffed content; a real JPEG sent as application/x-sh is accepted and recorded as image/jpeg. Blocked, uppercase and double extensions behave as before, as do unknown file type and category. Authorization: unauthenticated and unauthorized uploads are rejected and persist nothing. Upload handlers: with a lowered FILE_UPLOAD_MAX_MEMORY_SIZE, the view is verified to receive an InMemoryUploadedFile below the threshold and a TemporaryUploadedFile above it, and the temp-file path round-trips. Failure consistency: a storage failure reports failure and leaves no row; a database failure after the object is written also rolls the row back. The surviving gap is the orphan object, which is the pre-existing B8. Provider neutrality: the whole upload and download flow is exercised against InMemoryStorage substituted at the Django Storage boundary, which would break on any surviving S3 assumption. A further test parses the AST of the three transport modules to assert none imports a provider SDK and none reads CARE_STORAGE_BACKEND. It inspects the AST rather than the text because those modules legitimately mention boto3 and the backend classes in prose describing what they replaced. Schema: the request is multipart/form-data only, `file` is string/binary, `file_data` is absent, and `original_name` is not required. Also adds BinaryFileField. drf-spectacular renders a plain FileField as format: uri, because DRF serialises it to a URL on output; in a multipart request body it is raw bytes. Annotating the field is preferable to enabling COMPONENT_SPLIT_REQUEST, which would reshape every schema in the project. Co-Authored-By: Claude <noreply@anthropic.com>
ADR-0002 - Implementation checklist complete: base64 removed, multipart implemented, upload handlers verified, size limits configured, schema implemented, authorization tested, MinIO round trip verified, provider-neutral behaviour verified, ES-02 complete. - Notes that CARE never materialises the file, that range requests remain undesigned despite FileResponse advertising them, and that POST /files/ plus mark_upload_completed are now vestigial. frontend-file-flow.md - 11.3: base64 upload marked removed. - New section 12: flow status table (signed upload, signed download, unsigned bucket URLs, base64 all removed; multipart upload and server-mediated download implemented), the multipart request contract with each part, the FormData migration snippet for the external frontend repository, and the memory/size table. - Records that base64 inflated a 5 MB file past DATA_UPLOAD_MAX_MEMORY_SIZE, so the old endpoint could not accept a file at its own documented limit over JSON. - Records what ES-02 did not change: cover images and avatars already used ordinary multipart and never touched the base64 endpoint; authorization, naming, alias selection and persistence are untouched. storage-call-sites.md - Call site 12 now covers transport as well as persistence. - The base64 whole-file read is marked gone. - Removes the note describing base64 as a remaining transport. 07-configuration-reference.md - Section 15 corrected. It specified CARE_MAX_UPLOAD_SIZE, CARE_ALLOWED_UPLOAD_MIME_TYPES, CARE_ALLOWED_UPLOAD_EXTENSIONS and CARE_BLOCKED_UPLOAD_EXTENSIONS; none exists. Replaced with the real MAX_FILE_UPLOAD_SIZE (megabytes, not bytes), the two explicit Django memory settings, ALLOWED_MIME_TYPES with the note that the value is sniffed rather than taken from the request, and a pointer to FileNameValidator for extension policy. - 16.1 corrected: inline behaviour comes from SAFE_INLINE_FORMATS in code, not a CARE_INLINE_MIME_TYPES setting. Co-Authored-By: Claude <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe pull request moves CARE file persistence to Django Storage aliases, replaces base64 uploads with multipart uploads, adds CARE-mediated streaming downloads, and serves public image assets through CARE routes. It also adds backend configuration, compatibility coverage, tests, and architecture documentation. ChangesStorage and file transport modernization
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (15)
docs/xii/implementation/ES-02-file-transport-modernization.md-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the outer Markdown fence.
The fence at Line 1 encloses the specification as code. Nested fences then render incorrectly. This makes the implementation document unnecessarily difficult to use.
Proposed fix
-```markdown # ES-02: File Transport Modernization ... -```Also applies to: 1180-1180
🤖 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 `@docs/xii/implementation/ES-02-file-transport-modernization.md` at line 1, Remove the outer Markdown code fence surrounding the implementation specification in “ES-02: File Transport Modernization,” including its opening and closing delimiters, while preserving the document content and any intended nested fences.Source: Linters/SAST tools
docs/xii/implementation/ES-02-file-transport-modernization.md-95-109 (1)
95-109: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the current
docs/xii/document paths.The required-document list and ADR update target an obsolete
docs/hierarchy. The current specifications and inventories are underdocs/xii/. An implementer cannot reliably complete the required reading or update the ADR checklist otherwise.
docs/xii/implementation/ES-02-file-transport-modernization.md#L95-L109: Replace thedocs/architecture/,docs/adr/, anddocs/specifications/references with the currentdocs/xii/architecture/,docs/xii/adr/, anddocs/xii/implementation/paths.docs/xii/implementation/ES-02-file-transport-modernization.md#L1044-L1078: Update the ADR checklist path todocs/xii/adr/ADR-0002-file-transport.md.🤖 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 `@docs/xii/implementation/ES-02-file-transport-modernization.md` around lines 95 - 109, Update the required-document list in docs/xii/implementation/ES-02-file-transport-modernization.md lines 95-109 to use the current docs/xii/architecture/, docs/xii/adr/, and docs/xii/implementation/ paths. Also update the ADR checklist in lines 1044-1078 to reference docs/xii/adr/ADR-0002-file-transport.md.care/emr/tests/test_file_upload_multipart.py-299-305 (1)
299-305: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis test does not test what its name and comment claim.
The name says "incomplete rows remain cleanable" and the comment says the path "still sets
upload_completed=False". The assertions then requireupload_completedto beTrueand the object to exist. A successful multipart upload is complete, so nothing here is cleanable. Either rename the test to describe the completed-row invariant, or build a genuinely incomplete row and runcleanup_incomplete_file_uploadsagainst 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 `@care/emr/tests/test_file_upload_multipart.py` around lines 299 - 305, Correct test_incomplete_rows_remain_cleanable so its setup and assertions match its intent: either rename it to describe the completed multipart-upload invariant while preserving the current successful-upload assertions, or create an incomplete upload row with upload_completed=False and exercise cleanup_incomplete_file_uploads to verify it remains cleanable. Do not leave the current misleading name and comment paired with assertions for a completed upload.care/emr/utils/file_manager.py-149-153 (1)
149-153: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAnnotate
_LEGACY_ALIASESwithClassVar.Ruff reports RUF012 for this mutable class-level dict, even though it is used only as shared read-only lookup state. Add
typing.ClassVarand runruff check --fix ./ruff format ..Proposed fix
import logging import warnings +from typing import ClassVar - _LEGACY_ALIASES = { + _LEGACY_ALIASES: ClassVar[dict[str, str]] = {🤖 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 `@care/emr/utils/file_manager.py` around lines 149 - 153, Annotate the mutable class-level dictionary `_LEGACY_ALIASES` with `typing.ClassVar` to satisfy Ruff RUF012 while preserving its shared read-only lookup behavior. Add the required typing import, then run `ruff check --fix .` and `ruff format .`.Sources: Coding guidelines, Linters/SAST tools
docs/xii/adr/ADR-0002-file-transport.md-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the outer Markdown fence.
The file starts with a
markdownfence at Line 1 and closes it at Line 515. This wraps the entire ADR in a code block, so headings, lists, and inner examples render as literal code. Remove both wrapper lines and keep fences only around the examples.Also applies to: 515-515
🤖 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 `@docs/xii/adr/ADR-0002-file-transport.md` at line 1, Remove the outer Markdown code fences wrapping the entire ADR, including the opening fence at the document start and its matching closing fence at the end. Preserve all inner fences that intentionally delimit examples so headings and lists render as Markdown.Source: Linters/SAST tools
care/emr/resources/file_upload/spec.py-107-115 (1)
107-115: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not advertise a download URL for incomplete uploads.
download_urlis assigned for everyFileUploadRetrieveSpec, but detail access still permits rows withupload_completed=False. The new multipart flow writes the storage object before setting that flag, so an incomplete row can expose a URL that cannot resolve to an object. ReturnNoneuntil the upload is complete, and reject incomplete objects in the download action.Proposed serialization fix
- mapping["download_url"] = file_download_url(obj) + mapping["download_url"] = ( + file_download_url(obj) if obj.upload_completed else None + )🤖 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 `@care/emr/resources/file_upload/spec.py` around lines 107 - 115, Update FileUploadRetrieveSpec.perform_extra_serialization to assign download_url only when obj.upload_completed is true, leaving it None for incomplete uploads. Also update the download action that serves file uploads to reject objects with upload_completed=false before resolving or returning the storage URL.docs/xii/adr/ADR-0001-django-storage.md-81-83 (1)
81-83: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language identifiers to the fenced code blocks.
markdownlintreports MD040 for these fences. Usetextfor the storage path andenvortextfor the configuration values.Also applies to: 135-137, 148-150
🤖 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 `@docs/xii/adr/ADR-0001-django-storage.md` around lines 81 - 83, Add language identifiers to the fenced code blocks in ADR-0001, including the blocks around the S3Storage path and the configuration values at the referenced locations. Use text for the storage path and env or text for configuration-value blocks so all fences satisfy markdownlint MD040.Source: Linters/SAST tools
docs/xii/architecture/02-target-runtime.md-17-34 (1)
17-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the cross-document paths before merge.
These references use
docs/gcp/...ordocs/xii/gcp/..., while the reviewed documents usedocs/xii/architecture/.... The current links will not resolve to the documented files.
docs/xii/architecture/02-target-runtime.md#L17-L34: update the current-runtime and migration-plan paths.docs/xii/architecture/00-scope-and-goals.md#L845-L851: update the next-document path.docs/xii/architecture/02-target-runtime.md#L1874-L1880: update the next-document path.docs/xii/architecture/03-migration-plan.md#L9-L13: update alldepends_onpaths.docs/xii/architecture/03-migration-plan.md#L2008-L2014: update the testing-document path.docs/xii/architecture/04-testing.md#L9-L13: update alldepends_onpaths.docs/xii/architecture/04-testing.md#L1960-L1966: update the upstream-sync document path.🤖 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 `@docs/xii/architecture/02-target-runtime.md` around lines 17 - 34, Correct the broken cross-document references across docs/xii/architecture/02-target-runtime.md lines 17-34 and 1874-1880, docs/xii/architecture/00-scope-and-goals.md lines 845-851, docs/xii/architecture/03-migration-plan.md lines 9-13 and 2008-2014, and docs/xii/architecture/04-testing.md lines 9-13 and 1960-1966. Replace the outdated docs/gcp or docs/xii/gcp paths with the corresponding docs/xii/architecture paths, preserving each link’s intended target and relationship.docs/xii/architecture/04-testing.md-1811-1835 (1)
1811-1835: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a test-only task backend allowlist.
The runtime documentation says
CARE_TASK_BACKENDsupportscloud_tasks,celery, andpostgres, but the quick unit-test example setsfake. UseCARE_TASK_BACKEND=fakeonly as an explicit test-only value and document that production settings reject it, so this example does not conflict with the documented backend contract.🤖 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 `@docs/xii/architecture/04-testing.md` around lines 1811 - 1835, Update the “Test Configuration” section to explicitly define fake as a test-only CARE_TASK_BACKEND value, clarify that production configuration rejects fake, and preserve the existing real-backend integration-test guidance so the unit-test example does not conflict with the runtime backend contract.docs/xii/architecture/04-testing.md-1367-1376 (1)
1367-1376: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the Terraform checks cover the whole layout.
The snippet claims coverage for all modules and environments, while these commands target only the current directory. For the layout in
docs/xii/architecture/03-migration-plan.md, include-recursive, and runterraform validatethroughdeploy/gcp/terraformroot and each environment/module that requires initialization.🤖 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 `@docs/xii/architecture/04-testing.md` around lines 1367 - 1376, Update the “23.2 Formatting and validation” instructions to use recursive Terraform formatting across the full layout, and document validation from deploy/gcp/terraform plus each environment/module requiring initialization. Align the commands with the directory structure described in 03-migration-plan.md so the stated all-modules and all-environments coverage is accurate.docs/xii/architecture/05-upstream-sync.md-10-15 (1)
10-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign all documentation references with the repository’s actual documentation root.
The file mixes
docs/gcp/...references with the supplied architecture tree underdocs/xii/.... Unlessdocs/gcpis an intentional generated mirror, these paths break dependency tracking, search commands, conflict-register discovery, and document navigation.
docs/xii/architecture/05-upstream-sync.md#L10-L15: replace thedepends_onpaths with the correspondingdocs/xii/architecture/...paths.docs/xii/architecture/05-upstream-sync.md#L704-L708: update the search examples to use the approveddocs/xiiroot.docs/xii/architecture/05-upstream-sync.md#L884-L888: use the approved conflict-register path underdocs/xii.docs/xii/architecture/05-upstream-sync.md#L1162-L1168: replace the next-document path withdocs/xii/architecture/06-operations.md.🤖 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 `@docs/xii/architecture/05-upstream-sync.md` around lines 10 - 15, Align every documentation reference in docs/xii/architecture/05-upstream-sync.md with the approved docs/xii root: update the depends_on entries at lines 10-15 to their corresponding docs/xii/architecture paths, revise the search examples at lines 704-708, use the docs/xii conflict-register path at lines 884-888, and set the next-document reference at lines 1162-1168 to docs/xii/architecture/06-operations.md.docs/xii/architecture/06-operations.md-9-15 (1)
9-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the repository’s current documentation tree.
Both documents are under
docs/xii/architecture/, but their dependency and next-document references usedocs/gcp/. Update every affected reference to the actualdocs/xii/architecture/path.
docs/xii/architecture/06-operations.md#L9-L15: Update the dependency paths.docs/xii/architecture/06-operations.md#L2527-L2533: Update the next-document path.docs/xii/architecture/07-configuration-reference.md#L9-L16: Update the dependency paths.docs/xii/architecture/07-configuration-reference.md#L2842-L2848: Update the next-document path.🤖 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 `@docs/xii/architecture/06-operations.md` around lines 9 - 15, Update all dependency and next-document references in docs/xii/architecture/06-operations.md at lines 9-15 and 2527-2533, and docs/xii/architecture/07-configuration-reference.md at lines 9-16 and 2842-2848, replacing the incorrect docs/gcp/ paths with the corresponding docs/xii/architecture/ paths.docs/xii/architecture/inventory/storage-call-sites.md-368-369 (1)
368-369: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the migration totals or define their scope.
For numbered sites 1–19, the table contains 9 migrated sites, 2 temporary wrappers, and 8 removed sites. If the two unnumbered facility/user URL builders are included, the totals become 11 migrated, 2 temporary, and 8 removed across 21 sites. The current
11 migrated, 2 temporary, 6 removeddoes not match the table.🤖 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 `@docs/xii/architecture/inventory/storage-call-sites.md` around lines 368 - 369, Update the migration totals in the summary to match the documented call-site counts: use 9 migrated, 2 temporary wrappers, and 8 removed for numbered sites 1–19, or explicitly define a broader scope and use its corresponding 11 migrated, 2 temporary, and 8 removed totals including the two unnumbered builders.docs/xii/architecture/inventory/cache-and-redis.md-474-474 (1)
474-474: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language identifiers to all flagged Markdown fences.
Apply
text,bash,dockerfile,http, or the appropriate language at each listed site. This removes the repeated MD040 lint failures.
docs/xii/architecture/inventory/cache-and-redis.md#L474-L474: mark the per-file breakdown astext.docs/xii/architecture/inventory/frontend-file-flow.md#L94-L94: mark the request-field block.docs/xii/architecture/inventory/frontend-file-flow.md#L517-L517: mark the HTTP request block.docs/xii/architecture/inventory/plugin-impact.md#L256-L256: mark the download route block.docs/xii/architecture/inventory/runtime-and-deployment.md#L43-L43,#L73-L73,#L152-L152,#L454-L454,#L579-L579: mark the Procfile, command, Dockerfile, and error-output blocks.docs/xii/architecture/inventory/storage-call-sites.md#L52-L52: mark the bucket-type block.docs/xii/architecture/inventory/task-call-sites.md#L43-L43,#L73-L73,#L152-L152,#L454-L454,#L579-L579: mark the command, task configuration, and error-output blocks.🤖 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 `@docs/xii/architecture/inventory/cache-and-redis.md` at line 474, Add language identifiers to every flagged Markdown fence to resolve MD040: use text for the per-file breakdown in docs/xii/architecture/inventory/cache-and-redis.md:474-474; use the appropriate text or http identifiers for docs/xii/architecture/inventory/frontend-file-flow.md:94-94 and :517-517; mark the download route in docs/xii/architecture/inventory/plugin-impact.md:256-256; mark the Procfile, command, Dockerfile, and error-output blocks in docs/xii/architecture/inventory/runtime-and-deployment.md:43-43, 73-73, 152-152, 454-454, and 579-579; mark the bucket-type block in docs/xii/architecture/inventory/storage-call-sites.md:52-52; and mark the command, task configuration, and error-output blocks in docs/xii/architecture/inventory/task-call-sites.md:43-43, 73-73, 152-152, 454-454, and 579-579.Source: Linters/SAST tools
docs/xii/architecture/inventory/task-call-sites.md-110-124 (1)
110-124: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNarrow the report retry and orphan statement.
A normal
ClientErrorfrom storage causesgenerate_and_upload_reportto delete the row before re-raising. The orphan gap occurs when the process fails after the row save and before, or ambiguously during, the storage write. Describe the task as non-idempotent and potentially orphan-producing instead of stating that retries produce orphan rows unconditionally.🤖 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 `@docs/xii/architecture/inventory/task-call-sites.md` around lines 110 - 124, The Idempotency entry for generate_and_upload_report overstates orphan creation by claiming retries produce orphan rows unconditionally. Revise it to retain the non-idempotent classification while stating that retries can potentially produce orphan rows when execution fails before or ambiguously during the storage write; preserve the existing normal ClientError cleanup behavior.
🧹 Nitpick comments (8)
care/emr/tests/test_file_upload_api.py (1)
17-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
response_contentis now defined twice.The same helper exists in
care/emr/tests/test_storage_transport.pyat lines 17-19, andcare/emr/tests/test_file_upload_multipart.pyinlinesb"".join(...)in four places. Move it to a shared test utility so all three read the same way.🤖 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 `@care/emr/tests/test_file_upload_api.py` around lines 17 - 19, Move the duplicated response_content helper into a shared test utility, then update care/emr/tests/test_file_upload_api.py and care/emr/tests/test_storage_transport.py to import and use it. Replace the four inline b"".join(response.streaming_content) expressions in test_file_upload_multipart.py with the shared helper, preserving the existing streaming response behavior.care/emr/api/viewsets/file_upload.py (2)
133-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe docstring and the field declarations disagree.
The docstring says
FileUploadCreateSpec"remains the authoritative validator for the logical file type, category and filename" and that "this serializer does not restate those rules". TheChoiceFielddeclarations then restate the type and category rules. Both layers now have to be updated together. Either drop the twoChoiceFieldconstraints to plainCharField, or adjust the docstring to say that the choices are duplicated for schema generation.🤖 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 `@care/emr/api/viewsets/file_upload.py` around lines 133 - 155, Resolve the contract mismatch in FileUploadMultipartSerializer by either changing file_type and file_category to unconstrained CharField declarations so FileUploadCreateSpec remains authoritative, or revising the docstring to explicitly document the duplicated ChoiceField validation for schema generation; keep the implementation and documentation consistent.
337-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe catch-all reports a storage failure for database failures too.
The
tryblock coversput_objectand bothsavecalls. A failure in eithersaveproduces "Failed to upload file to storage", which is not what happened. Your own testtest_database_failure_after_save_reports_failureincare/emr/tests/test_file_upload_multipart.pydocuments exactly this path. Narrow the block, or make the message honest.♻️ Proposed narrowing
try: - # The UploadedFile is handed straight to Django Storage; it is - # not read into memory first. - file_upload.files_manager.put_object(file_upload, uploaded_file) - file_upload.upload_completed = True - file_upload.updated_by = request.user - file_upload.save(skip_internal_name=True) + # The UploadedFile is handed straight to Django Storage; it is + # not read into memory first. + file_upload.files_manager.put_object(file_upload, uploaded_file) except Exception as e: error_msg = "Failed to upload file to storage" raise ValidationError(error_msg) from e + + file_upload.upload_completed = True + file_upload.updated_by = request.user + file_upload.save(skip_internal_name=True)🤖 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 `@care/emr/api/viewsets/file_upload.py` around lines 337 - 346, Update the try/except around file upload handling so only file_upload.files_manager.put_object is reported as a storage failure. Keep the upload state updates and save calls outside that storage-specific exception block, preserving test_database_failure_after_save_reports_failure’s database-error behavior.config/settings/base.py (2)
690-725: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm that suppressing
endpoint_urlfor role-based credentials is intended.
_ROLE_BASED_BUCKETcontrols three options, but the comment only explains key and secret. Credential resolution and endpoint selection are independent concerns. A deployment that uses an instance role together with a VPC endpoint or an S3-compatible gateway loses itsFILE_UPLOAD_BUCKET_ENDPOINTsilently. If this matches the previous behavior, extend the comment to say so.♻️ Optional: decouple endpoint from credential mode
- endpoint_url=None if _ROLE_BASED_BUCKET else FILE_UPLOAD_BUCKET_ENDPOINT, + endpoint_url=FILE_UPLOAD_BUCKET_ENDPOINT or None,🤖 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 `@config/settings/base.py` around lines 690 - 725, Update the comment above _ROLE_BASED_BUCKET to explicitly document whether endpoint_url is intentionally omitted for role-based credentials, including that VPC or S3-compatible endpoints are not preserved; if this suppression is not intended, decouple endpoint_url from _ROLE_BASED_BUCKET while retaining the credential behavior.
668-684: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider validating the bucket names, not only the backend.
validate_storage_backendfails fast on a bad backend. The bucket names have no such check, andFILE_UPLOAD_BUCKETandFACILITY_S3_BUCKETboth default to"". An empty alias bucket produces a provider error at the first upload instead of at startup. A short check here would keep the failure at configuration time, where it is much easier to read.🤖 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 `@config/settings/base.py` around lines 668 - 684, Validate CARE_PATIENT_STORAGE_BUCKET, CARE_FACILITY_STORAGE_BUCKET, and CARE_REPORT_STORAGE_BUCKET during configuration loading, rejecting empty or whitespace-only values after applying their existing defaults. Reuse an established bucket-name validation helper if available, and fail startup with a clear configuration error before any upload occurs.config/storage.py (1)
69-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe GCS branch drops S3 credentials without a signal.
If
backend == "gcs", thenregion_name,access_key,secret_keyandendpoint_urlare discarded silently.config/settings/base.pyalways passes them. A deployer who switches the backend and keeps the old environment variables gets no hint that those values now do nothing. A short docstring line, or a log warning, would remove the guesswork.🤖 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 `@config/storage.py` around lines 69 - 78, The GCS branch in the storage configuration silently ignores S3-specific settings. Add a concise docstring or warning near the backend selection logic explicitly stating that region_name, access_key, secret_key, and endpoint_url are unused when backend is "gcs", without changing the existing GCS options behavior.care/emr/tests/test_file_upload_multipart.py (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRun
ruff formaton this file.Lines 42 and 334 exceed the default 88-character limit, and the call at lines 90-92 is not in formatter shape. The repository guidelines require
ruff check --fix .andruff format .after code changes, so CI will most likely point at the same lines.As per coding guidelines: "After code changes, run 'ruff check --fix .' and 'ruff format .' to lint and format Python code".
Also applies to: 90-92, 334-334
🤖 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 `@care/emr/tests/test_file_upload_multipart.py` at line 42, Run ruff check --fix . followed by ruff format . to format care/emr/tests/test_file_upload_multipart.py, including the long upload method signature and the call around lines 90-92, without changing behavior.Source: Coding guidelines
docs/xii/adr/ADR-0002-file-transport.md (1)
133-148: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocument the public-asset exception to authenticated downloads.
This ADR requires authenticated CARE downloads, but
config/api_router.pyadds unauthenticated facility-cover and user-profile-picture routes. The implementation is intentional, but the ADR currently gives a broader security guarantee than the code. State that these two asset classes are public exceptions and that file and report downloads remain authenticated.🤖 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 `@docs/xii/adr/ADR-0002-file-transport.md` around lines 133 - 148, Update the “Download Transport” section of ADR-0002 to explicitly identify facility covers and user-profile pictures as intentional public exceptions served by unauthenticated routes. Clarify that all file and report downloads continue to require authenticated CARE endpoints, preserving the existing authenticated download contract for those resources.
🤖 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 `@care/emr/api/viewsets/file_assets.py`:
- Around line 26-41: Update PublicAssetView.serve to add long-lived
Cache-Control headers to the response returned by storage_file_response,
reflecting that public assets are immutable per object key and safe for
anonymous CDN or reverse-proxy caching.
In `@docs/xii/adr/ADR-0001-django-storage.md`:
- Around line 180-195: Synchronize transport documentation with the implemented
multipart-upload and CARE-mediated-download contract: in
docs/xii/adr/ADR-0001-django-storage.md lines 180-195 replace the
unchanged-transport statement, lines 307-315 reference the current
implementation document, and lines 319-346 mark transport work complete while
removing the claim that base64 uploads remain; in
docs/xii/architecture/00-scope-and-goals.md lines 354-364 remove or label
signed-upload flows as historical, and lines 633-649 replace the signed-URL
requirement with CARE-mediated downloads.
In `@docs/xii/architecture/00-scope-and-goals.md`:
- Around line 278-288: Update the distributed locks row in the default GCP
strategy table so PostgreSQL advisory locks are marked as pending rather than
advertised as the default until ADR-0005 prerequisites are satisfied.
Alternatively, only restore the default designation after completing the
required per-call-site analysis, concurrency evidence, and corresponding ADR and
implementation-status updates.
In `@docs/xii/architecture/01-current-runtime.md`:
- Around line 1215-1217: Align all listed inventories with the delivered
IS-01/ES-02 state: in docs/xii/architecture/01-current-runtime.md (lines
1215-1217), mark the S3 and signed-URL sections as historical or update them to
Django Storage and CARE routes; in
docs/xii/architecture/inventory/frontend-file-flow.md (lines 15-21), identify
§§1-10 as baseline and §12 as the current contract; in
docs/xii/architecture/inventory/runtime-and-deployment.md (lines 325-326), add
django-storages or label the dependency inventory as baseline; in
docs/xii/architecture/inventory/storage-call-sites.md (lines 456-459), remove
the claim that the deleted resolver remains used; and in
docs/xii/architecture/inventory/unresolved-items.md (lines 338-346 and 412-418),
close C2 with the private-bucket/anonymous-CARE-route decision and replace C10
with the current multipart/OpenAPI status.
In `@docs/xii/architecture/02-target-runtime.md`:
- Around line 529-533: The report generation path currently retries only
botocore ClientError, so GCS upload failures bypass generate_report_task’s retry
handling. Update the storage boundary to normalize provider-specific failures
into the exception type consumed by generate_report_task, or remove report
generation from the documented full-GCS production readiness claim; do not leave
this as an inventory-only exception.
In `@docs/xii/architecture/05-upstream-sync.md`:
- Around line 343-347: Update the pre-reset verification instructions around the
upstream sync procedure to check that local develop has a clean worktree,
inspect local-only commits with upstream/develop..develop, and compare
upstream/develop and local develop rather than origin/develop. After pushing,
add a two-way mirror-equality check, using an equivalent comparison such as git
diff --exit-code, before allowing the destructive reset.
In `@docs/xii/architecture/07-configuration-reference.md`:
- Around line 720-722: Update the test profile configuration in the documented
configuration reference to remove CARE_STORAGE_BACKEND=filesystem. Replace it
with the test-only InMemoryStorage override via override_settings, while keeping
production backend values limited to s3 and gcs.
- Around line 865-885: Update the local MinIO profile’s storage environment
variables to use the documented FILE_UPLOAD_*, FACILITY_S3_*, or shared BUCKET_*
names instead of unsupported S3_ENDPOINT_URL, S3_ACCESS_KEY, and S3_SECRET_KEY
entries. Apply the same replacement to the additional occurrence referenced in
the profile, preserving the existing alias-specific configuration behavior.
- Around line 167-182: Update the DJANGO_SETTINGS_MODULE examples and
accepted-value references in the configuration documentation, including the
sections around 4.1 and the additional GCP references, to use the supported
production module config.settings.deployment instead of config.settings.gcp.
Keep development references aligned with config.settings.local and ensure all
main contract/profile examples consistently use the repository policy.
In `@docs/xii/architecture/inventory/frontend-file-flow.md`:
- Around line 590-594: The documented file-upload flow must not allow clients to
create and complete records without verifying stored content. Update the section
describing POST /api/v1/files/ and mark_upload_completed to specify deprecation,
server-side completion, or a successful storage-existence check, and remove the
statement that leaves client-asserted completion as an accepted state.
In `@docs/xii/architecture/inventory/runtime-and-deployment.md`:
- Around line 118-120: Update the documented Redis TLS configuration to require
certificate verification using the deployment CA, and remove the production use
of ssl_cert_reqs=none. If disabling verification must remain, explicitly scope
it to local development only.
In `@docs/xii/architecture/inventory/task-call-sites.md`:
- Around line 35-37: Update the verified summary to distinguish three async-only
tasks—generate_report_task, send_totp_enabled_email, and
send_totp_disabled_email—from one mixed-dispatch task,
summarise_monetary_components, which also uses a recursive Cloud Tasks path.
In `@docs/xii/prompts/00-Complete-CARE-Runtime-Inventory`:
- Around line 41-52: Move the Phase 0 document contract from docs/gcp/ to the
docs/xii/ hierarchy throughout
docs/xii/prompts/00-Complete-CARE-Runtime-Inventory: update prerequisite
references at lines 41-52 to the current docs/xii/architecture/ paths, change
inventory outputs at lines 90-100 to docs/xii/architecture/inventory/, and
update verification and baseline output paths at lines 306-371 consistently;
apply the requested changes at all three sites in this file.
---
Minor comments:
In `@care/emr/resources/file_upload/spec.py`:
- Around line 107-115: Update FileUploadRetrieveSpec.perform_extra_serialization
to assign download_url only when obj.upload_completed is true, leaving it None
for incomplete uploads. Also update the download action that serves file uploads
to reject objects with upload_completed=false before resolving or returning the
storage URL.
In `@care/emr/tests/test_file_upload_multipart.py`:
- Around line 299-305: Correct test_incomplete_rows_remain_cleanable so its
setup and assertions match its intent: either rename it to describe the
completed multipart-upload invariant while preserving the current
successful-upload assertions, or create an incomplete upload row with
upload_completed=False and exercise cleanup_incomplete_file_uploads to verify it
remains cleanable. Do not leave the current misleading name and comment paired
with assertions for a completed upload.
In `@care/emr/utils/file_manager.py`:
- Around line 149-153: Annotate the mutable class-level dictionary
`_LEGACY_ALIASES` with `typing.ClassVar` to satisfy Ruff RUF012 while preserving
its shared read-only lookup behavior. Add the required typing import, then run
`ruff check --fix .` and `ruff format .`.
In `@docs/xii/adr/ADR-0001-django-storage.md`:
- Around line 81-83: Add language identifiers to the fenced code blocks in
ADR-0001, including the blocks around the S3Storage path and the configuration
values at the referenced locations. Use text for the storage path and env or
text for configuration-value blocks so all fences satisfy markdownlint MD040.
In `@docs/xii/adr/ADR-0002-file-transport.md`:
- Line 1: Remove the outer Markdown code fences wrapping the entire ADR,
including the opening fence at the document start and its matching closing fence
at the end. Preserve all inner fences that intentionally delimit examples so
headings and lists render as Markdown.
In `@docs/xii/architecture/02-target-runtime.md`:
- Around line 17-34: Correct the broken cross-document references across
docs/xii/architecture/02-target-runtime.md lines 17-34 and 1874-1880,
docs/xii/architecture/00-scope-and-goals.md lines 845-851,
docs/xii/architecture/03-migration-plan.md lines 9-13 and 2008-2014, and
docs/xii/architecture/04-testing.md lines 9-13 and 1960-1966. Replace the
outdated docs/gcp or docs/xii/gcp paths with the corresponding
docs/xii/architecture paths, preserving each link’s intended target and
relationship.
In `@docs/xii/architecture/04-testing.md`:
- Around line 1811-1835: Update the “Test Configuration” section to explicitly
define fake as a test-only CARE_TASK_BACKEND value, clarify that production
configuration rejects fake, and preserve the existing real-backend
integration-test guidance so the unit-test example does not conflict with the
runtime backend contract.
- Around line 1367-1376: Update the “23.2 Formatting and validation”
instructions to use recursive Terraform formatting across the full layout, and
document validation from deploy/gcp/terraform plus each environment/module
requiring initialization. Align the commands with the directory structure
described in 03-migration-plan.md so the stated all-modules and all-environments
coverage is accurate.
In `@docs/xii/architecture/05-upstream-sync.md`:
- Around line 10-15: Align every documentation reference in
docs/xii/architecture/05-upstream-sync.md with the approved docs/xii root:
update the depends_on entries at lines 10-15 to their corresponding
docs/xii/architecture paths, revise the search examples at lines 704-708, use
the docs/xii conflict-register path at lines 884-888, and set the next-document
reference at lines 1162-1168 to docs/xii/architecture/06-operations.md.
In `@docs/xii/architecture/06-operations.md`:
- Around line 9-15: Update all dependency and next-document references in
docs/xii/architecture/06-operations.md at lines 9-15 and 2527-2533, and
docs/xii/architecture/07-configuration-reference.md at lines 9-16 and 2842-2848,
replacing the incorrect docs/gcp/ paths with the corresponding
docs/xii/architecture/ paths.
In `@docs/xii/architecture/inventory/cache-and-redis.md`:
- Line 474: Add language identifiers to every flagged Markdown fence to resolve
MD040: use text for the per-file breakdown in
docs/xii/architecture/inventory/cache-and-redis.md:474-474; use the appropriate
text or http identifiers for
docs/xii/architecture/inventory/frontend-file-flow.md:94-94 and :517-517; mark
the download route in docs/xii/architecture/inventory/plugin-impact.md:256-256;
mark the Procfile, command, Dockerfile, and error-output blocks in
docs/xii/architecture/inventory/runtime-and-deployment.md:43-43, 73-73, 152-152,
454-454, and 579-579; mark the bucket-type block in
docs/xii/architecture/inventory/storage-call-sites.md:52-52; and mark the
command, task configuration, and error-output blocks in
docs/xii/architecture/inventory/task-call-sites.md:43-43, 73-73, 152-152,
454-454, and 579-579.
In `@docs/xii/architecture/inventory/storage-call-sites.md`:
- Around line 368-369: Update the migration totals in the summary to match the
documented call-site counts: use 9 migrated, 2 temporary wrappers, and 8 removed
for numbered sites 1–19, or explicitly define a broader scope and use its
corresponding 11 migrated, 2 temporary, and 8 removed totals including the two
unnumbered builders.
In `@docs/xii/architecture/inventory/task-call-sites.md`:
- Around line 110-124: The Idempotency entry for generate_and_upload_report
overstates orphan creation by claiming retries produce orphan rows
unconditionally. Revise it to retain the non-idempotent classification while
stating that retries can potentially produce orphan rows when execution fails
before or ambiguously during the storage write; preserve the existing normal
ClientError cleanup behavior.
In `@docs/xii/implementation/ES-02-file-transport-modernization.md`:
- Line 1: Remove the outer Markdown code fence surrounding the implementation
specification in “ES-02: File Transport Modernization,” including its opening
and closing delimiters, while preserving the document content and any intended
nested fences.
- Around line 95-109: Update the required-document list in
docs/xii/implementation/ES-02-file-transport-modernization.md lines 95-109 to
use the current docs/xii/architecture/, docs/xii/adr/, and
docs/xii/implementation/ paths. Also update the ADR checklist in lines 1044-1078
to reference docs/xii/adr/ADR-0002-file-transport.md.
---
Nitpick comments:
In `@care/emr/api/viewsets/file_upload.py`:
- Around line 133-155: Resolve the contract mismatch in
FileUploadMultipartSerializer by either changing file_type and file_category to
unconstrained CharField declarations so FileUploadCreateSpec remains
authoritative, or revising the docstring to explicitly document the duplicated
ChoiceField validation for schema generation; keep the implementation and
documentation consistent.
- Around line 337-346: Update the try/except around file upload handling so only
file_upload.files_manager.put_object is reported as a storage failure. Keep the
upload state updates and save calls outside that storage-specific exception
block, preserving test_database_failure_after_save_reports_failure’s
database-error behavior.
In `@care/emr/tests/test_file_upload_api.py`:
- Around line 17-19: Move the duplicated response_content helper into a shared
test utility, then update care/emr/tests/test_file_upload_api.py and
care/emr/tests/test_storage_transport.py to import and use it. Replace the four
inline b"".join(response.streaming_content) expressions in
test_file_upload_multipart.py with the shared helper, preserving the existing
streaming response behavior.
In `@care/emr/tests/test_file_upload_multipart.py`:
- Line 42: Run ruff check --fix . followed by ruff format . to format
care/emr/tests/test_file_upload_multipart.py, including the long upload method
signature and the call around lines 90-92, without changing behavior.
In `@config/settings/base.py`:
- Around line 690-725: Update the comment above _ROLE_BASED_BUCKET to explicitly
document whether endpoint_url is intentionally omitted for role-based
credentials, including that VPC or S3-compatible endpoints are not preserved; if
this suppression is not intended, decouple endpoint_url from _ROLE_BASED_BUCKET
while retaining the credential behavior.
- Around line 668-684: Validate CARE_PATIENT_STORAGE_BUCKET,
CARE_FACILITY_STORAGE_BUCKET, and CARE_REPORT_STORAGE_BUCKET during
configuration loading, rejecting empty or whitespace-only values after applying
their existing defaults. Reuse an established bucket-name validation helper if
available, and fail startup with a clear configuration error before any upload
occurs.
In `@config/storage.py`:
- Around line 69-78: The GCS branch in the storage configuration silently
ignores S3-specific settings. Add a concise docstring or warning near the
backend selection logic explicitly stating that region_name, access_key,
secret_key, and endpoint_url are unused when backend is "gcs", without changing
the existing GCS options behavior.
In `@docs/xii/adr/ADR-0002-file-transport.md`:
- Around line 133-148: Update the “Download Transport” section of ADR-0002 to
explicitly identify facility covers and user-profile pictures as intentional
public exceptions served by unauthenticated routes. Clarify that all file and
report downloads continue to require authenticated CARE endpoints, preserving
the existing authenticated download contract for those resources.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 743b46be-eaff-4220-b5ac-4395571c0d63
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (53)
Pipfilecare/emr/api/viewsets/file_assets.pycare/emr/api/viewsets/file_upload.pycare/emr/api/viewsets/report/report_upload.pycare/emr/models/file_upload.pycare/emr/models/report/report_upload.pycare/emr/reports/context_builder/data_points/fileupload.pycare/emr/reports/report_utils.pycare/emr/resources/file_upload/spec.pycare/emr/resources/report/report_upload/spec.pycare/emr/tasks/cleanup_incomplete_file_uploads.pycare/emr/tests/test_file_upload_api.pycare/emr/tests/test_file_upload_multipart.pycare/emr/tests/test_storage.pycare/emr/tests/test_storage_transport.pycare/emr/utils/file_download.pycare/emr/utils/file_manager.pycare/facility/models/facility.pycare/users/models.pycare/utils/csp/__init__.pycare/utils/csp/config.pycare/utils/file_uploads/cover_image.pycare/utils/tests/test_storage_config.pyconfig/api_router.pyconfig/settings/base.pyconfig/settings/config.pyconfig/storage.pydocs/xii/adr/ADR-0001-django-storage.mddocs/xii/adr/ADR-0002-file-transport.mddocs/xii/adr/ADR-0003-asynchronous-execution.mddocs/xii/adr/ADR-0004-configurable-application-cache.mddocs/xii/adr/ADR-0005-distributed-locking.mddocs/xii/adr/ADR-0006-portable-runtime-profiles.mddocs/xii/adr/ADR-0007-terraform-for-GCP.mddocs/xii/adr/ADR-0008-automated-continuous-integration.mddocs/xii/architecture/00-scope-and-goals.mddocs/xii/architecture/01-current-runtime.mddocs/xii/architecture/02-target-runtime.mddocs/xii/architecture/03-migration-plan.mddocs/xii/architecture/04-testing.mddocs/xii/architecture/05-upstream-sync.mddocs/xii/architecture/06-operations.mddocs/xii/architecture/07-configuration-reference.mddocs/xii/architecture/inventory/cache-and-redis.mddocs/xii/architecture/inventory/frontend-file-flow.mddocs/xii/architecture/inventory/plugin-impact.mddocs/xii/architecture/inventory/runtime-and-deployment.mddocs/xii/architecture/inventory/storage-call-sites.mddocs/xii/architecture/inventory/task-call-sites.mddocs/xii/architecture/inventory/unresolved-items.mddocs/xii/implementation/ES-01-storage.mddocs/xii/implementation/ES-02-file-transport-modernization.mddocs/xii/prompts/00-Complete-CARE-Runtime-Inventory
💤 Files with no reviewable changes (1)
- care/utils/csp/config.py
Code: - Public asset routes send a long-lived immutable Cache-Control. upload_cover_image mints a key per upload, so the bytes behind a key never change. Without this CARE serves every avatar view itself, which reading the bucket directly did not. - FileUploadRetrieveSpec returns download_url only once upload_completed is set, and the download action refuses an incomplete row. A row exists before its bytes do, so the route could previously only 404. - Annotate S3FilesManager._LEGACY_ALIASES as ClassVar (RUF012). - Record that build_object_storage drops the S3 credential and endpoint arguments under gcs, and that _ROLE_BASED_BUCKET suppresses endpoint_url along with the key and secret. Docs, where they contradicted the code: - ADR-0001 still described transport as unchanged and listed presigned upload and download as future work; the IS-01 completion pass removed both. Only the base64 upload is left for IS-02. - ADR-0002 promised authenticated downloads while two anonymous public asset routes exist. Records them as a bounded exception. - 02-target-runtime makes the botocore ClientError retry gap a requirement rather than a note: under gcs the policy cannot fire, so a transient failure fails the report on its first attempt. - scope-and-goals drops signed uploads and direct boto3 as reasons for a custom adapter, replaces the signed-URL expiry rule with CARE-mediated reads, and marks distributed locking undecided -- advisory locks are session-scoped where CARE's lock is TTL-scoped. - storage-call-sites totals were computed over an inconsistent denominator (11/2/6); now 11/2/8 over 21 sites, members listed. Drops the claim that the care/utils/csp resolvers survive; they are deleted. - task-call-sites separates 3 always-async tasks from 1 mixed-dispatch, and narrows "retries produce orphan rows" to the actual window. - runtime-and-deployment flags ssl_cert_reqs=none as a defect, and corrects the now-false django-storages and GCP-reference records. - unresolved-items closes C2, updates C10, sharpens S2. - 01-current-runtime gets a scoping note rather than a rewrite: it is a pinned baseline, and rewriting it would erase the reference point. Mechanical: 54 stale docs/gcp paths -> docs/xii/architecture; config.settings.gcp -> config.settings.deployment (no such module); invented S3_* variables in the MinIO profile -> the real BUCKET_* names; filesystem removed from the test storage profile; CARE_TASK_BACKEND=fake marked test-only; terraform fmt -recursive with per-environment validate; the pre-reset check compares against local develop, which is what the reset destroys; 12 unlabeled code fences labelled. Co-Authored-By: Claude <noreply@anthropic.com>
Code: - Public asset routes send a long-lived immutable Cache-Control. upload_cover_image mints a key per upload, so the bytes behind a key never change. Without this CARE serves every avatar view itself, which reading the bucket directly did not. - FileUploadRetrieveSpec returns download_url only once upload_completed is set, and the download action refuses an incomplete row. A row exists before its bytes do, so the route could previously only 404. - Annotate S3FilesManager._LEGACY_ALIASES as ClassVar (RUF012). - Record that build_object_storage drops the S3 credential and endpoint arguments under gcs, and that _ROLE_BASED_BUCKET suppresses endpoint_url along with the key and secret. Docs, where they contradicted the code: - ADR-0001 still described transport as unchanged and listed presigned upload and download as future work; the IS-01 completion pass removed both. Only the base64 upload is left for IS-02. - ADR-0002 promised authenticated downloads while two anonymous public asset routes exist. Records them as a bounded exception. - 02-target-runtime makes the botocore ClientError retry gap a requirement rather than a note: under gcs the policy cannot fire, so a transient failure fails the report on its first attempt. - scope-and-goals drops signed uploads and direct boto3 as reasons for a custom adapter, replaces the signed-URL expiry rule with CARE-mediated reads, and marks distributed locking undecided -- advisory locks are session-scoped where CARE's lock is TTL-scoped. - storage-call-sites totals were computed over an inconsistent denominator (11/2/6); now 11/2/8 over 21 sites, members listed. Drops the claim that the care/utils/csp resolvers survive; they are deleted. - task-call-sites separates 3 always-async tasks from 1 mixed-dispatch, and narrows "retries produce orphan rows" to the actual window. - runtime-and-deployment flags ssl_cert_reqs=none as a defect, and corrects the now-false django-storages and GCP-reference records. - unresolved-items closes C2, updates C10, sharpens S2. - 01-current-runtime gets a scoping note rather than a rewrite: it is a pinned baseline, and rewriting it would erase the reference point. Mechanical: 54 stale docs/gcp paths -> docs/xii/architecture; config.settings.gcp -> config.settings.deployment (no such module); invented S3_* variables in the MinIO profile -> the real BUCKET_* names; filesystem removed from the test storage profile; CARE_TASK_BACKEND=fake marked test-only; terraform fmt -recursive with per-environment validate; the pre-reset check compares against local develop, which is what the reset destroys; 12 unlabeled code fences labelled. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/xii/architecture/02-target-runtime.md (1)
1604-1617: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDocument
CARE_STORAGE_BACKENDin the environment reference.The storage architecture defines
CARE_STORAGE_BACKENDas the selector fors3andgcs, withs3as the default. The core environment-variable list omits it. AddCARE_STORAGE_BACKEND=s3|gcsso operators can configure the documented GCS profile without searching other documents.🤖 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 `@docs/xii/architecture/02-target-runtime.md` around lines 1604 - 1617, Add CARE_STORAGE_BACKEND=s3|gcs to the core environment-variable reference alongside the other CARE_* backend selectors, documenting the default as s3 so operators can select GCS.docs/xii/adr/ADR-0002-file-transport.md (1)
54-58: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winLimit the authentication exception to downloads.
Line 54 can be read as allowing unauthenticated uploads for public asset classes. Later text correctly requires authentication for asset writes. State this explicitly: all uploads require authentication; only downloads of facility cover images and user profile pictures may be unauthenticated.
🤖 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 `@docs/xii/adr/ADR-0002-file-transport.md` around lines 54 - 58, Update the file transport requirements under Authorization so every upload explicitly requires authentication, while only downloads of facility cover images and user profile pictures may be unauthenticated. Preserve the existing CARE mediation and Django Storage API requirements.
🤖 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 `@docs/xii/architecture/inventory/task-call-sites.md`:
- Line 120: Update the idempotency statement near the ReportUpload flow to
narrow the retry claim: do not state that every retry duplicates both the row
and object. Explain that a normal ClientError from put_object triggers row
deletion, while an ambiguous storage-write failure can leave an orphan object
before a retry creates a new row and object; preserve the existing references to
report_utils.py:102 and report_utils.py:130.
---
Outside diff comments:
In `@docs/xii/adr/ADR-0002-file-transport.md`:
- Around line 54-58: Update the file transport requirements under Authorization
so every upload explicitly requires authentication, while only downloads of
facility cover images and user profile pictures may be unauthenticated. Preserve
the existing CARE mediation and Django Storage API requirements.
In `@docs/xii/architecture/02-target-runtime.md`:
- Around line 1604-1617: Add CARE_STORAGE_BACKEND=s3|gcs to the core
environment-variable reference alongside the other CARE_* backend selectors,
documenting the default as s3 so operators can select GCS.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 75142627-3c14-49e9-939c-cc9b42daa4ee
📒 Files selected for processing (27)
care/emr/api/viewsets/file_assets.pycare/emr/api/viewsets/file_upload.pycare/emr/resources/file_upload/spec.pycare/emr/tests/test_file_upload_multipart.pycare/emr/tests/test_storage_transport.pycare/emr/utils/file_manager.pyconfig/settings/base.pyconfig/storage.pydocs/xii/adr/ADR-0001-django-storage.mddocs/xii/adr/ADR-0002-file-transport.mddocs/xii/architecture/00-scope-and-goals.mddocs/xii/architecture/01-current-runtime.mddocs/xii/architecture/02-target-runtime.mddocs/xii/architecture/03-migration-plan.mddocs/xii/architecture/04-testing.mddocs/xii/architecture/05-upstream-sync.mddocs/xii/architecture/06-operations.mddocs/xii/architecture/07-configuration-reference.mddocs/xii/architecture/inventory/cache-and-redis.mddocs/xii/architecture/inventory/frontend-file-flow.mddocs/xii/architecture/inventory/plugin-impact.mddocs/xii/architecture/inventory/runtime-and-deployment.mddocs/xii/architecture/inventory/storage-call-sites.mddocs/xii/architecture/inventory/task-call-sites.mddocs/xii/architecture/inventory/unresolved-items.mddocs/xii/implementation/ES-02-file-transport-modernization.mddocs/xii/prompts/00-Complete-CARE-Runtime-Inventory
🚧 Files skipped from review as they are similar to previous changes (19)
- care/emr/resources/file_upload/spec.py
- docs/xii/architecture/inventory/cache-and-redis.md
- docs/xii/architecture/06-operations.md
- care/emr/tests/test_storage_transport.py
- docs/xii/architecture/01-current-runtime.md
- docs/xii/prompts/00-Complete-CARE-Runtime-Inventory
- care/emr/tests/test_file_upload_multipart.py
- docs/xii/architecture/07-configuration-reference.md
- docs/xii/architecture/04-testing.md
- care/emr/utils/file_manager.py
- care/emr/api/viewsets/file_assets.py
- docs/xii/architecture/05-upstream-sync.md
- config/settings/base.py
- docs/xii/architecture/inventory/storage-call-sites.md
- config/storage.py
- docs/xii/architecture/inventory/plugin-impact.md
- docs/xii/architecture/inventory/frontend-file-flow.md
- docs/xii/implementation/ES-02-file-transport-modernization.md
- docs/xii/architecture/03-migration-plan.md
response_content was defined identically in two test modules and inlined as b"".join(...) in three more places. Move it to care/utils/tests/base so all five read the same way. Co-Authored-By: Claude <noreply@anthropic.com>
…ages # Conflicts: # care/emr/api/viewsets/file_upload.py # docs/xii/adr/ADR-0002-file-transport.md # docs/xii/architecture/inventory/frontend-file-flow.md
The line numbers carried over from the pre-IS-01 document and no longer matched report_utils.py: internal_name is at 103, the put_object at 127-129, the cleanup delete at 133, LOCK_DURATION at 21. Also sharpens the retry story. Saying "retries produce orphan rows" conflated two different things: autoretry_for fires only on ClientError from put_object, and that path deletes the row before re-raising, so a retry does not accumulate rows. What it can accumulate is objects -- a write ambiguous enough to raise after the bytes landed is never cleaned up, and the retry writes another under a fresh key. Orphan rows need a failure between the row save and the storage write. Co-Authored-By: Claude <noreply@anthropic.com>
Merging the storage branch into the transport branch left documents written against the pre-ES-02 tree describing code that had already changed underneath them. - ADR-0001 listed base64 upload as still outstanding and left the IS-02 checkbox open. ES-02 delivered it; the persistence seam is unchanged. - unresolved-items C10 claimed the upload endpoint takes base64 and carries no @extend_schema. It is multipart and annotated. Closed, and the stale line reference (230) corrected to 294. - frontend-file-flow now states its own layering: 1-10 baseline, 11 the IS-01 record, 12 the current contract. The IS-02 rows in the 11.4 table are marked closed rather than rewritten, so the IS-01 record survives. - storage-call-sites section 11 opened by repeating a prediction the completion pass had already falsified. Co-Authored-By: Claude <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
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.
This PR modernizes CARE's file storage implementation by adopting Django's Storage API and simplifying the HTTP file transport contract, according to #3731
The objective is to reduce provider-specific code while preserving local MinIO compatibility and enabling additional storage backends through configuration.
What changed
Storage
django-storagesFile transport
FileResponseBenefits
Local development
The existing Docker development workflow continues using MinIO through
S3Storage.No cloud provider is required for local development.
Breaking changes
This PR changes the file API contract.
Removed:
signed_urlread_signed_urlAdded:
download_url)The frontend must be updated accordingly.
Out of scope
This PR intentionally does not include:
Those are planned as separate architectural phases.
Testing
The implementation has been validated with:
Notes
The architectural work was intentionally split into independent phases.
This PR contains only the storage and file-transport modernization to keep review focused.
Merge Checklist
/docsSummary by CodeRabbit
New Features
Bug Fixes
Documentation