[WouterGlorieux]

SWE Pipeline — Automated Code Improvements

This pull request was automatically generated by the SWE Pipeline.

Summary

• 1 issues fixed: 1 bugs, 0 refactors, 0 enhancements
• Coverage: 100%
• Tests: all passing

What changed

See the commit history on the swe-pipeline/integration branch for the full list of changes.

Review

The pipeline performed multiple code reviews and all issues found were addressed. The codebase is at 100% test coverage with a green test suite.

Automatically generated at commit 82fe0e45.

[WouterGlorieux]

Summary

This PR addresses two related issues in the connect() function: (1) it was modifying the global multi_address even when the connection attempt failed, and (2) it wasn't properly catching StringParseError (and other errors) raised during the connection attempt, instead letting them propagate as raw exceptions rather than being wrapped in IPFSError. The fix moves the Multiaddr construction inside the try block and defers the global multi_address update until after a successful connection test. The change also adds new tests for the failure-preservation and success-update scenarios, and updates the README and CHANGELOG accordingly.

The intent of the fix is sound, and the error-handling improvement (catching StringParseError and re-raising as IPFSError) is correct. However, the implementation introduces a critical regression: _test_connection() reads the global multi_address to construct its client, but in the new connect(), the global is only updated after _test_connection() succeeds. This means the connection test always targets the previous address, not the new address the user is trying to connect to.

Changes Overview

• ipfs_dict_chain/IPFS.py: Modified connect() to construct the new Multiaddr inside the try block (so StringParseError is caught and re-raised as IPFSError), and to only update the global multi_address after _test_connection() succeeds.
• tests/test_IPFS.py: Removed the manual StringParseError workaround in test_connect_invalid_host; added test_connect_failure_preserves_multi_address, test_connect_catches_string_parse_error, and test_connect_updates_multi_address_on_success; imported Multiaddr and multi_address.
• README.md: Updated the connect() documentation to describe the new behavior (read-only client.id() test, no modification of global settings on failure).
• CHANGELOG.md: Added a changelog entry for the fix.

Issues & Concerns

1. Critical regression: _test_connection() tests the old address, not the new one.

The _test_connection() function (unchanged by this PR) constructs its client using the global multi_address: client = aioipfs.AsyncIPFS(maddr=multi_address). In the original connect(), multi_address was assigned before calling _test_connection(), so the test targeted the new address. In the new code, multi_address is only updated after _test_connection() succeeds:

def connect(host: str, port: int) -> None:
    global multi_address
    try:
        new_address = Multiaddr(f'/ip4/{host}/tcp/{port}')
        _ = _get_loop().run_until_complete(_test_connection())  # uses OLD multi_address!
        multi_address = new_address  # Only update on success
    except Exception as e:
        raise IPFSError(...)

Consequently, if a user is already connected to 127.0.0.1:5001 and calls connect('10.0.0.1', 5001), _test_connection() will attempt to reach 127.0.0.1:5001 (the old address), not 10.0.0.1:5001. If the old address is reachable but the new one is not, connect() will incorrectly succeed and set multi_address to an unreachable address. Conversely, if the old address is down but the new one is up, connect() will incorrectly fail. This defeats the entire purpose of the fix — the connection test is not actually validating the address being connected to.

Suggested fix: Either (a) temporarily set multi_address = new_address before calling _test_connection() and restore the old value on failure, or (b) better, refactor _test_connection() to accept an optional maddr parameter so it can test a specific address without mutating global state. Option (b) is cleaner and avoids the need for try/except rollback logic.

2. Test gap: test_connect_updates_multi_address_on_success masks the regression.

The new test test_connect_updates_multi_address_on_success connects to 127.0.0.1:5001, which is identical to the default multi_address. Since _test_connection() is mocked (it doesn't actually read the global), and since the test connects to the same address as the default, the bug described above is not exposed. A test that connects to a different address (e.g., 192.168.1.100, 5001) while mocking _test_connection to inspect which address it receives would catch this regression. The test should also verify that _test_connection was invoked with the correct (new) address.

3. Minor: Redundant test coverage.

test_connect_invalid_host and test_connect_catches_string_parse_error now test the exact same scenario — calling connect('invalid_host', 5001) and expecting IPFSError. The first was updated to remove the old StringParseError workaround, and the second was added as an explicit test. While not harmful, one of them is redundant and could be consolidated.

4. Minor: Nested error messages.

When _test_connection() raises an IPFSError (which it does when the connection fails), the except Exception block in connect() catches it and wraps it in another IPFSError. The resulting error message is nested, e.g.: "Failed to connect to IPFS daemon at /ip4/10.0.0.1/tcp/5001: Failed to connect to IPFS daemon at /ip4/127.0.0.1/tcp/5001: Connection refused". This is confusing because it mentions both the old and new addresses. Consider special-casing IPFSError (or checking isinstance(e, IPFSError)) to avoid double-wrapping.

Recommendation

Changes needed before merging. The fix's intent is correct and the error-handling improvement is good, but the implementation has a critical regression: _test_connection() is called before multi_address is updated, so it always tests the previous address rather than the new one being connected to. This defeats the purpose of the connection test and can lead to connect() succeeding against the wrong address or failing against a reachable one. The _test_connection() function should be refactored to accept the target address as a parameter (or connect() should temporarily set and roll back multi_address), and a test should be added that connects to a non-default address to verify the correct address is tested.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines	Coverage Δ
ipfs_dict_chain/IPFS.py	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[WouterGlorieux]

Summary

This PR addresses two related bugs in the connect() function of ipfs_dict_chain/IPFS.py: (1) the global multi_address was being modified even when the connection attempt failed, leaving the module in a broken state; and (2) StringParseError raised during Multiaddr construction was not caught and wrapped in IPFSError because the constructor call was outside the try block. The fix introduces a local new_address that is tested first via a new maddr parameter on _test_connection(), and only assigned to the global on success. It also moves Multiaddr() inside the try block and re-raises IPFSError directly to avoid double-wrapping.

The implementation is clean, the tests are comprehensive (37 tests passing with 100% coverage on IPFS.py), and the documentation (README.md, CHANGELOG.md) has been updated to reflect the new behavior accurately. After multiple review cycles, the core logic is sound and the edge cases are well covered.

Changes Overview

• ipfs_dict_chain/IPFS.py:

  • connect() now creates a local new_address, tests it via _test_connection(maddr=new_address), and only updates the global multi_address on success.
  • Multiaddr() construction is moved inside the try block so StringParseError and other construction errors are caught and wrapped in IPFSError.
  • IPFSError raised by _test_connection() is re-raised directly (via isinstance check) to avoid double-wrapping with redundant context.
  • _test_connection() now accepts an optional maddr: Multiaddr parameter (defaulting to the module-level multi_address) so it can test a specific address without relying on the global.

• tests/test_IPFS.py:

  • Added setUp/tearDown to save and restore the global multi_address for test isolation.
  • Added tests for: failure preserving multi_address, success updating it, IPFSError re-raised without double-wrapping, non-IPFSError wrapped in IPFSError, and the new address being passed to _test_connection.
  • Added a test verifying _test_connection uses the passed maddr parameter.
  • Simplified test_connect_invalid_host since connect() now handles StringParseError internally.

• README.md: Updated the connect() description to reflect that it uses a read-only client.id() operation and leaves the global settings unchanged on failure.

• CHANGELOG.md: Added an entry describing the fix.

Issues & Concerns

No critical issues identified. The changes correctly fix both reported bugs and the test coverage is thorough. One minor observation worth noting for future maintenance: the default parameter maddr: Multiaddr = multi_address on _test_connection() is bound at function definition time, not at call time. If the global multi_address is updated via connect() and _test_connection() is later called without arguments, it will still use the original default (127.0.0.1:5001) rather than the current global value. This does not affect any current code path since connect() always passes maddr explicitly, but the docstring ("Defaults to the global multi_address") is slightly misleading. Using maddr: Multiaddr | None = None with a runtime fallback to the global would be more robust for future callers.

Recommendation

Ready to merge. The PR correctly fixes the two reported issues (global state corruption on failed connection and uncaught StringParseError), the tests are comprehensive with 100% coverage on the modified module, and the documentation accurately reflects the new behavior. The minor default-parameter observation noted above is a latent concern for future code, not a blocker for this change.