feat: Validate tool function arities at registration time (#42)

[andreasronge]

Summary

This PR implements tool function arity validation at registration time rather than call-time, providing:

• Fail-fast validation: Errors detected immediately when tools are passed to run/2
• Better DX: Clear error messages identifying which tools have wrong arity
• Earlier error detection: Invalid tools are caught before execution starts

Changes

Core Implementation

• Added validate_tools/1 private function in PtcRunner module
• Validates all tools have arity 1 when passed to run/2
• Returns {:error, {:validation_error, msg}} for invalid tools
• Error message clearly identifies which tools are invalid

Test Updates

• Updated existing test at line 3214 to expect validation_error instead of execution_error (since validation now happens earlier)
• Added 6 comprehensive new tests:
  • Test arity-0 function detection
  • Test arity-2 function detection
  • Test non-function value detection
  • Test multiple invalid tools reporting
  • Test valid arity-1 functions pass validation
  • Test empty tools map handling

Verification

• All 318 tests pass
• mix precommit passes (format, credo, compile checks)
• No breaking changes to public API

Fixes #42

[github-actions]

PR Review: feat: Validate tool function arities at registration time (#42)

Summary

This PR moves tool arity validation from call-time to registration-time, providing fail-fast behavior and better developer experience. The implementation is clean and well-tested.

What's Good

• Clean implementation: The validate_tools/1 function is concise and follows established patterns
• Consistent error type: Uses {:error, {:validation_error, msg}} matching existing validation patterns
• Good test coverage: 6 new tests covering various invalid tool scenarios (arity-0, arity-2, non-functions, multiple invalids, valid tools, empty map)
• Correct test update: Properly updated the existing test at line 3214 to expect validation_error instead of execution_error
• Error message quality: Messages clearly identify which tools are invalid

Issues (Must Fix)

1. Dead code in operations.ex - lib/ptc_runner/operations.ex:385-386
   • Problem: The old call-time arity validation branch is now unreachable dead code. Since tools are validated at registration time, the _tool_fn -> branch that returns "is not a function with arity 1" can never be reached.
   • Impact: Confusing for maintainers - suggests two validation paths when only one is active. Also makes the match clause at line 372 when is_function(tool_fn, 1) unnecessary since all tools are guaranteed to be arity-1 functions.
   • Fix: Remove the _tool_fn -> branch (lines 385-386) and simplify the case to only have two branches: nil -> (tool not found) and the function execution branch (without the guard). Alternatively, keep as defense-in-depth but update the error message to indicate this is an internal error.
   • Complexity: Mechanical - straightforward deletion

Suggestions (Optional)

1. Consider defense-in-depth vs clean code - Nice-to-have
   • Suggestion: If you intentionally keep the call-time validation as defense-in-depth, consider adding a comment explaining why. Otherwise, removing it makes the code cleaner.
   • Benefit: Either approach is valid; explicit documentation prevents future confusion
   • Complexity: Trivial

Security

No concerns. The change improves validation without introducing any security risks.

Verdict

Request Changes - The implementation correctly achieves the goal, but the dead code in operations.ex should be addressed before merge. This is a mechanical change that follows directly from the PR's purpose of moving validation to registration time.

[andreasronge]

@claude please fix these issues from PR review:

Issue 1: Remove dead code in operations.ex

Location: lib/ptc_runner/operations.ex:372-386
Problem: The catch-all branch at lines 385-386 that returns "Tool '#{tool_name}' is not a function with arity 1" is now unreachable dead code. Since tools are validated at registration time (in the new validate_tools/1 function), all tools in the registry are guaranteed to be arity-1 functions. Additionally, the guard when is_function(tool_fn, 1) at line 372 is now unnecessary.
Fix: Simplify the case statement to remove dead code:

1. Remove the guard when is_function(tool_fn, 1) from line 372 (change to just tool_fn ->)
2. Remove the catch-all _tool_fn -> branch at lines 385-386

This leaves only two branches:

• nil -> for tool not found
• tool_fn -> for executing the tool (without the guard since validation ensures it's arity-1)

Please fix this issue in a SINGLE commit. Make minimal changes to resolve the dead code.

[andreasronge]

Auto-Triage Summary

Decisions Made

#	Issue	Decision	Action
1	Dead code in operations.ex (lines 385-386)	FIX_NOW	Included in fix request above

Reasoning

Issue 1: Dead code removal (FIX_NOW)

• Complexity: Mechanical - straightforward deletion of unreachable code
• Scope: In-scope - The PR moved tool arity validation to registration time, which directly made the call-time validation branch unreachable. Completing this cleanup is part of the same logical change.
• Pattern: Clear fix - remove the guard when is_function(tool_fn, 1) and the catch-all _tool_fn -> branch that can never be reached now that all tools are validated at registration
• Impact: Improves code clarity by removing confusing dead code that suggests two validation paths when only one is active

The optional suggestion about defense-in-depth was not actioned because:

• The reviewer correctly identified this as dead code (not defense-in-depth)
• Removing it makes the code cleaner and less confusing
• If defense-in-depth was truly needed, the registration-time validation would need to be optional, which contradicts the PR's purpose

Status

• FIX_NOW items: 1 (batched in single fix comment above)
• Issues created: 0
• Items ignored: 0

All review findings have been addressed. Awaiting claude.yml workflow to apply the fix.

[andreasronge]

Fixed Dead Code Issue in operations.ex

The dead code in lib/ptc_runner/operations.ex:372-386 has been removed.

Changes Made

• Removed the guard clause when is_function(tool_fn, 1) from line 372
• Removed the unreachable catch-all _tool_fn -> branch at lines 385-386
• Simplified case statement to only handle:
  • nil → tool not found error
  • tool_fn → execute tool (no guard needed since validation happens at registration time)

Verification

• ✅ All 318 tests pass
• ✅ mix precommit passes (formatting, credo, compilation checks)
• ✅ Changes committed: 6f933c1

The fix removes the unreachable dead code since tools are now validated at registration time via the new validate_tools/1 function, guaranteeing all tools in the registry are arity-1 functions.

[github-actions]

PR Review: feat: Validate tool function arities at registration time (#42)

Summary

This PR moves tool arity validation from call-time to registration-time, providing fail-fast validation when tools are passed to run/2. The implementation is clean and follows established patterns in the codebase.

What's Good

1. Correct implementation location - validate_tools/1 in PtcRunner validates before creating the context, ensuring early failure
2. Proper error type - Uses {:error, {:validation_error, msg}} consistent with other validation errors
3. Clear error messages - Lists all invalid tool names, making debugging easy for library users
4. Dead code removal - Correctly removed the now-unnecessary call-time arity check in operations.ex:372-386
5. Comprehensive tests - 6 new tests covering: arity-0, arity-2, non-function values, multiple invalid tools, valid tools, and empty tools map
6. Updated existing test - Test at line 3214 properly updated to expect validation_error instead of execution_error

Issues (Must Fix)

None. The implementation is complete and consistent.

Suggestions (Optional)

1. OPTIONAL: Consider sorting tool names in error message - Nice-to-have
   • Suggestion: The error message lists invalid tools in iteration order (non-deterministic for maps). Sorting alphabetically would make error messages deterministic for testing.
   • Benefit: Consistent error messages across runs
   • Complexity: Trivial (add |> Enum.sort() before Enum.join)
   • Recommendation: Very minor, can be ignored or addressed in future polish work

Security

No concerns. The change improves security by catching misconfigurations earlier.

Verdict

Approve - The implementation is clean, complete, and well-tested. The PR correctly addresses issue #42 by moving tool validation from call-time to registration-time, with proper error handling and comprehensive test coverage.

[andreasronge]

Auto-Triage Summary (Cycle 2/3)

Latest Review Analysis

The latest review (2025-12-02T15:44:34Z) gave Approve verdict with no must-fix issues.

Decisions Made

#	Issue	Decision	Action
1	Sort tool names in error message (lib/ptc_runner.ex:127)	IGNORE	Truly optional style preference marked "can be ignored" by reviewer

Reasoning

Issue 1: Sort tool names (IGNORE)

• Reviewer's intent: Explicitly marked as "OPTIONAL", "Nice-to-have", and "Very minor, can be ignored or addressed in future polish work"
• Not tested: Tests use String.contains? which doesn't require deterministic ordering, so sorting provides no functional benefit
• Not a pattern: No other error messages in the codebase sort their lists
• No evidence of need: No flaky tests, no existing issues about error message determinism
• Design decision required: Adding sorting would establish a new standard for error messages across the project - this is an architectural decision, not a mechanical fix

Per the triage guidelines, this falls under "Truly optional stylistic preference with no functional benefit" → IGNORE.

Status

• FIX_NOW items: 0
• Issues created: 0
• Items ignored: 1

Next Steps

All review findings have been addressed. The PR has been approved with no required changes. Adding the ready-to-merge label.