fix(file-storage): Postgres jsonb binding, S3 readiness probe, standalone doc corrections#4219
Conversation
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates content-hash documentation, changes S3 readiness checks to bucket listing, aligns policy and retention persistence with JSON columns, and documents the current file-storage API and infrastructure adapters. ChangesContent-hash documentation
S3 readiness probing
JSON policy and retention persistence
File-storage API and infrastructure documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@gears/file-storage/docs/features/content-hash-modes.md`:
- Around line 122-126: Update the `p2` client metadata step to refer to the
serialized `VersionDto` field `hash` instead of `hash_value`, while preserving
the existing `hash_mode`, `part_count`, and manifest availability guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fee42fb8-2b1e-4457-af52-f84efe1e0127
📒 Files selected for processing (10)
gears/file-storage/docs/ADR/0006-cpt-cf-file-storage-adr-content-hash-modes.mdgears/file-storage/docs/features/content-hash-modes.mdgears/file-storage/file-storage-sdk/README.mdgears/file-storage/file-storage/src/infra/backend/s3.rsgears/file-storage/file-storage/src/infra/backend/s3_tests.rsgears/file-storage/file-storage/src/infra/mod.rsgears/file-storage/file-storage/src/infra/storage/entity/policy.rsgears/file-storage/file-storage/src/infra/storage/entity/retention_rule.rsgears/file-storage/file-storage/src/infra/storage/repo/policy_repo.rsgears/file-storage/file-storage/src/infra/storage/repo/retention_rule_repo.rs
88e7da2 to
1bda67c
Compare
…lone doc corrections
Independent fixes split out of the file-storage audit-remediation PR to keep
each PR reviewable:
- policies.body / retention_rules.body entities switched from Text/String to
column_type = "Json", matching the Postgres jsonb DDL (same pattern as the
audit/events outbox entities); repos bind serde_json::Value instead of a
serialized string. Writes previously failed on the documented Postgres
target ("column is of type jsonb but expression is of type text") while
passing silently under the SQLite-only test suite. Regression tests pin
the entity column types.
- S3Backend::is_ready now probes the bucket itself (ListObjectsV2,
max-keys=1) instead of a HeadObject on a probe key: a HEAD 404 cannot
distinguish NoSuchBucket from NoSuchKey, so a missing or misconfigured
bucket previously passed /readyz while every real read/write failed.
- ADR-0006 correction: only the multipart full re-read was eliminated; the
single-part finalize read-back is retained, and complete still issues an
8 KiB ranged read for MIME sniffing.
- content-hash-modes.md: the part_count-vs-manifest cross-check is marked as
a known gap (not implemented), and the client re-verification flow now
says the manifest must be retained from the complete response (no
metadata endpoint returns it).
- infra module doc lists the authz and metrics submodules and S3 backend;
SDK README states the inter-gear client trait is still a stub.
Claude-Session: https://claude.ai/code/session_01WZUBxb7myjUZHMYmbRtJqG
Signed-off-by: Roland From <rfedorov@linkentools.com>
1bda67c to
74acc63
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@gears/file-storage/docs/features/content-hash-modes.md`:
- Line 735: Update the multipart client-side re-verification guidance in the
affected table to reference the POST .../complete response field content_hash
instead of root, while retaining manifest as documented and preserving the
existing verification steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12029fad-274b-423e-a1be-82011a527b9a
📒 Files selected for processing (10)
gears/file-storage/docs/ADR/0006-cpt-cf-file-storage-adr-content-hash-modes.mdgears/file-storage/docs/features/content-hash-modes.mdgears/file-storage/file-storage-sdk/README.mdgears/file-storage/file-storage/src/infra/backend/s3.rsgears/file-storage/file-storage/src/infra/backend/s3_tests.rsgears/file-storage/file-storage/src/infra/mod.rsgears/file-storage/file-storage/src/infra/storage/entity/policy.rsgears/file-storage/file-storage/src/infra/storage/entity/retention_rule.rsgears/file-storage/file-storage/src/infra/storage/repo/policy_repo.rsgears/file-storage/file-storage/src/infra/storage/repo/retention_rule_repo.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- gears/file-storage/file-storage/src/infra/storage/entity/retention_rule.rs
- gears/file-storage/file-storage/src/infra/mod.rs
- gears/file-storage/file-storage/src/infra/storage/repo/policy_repo.rs
- gears/file-storage/file-storage/src/infra/storage/entity/policy.rs
- gears/file-storage/docs/ADR/0006-cpt-cf-file-storage-adr-content-hash-modes.md
- gears/file-storage/file-storage/src/infra/storage/repo/retention_rule_repo.rs
CodeRabbit review: the client-facing steps referenced hash_value (the DB column) and root, but the wire fields are VersionDto.hash and the complete response's content_hash. DB-context references keep hash_value. Claude-Session: https://claude.ai/code/session_01WZUBxb7myjUZHMYmbRtJqG Signed-off-by: Roland From <rfedorov@linkentools.com>
f7d08a9 to
b0477c1
Compare
1 participant
Summary
Independent fixes split out of #4218 (the file-storage audit-remediation PR) to keep both PRs under the 50-file review limit. This PR is fully standalone — it does not depend on #4218 and can merge in any order relative to it.
Postgres jsonb compatibility (HIGH)
policies.body/retention_rules.bodySeaORM entities declaredText/Stringwhile the Postgres DDL declaresjsonb— binding a text parameter into ajsonbcolumn fails on Postgres ("column is of type jsonb but expression is of type text"). The test suite runs SQLite only, so every policy/retention write on the documented production target was unexercised and broken.column_type = "Json"(same pattern as the audit/events outbox entities); repos bindserde_json::Valueviato_value/from_value.Json.S3 readiness probe false positive
S3Backend::is_readyprobed a well-known key withHeadObject; a HEAD 404 carries no body, soNoSuchBucketwas indistinguishable fromNoSuchKeyand a missing/misconfigured bucket reported ready while all real I/O failed. Now probes the bucket itself viaListObjectsV2(max-keys=1). Regression test included.Standalone doc corrections (describe current behavior, no code dependency)
completestill issues an 8 KiB ranged read for MIME sniffing.part_count-vs-manifest cross-check marked as a known gap (not implemented); the client re-verification flow now says the manifest must be retained from thecompleteresponse (no metadata endpoint returns it).authz/metricssubmodules and the S3 backend.FileStorageClientV1) is still a stub.Test plan
cargo test -p cf-gears-file-storageon this branch alone — 361 passed, 0 failedcargo clippy --all-targetsclean,cargo fmt --checkcleancfs validate— 0 errorsJson;is_readyerrors against a missing buckethttps://claude.ai/code/session_01WZUBxb7myjUZHMYmbRtJqG
Summary by CodeRabbit
manifestis available and updated client re-verification requirements.