Skip to content

fix(coder): restore package exports + CLAUDE.md fail-loudly compliance#823

Merged
kovtcharov merged 1 commit into
coderfrom
fix/coder-review-followups
Apr 20, 2026
Merged

fix(coder): restore package exports + CLAUDE.md fail-loudly compliance#823
kovtcharov merged 1 commit into
coderfrom
fix/coder-review-followups

Conversation

@kovtcharov
Copy link
Copy Markdown
Collaborator

Summary

Three review-followups from the #818 / #819 / #820 merge, flagged by the auto-review bot. All tests (73/73 on coder) pass with the fixes.

What this changes

  • Criticalsrc/gaia/coder/__init__.py: restore the CoderAgent / DEFAULT_LOOP / Loop / State / Transition re-exports that feat(coder): package skeleton + loop.py with DEFAULT_LOOP #819 added and feat(coder): File/CLI/Search mixins per §15.2 #818's rebase accidentally dropped. Without this, tests/coder/test_skeleton.py::test_package_imports fails on coder HEAD.

  • Importantsrc/gaia/coder/tools/cli.py:135: replace bare except Exception: pass in the stream-reader teardown with a targeted OSError catch + logger.debug. CLAUDE.md's No Silent Fallbacks rule explicitly forbids this pattern.

  • Importantsrc/gaia/coder/tools/search.py:59: stop reaching into the private _TOOL_REGISTRY from grep(). Use get_tool_metadata() — the public accessor at src/gaia/agents/base/tools.py:113 exists for this.

Test plan

  • pytest tests/coder/ -x — 73/73 pass (5 skeleton + 38 mixins + 30 stores)
  • python -c "from gaia.coder import CoderAgent, DEFAULT_LOOP, Loop, State, Transition" — all imports resolve

Three review-followups from the #818/#819/#820 merge:

- src/gaia/coder/__init__.py: restore the CoderAgent/DEFAULT_LOOP/Loop/State/Transition
  re-exports that #819 added and the #818 rebase accidentally dropped. test_package_imports
  in test_skeleton.py relies on these.

- src/gaia/coder/tools/cli.py: replace the bare except Exception: pass in the stream-reader
  teardown with a targeted OSError catch + logger.debug. Per CLAUDE.md's fail-loudly rule,
  silent swallows hide reader-thread bugs; a stream close failing under pipe tear-down is
  real and worth a debug line.

- src/gaia/coder/tools/search.py: stop reaching into the private _TOOL_REGISTRY dict from
  grep(). Use get_tool_metadata() — the public accessor added in base/tools.py:113 exists
  precisely for this.

Tests: all 73 (5 skeleton + 38 mixins + 30 stores) pass on coder HEAD with the fixes.
@kovtcharov kovtcharov merged commit 78148bf into coder Apr 20, 2026
6 checks passed
@kovtcharov kovtcharov deleted the fix/coder-review-followups branch April 20, 2026 09:37
@github-actions
Copy link
Copy Markdown
Contributor

Summary

Tight, well-scoped cleanup PR (+27/-8 across 3 files) addressing three bot-review findings from the #818/#819/#820 merge chain. Each change is a targeted fix with a clear rationale tied directly to a CLAUDE.md rule, the diff is easy to verify end-to-end, and the pre-existing test suite already covers the affected paths. The single most important thing to note: all three fixes are correct and the commit messages / inline comments teach the why, which is exactly the pattern the repo prefers.

Issues Found

None blocking. One very minor observation, listed below as 🟢.

🟢 Adjacent except ValueError: pass in the same function (src/gaia/coder/tools/cli.py:129-131) — While fixing the except Exception: pass at line 133-139, the sibling except ValueError: pass at lines 129-131 ("Stream closed mid-read.") was left untouched. It catches a specific type (not broad Exception) and the comment does explain the intent, so it isn't a direct CLAUDE.md violation — but it's the same shape of "silently swallow on teardown" pattern. Consider mirroring the new treatment for consistency:

        except ValueError as read_err:
            # Stream closed mid-read — expected when the child process exits
            # while readline() is blocked. Log at debug per CLAUDE.md fail-loudly;
            # nothing to recover here since the process is already tearing down.
            logger.debug("reader thread: stream read failed (%s)", read_err)

Not blocking — reasonable people can disagree on whether a narrow, commented catch needs further annotation. Flagging only for consistency with the fix you just landed three lines below.

Strengths

  • Public accessor used over private registry (src/gaia/coder/tools/search.py:54-67) — switching from _TOOL_REGISTRY["search_code"]["function"] to get_tool_metadata("search_code") is the right architectural move: _TOOL_REGISTRY is module-private (src/gaia/agents/base/tools.py), get_tool_metadata is explicitly documented there as "the public accessor for _TOOL_REGISTRY" (line 113), and the monkey-patch behaviour tests rely on is preserved because both paths hit the same dict.
  • Actionable error beats silent breakage — the new RuntimeError in grep() names what failed ("search_code tool is not registered") and what the caller should do ("SearchToolsMixin must be registered after FileToolsMixin"). That matches the CLAUDE.md "actionable errors name three things" guidance rather than just letting a KeyError/NoneType surface deep in the call stack.
  • Fail-loudly fix is correctly scoped (src/gaia/coder/tools/cli.py:135-139) — replaces except Exception: pass with a specific except OSError + logger.debug, with an inline comment explicitly citing the CLAUDE.md rule. OSError is the correct narrowing: a torn-down pipe raises OSError (or subclasses like BrokenPipeError), not the full Exception hierarchy. logger is already imported at line 13 — no dangling dependency.
  • Re-exports restore a previously-tested contract (src/gaia/coder/__init__.py:9-18)tests/coder/test_skeleton.py:48-51 (test_package_imports) exercises exactly these imports, so this fix lands the re-exports and the guard test simultaneously. Good rebase hygiene.
  • PR discipline — three unrelated-looking changes grouped correctly (all are "feat(coder): File/CLI/Search mixins per §15.2 #818/feat(coder): package skeleton + loop.py with DEFAULT_LOOP #819/feat(coder): SQLite stores per §15.1 DDL #820 review follow-ups"), each labelled with severity (Critical/Important/Important) and mapped to the exact file:line that flagged it. Exactly the shape CLAUDE.md's PR-description rules ask for.

Verdict

Approve. Fixes are correct, scope-clean, and directly traceable to CLAUDE.md rules. The 🟢 point on the adjacent except ValueError: pass is worth addressing in a follow-up but is not blocking — the bar for "bulletproof" was clearly met for the bugs this PR set out to fix.

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.

1 participant