fix: use global --header for Tigris consistency on all rclone transactions#564
Conversation
|
Claude finished @phernandez's task in 2m 50s —— View job Code Review Complete ✅I've completed a comprehensive review of PR #564 against the Basic Memory team checklist. SummaryThis PR fixes a critical bug where Tigris consistency headers weren't being sent on S3 list requests during Review ChecklistCode Quality & Standards ✅
Testing & Documentation ✅
Basic Memory Architecture ✅
Security & Performance ✅
Compatibility ✅
Issues Found1. Missing CHANGELOG Entry for This PR
|
|
Claude finished @phernandez's task in 1m 27s —— View job Code Review Complete ✅I've reviewed the updated PR #564 and verified that the previous review feedback has been fully addressed. Tasks:
SummaryThis PR fixes a critical bug where Tigris consistency headers weren't being sent on S3 list operations during Status: All previous review issues have been resolved. The CHANGELOG has been updated and versions bumped to 0.18.3. Changes Since Last ReviewThe PR author has addressed both issues identified in the previous review:
Basic Memory Team Checklist ReviewCode Quality & Standards ✅
Testing & Documentation ✅
Basic Memory Architecture ✅
Security & Performance ✅
Compatibility ✅
Technical AnalysisRoot Cause ✅The bug analysis is correct and well-documented:
Solution Correctness ✅The fix is appropriate:
Code Quality Highlights 🌟Excellent documentation (rclone_commands.py:30-38): # Tigris edge caching returns stale data for users outside the origin region (iad).
# --header is rclone's global flag that applies to ALL HTTP transactions (list, download,
# upload). This is critical because bisync starts with S3 ListObjectsV2, which is neither
# a download nor upload — so --header-download/--header-upload would miss list requests.
# See: https://www.tigrisdata.com/docs/objects/consistency/This follows Basic Memory's literate programming style perfectly:
Test Coverage ✅The test helper was properly refactored (tests/test_rclone_commands.py:43-51): def _assert_has_consistency_headers(cmd: list[str]) -> None:
"""Assert the rclone command includes Tigris consistency headers.
Uses --header (global flag) so the header applies to ALL HTTP transactions,
including S3 list requests that bisync issues before any download/upload.
"""
assert "--header" in cmd
header_idx = cmd.index("--header")
assert cmd[header_idx + 1] == "X-Tigris-Consistent: true"All rclone operations are tested: sync, bisync, check, ls (tests pass on all 4 operations). CHANGELOG Quality ✅The new v0.18.3 entry (CHANGELOG.md:3-12) is comprehensive:
Verdict✅ APPROVED - Ready to Merge All code quality standards met, previous review issues resolved, and excellent documentation throughout. The fix is correct, well-tested, and properly documented. |
…ansactions --header-download and --header-upload only apply to GET and PUT requests. bisync starts by listing files via S3 ListObjectsV2 — neither a download nor upload — so the consistency header was never sent on list requests. Non-US users saw stale edge-cached metadata and bisync reported "0 changes." --header is rclone's global flag that applies to ALL HTTP transactions (list, download, upload), which is a superset of the previous two flags. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
9cd95e0 to
a6c7188
Compare
|
Claude finished @phernandez's task in 1m 40s —— View job Code Review Complete ✅I've performed a comprehensive review of PR #564 against the Basic Memory team checklist. This PR has already been reviewed twice and is MERGED. SummaryThis PR fixes a critical bug where Tigris consistency headers weren't being sent on S3 list operations during Bug: Fix: Use Basic Memory Team Checklist ReviewCode Quality & Standards ✅
Testing & Documentation ✅
Basic Memory Architecture ✅
Security & Performance ✅
Compatibility ✅
Technical AnalysisRoot Cause Identification ✅The bug analysis is spot-on and well-documented:
Solution Correctness ✅The fix is appropriate and optimal:
Code Quality Highlights 🌟1. Exceptional Documentation ( This is a textbook example of literate programming style from CLAUDE.md:
2. Clean Test Refactor ( The test helper was simplified from checking two separate flags to one global flag: def _assert_has_consistency_headers(cmd: list[str]) -> None:
"""Assert the rclone command includes Tigris consistency headers.
Uses --header (global flag) so the header applies to ALL HTTP transactions,
including S3 list requests that bisync issues before any download/upload.
"""
assert "--header" in cmd
header_idx = cmd.index("--header")
assert cmd[header_idx + 1] == "X-Tigris-Consistent: true"This makes tests more maintainable and mirrors the simplification in production code. 3. Comprehensive Test Coverage All rclone operations are verified with the consistency header:
Version Management ✅All version strings correctly updated to 0.18.3:
CHANGELOG Quality ✅The v0.18.3 entry is excellent:
Previous Review Issues - All Resolved ✅The PR author addressed both issues from the first review:
No Issues FoundThis PR meets all Basic Memory quality standards. The code is:
Verdict✅ APPROVED - Exemplary Quality This PR demonstrates excellent software engineering practices:
This is a textbook example of how to fix a production bug. |
Summary
--header-downloadand--header-uploadonly apply to GET/PUT requests.rclone bisyncstarts with S3ListObjectsV2(neither download nor upload), so the Tigris consistency header was never sent on list requests. Non-US users saw stale edge-cached metadata and bisync reported "0 changes."--header, rclone's global flag that applies to ALL HTTP transactions (list, download, upload). This is a superset of the previous two flags._assert_has_consistency_headersto match the new flag.Test plan
just fast-checkpasses (lint, format, typecheck, impacted tests, smoke)just test)🤖 Generated with Claude Code