Skip to content

test: add unit tests for src/tools/registry.zig covering getTool and getEnabledTools#53

Open
javimosch wants to merge 1 commit into
masterfrom
mago/task-52
Open

test: add unit tests for src/tools/registry.zig covering getTool and getEnabledTools#53
javimosch wants to merge 1 commit into
masterfrom
mago/task-52

Conversation

@javimosch

@javimosch javimosch commented Jun 25, 2026

Copy link
Copy Markdown
Owner

What changed

Added 6 unit tests directly in src/tools/registry.zig covering both public functions:

getTool

  • Hit: "bash" returns a non-null pointer with .name == "bash"
  • Miss: unknown name returns null

getEnabledTools

  • No-filter: null allowlist + null denylist → all 7 tools returned
  • Allowlist: ["bash", "read"] → 2 tools returned in that order
  • Denylist: ["bash"] → 6 tools returned, none named "bash"
  • Allowlist with unknown: ["bash", "no_such_tool"] → 1 tool (unknown silently skipped)

Why

Task #52: improve test coverage for the tool registry — a critical dispatch layer used by both agent.zig and acp.zig. Both functions are pure (no I/O), so tests need only std.testing.allocator with defer gpa.free(result).

Verification

zig build test
# Build Summary: 138/139 tests passed (1 pre-existing agents_md ABRT, unchanged)

Closes #52 (mago task #52)

Summary by CodeRabbit

  • Tests
    • Added coverage for tool lookup behavior, including matching known tools and handling unknown names.
    • Added coverage for enabled-tool filtering, including no filtering, allowlists, denylists, and allowlists with unknown entries.

…up and getEnabledTools paths

Adds 6 test cases directly in registry.zig:
- getTool hit: 'bash' returns non-null pointer with correct name
- getTool miss: unknown name returns null
- getEnabledTools no-filter: null allow+deny returns all 7 tools
- getEnabledTools allowlist: requested tools returned in order
- getEnabledTools denylist: denied tool excluded from results
- getEnabledTools allowlist with unknown: unknown names silently skipped

zig build test: 138/139 pass (1 pre-existing agents_md ABRT unchanged).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

javimosch pushed a commit that referenced this pull request Jun 25, 2026
…l hit/miss and getEnabledTools no-filter/allowlist/denylist/allowlist-with-unknown. zig build test 138/139 pass (1 pre-existing agents_md ABRT). Pushed mago/task-52, opened PR #53.
@javimosch

Copy link
Copy Markdown
Owner Author

Review — Head of Org Engineering: The diff adds unit tests for existing getTool and getEnabledTools functions in registry.zig, which is exactly what a test-addition PR should do. The Zig test syntax is valid, the tests are scoped to the correct file, and there are no correctness bugs, secrets, or destructive changes.

(approved — review-only mode; merge when ready.)

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f02ac9c8-5fa1-41c3-9d27-6288c6f98c8a

📥 Commits

Reviewing files that changed from the base of the PR and between 774298b and 1d6914f.

📒 Files selected for processing (1)
  • src/tools/registry.zig

📝 Walkthrough

Walkthrough

Adds unit tests in src/tools/registry.zig for tool lookup and enabled-tool filtering behavior, covering direct hits, misses, allowlists, denylists, and unknown names.

Changes

Registry lookup and filtering tests

Layer / File(s) Summary
Lookup and filtering coverage
src/tools/registry.zig
Adds tests for getTool and getEnabledTools, including no-filter results, allowlist ordering, denylist exclusion, and skipping unknown allowlist entries.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A bunny hopped through registry lanes,
Checking tools and naming chains.
Bash was found, unknowns were light,
Allow and deny were trimmed just right.
Hop-hop! The tests now gleam tonight. 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly states the main change: adding unit tests for getTool and getEnabledTools in src/tools/registry.zig.
Linked Issues check ✅ Passed The added tests match issue #52 by covering getTool lookup and getEnabledTools for allowlist, denylist, and no-filter paths.
Out of Scope Changes check ✅ Passed The PR only adds tests for the targeted registry functions and stays within the linked issue's scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mago/task-52

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

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

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.

Add unit tests for src/tools/registry.zig covering getTool lookup and getEnabledTools with allowlist, denylist, and no-filter paths

1 participant