ci(gha): switch to custom gha which allows 0.36 of kubo installation #76
Conversation
WalkthroughUpdated CI IPFS setup/version, strengthened KuboCAS httpx client lifecycle and cleanup across event-loop states, added read-only wrapper APIs for HAMT stores, pinned zarr to 3.0.9, and revised/expanded IPFS- and lifecycle-related tests for robustness and timeout/error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant KuboCAS
participant EventLoop as "Event Loop / asyncio"
participant httpx as "httpx.AsyncClient (per-loop)"
Note over User,KuboCAS: Normal usage
User->>KuboCAS: create(instance, optional client)
KuboCAS->>EventLoop: determine current loop
KuboCAS->>httpx: create per-loop client if owned & missing
httpx-->>KuboCAS: client cached per loop
Note over User,KuboCAS: Closing/cleanup paths
User->>KuboCAS: await aclose()
alt KuboCAS owns clients
KuboCAS->>httpx: close all per-loop clients (async)
KuboCAS-->>KuboCAS: clear mapping, set _closed=True
else external client provided
KuboCAS-->>User: return immediately (no-op)
end
Note over EventLoop,KuboCAS: Interpreter shutdown / __del__
EventLoop->>KuboCAS: GC invokes __del__()
alt running loop exists
KuboCAS->>EventLoop: schedule aclose() via create_task
else no running loop
KuboCAS->>KuboCAS: try asyncio.run(aclose()) or clear refs synchronously
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/run-checks.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Create project environment, run all checks (3.12)
🔇 Additional comments (1)
.github/workflows/run-checks.yaml (1)
42-42: Verifyipfs_version: "0.36.0"actually exists in Kubo releasesThe latest official Kubo releases are currently well below 0.36.x. If the action cannot locate this version the workflow will fail. Double-check the intended artifact name/version or update to a valid release.
| uses: Faolain/setup-ipfs@v0.7.0 | ||
| with: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pin the setup-ipfs action to a commit SHA to mitigate supply-chain risk
Using a moving Git tag (v0.7.0) means the referenced code can change without notice.
For reproducible, tamper-resistant builds, pin to the exact commit hash published for that tag and optionally add the tag as an inline comment:
- uses: Faolain/setup-ipfs@v0.7.0
+ uses: Faolain/setup-ipfs@0912abdf4823f4b7a724ddd9c2ad2fcd1b9c9c5d # v0.7.0🏁 Script executed:
#!/bin/bash
# Fetch the commit SHA for the v0.7.0 tag of the setup-ipfs action
curl -s https://api.github.com/repos/Faolain/setup-ipfs/git/refs/tags/v0.7.0 \
| sed -n 's/.*"sha": *"\([0-9a-f]\{40\}\)".*/\1/p'Length of output: 170
Pin setup-ipfs to a specific commit SHA
To ensure tamper-resistant and reproducible workflows, replace the moving tag with the exact commit SHA for v0.7.0:
Files to update:
- .github/workflows/run-checks.yaml (around lines 40–41)
Suggested diff:
- uses: Faolain/setup-ipfs@v0.7.0
+ uses: Faolain/setup-ipfs@f1d58ed0d37251a9e6cea86fe0386a56d9dfbd5a # v0.7.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: Faolain/setup-ipfs@v0.7.0 | |
| with: | |
| uses: Faolain/setup-ipfs@f1d58ed0d37251a9e6cea86fe0386a56d9dfbd5a # v0.7.0 | |
| with: |
🤖 Prompt for AI Agents
In .github/workflows/run-checks.yaml around lines 40 to 41, the action
'Faolain/setup-ipfs@v0.7.0' uses a moving tag which can lead to non-reproducible
workflows. Replace the version tag 'v0.7.0' with the exact commit SHA
corresponding to that release to pin the action to a specific commit, ensuring
tamper resistance and reproducibility.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 629 673 +44
=========================================
+ Hits 629 673 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_public_gateway.py (3)
147-167: Good approach to ensure content exists before testing.The pattern of saving content first is solid. Consider whether the 1-second sleep is necessary or if it could be reduced to improve test speed.
268-285: Consider using pytest.mark.parametrize for test cases.While the current implementation is correct, using pytest's parametrize decorator would make the tests more maintainable and provide better failure reporting.
-async def test_fix_kubocas_load(): +@pytest.mark.parametrize("input_url,expected_base", [ + ("http://127.0.0.1:8080", "http://127.0.0.1:8080/ipfs/"), + ("http://127.0.0.1:8080/", "http://127.0.0.1:8080/ipfs/"), + ("https://ipfs.io", "https://ipfs.io/ipfs/"), + ("https://ipfs.io/", "https://ipfs.io/ipfs/"), + ("https://gateway.ipfs.io/ipfs/", "https://gateway.ipfs.io/ipfs/"), +]) +async def test_kubocas_url_construction(input_url, expected_base): + """Test URL construction with various gateway configurations""" + cas = KuboCAS(rpc_base_url="http://127.0.0.1:5001", gateway_base_url=input_url) + assert cas.gateway_base_url == expected_base, ( + f"URL construction failed for {input_url}" + ) + await cas.aclose() + + +async def test_kubocas_load_functionality(): """Test URL construction and loading behavior of KuboCAS""" - - # Test URL construction with various gateway configurations - test_cases = [ - ("http://127.0.0.1:8080", "http://127.0.0.1:8080/ipfs/"), - ("http://127.0.0.1:8080/", "http://127.0.0.1:8080/ipfs/"), - ("https://ipfs.io", "https://ipfs.io/ipfs/"), - ("https://ipfs.io/", "https://ipfs.io/ipfs/"), - ("https://gateway.ipfs.io/ipfs/", "https://gateway.ipfs.io/ipfs/"), - ] - - for input_url, expected_base in test_cases: - cas = KuboCAS(rpc_base_url="http://127.0.0.1:5001", gateway_base_url=input_url) - assert cas.gateway_base_url == expected_base, ( - f"URL construction failed for {input_url}" - ) - await cas.aclose() - # Test actual loading with local gateway cas = KuboCAS( rpc_base_url="http://127.0.0.1:5001", gateway_base_url="http://127.0.0.1:8080"
83-83: Consider a more deterministic approach to waiting for IPFS content availability.The tests use various sleep delays (0.5s to 1s) after saving content. While this works, consider implementing a retry mechanism or polling approach to make tests more reliable and potentially faster.
Would you like me to suggest a utility function that polls for content availability instead of using fixed delays?
Also applies to: 163-163, 229-229, 297-297
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_public_gateway.py(5 hunks)
🔇 Additional comments (5)
tests/test_public_gateway.py (5)
8-14: Good addition of module-level documentation.The docstring clearly explains the test environment constraints and requirements, which will help future maintainers understand why tests are structured this way.
16-18: Appropriate use of well-known test CID.Good practice to use a known IPFS example CID for public gateway tests, with clear documentation about potential availability issues.
20-24: Excellent improvements to error handling and timeout management.The addition of configurable timeouts, specific timeout exception handling, and safe content preview for short content significantly improves test robustness and debugging capabilities.
Also applies to: 32-32, 41-43, 58-61
71-86: Well-structured gateway testing with appropriate failure handling.The approach of creating local content first and using a
must_succeedflag for each gateway is excellent. This ensures reliable testing of the local gateway while being tolerant of public gateway failures.Also applies to: 88-104, 107-114, 117-140
223-262: Comprehensive test for trailing slash handling with excellent error coverage.The test thoroughly validates URL construction and includes appropriate error handling for various CI environment scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_public_gateway.py (2)
16-17: Consider documenting the source of the well-known CID.While using a well-known CID is a good approach, consider adding a comment about where this CID comes from or what content it represents for better maintainability.
-# Well-known test CID from IPFS examples (may or may not be available) +# Well-known test CID from IPFS examples - "hello world" content (may or may not be available)
20-65: Robust error handling and timeout management implementation.The function properly handles timeouts and exceptions, with good debugging output. The timeout parameter addition and graceful error handling are excellent improvements.
However, consider making the timeout handling more specific:
- except httpx.TimeoutException: - return {"url": url, "error": "Timeout"} + except httpx.TimeoutException as e: + return {"url": url, "error": f"Timeout after {timeout}s: {str(e)}"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_public_gateway.py(5 hunks)
🔇 Additional comments (12)
tests/test_public_gateway.py (12)
8-14: Excellent documentation for CI environment context.The module docstring clearly explains the testing environment constraints, which is crucial for understanding why tests are structured this way.
41-43: Safe handling of content display with length check.Good defensive coding to handle both short and long content when displaying debug information.
71-86: Proper setup of local content for reliable testing.Creating local content before testing retrieval is the right approach for CI environments. The async sleep gives IPFS time to process the content.
88-104: Well-structured gateway test configuration.The tuple structure with name and required flag is clean and allows for flexible testing of both reliable and unreliable gateways.
107-141: Comprehensive result processing with proper assertions.The test correctly distinguishes between required and optional gateways, provides detailed output, and fails appropriately when required gateways don't work.
147-210: Intelligent approach to public gateway testing.Using the local gateway as a "public" gateway for testing while gracefully handling actual public gateway failures is a smart strategy for CI reliability.
197-204: Excellent error handling strategy for unreliable services.The selective skipping of tests for public gateways while still failing for local gateway issues strikes the right balance between test reliability and thoroughness.
223-246: Thorough testing of URL construction with trailing slashes.The test properly verifies both functionality and URL formatting, which is important for gateway URL handling.
247-262: Comprehensive exception handling for various failure modes.The multiple exception handlers cover different scenarios that can occur in CI environments, preventing flaky test failures.
271-284: Thorough URL construction testing.The test cases cover various input formats and verify correct normalization, which is essential for reliable gateway communication.
304-305: Consistent error handling pattern.Good consistency in handling connection errors across all test functions.
83-83: Consider replacing fixed asyncio.sleep delays with a polling strategy
The hard-coded sleeps (0.5 s / 1.0 s) in the public-gateway tests may not reliably cover IPFS 0.36.0’s publication latency on all environments. Instead of tuning these magic numbers, you can:
- Poll the gateway until it returns a 200 (with a short timeout/retry backoff)
- Fail the test only after a maximum overall timeout is exceeded
Locations to update:
• tests/test_public_gateway.py:83 (await asyncio.sleep(0.5))
• tests/test_public_gateway.py:163 (await asyncio.sleep(1.0))
• tests/test_public_gateway.py:229 (await asyncio.sleep(0.5))Example diff sketch:
- await asyncio.sleep(0.5) # Give IPFS time to process + # Poll until the CID is available or timeout + await wait_for_availability(url, client, timeout=5.0, interval=0.2)Where
wait_for_availabilityhandles repeated GETs with backoff and raises on overall timeout.This approach decouples test stability from arbitrary sleep durations.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_async.py (1)
107-130: Consider adding tests for uncovered edge cases.While the test improvements are valuable, consider adding tests for the uncovered lines identified by the pipeline:
- Early object destruction (line 292): Test
__del__on a partially initialized object- Non-running loop with asyncio.run (line 317): Test when loop exists but isn't running
- Exception during cleanup (lines 318-322): Test when cleanup operations fail
Would you like me to generate additional test cases to achieve 100% coverage for these edge cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
py_hamt/store_httpx.py(4 hunks)tests/test_async.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_async.py (1)
py_hamt/store_httpx.py (3)
aclose(252-279)KuboCAS(75-352)_loop_client(231-247)
🪛 GitHub Actions: Triggered on push from Faolain to branch/tag ci/switch-setup-ipfs-gha
py_hamt/store_httpx.py
[error] 292-322: Coverage failure: total coverage 99% is less than required 100%. Missing coverage on lines 292, 317-322.
🔇 Additional comments (2)
py_hamt/store_httpx.py (2)
186-188: LGTM! Good lifecycle management setup.The initialization of instance variables for tracking client ownership and closure state is well-designed. Setting
_owns_client=Falsewhen an external client is provided correctly delegates cleanup responsibility.Also applies to: 220-220
258-267: Well-handled edge case for missing event loop.The addition properly handles scenarios where
aclose()is called without a running event loop (e.g., during interpreter shutdown or from synchronous contexts). Clearing references and marking as closed is the appropriate fallback behavior.
| if not hasattr(self, "_owns_client") or not hasattr(self, "_closed"): | ||
| return | ||
|
|
||
| if not self._owns_client or self._closed: | ||
| return | ||
|
|
||
| # Attempt proper cleanup if possible | ||
| try: | ||
| loop = asyncio.get_running_loop() | ||
| except RuntimeError: | ||
| loop = None | ||
| # No running loop - can't do async cleanup | ||
| # Just clear the client references synchronously | ||
| if hasattr(self, "_client_per_loop"): | ||
| # We can't await client.aclose() without a loop, | ||
| # so just clear the references | ||
| self._client_per_loop.clear() | ||
| self._closed = True | ||
| return | ||
|
|
||
| # If we get here, we have a running loop | ||
| try: | ||
| if loop is None or not loop.is_running(): | ||
| asyncio.run(self.aclose()) | ||
| else: | ||
| if loop.is_running(): | ||
| # Schedule cleanup in the existing loop | ||
| loop.create_task(self.aclose()) | ||
| else: | ||
| # Loop exists but not running - try asyncio.run | ||
| asyncio.run(self.aclose()) | ||
| except Exception: | ||
| # Suppress all errors during interpreter shutdown or loop teardown | ||
| pass | ||
| # If all else fails, just clear references | ||
| if hasattr(self, "_client_per_loop"): | ||
| self._client_per_loop.clear() | ||
| self._closed = True |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Comprehensive cleanup logic, but missing test coverage for edge cases.
The __del__ implementation handles multiple scenarios well, but the pipeline indicates missing coverage for:
- Line 292: Early return when attributes don't exist
- Lines 317-322: The
asyncio.runpath and exception fallback
Consider adding tests to cover these edge cases to meet the 100% coverage requirement.
To verify which specific scenarios need test coverage, run:
🏁 Script executed:
#!/bin/bash
# Check current test coverage for the __del__ method
rg -A 30 "def __del__" py_hamt/store_httpx.py
# Search for existing tests that might exercise __del__
rg -B 5 -A 10 "__del__" tests/Length of output: 3460
Address missing coverage in __del__ branches
Current tests exercise:
- The no-running-loop path (
asyncio.get_running_loop→RuntimeError) - The exception fallback when
asyncio.runfails
However, the coverage report still flags these untested branches:
- Early exit when neither
_owns_clientnor_closedexist - Scheduling cleanup via
loop.create_task(self.aclose())when inside an active loop - The successful
asyncio.run(self.aclose())path (loop exists but isn’t running, and no error)
Please add tests for:
- Calling
__del__on an instance with no_owns_client/_closedattributes - Invoking
__del__inside a runningasyncioloop to hitloop.create_task(...) - A clean
asyncio.run(self.aclose())execution (mockloop.is_running()→Falsewithout raising)
These will close the remaining coverage gaps.
🧰 Tools
🪛 GitHub Actions: Triggered on push from Faolain to branch/tag ci/switch-setup-ipfs-gha
[error] 292-322: Coverage failure: total coverage 99% is less than required 100%. Missing coverage on lines 292, 317-322.
🤖 Prompt for AI Agents
In py_hamt/store_httpx.py around lines 291 to 322, the __del__ method has
branches not covered by tests: early exit when _owns_client and _closed
attributes are missing, scheduling cleanup with loop.create_task(self.aclose())
when the event loop is running, and successful asyncio.run(self.aclose()) when
the loop exists but is not running. Add tests that create instances without
_owns_client/_closed attributes and call __del__, tests that run __del__ inside
an active asyncio loop to trigger loop.create_task, and tests that mock
loop.is_running() to return False and verify asyncio.run(self.aclose()) executes
cleanly. This will ensure full coverage of all __del__ branches.
| # Now mock no running loop for aclose | ||
| with unittest.mock.patch( | ||
| "asyncio.get_running_loop", side_effect=RuntimeError("No running loop") | ||
| ): | ||
| await cas.aclose() | ||
| # The aclose method should handle this gracefully | ||
| await cas2.aclose() |
There was a problem hiding this comment.
Test logic issue: Cannot await inside no-loop mock context.
The test attempts to await cas2.aclose() while mocking get_running_loop to raise RuntimeError. This creates a paradox - you need a running loop to execute await, but the mock simulates no loop being available.
Consider restructuring the test to properly simulate the no-loop scenario:
- # Now mock no running loop for aclose
- with unittest.mock.patch(
- "asyncio.get_running_loop", side_effect=RuntimeError("No running loop")
- ):
- # The aclose method should handle this gracefully
- await cas2.aclose()
+ # Test aclose in a synchronous context (simulating no event loop)
+ def test_sync_aclose():
+ cas3 = KuboCAS()
+ # Don't create any clients in this sync context
+ # Now patch get_running_loop only inside aclose
+ with unittest.mock.patch(
+ "asyncio.get_running_loop", side_effect=RuntimeError("No running loop")
+ ):
+ # Can't await in sync context, but we can call the coroutine
+ coro = cas3.aclose()
+ # The coroutine should complete without error when stepped through
+ try:
+ coro.send(None)
+ except StopIteration:
+ pass # Expected - coroutine completed
+
+ # Run the sync test
+ test_sync_aclose()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/test_async.py around lines 122 to 127, the test tries to await
cas2.aclose() while mocking asyncio.get_running_loop to raise RuntimeError,
which conflicts because awaiting requires a running event loop. To fix this,
restructure the test by separating the mocking of get_running_loop from the
await call, for example by running the await outside the mock context or using
an alternative approach to simulate no running loop without blocking the await
execution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_kubocas_session.py (1)
129-138: LGTM: Good edge case test for partial object construction.This test correctly verifies the early return guard in
__del__()when internal sentinel attributes are missing. The explicit dunder call with# noqa: B023is appropriate for testing destructor behavior.Consider also testing the missing
_closedattribute case for complete coverage:def test_del_missing_closed_attribute(): cas = KuboCAS() del cas._closed # test the other sentinel attribute cas.__del__() # should also return safely
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_kubocas_session.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_kubocas_session.py (1)
py_hamt/store_httpx.py (1)
KuboCAS(75-352)
🔇 Additional comments (2)
tests/test_kubocas_session.py (2)
3-3: LGTM: Import addition is necessary for new tests.The
unittestimport is required for theunittest.mock.Mockusage in the new test function.
144-188: Excellent comprehensive test for complex async cleanup logic.This test thoroughly exercises the
__del__method's branch where an event loop exists but is not running, and handles the exception case whenasyncio.run()fails. The test design is excellent:
- Clear section organization with descriptive comments
- Proper mocking of
asyncio.get_running_loop()andasyncio.run()- Injection of placeholder client to test cleanup behavior
- Comprehensive verification of all expected side effects
The test correctly validates that even when cleanup fails, the object is properly marked as closed and client references are cleared.
…cas-instance Allow KuboCAS to reopen after being closed
Ci/update zarr 3.0.9
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
tests/test_kubocas_session.py (1)
127-191: Good coverage of del edge branches (early guard and asyncio.run fallback)These tests exercise the “missing sentinel attrs” early return and the patched “loop exists but not running → asyncio.run(...) path,” aligning with the destructor’s defensive branches. This should close the previously reported coverage gaps.
🧹 Nitpick comments (12)
tests/testing_utils.py (2)
185-187: Kubo 0.36 cold starts can exceed 15s on CI; consider a slightly longer readiness windowSeen occasional >15s init times when the repo is created and plugins load. Bump attempts from 30 to 60 (30s) or make it env-configurable to deflake.
- for _ in range(30): # 30 attempts, 0.5s each = 15s max + # 60 attempts, 0.5s each = 30s max (helps with cold-starts on CI) + for _ in range(60):If you prefer configurability:
- for _ in range(30): # 30 attempts, 0.5s each = 15s max + timeout_s = float(os.getenv("IPFS_STARTUP_TIMEOUT_S", "30")) + attempts = int(timeout_s / 0.5) + for _ in range(attempts):
60-64: Close HTTP connections in probes to avoid leaking sockets in long sessionsThe probe helpers return early without closing the connection. Use a
try/finallyor context manager to close them. Minor but helps on socket-limited runners.- try: - conn = http.client.HTTPConnection(p.hostname, p.port, timeout=1) - conn.request("POST", "/api/v0/version") - return conn.getresponse().status == 200 - except Exception: - return False + conn = None + try: + conn = http.client.HTTPConnection(p.hostname, p.port, timeout=1) + conn.request("POST", "/api/v0/version") + return conn.getresponse().status == 200 + except Exception: + return False + finally: + try: + conn and conn.close() + except Exception: + passRepeat the same pattern for
_gw_is_up.Also applies to: 73-77
pyproject.toml (1)
12-12: Prefer a compatible range over a hard pin for a library dependencyPinning zarr to an exact version can create resolver conflicts for downstream consumers of this library. If 3.0.9 is required due to a specific regression/compatibilty need, consider documenting that in the commit message. Otherwise, a bounded range keeps you safe while allowing bugfixes.
Suggested change:
- "zarr==3.0.9", + "zarr>=3.0.9,<3.1",If the hard pin is needed for CI stability only, consider moving the pin to the workflow or a constraints file while keeping a compatible range here.
py_hamt/store_httpx.py (2)
269-287: aclose(): behavior is sound; consider logging suppressed exceptionsThe best-effort cleanup and state reset looks good. You might optionally add a debug log in the except to aid troubleshooting, but leaving it silent is acceptable for a destructor-adjacent path.
296-336: Unreachable asyncio.run branch due to get_running_loop semantics; simplify or switch to get_event_loopasyncio.get_running_loop() only returns when a loop is running. Therefore loop.is_running() is guaranteed True, making the “else: asyncio.run(...)” branch effectively unreachable in real executions (your tests hit it via monkeypatch). Consider simplifying:
- Either switch to asyncio.get_event_loop() (deprecated but returns even if not running) if you want to handle “loop exists but isn’t running”, or
- Drop the asyncio.run path to reduce complexity and rely on create_task() when a loop is running, otherwise fall back to clearing references.
No functional break, just reduced cognitive load.
If you keep the branch, retain the targeted test to ensure coverage remains 100%.
tests/test_kubocas_session.py (1)
231-245: Docstring is misleading; aclose() does not “fall back to .close()”aclose() never consults asyncio.get_running_loop and always awaits client.aclose() in the test’s async loop. The current docstring implies a sync fallback that does not exist here. Tweak wording to avoid confusion.
- """ - aclose() must fall back to .close() when no loop is running. - """ + """ + aclose() closes all internally-managed clients in the current async context. + """tests/test_zarr_ipfs.py (1)
195-213: Solid smoke test for read-only reopening via wrapperThis exercises the Zarr v3 helper path and validates the with_read_only wrapper indirectly via xr.open_zarr. Consider a small follow-up that explicitly validates wrapper idempotence:
wrapped = store_rw.with_read_only(True) assert wrapped.read_only is True assert wrapped is not store_rw assert wrapped.with_read_only(True) is wrappedpy_hamt/encryption_hamt_store.py (1)
113-125: Align cache semantics with base store: copy the metadata cache in the wrapperZarrHAMTStore.with_read_only() copies metadata_read_cache to isolate caches between wrappers. Here you share the dict, which can cause surprising cross-talk if a writable wrapper mutates the cache. Make this consistent with the base class.
- clone.metadata_read_cache = self.metadata_read_cache + clone.metadata_read_cache = self.metadata_read_cache.copy()Optional: Add a unit test asserting the caches are independent objects between wrappers.
I can add the test for cache independence if you’d like.
tests/test_read_only_guards.py (2)
10-21: Good helper builders; consider pytest fixtures to reuse across testsHelpers are clear and minimal. If you plan to extend coverage, turning these into async fixtures (e.g., rw_plain, rw_enc) will reduce duplication and enable parametrized tests.
I can supply a pytest fixture refactor if you want.
63-71: Encrypted variant: extend parity checks to match the plain storeGood that you check the same-flag fast path. To keep parity with the plain store, also verify:
- ro.read_only is True
- ro.hamt is rw.hamt
- ro.with_read_only(True) is ro
- ro.with_read_only(False) returns a new wrapper sharing the same HAMT
- supports_writes / supports_deletes mirror the wrapper state
Apply this diff to extend the test:
@@ async def test_encrypted_read_only_guards_and_self(): rw = await _rw_enc() assert rw.with_read_only(False) is rw # same‑flag path ro = rw.with_read_only(True) + assert ro.read_only is True + assert ro.hamt is rw.hamt + assert ro.with_read_only(True) is ro + rw2 = ro.with_read_only(False) + assert rw2 is not ro and rw2.hamt is rw.hamt with pytest.raises(Exception): await ro.set("k", np.array([2], dtype="u1")) with pytest.raises(Exception): await ro.delete("k") + assert ro.supports_writes is False and ro.supports_deletes is False + assert rw2.supports_writes is True and rw2.supports_deletes is Truepy_hamt/zarr_hamt_store.py (2)
176-178: Raise a more specific exception type for RO violationsUsing a generic Exception makes it harder for callers/tests to target RO violations. PermissionError is the conventional choice.
Apply this diff:
@@ - if self.read_only: - raise Exception("Cannot write to a read-only store.") + if self.read_only: + raise PermissionError("Cannot write to a read-only store.") @@ - if self.read_only: - raise Exception("Cannot write to a read-only store.") + if self.read_only: + raise PermissionError("Cannot write to a read-only store.")If you adopt this, update tests in tests/test_read_only_guards.py to expect PermissionError.
I can push the accompanying test changes as well.
Also applies to: 201-203
25-27: Docstring now outdated given with_read_only(); update guidanceThe note recommends reinitializing a new store for RO mode because the class “will not touch its super class’s settings.” That’s no longer true—with_read_only reinitializes the zarr base and safely presents RO without rebuilding the HAMT. Please align the docstring and sample code to illustrate using with_read_only.
Suggested wording:
- “If you wrote with a writable store and need a read‑only view, call store = store.with_read_only(True). This returns self if already in that mode or a shallow clone sharing the same HAMT.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
py_hamt/encryption_hamt_store.py(1 hunks)py_hamt/store_httpx.py(5 hunks)py_hamt/zarr_hamt_store.py(4 hunks)pyproject.toml(1 hunks)tests/test_kubocas_session.py(2 hunks)tests/test_read_only_guards.py(1 hunks)tests/test_zarr_ipfs.py(1 hunks)tests/testing_utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-21T07:17:10.677Z
Learnt from: TheGreatAlgo
PR: dClimate/py-hamt#78
File: py_hamt/encryption_hamt_store.py:113-125
Timestamp: 2025-08-21T07:17:10.677Z
Learning: In py_hamt codebase, SimpleEncryptedZarrHAMTStore inherits from ZarrHAMTStore, which means it inherits the read_only property that checks _forced_read_only attribute. When analyzing with_read_only methods in encrypted store classes, remember to consider inherited properties from the parent ZarrHAMTStore class.
Applied to files:
py_hamt/encryption_hamt_store.pypy_hamt/zarr_hamt_store.pytests/test_read_only_guards.pytests/test_zarr_ipfs.py
📚 Learning: 2024-12-02T20:09:32.152Z
Learnt from: abidsikder
PR: dClimate/py-hamt#7
File: py_hamt/hamt.py:366-367
Timestamp: 2024-12-02T20:09:32.152Z
Learning: In the `py_hamt` HAMT implementation, always obtaining a lock prevents multithreaded reads in read-only mode. To allow multithreaded reads in read-only mode, locks should not be acquired during read operations.
Applied to files:
tests/test_read_only_guards.py
🧬 Code graph analysis (5)
py_hamt/encryption_hamt_store.py (1)
py_hamt/zarr_hamt_store.py (2)
with_read_only(89-112)read_only(84-87)
py_hamt/zarr_hamt_store.py (1)
py_hamt/encryption_hamt_store.py (1)
with_read_only(113-124)
tests/test_read_only_guards.py (3)
py_hamt/store_httpx.py (1)
InMemoryCAS(38-72)py_hamt/encryption_hamt_store.py (1)
SimpleEncryptedZarrHAMTStore(12-195)py_hamt/zarr_hamt_store.py (4)
ZarrHAMTStore(11-285)read_only(84-87)with_read_only(89-112)delete(199-211)
tests/test_zarr_ipfs.py (3)
py_hamt/store_httpx.py (1)
InMemoryCAS(38-72)py_hamt/hamt.py (2)
HAMT(287-696)build(385-394)py_hamt/zarr_hamt_store.py (2)
ZarrHAMTStore(11-285)read_only(84-87)
tests/test_kubocas_session.py (2)
py_hamt/store_httpx.py (3)
KuboCAS(75-365)_loop_client(234-264)aclose(269-286)tests/conftest.py (1)
global_client_session(9-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Create project environment, run all checks (3.12)
🔇 Additional comments (7)
tests/testing_utils.py (1)
172-172: Ensure Docker pull is validated locally and on CII attempted to run the provided verification script to confirm that pre-pulling the Kubo image succeeds, but Docker isn’t available in this environment. Please verify manually that:
- Docker is installed and accessible in both your local and CI environments.
docker pull ipfs/kubo:v0.36.0completes successfully.- Starting the container and hitting the
/api/v0/versionendpoint returns the expected response.Meanwhile, consider applying this optional refactor in tests/testing_utils.py around line 172:
- image = "ipfs/kubo:v0.36.0" + # Allow overriding via environment variable to sync CI/dev without code changes + image = os.getenv("KUBO_IMAGE", "ipfs/kubo:v0.36.0") + # Pre-pull the exact image now to avoid using a stale, locally cached tag + client.images.pull(image)Optional: For stronger supply-chain guarantees, pin by digest (e.g.,
ipfs/kubo@sha256:…) and defaultKUBO_IMAGEto that value.tests/test_kubocas_session.py (1)
193-207: Reopen-after-close behavior validatedConfirms that owned clients are lazily recreated post-aclose(). This protects against stale client reuse. Looks solid.
tests/test_read_only_guards.py (2)
24-34: RO wrapper correctly blocks writes/deletesThese assertions validate the guard rails effectively and fail-fast before any buffer conversion occurs. Nice.
36-41: Idempotent fast-path coveredAsserting rw.with_read_only(False) is rw exercises the early-return path. Good touch.
py_hamt/zarr_hamt_store.py (3)
54-54: Sentinel for wrapper clones is appropriateThe class-level _forced_read_only sentinel is a clean, low-overhead way to let clones present different modes without mutating the underlying HAMT. LGTM.
83-87: read_only property override: correct precedence and minimal surface areaDeferring to _forced_read_only when set and otherwise to hamt.read_only is the right layering. This makes wrappers authoritative without touching the HAMT. Looks good.
89-113: Script queued to confirm cache semantics in SimpleEncryptedZarrHAMTStore.
| if client is not None: | ||
| # user supplied → bind it to *their* current loop | ||
| self._client_per_loop[asyncio.get_running_loop()] = client | ||
| self._owns_client: bool = False | ||
| # A client was supplied by the user. We don't own it. | ||
| self._owns_client = False | ||
| self._client_per_loop = {asyncio.get_running_loop(): client} | ||
| else: | ||
| self._owns_client = True # we'll create clients lazily | ||
| # No client supplied. We will own any clients we create. | ||
| self._owns_client = True | ||
| self._client_per_loop = {} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t call asyncio.get_running_loop() in init when a user supplies the client
Constructors run in both sync and async contexts. Calling asyncio.get_running_loop() here can raise RuntimeError if KuboCAS is instantiated outside a running loop (valid when a pre-built AsyncClient is passed). Defer binding to the first use instead, and remember the user client for that loop.
Proposed minimal change: keep ownership flag, store the user client for deferred binding, and only bind once a loop exists.
- if client is not None:
- # A client was supplied by the user. We don't own it.
- self._owns_client = False
- self._client_per_loop = {asyncio.get_running_loop(): client}
+ if client is not None:
+ # A client was supplied by the user. We don't own it.
+ self._owns_client = False
+ # Defer per-loop binding to _loop_client() to avoid requiring a running loop here.
+ self._client_per_loop = {}
+ self._deferred_user_client = client # bound on first useThis pairs with the _loop_client() fix below.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if client is not None: | |
| # user supplied → bind it to *their* current loop | |
| self._client_per_loop[asyncio.get_running_loop()] = client | |
| self._owns_client: bool = False | |
| # A client was supplied by the user. We don't own it. | |
| self._owns_client = False | |
| self._client_per_loop = {asyncio.get_running_loop(): client} | |
| else: | |
| self._owns_client = True # we'll create clients lazily | |
| # No client supplied. We will own any clients we create. | |
| self._owns_client = True | |
| self._client_per_loop = {} | |
| if client is not None: | |
| # A client was supplied by the user. We don't own it. | |
| self._owns_client = False | |
| # Defer per-loop binding to _loop_client() to avoid requiring a running loop here. | |
| self._client_per_loop = {} | |
| self._deferred_user_client = client # bound on first use | |
| else: | |
| # No client supplied. We will own any clients we create. | |
| self._owns_client = True | |
| self._client_per_loop = {} |
🤖 Prompt for AI Agents
In py_hamt/store_httpx.py around lines 213 to 221, do not call
asyncio.get_running_loop() in __init__; instead keep the ownership flag as
currently set, store the supplied AsyncClient in a dedicated attribute (e.g.
self._user_client) and initialize self._client_per_loop = {} (empty dict) so no
loop lookup happens in the constructor; defer binding the user-supplied client
to a loop until first use by resolving asyncio.get_running_loop() inside the
_loop_client() helper and then associate the stored self._user_client with that
loop in self._client_per_loop (and clear self._user_client) so the client is
only bound once a running loop exists.
| if self._closed: | ||
| if not self._owns_client: | ||
| raise RuntimeError("KuboCAS is closed; create a new instance") | ||
| # We previously closed all internally-owned clients. Reset the | ||
| # state so that new clients can be created lazily. | ||
| self._closed = False | ||
| self._client_per_loop = {} | ||
|
|
There was a problem hiding this comment.
Critical: leaked AsyncClients when a user-supplied client is used across loops
When self._owns_client is False and _loop_client() is called from a different loop, the KeyError branch creates a new AsyncClient but aclose() is a no-op for non-owned clients. Result: clients that will never be closed. Instead, either bind the deferred user client to the first loop only, or raise if callers try to use the instance in other loops.
Suggested fix (works with the deferred-binding change above):
def _loop_client(self) -> httpx.AsyncClient:
@@
if self._closed:
if not self._owns_client:
raise RuntimeError("KuboCAS is closed; create a new instance")
# We previously closed all internally-owned clients. Reset the
# state so that new clients can be created lazily.
self._closed = False
self._client_per_loop = {}
@@
try:
return self._client_per_loop[loop]
except KeyError:
- # Create a new client
- client = httpx.AsyncClient(
- timeout=60.0,
- headers=self._default_headers,
- auth=self._default_auth,
- limits=httpx.Limits(max_connections=64, max_keepalive_connections=32),
- # Uncomment when they finally support Robust HTTP/2 GOAWAY responses
- # http2=True,
- )
- self._client_per_loop[loop] = client
- return client
+ # If a user-supplied client was provided, bind it to the first loop only.
+ if not self._owns_client:
+ if hasattr(self, "_deferred_user_client"):
+ self._client_per_loop[loop] = self._deferred_user_client # type: ignore[attr-defined]
+ del self._deferred_user_client
+ return self._client_per_loop[loop]
+ # Using the instance from a different loop is unsafe — refuse.
+ raise RuntimeError(
+ "User-supplied AsyncClient is bound to a different event loop; "
+ "create a per-loop KuboCAS or omit `client=` to let KuboCAS manage clients per loop."
+ )
+ # Create an internally-owned client for this loop.
+ client = httpx.AsyncClient(
+ timeout=60.0,
+ headers=self._default_headers,
+ auth=self._default_auth,
+ limits=httpx.Limits(max_connections=64, max_keepalive_connections=32),
+ # http2=True, # enable when httpx supports robust GOAWAY handling
+ )
+ self._client_per_loop[loop] = client
+ return clientThis prevents unowned clients from being created (and leaked) and still supports the per-loop model.
Also applies to: 254-263
🤖 Prompt for AI Agents
In py_hamt/store_httpx.py around lines 242-249 (and likewise 254-263), the code
can create new AsyncClient instances when self._owns_client is False but used
from a different event loop, leaking clients because aclose() is skipped for
non-owned clients; modify _loop_client() so that if a user-supplied client
(self._owns_client is False) is accessed from any loop other than the one it was
first bound to, you raise a RuntimeError instead of creating a new client, or
alternatively bind the user client to the first loop on first use and enforce it
thereafter; ensure the code never lazily instantiates a non-owned client for a
new loop, and update the _closed handling so internal resets only affect owned
clients and do not leave unowned clients unclosed.
| if self.read_only: | ||
| raise Exception("Cannot write to a read-only store.") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
supports_ currently diverge from wrapper state; fix to reflect presented mode*
supports_writes and supports_deletes derive from hamt.read_only. In a read‑only wrapper over a writable HAMT, these will incorrectly return True, encouraging callers to attempt writes and only fail at runtime. They should reflect self.read_only.
Apply this diff:
@@
@property
def supports_writes(self) -> bool:
"""@private"""
- return not self.hamt.read_only
+ return not self.read_only
@@
@property
def supports_deletes(self) -> bool:
"""@private"""
- return not self.hamt.read_only
+ return not self.read_onlyTo strengthen the contract, also consider raising a more specific exception (PermissionError) below. See next comment.
Also applies to: 201-203
🤖 Prompt for AI Agents
In py_hamt/zarr_hamt_store.py around lines 176-178 (and also update lines
201-203), supports_writes and supports_deletes currently query the underlying
hamt.read_only and thus can return True when this wrapper was constructed as
read-only; change both to reflect the wrapper's state by returning not
self.read_only (or using self.read_only to return False when read-only) so
callers get the correct capability surface, and update the write-path exception
to raise PermissionError instead of a generic Exception for clearer semantics.
| @pytest.mark.asyncio | ||
| async def test_loop_client_rejects_reuse_of_external_client(global_client_session): | ||
| """Calling _loop_client() after aclose() raises when client is user-supplied.""" | ||
| cas = KuboCAS( | ||
| client=global_client_session, | ||
| rpc_base_url="http://127.0.0.1:5001", | ||
| gateway_base_url="http://127.0.0.1:8080", | ||
| ) | ||
| assert cas._loop_client() is global_client_session | ||
|
|
||
| await cas.aclose() | ||
| cas._closed = True # simulate closed instance with external client | ||
| with pytest.raises(RuntimeError, match="KuboCAS is closed; create a new instance"): | ||
| cas._loop_client() | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add a test to forbid cross-loop use with a user-supplied client
To guard against accidental client leaks (and to lock in the proposed fix), add a test that using a user-supplied AsyncClient from a second loop raises, rather than silently creating an unowned client.
Example:
@pytest.mark.asyncio
async def test_user_client_cross_loop_is_rejected(global_client_session):
cas = KuboCAS(client=global_client_session)
# Bound in primary loop:
assert cas._loop_client() is global_client_session
# Attempt in a distinct loop should be rejected (no implicit client creation)
client_2, loop_2 = await _client_in_new_loop(cas) # helper from this module
# Because _client_in_new_loop currently returns the created client, adjust helper or assert via raises:
with pytest.raises(RuntimeError, match="bound to a different event loop"):
await _client_in_new_loop(cas)This prevents regressions around ownership semantics.
🏁 Script executed:
#!/bin/bash
set -e
# Locate the file defining class KuboCAS
rg -nP 'class\s+KuboCAS' -g '*.py'
# Show implementation of _loop_client
rg -nP 'def\s+_loop_client' -C5 -g '*.py'
# Check if helper _client_in_new_loop exists in tests
rg -n '_client_in_new_loop' -C5 -g 'tests/test_kubocas_session.py'Length of output: 2779
🏁 Script executed:
#!/bin/bash
# Display the helper and initial tests for _client_in_new_loop
sed -n '1,120p' tests/test_kubocas_session.pyLength of output: 3690
🏁 Script executed:
#!/bin/bash
# Display the implementation of KuboCAS._loop_client to verify handling of external clients across loops
sed -n '220,300p' py_hamt/store_httpx.pyLength of output: 3421
🏁 Script executed:
#!/bin/bash
# Show the initializer of KuboCAS to inspect ownership logic
sed -n '1,200p' py_hamt/store_httpx.pyLength of output: 8925
🏁 Script executed:
#!/bin/bash
sed -n '138,200p' py_hamt/store_httpx.pyLength of output: 3428
🏁 Script executed:
#!/bin/bash
sed -n '170,234p' py_hamt/store_httpx.pyLength of output: 3151
Enforce single‐loop usage for user-supplied clients
The current implementation of _loop_client will silently create a new AsyncClient when a user-supplied client is first used on a different event loop. We need to prevent that and ensure cross-loop calls raise immediately. Please update both the helper and the tests as follows:
• In py_hamt/store_httpx.py, within _loop_client (around line 244), add a guard before creating a new client:
loop: asyncio.AbstractEventLoop = asyncio.get_running_loop()
+ if not self._owns_client and loop not in self._client_per_loop:
+ raise RuntimeError("external client bound to a different event loop")
try:
return self._client_per_loop[loop]
except KeyError:
# Create a new client
client = httpx.AsyncClient(...)• In tests/test_kubocas_session.py, add a new test after test_get_client_respects_user_supplied_client:
@pytest.mark.asyncio
async def test_user_client_cross_loop_is_rejected(global_client_session):
"""Using a user-supplied client in a different loop must raise, not create one."""
cas = KuboCAS(client=global_client_session)
# Primary loop binds as expected
assert await _maybe_await(cas._loop_client()) is global_client_session
# Cross-loop use should error
with pytest.raises(RuntimeError, match="external client bound to a different event loop"):
await _client_in_new_loop(cas)These changes mandate that user-supplied clients remain bound to their original loop and lock in the intended ownership semantics.
| async def test_roundtrip_plain_store(): | ||
| rw = await _rw_plain() # writable store | ||
| ro = rw.with_read_only(True) # clone → RO | ||
| assert ro.read_only is True | ||
| assert ro.hamt is rw.hamt | ||
|
|
||
| # idempotent: RO→RO returns same object | ||
| assert ro.with_read_only(True) is ro | ||
|
|
||
| # back to RW (new wrapper) | ||
| rw2 = ro.with_read_only(False) | ||
| assert rw2.read_only is False and rw2 is not ro | ||
| assert rw2.hamt is rw.hamt | ||
|
|
||
| # guard: cannot write through RO wrapper | ||
| with pytest.raises(Exception): | ||
| await ro.set("k", np.array([0], dtype="u1")) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Round‑trip semantics verified; add supports_ checks to catch interface mismatches*
The identity and idempotence checks are solid. One gap: verify feature flags exposed by the store reflect the wrapper, not the underlying HAMT. Today, supports_writes and supports_deletes are derived from hamt.read_only (see py_hamt/zarr_hamt_store.py Lines 164-168, 194-198), which can diverge from the wrapper’s read_only in RO clones. Add assertions so CI catches regressions.
Apply this diff to extend the test:
@@ async def test_roundtrip_plain_store():
# guard: cannot write through RO wrapper
with pytest.raises(Exception):
await ro.set("k", np.array([0], dtype="u1"))
+
+ # feature flags should reflect the wrapper, not the underlying HAMT
+ assert ro.read_only is True
+ assert getattr(ro, "supports_writes") is False
+ assert getattr(ro, "supports_deletes") is False
+ assert getattr(rw2, "supports_writes") is True
+ assert getattr(rw2, "supports_deletes") is TrueIf you adopt PermissionError in the store (see my note in zarr_hamt_store.py), switch the two Exception contexts to PermissionError here too.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def test_roundtrip_plain_store(): | |
| rw = await _rw_plain() # writable store | |
| ro = rw.with_read_only(True) # clone → RO | |
| assert ro.read_only is True | |
| assert ro.hamt is rw.hamt | |
| # idempotent: RO→RO returns same object | |
| assert ro.with_read_only(True) is ro | |
| # back to RW (new wrapper) | |
| rw2 = ro.with_read_only(False) | |
| assert rw2.read_only is False and rw2 is not ro | |
| assert rw2.hamt is rw.hamt | |
| # guard: cannot write through RO wrapper | |
| with pytest.raises(Exception): | |
| await ro.set("k", np.array([0], dtype="u1")) | |
| async def test_roundtrip_plain_store(): | |
| rw = await _rw_plain() # writable store | |
| ro = rw.with_read_only(True) # clone → RO | |
| assert ro.read_only is True | |
| assert ro.hamt is rw.hamt | |
| # idempotent: RO→RO returns same object | |
| assert ro.with_read_only(True) is ro | |
| # back to RW (new wrapper) | |
| rw2 = ro.with_read_only(False) | |
| assert rw2.read_only is False and rw2 is not ro | |
| assert rw2.hamt is rw.hamt | |
| # guard: cannot write through RO wrapper | |
| with pytest.raises(Exception): | |
| await ro.set("k", np.array([0], dtype="u1")) | |
| # feature flags should reflect the wrapper, not the underlying HAMT | |
| assert ro.read_only is True | |
| assert getattr(ro, "supports_writes") is False | |
| assert getattr(ro, "supports_deletes") is False | |
| assert getattr(rw2, "supports_writes") is True | |
| assert getattr(rw2, "supports_deletes") is True |
Summary by CodeRabbit