Skip to content

Conversation

@Faolain
Copy link
Contributor

@Faolain Faolain commented Jul 27, 2025

Summary by CodeRabbit

  • New Features

    • Added methods to support toggling read-only mode for HAMT-based Zarr stores, allowing seamless switching between read and write access.
    • Enhanced test coverage for IPFS gateway interactions, including support for DAG-CBOR content and gateway URL variations.
  • Bug Fixes

    • Enforced read-only restrictions on Zarr stores, preventing write or delete operations when in read-only mode.
    • Improved cleanup and lifecycle management of HTTP clients, ensuring proper resource release in both synchronous and asynchronous contexts.
  • Tests

    • Expanded and added tests for edge cases in client cleanup, gateway behavior, and read-only store access.
    • Added tests verifying read-only guard behavior for encrypted and plain stores.
    • Updated test utilities to use the latest IPFS Docker image for more reliable integration testing.
  • Chores

    • Updated IPFS installation in CI workflows and pinned the "zarr" dependency to version 3.0.9 for improved stability.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jul 27, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This update introduces enhancements to IPFS client lifecycle management, read-only mode toggling for Zarr HAMT stores, and improved test coverage for edge cases and gateway behaviors. The workflow and dependency versions are updated, and tests now dynamically create and verify IPFS content, with robust error handling and new scenarios.

Changes

Cohort / File(s) Change Summary
.github/workflows/run-checks.yaml, pyproject.toml, tests/testing_utils.py Updated IPFS setup action and version in CI workflow, pinned Zarr dependency to exact version, and updated IPFS Docker image tag in test utils.
py_hamt/store_httpx.py Refactored KuboCAS client lifecycle: introduced _closed flag, improved cleanup in aclose and __del__, added robust event loop handling, and ensured no usage after closure.
py_hamt/zarr_hamt_store.py, py_hamt/encryption_hamt_store.py Added with_read_only method for toggling store read-only mode, introduced _forced_read_only attribute, updated read_only property, and enforced read-only restrictions in set and delete.
tests/test_async.py, tests/test_kubocas_session.py Expanded and added tests for KuboCAS cleanup behavior, edge cases in destructor, handling absence of event loop, and client closure logic. Included tests for missing attributes, event loop states, and error handling.
tests/test_public_gateway.py Overhauled and expanded IPFS gateway tests: dynamic content creation, robust error handling, diagnostics, new tests for trailing slashes, DAG-CBOR content negotiation, and improved gateway URL coverage.
tests/test_zarr_ipfs.py Added test to verify that a write-enabled ZarrHAMTStore can be reopened in read mode without error, confirming compatibility and correct helper triggering.
tests/test_read_only_guards.py New test module verifying read-only guard behavior for plain and encrypted Zarr HAMT stores, testing with_read_only method correctness and mutation prevention in read-only mode.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant ZarrHAMTStore
    participant HAMT
    User->>ZarrHAMTStore: with_read_only(read_only)
    alt Requested mode == current
        ZarrHAMTStore-->>User: Return self
    else
        ZarrHAMTStore->>ZarrHAMTStore: Clone instance with new read_only
        ZarrHAMTStore-->>User: Return new instance
    end
Loading
sequenceDiagram
    participant User
    participant KuboCAS
    participant httpx.AsyncClient
    User->>KuboCAS: aclose()
    alt Owns client(s)
        KuboCAS->>httpx.AsyncClient: await aclose()
        KuboCAS->>KuboCAS: Clear client cache, set _closed=True
    else
        KuboCAS->>KuboCAS: Skip closing clients
    end
    KuboCAS-->>User: Cleanup complete
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Suggested reviewers

  • abidsikder

Poem

In the garden of code where the HAMTs gently grow,
A bunny toggles read-only, letting streams ebb and flow.
KuboCAS now cleans up, so neat and so spry,
As gateways are tested, with carrots held high.
From Docker to DAG-CBOR, the tests hop along—
This patch is a warren where all bits belong! 🐇✨

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ci/update-zarr-3.0.9

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (a276069) to head (8086d50).

Additional details and impacted files
@@                    Coverage Diff                     @@
##           ci/switch-setup-ipfs-gha       #78   +/-   ##
==========================================================
  Coverage                    100.00%   100.00%           
==========================================================
  Files                             5         5           
  Lines                           643       673   +30     
==========================================================
+ Hits                            643       673   +30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 964496b and 43c6dc7.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (10)
  • .github/workflows/run-checks.yaml (1 hunks)
  • py_hamt/encryption_hamt_store.py (1 hunks)
  • py_hamt/store_httpx.py (4 hunks)
  • py_hamt/zarr_hamt_store.py (4 hunks)
  • pyproject.toml (1 hunks)
  • tests/test_async.py (1 hunks)
  • tests/test_kubocas_session.py (2 hunks)
  • tests/test_public_gateway.py (5 hunks)
  • tests/test_zarr_ipfs.py (1 hunks)
  • tests/testing_utils.py (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
pyproject.toml (3)

Learnt from: abidsikder
PR: #7
File: pyproject.toml:7-13
Timestamp: 2024-12-02T20:05:21.672Z
Learning: When reviewing dependencies in pyproject.toml, note that using open-ended version ranges is acceptable because uv generates lockfiles, preventing future breaking changes from affecting the project.

Learnt from: abidsikder
PR: #7
File: pyproject.toml:19-31
Timestamp: 2024-12-02T20:05:50.327Z
Learning: The project uses a uv lockfile to manage dependencies, ensuring consistent versions and preventing issues with dependency version constraints in pyproject.toml.

Learnt from: abidsikder
PR: #7
File: pyproject.toml:1-6
Timestamp: 2024-12-02T20:06:24.670Z
Learning: The codebase uses Python 3.12's type parameter syntax (PEP 695), making Python >=3.12 a necessary requirement.

tests/test_zarr_ipfs.py (1)

Learnt from: abidsikder
PR: #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.

py_hamt/encryption_hamt_store.py (1)

Learnt from: abidsikder
PR: #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.

py_hamt/zarr_hamt_store.py (1)

Learnt from: abidsikder
PR: #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.

🧬 Code Graph Analysis (4)
tests/test_async.py (1)
py_hamt/store_httpx.py (3)
  • aclose (257-274)
  • KuboCAS (75-353)
  • _loop_client (234-252)
py_hamt/encryption_hamt_store.py (1)
py_hamt/zarr_hamt_store.py (2)
  • with_read_only (87-110)
  • read_only (84-85)
tests/test_kubocas_session.py (1)
py_hamt/store_httpx.py (3)
  • KuboCAS (75-353)
  • aclose (257-274)
  • _loop_client (234-252)
py_hamt/zarr_hamt_store.py (1)
py_hamt/encryption_hamt_store.py (1)
  • with_read_only (113-124)
🪛 GitHub Actions: Triggered on push from Faolain to branch/tag ci/update-zarr-3.0.9
py_hamt/encryption_hamt_store.py

[error] 115-115: Coverage missing at line 115. Coverage is below the required 100% threshold.


[error] 188-188: Coverage missing at line 188. Coverage is below the required 100% threshold.

py_hamt/zarr_hamt_store.py

[error] 175-175: Coverage missing at line 175. Coverage is below the required 100% threshold.


[error] 200-200: Coverage missing at line 200. Coverage is below the required 100% threshold.

🔇 Additional comments (13)
tests/test_async.py (1)

107-130: Excellent enhancement of async lifecycle testing.

The expanded test now comprehensively covers both __del__ and aclose behavior when no event loop is running. The test properly:

  1. Tests __del__ graceful handling of missing event loops
  2. Creates separate instances to avoid test interference
  3. Validates that aclose works correctly under no-loop conditions
  4. Verifies proper cleanup through assertions

This thorough testing of edge cases strengthens the reliability of the KuboCAS async lifecycle management.

tests/test_zarr_ipfs.py (1)

194-212: Well-designed regression test for read-only mode functionality.

This test effectively validates the fix for opening write-enabled ZarrHAMTStore instances in read mode. The test:

  1. Clearly documents the before/after behavior (ValueError → success)
  2. Uses appropriate test data and setup (in-memory CAS, simple dataset)
  3. Tests the specific scenario that was problematic (reopening RW store as read-only)
  4. Includes proper verification of the reopened data

The test provides valuable coverage for the read-only mode functionality and helps prevent regression of the helper method implementation.

py_hamt/zarr_hamt_store.py (3)

54-54: Good use of sentinel pattern for read-only state tracking

The _forced_read_only attribute with None default is a clean way to distinguish between unset state and explicitly set boolean values.


83-85: Correct implementation of read-only property with fallback

The property correctly checks for _forced_read_only first before falling back to the HAMT's read-only state, enabling proper read-only enforcement for clones.


174-175: Proper enforcement of read-only constraints

Good addition of explicit read-only checks that raise the appropriate Zarr exception type. This ensures consistent behavior regardless of the underlying HAMT's state.

Also applies to: 199-200

tests/test_kubocas_session.py (1)

126-257: Comprehensive test coverage for KuboCAS lifecycle edge cases

Excellent addition of tests covering critical edge cases:

  • Missing internal attributes handling
  • Event loop not running scenarios
  • Closed instance behavior
  • Asynchronous cleanup paths

These tests ensure robust behavior of the KuboCAS lifecycle management in various edge conditions.

py_hamt/store_httpx.py (4)

214-223: Clean initialization of client tracking attributes

Good refactoring that ensures _client_per_loop is always initialized and properly tracks client ownership. This prevents potential AttributeErrors and makes the lifecycle management more robust.


236-237: Good defensive check against using closed instances

The early check prevents potential issues from using a closed KuboCAS instance, with a clear error message guiding users to create a new instance.


257-274: Robust asynchronous cleanup implementation

The aclose method correctly:

  • Respects client ownership (doesn't close external clients)
  • Handles exceptions during cleanup gracefully
  • Properly marks the instance as closed

Good defensive programming for resource cleanup.


284-323: Excellent destructor implementation with comprehensive edge case handling

The __del__ method handles all scenarios gracefully:

  • Defensive attribute checks prevent errors in partially-constructed instances
  • Proper handling of different event loop states (none, not running, running)
  • Correct coroutine cleanup to prevent warnings (line 317)
  • Multiple fallback strategies ensure cleanup always happens

This is exemplary defensive programming for asynchronous resource management.

tests/test_public_gateway.py (3)

21-66: Well-structured response verification with improved error handling

The enhancements to verify_response_content improve debuggability:

  • Timeout parameter allows per-test configuration
  • Structured error returns help diagnose failures
  • Hex output for binary content aids debugging

68-142: Robust gateway comparison test with dynamic content creation

Excellent test improvements:

  • Creates content locally first, ensuring it exists for testing
  • Uses must_succeed flags to handle unreliable public gateways gracefully
  • Detailed diagnostics help debug gateway issues
  • Ensures critical gateways (local) always work

314-353: Good test for DAG-CBOR content negotiation

The test properly verifies that the IPFS gateway honors the Accept header for DAG-CBOR content:

  • Creates valid DAG-CBOR test data
  • Checks both content-type header and actual content
  • Handles connection and gateway errors gracefully

Comment on lines 40 to 42
uses: Faolain/setup-ipfs@v0.7.0
with:
ipfs_version: "0.35.0"
ipfs_version: "0.36.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify the IPFS setup action and version compatibility.

The workflow now uses Faolain/setup-ipfs@v0.7.0 instead of the previous oduwsdl/setup-ipfs action, and updates IPFS from 0.35.0 to 0.36.0.

Please verify:

  1. That Faolain/setup-ipfs@v0.7.0 is a trusted action source
  2. That IPFS version 0.36.0 is stable and compatible with the test suite
  3. That this change aligns with the Docker image update in tests/testing_utils.py

🏁 Script executed:

#!/bin/bash
# Check if the IPFS versions are consistent across files
echo "Checking IPFS version consistency..."
rg -n "ipfs.*version|kubo.*v[0-9]" --type yaml --type py

Length of output: 141


🏁 Script executed:

#!/bin/bash
set -e

echo "1. Workflow snippet (lines 35–50):"
sed -n '35,50p' .github/workflows/run-checks.yaml

echo
echo "2. All occurrences of 'ipfs_version' in the repo:"
rg -n 'ipfs_version' .

echo
echo "3. Locate testing_utils.py and show IPFS/Kubo references:"
utils_file=$(fd testing_utils.py)
if [[ -n "$utils_file" ]]; then
  echo "Found at: $utils_file"
  rg -n 'ipfs|kubo' "$utils_file" || echo "No IPFS/Kubo patterns found in $utils_file"
else
  echo "testing_utils.py not found"
fi

echo
echo "4. Fetching GitHub info for Faolain/setup-ipfs action:"
curl -s https://api.github.com/repos/Faolain/setup-ipfs | jq '.full_name, .fork, .license, .stargazers_count, .description'

Length of output: 750


🏁 Script executed:

#!/bin/bash
# Search for ipfs_version anywhere (including hidden dirs)
echo "1. Searching for 'ipfs_version' across the repo (including .github):"
rg -n 'ipfs_version' --hidden || echo "→ No matches for ipfs_version"

echo
# Locate testing_utils.py
echo "2. Locating testing_utils.py:"
files=$(fd --hidden --exclude .git testing_utils.py || true)
if [[ -z "$files" ]]; then
  echo "→ testing_utils.py not found"
else
  for f in $files; do
    echo "→ Found at: $f"
    echo "   Searching for ipfs|kubo patterns in $f:"
    rg -n 'ipfs|kubo' --hidden "$f" || echo "   → No IPFS/Kubo references in $f"
  done
fi

echo
# Fetch GitHub metadata for Faolain/setup-ipfs
echo "3. GitHub repo info for Faolain/setup-ipfs:"
curl -s https://api.github.com/repos/Faolain/setup-ipfs | jq '{ full_name, fork, license: .license.spdx_id, stargazers_count, description }'

Length of output: 1206


Align IPFS versions & verify setup action source

The workflow now installs IPFS v0.36.0 via the forked Faolain/setup-ipfs@v0.7.0 (MIT-licensed, zero stars, forked repo), but your test helper spins up ipfs/kubo:master-latest. To avoid version drift and ensure trust:

• Audit or replace Faolain/setup-ipfs@v0.7.0 with an official action (the current repo is a fork with no stars).
• Pin your test Docker image in tests/testing_utils.py to the same version—e.g.

image = "ipfs/kubo:v0.36.0"

• Run the full test suite against v0.36.0 locally to confirm compatibility.
• Once versions are aligned and the action source is vetted, the workflow should remain stable.

🤖 Prompt for AI Agents
In .github/workflows/run-checks.yaml around lines 40 to 42, the IPFS version
used is 0.36.0 via the forked Faolain/setup-ipfs@v0.7.0 action, which is
untrusted and may cause version drift with the test helper using
ipfs/kubo:master-latest. Replace this setup action with an official,
well-maintained IPFS setup action to ensure trustworthiness. Then, in
tests/testing_utils.py, pin the Docker image version to "ipfs/kubo:v0.36.0" to
match the workflow IPFS version. Finally, run the full test suite locally
against v0.36.0 to verify compatibility and stability.

Comment on lines +113 to +125
def with_read_only(self, read_only: bool = False) -> "SimpleEncryptedZarrHAMTStore":
if read_only == self.read_only:
return self

clone = type(self).__new__(type(self))
clone.hamt = self.hamt
clone.encryption_key = self.encryption_key
clone.header = self.header
clone.metadata_read_cache = self.metadata_read_cache
clone._forced_read_only = read_only # safe; attribute is declared
zarr.abc.store.Store.__init__(clone, read_only=read_only)
return clone

Copy link
Contributor

@coderabbitai coderabbitai bot Jul 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Missing read_only property to use _forced_read_only attribute

The with_read_only method sets _forced_read_only on line 122, but the class doesn't have a read_only property that checks this attribute. Without such a property, the cloned instance won't actually respect the forced read-only state.

Add a read_only property similar to the one in ZarrHAMTStore:

@property
def read_only(self) -> bool:
    return getattr(self, "_forced_read_only", self.hamt.read_only)

This property should be added before the with_read_only method to ensure the forced read-only state is properly respected throughout the class.

🧰 Tools
🪛 GitHub Actions: Triggered on push from Faolain to branch/tag ci/update-zarr-3.0.9

[error] 115-115: Coverage missing at line 115. Coverage is below the required 100% threshold.

🤖 Prompt for AI Agents
In py_hamt/encryption_hamt_store.py around lines 113 to 125, the with_read_only
method sets the _forced_read_only attribute on the cloned instance, but the
class lacks a read_only property that returns this attribute. To fix this, add a
read_only property before the with_read_only method that returns
_forced_read_only if set, otherwise falls back to self.hamt.read_only. This
ensures the forced read-only state is respected by the cloned instance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think this understands that the encryption class is built from the ZarrHAMTStore so we should be able to safely ignore this as the ZarrHAMTStore does have this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheGreatAlgo You're absolutely correct! I apologize for the oversight. Since SimpleEncryptedZarrHAMTStore inherits from ZarrHAMTStore, it automatically inherits the read_only property that was added to the parent class.

Looking at the inheritance relationship:

class SimpleEncryptedZarrHAMTStore(ZarrHAMTStore):

The parent ZarrHAMTStore class already has the read_only property that returns _forced_read_only if set, otherwise falls back to self.hamt.read_only. This means the with_read_only method will work correctly as the cloned instance will respect the _forced_read_only attribute through the inherited property.

The comment can be safely ignored as the implementation is correct through inheritance.

🐰✨
/| /|
( ✌️ )
|(_)|
^ ^


✏️ Learnings added
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.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

damn right it will work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheGreatAlgo Exactly! The inheritance chain makes it all work seamlessly. Thanks for keeping me on track with the codebase architecture! 🎯


🧠 Learnings used
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.

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.

Comment on lines 87 to 110
def with_read_only(self, read_only: bool = False) -> "ZarrHAMTStore":
"""
Return this store (if the flag already matches) or a *shallow*
clone that presents the requested read‑only status.
The clone **shares** the same :class:`~py_hamt.hamt.HAMT`
instance; no flushing, network traffic or async work is done.
"""
# Fast path
if read_only == self._read_only:
return self # Same mode, return same instance

# Create new instance with different read_only flag
# Creates a *bare* instance without running its __init__
clone = type(self).__new__(type(self))

# Copy attributes that matter
clone.hamt = self.hamt # Share the HAMT
clone._read_only = read_only
clone.metadata_read_cache = self.metadata_read_cache.copy()

# Re‑initialise the zarr base class so that Zarr sees the flag
zarr.abc.store.Store.__init__(clone, read_only=read_only)
return clone
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix attribute access errors in with_read_only method

The method has two critical issues:

  1. Line 96 accesses self._read_only which doesn't exist - it should be self.read_only
  2. Line 105 sets clone._read_only but should set clone._forced_read_only to match the sentinel pattern used elsewhere

Apply this fix:

-        if read_only == self._read_only:
+        if read_only == self.read_only:
             return self  # Same mode, return same instance

         # Create new instance with different read_only flag
         # Creates a *bare* instance without running its __init__
         clone = type(self).__new__(type(self))

         # Copy attributes that matter
         clone.hamt = self.hamt  # Share the HAMT
-        clone._read_only = read_only
+        clone._forced_read_only = read_only
         clone.metadata_read_cache = self.metadata_read_cache.copy()
🤖 Prompt for AI Agents
In py_hamt/zarr_hamt_store.py between lines 87 and 110, the with_read_only
method incorrectly accesses self._read_only which should be self.read_only, and
it sets clone._read_only instead of clone._forced_read_only. Fix these by
replacing self._read_only with self.read_only on line 96 and setting
clone._forced_read_only instead of clone._read_only on line 105 to align with
the existing attribute pattern.

pytest.skip("Neither IPFS daemon nor Docker available – skipping IPFS tests")

image = "ipfs/kubo:v0.35.0"
image = "ipfs/kubo:master-latest"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use a specific version tag instead of "master-latest" for reproducible tests.

Changing from "ipfs/kubo:v0.35.0" to "ipfs/kubo:master-latest" introduces non-deterministic behavior in tests. The master-latest tag can change at any time, potentially breaking tests or causing inconsistent results across different test runs.

Use a specific version tag that aligns with the CI setup:

-    image = "ipfs/kubo:master-latest"
+    image = "ipfs/kubo:v0.36.0"

This ensures:

  • Reproducible test results
  • Consistency with the GitHub workflow IPFS version
  • Protection against unexpected changes in the master branch
📝 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.

Suggested change
image = "ipfs/kubo:master-latest"
image = "ipfs/kubo:v0.36.0"
🤖 Prompt for AI Agents
In tests/testing_utils.py at line 172, the Docker image tag is set to
"ipfs/kubo:master-latest", which causes non-deterministic test behavior. Replace
"master-latest" with a specific version tag such as "ipfs/kubo:v0.35.0" to
ensure reproducible and consistent test results aligned with the CI setup.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
tests/test_read_only_guards.py (2)

24-34: Consider more specific exception testing.

The test logic is sound, but using generic Exception in pytest.raises could mask unexpected errors. Consider testing for specific exception types if the implementation raises them.

-    with pytest.raises(Exception):
+    with pytest.raises(ValueError, match="read.*only"):
         await ro.set("k", np.array([1], dtype="u1"))
-    with pytest.raises(Exception):
+    with pytest.raises(ValueError, match="read.*only"):
         await ro.delete("k")

43-52: Consider more specific exception testing and verify test completeness.

The test logic correctly covers both the self-return optimization and read-only guards for encrypted stores. However, similar to the plain store tests, consider using more specific exception types.

-    with pytest.raises(Exception):
+    with pytest.raises(ValueError, match="read.*only"):
         await ro.set("k", np.array([2], dtype="u1"))
-    with pytest.raises(Exception):
+    with pytest.raises(ValueError, match="read.*only"):
         await ro.delete("k")

The combined test approach is acceptable, though you might consider splitting into separate functions for consistency with the plain store tests.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 43c6dc7 and 6d48c19.

📒 Files selected for processing (2)
  • py_hamt/zarr_hamt_store.py (4 hunks)
  • tests/test_read_only_guards.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • py_hamt/zarr_hamt_store.py
🧰 Additional context used
🧠 Learnings (1)
tests/test_read_only_guards.py (1)

Learnt from: abidsikder
PR: #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.

🔇 Additional comments (3)
tests/test_read_only_guards.py (3)

1-6: LGTM! Import statements are appropriate.

All imports are relevant and necessary for the test functionality being implemented.


9-21: LGTM! Helper functions are well-structured.

The helper functions provide consistent setup patterns for both plain and encrypted stores with appropriate async handling and secure encryption setup.


36-40: LGTM! Instance identity check is appropriate.

The test correctly verifies the optimization where with_read_only(False) on a read-write store returns the same instance.

@Faolain Faolain changed the base branch from main to ci/switch-setup-ipfs-gha July 27, 2025 23:10
@Faolain Faolain requested a review from 0xSwego July 29, 2025 05:53
@Faolain Faolain merged commit f400438 into ci/switch-setup-ipfs-gha Aug 21, 2025
3 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Aug 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants