fix/bug hunt critical fixes by gbrennon · Pull Request #292 · forging-blocks-org/forging-blocks

gbrennon · 2026-07-24T03:43:37Z

chore(release): bump version to 1.0.0
Revert "chore(release): bump version to 1.0.0"
fix(foundation): prevent FieldErrors iterator exhaustion on has_errors check
fix(infrastructure): consolidate InMemoryRepository diamond init storage assignment
fix(foundation): add auto_freeze to foundation package exports and all
test(infrastructure): add missing assertion after message bus query dispatch

This reverts commit 5ec5128.

…s check The method was calling then checking , which tested the now-exhausted iterator instead of the materialized tuple. Replaced with — the already-materialized errors tuple — avoiding silent false negatives when a field has errors.

…age assignment The cooperative super() chain in the InMemoryRepository diamond hierarchy (double-inheriting InMemoryReadRepository and InMemoryWriteRepository) caused _storage to be set twice: once by super().__init__() and once by the subclass. Consolidated to a single assignment after super().__init__, removing the fragile dependency on statement ordering.

…all__ auto_freeze was importable from its submodule but not re-exported from the foundation package, unlike auto_eq and auto_hash which were already in __all__. This caused 4 doc examples using the canonical import 'from forging_blocks.foundation import auto_freeze' to break. Added both the import and __all__ entry.

…ispatch The test_dispatches_and_returns_result test called bus.dispatch(query) but never assigned or asserted the return value, making the test pass regardless of dispatch behavior. Added result assignment and equality assertion to verify correct dispatch output.

Copilot

Pull request overview

This PR applies targeted fixes and small API/test improvements across the Forging Blocks toolkit’s foundation and infrastructure layers, focusing on correctness and reliability in common building blocks (errors, repositories, and the in-memory message bus).

Changes:

Improves FieldErrors construction by avoiding iterator exhaustion when validating provided errors.
Adjusts InMemoryRepository initialization to ensure a single, explicit storage assignment for the composed read/write repository.
Exposes auto_freeze via forging_blocks.foundation and strengthens message-bus query dispatch test assertions.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
tests/forging_blocks/infrastructure/message_bus/test_in_memory_message_bus.py	Adds an assertion ensuring query dispatch returns the handler’s result.
src/forging_blocks/infrastructure/repositories/in_memory_repository.py	Refactors storage initialization for the combined in-memory repository.
src/forging_blocks/foundation/errors/field_errors.py	Prevents iterator exhaustion by validating against the materialized error sequence.
src/forging_blocks/foundation/init.py	Re-exports `auto_freeze` at the foundation package boundary.

+        if not self._errors or not field:
            raise ValueError("FieldErrors must contain at least one error and field defined.")

        message = ErrorMessage(f"{len(self._errors)} error(s) for field '{field}'.")


…ng Raises section

…onsistency

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

+        super().__init__()
+        self._storage: dict[TId, TEntity] = dict(storage) if storage is not None else {}


        """Write bytes to a file, creating parent directories."""
        target = Path(path)
-        target.parent.mkdir(parents=True, exist_ok=True)
+        await asyncio.to_thread(lambda: target.parent.mkdir(parents=True, exist_ok=True))


    Raises:
-        urllib.error.HTTPError: On HTTP 4xx/5xx responses.
-        urllib.error.URLError: On network/connection failures.
+        OSError: On network or connection failures.
        ConfigurationError: On misconfigured URLs (e.g., non-HTTP schemes).


personal-reviewer

AI Code Review

Verdict: Changes Requested

The PR makes several important improvements including adding auto_freeze, improving test coverage, and fixing potential iterator exhaustion issues. However, security risks in file system operations and potential test compatibility issues from exception type changes require attention. The code generally follows good practices in async handling and module exports.

Issues

[security] [MINOR] src/forging_blocks/infrastructure/file_system/os_file_system.py

Path input validation is missing in read/write operations. User-provided paths could lead to security risks if not properly sanitized.

Current:

async def read(self, path: Path | str) -> bytes:
        async def write(self, path: Path | str, data: bytes) -> None:
            """Write bytes to a file, creating parent directories."""
            target = Path(path)
            await asyncio.to_thread(lambda: target.parent.mkdir(parents=True, exist_ok=True))
            await asyncio.to_thread(target.write_bytes, data)

Suggested:

async def read(self, path: Path | str) -> bytes:
        path = Path(path).resolve()
        if not path.is_absolute():
            raise ValueError("Path must be absolute")
        async def write(self, path: Path | str, data: bytes) -> None:
            """Write bytes to a file, creating parent directories."""
            target = Path(path).resolve()
            if not target.parent.exists():
                target.parent.mkdir(parents=True, exist_ok=True)
            await asyncio.to_thread(target.write_bytes, data)

[testability] [MINOR] src/forging_blocks/infrastructure/http_client/urllib_client.py

Exception type change from specific urllib errors to generic OSError may break existing test assumptions and reduce testability.

Current:

Raises:
-        urllib.error.HTTPError: On HTTP 4xx/5xx responses.
-        urllib.error.URLError: On network/connection failures.
+        OSError: On network or connection failures.

Suggested:

Raises:
-        urllib.error.HTTPError: On HTTP 4xx/5xx responses.
-        urllib.error.URLError: On network/connection failures.
+        urllib.error.HTTPError: On HTTP 4xx/5xx responses.
+        urllib.error.URLError: On network/connection failures.
+        # Note: These exceptions are preserved for compatibility with existing test expectations.

Praise

src/forging_blocks/foundation/init.py: Properly added auto_freeze to all exports, maintaining consistent module interface and improving type safety.
tests/forging_blocks/infrastructure/message_bus/test_in_memory_message_bus.py: Improved test coverage by adding assertion for dispatch result, ensuring correct handler behavior is validated.
src/forging_blocks/infrastructure/file_system/os_file_system.py: Correctly uses asyncio.to_thread for blocking operations, maintaining async compatibility while handling file system operations.

Suggestions

No suggestions to apply.

Reviewed by code-review:latest

personal-reviewer

AI Code Review

Verdict: Approved

The changes adhere to SOLID principles, improve type safety, and address resource management concerns. All modifications are well-documented and include necessary test assertions. No critical issues were found.

Issues

No issues found.

Praise

src/forging_blocks/foundation/init.py: Properly updated all with auto_freeze and included the necessary import, maintaining module consistency.
src/forging_blocks/infrastructure/file_system/os_file_system.py: Correctly uses asyncio.to_thread for blocking filesystem operations, ensuring async compatibility.
tests/forging_blocks/infrastructure/message_bus/test_in_memory_message_bus.py: Added a meaningful assertion to validate message bus dispatch behavior, improving test coverage.

Suggestions

No suggestions to apply.

Reviewed by code-review:latest

gbrennon added 9 commits July 22, 2026 04:31

Merge remote-tracking branch 'origin/main'

833a849

Merge remote-tracking branch 'origin/main'

fcfbbf6

chore(release): bump version to 1.0.0

5ec5128

Revert "chore(release): bump version to 1.0.0"

2646c9f

This reverts commit 5ec5128.

Merge branch 'main' of github.com:forging-blocks-org/forging-blocks

48b1465

gbrennon temporarily deployed to pypi July 24, 2026 03:45 — with GitHub Actions Inactive

gbrennon requested a review from Copilot July 24, 2026 03:48

Copilot started reviewing on behalf of gbrennon July 24, 2026 03:49 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread src/forging_blocks/foundation/errors/field_errors.py

Comment on lines +30 to 33

if not self._errors or not field:

raise ValueError("FieldErrors must contain at least one error and field defined.")

message = ErrorMessage(f"{len(self._errors)} error(s) for field '{field}'.")

gbrennon added 4 commits July 24, 2026 03:58

fix(foundation): remove dead OkType TypeVar from Ok result variant

45f1402

fix(foundation): remove dead ErrType TypeVar from Err result variant

9ff44c2

docs(infrastructure): correct exception types in URLLibClient docstri…

27572bb

…ng Raises section

fix(infrastructure): wrap mkdir call in asyncio.to_thread for async c…

11e27b5

…onsistency

gbrennon requested a review from Copilot July 24, 2026 07:04

gbrennon deployed to pypi July 24, 2026 07:04 — with GitHub Actions Active

Copilot started reviewing on behalf of gbrennon July 24, 2026 07:04 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

personal-reviewer requested changes Jul 24, 2026

View reviewed changes

personal-reviewer approved these changes Jul 25, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix/bug hunt critical fixes#292

fix/bug hunt critical fixes#292
gbrennon wants to merge 13 commits into
mainfrom
fix/bug-hunt-critical-fixes

gbrennon commented Jul 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

personal-reviewer left a comment

Uh oh!

personal-reviewer left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		super().__init__()
		self._storage: dict[TId, TEntity] = dict(storage) if storage is not None else {}

Uh oh!

Conversation

gbrennon commented Jul 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

personal-reviewer left a comment

Choose a reason for hiding this comment

AI Code Review

Issues

Current:

Suggested:

Current:

Suggested:

Praise

Suggestions

Uh oh!

personal-reviewer left a comment

Choose a reason for hiding this comment

AI Code Review

Issues

Praise

Suggestions

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants