feat(lisp): Normalize hyphenated keys to underscores at tool boundary

[andreasronge]

Summary

• Converts hyphenated PTC-Lisp keywords to underscored string keys at the tool boundary
• LLMs write Clojure-style :was-improved, Elixir tools receive %{"was_improved" => value}
• Follows Phoenix controller params conventions for consistency

Changes

1. Tool Arguments (eval.ex): stringify_key now normalizes hyphens to underscores
2. Return Values (loop.ex): Added normalize_return_keys for multi-turn and single-shot modes
3. Single-shot Mode (sub_agent.ex): Return values normalized in run_single_shot
4. JSON Mode (json_mode.ex): Return values normalized in build_success_step
5. Signature Validation (validator.ex): Field names normalized when looking up fields

Test Plan

• Updated all test assertions expecting atom keys to expect string keys
• Updated test "converts string keys to atoms" to "returns string keys (not atoms)"
• Fixed pre-existing validator test for default timeout value
• All tests pass (mix precommit)

Breaking Change

Return values from SubAgent now use string keys instead of atom keys:

# Before
%{result: 42, was_improved: true}

# After  
%{"result" => 42, "was_improved" => true}

Closes #704

🤖 Generated with Claude Code

Fix Automation State

Fix attempts: 2/3

[claude]

PR Review: Normalize hyphenated keys to underscores at tool boundary

Summary

This PR normalizes hyphenated PTC-Lisp keyword keys (like :was-improved) to underscored string keys ("was_improved") at the tool boundary, following Phoenix controller params conventions. The implementation is correct and comprehensive.

What's Good

• Clear separation of concerns: tool arguments normalized in eval.ex, return values normalized at each exit point
• Recursive handling of nested maps and lists is correct
• The validator fix properly normalizes field names for lookup
• Test updates are thorough and consistent
• The validator test fix (timeout: 5000 → timeout: 1000) corrects a pre-existing bug

Issues (Must Fix)

1. Code duplication - sub_agent.ex:1185-1197, loop.ex:924-936, json_mode.ex:769-781
   • Problem: The normalize_return_keys/1 and normalize_key/1 functions are copy-pasted identically across 3 files (18 lines × 3 = 54 lines of duplicated code)
   • Impact: Maintenance burden - any future bug fix or enhancement would need to be applied in 3 places
   • Fix: Extract to a shared module. Options:
     • Create PtcRunner.SubAgent.KeyNormalizer with public normalize_keys/1
     • Add to existing PtcRunner.Step since it operates on step return values
     • Use defmodule with use macro if private functions are preferred
   • Complexity: Mechanical refactoring - extract function, add import or alias

Suggestions (Optional)

1. File size - Several files exceed 800 lines:

   • lib/ptc_runner/sub_agent.ex: 1198 lines (+19 in this PR)
   • lib/ptc_runner/sub_agent/loop.ex: 937 lines (+24 in this PR)
   • These were already large before this PR, so this is out of scope. Worth a future GitHub issue to consider splitting.

2. Consistent naming - eval.ex:602 uses just normalize_key (for single string), while the other modules have both normalize_key and normalize_return_keys (for recursive map traversal). Consider aligning naming, though this is minor.

Security

No concerns. Key normalization is a string transformation with no security implications.

Documentation

No updates needed. This is an internal implementation detail that doesn't change the public API semantics - users still define fields with hyphens in PTC-Lisp.

Verdict

Request Changes - The code duplication must be addressed before merge. Extract the identical normalize_return_keys/1 and normalize_key/1 functions to a shared location. This is a ~15 minute mechanical fix.

[claude]

PR Review: feat(lisp): Normalize hyphenated keys to underscores at tool boundary

Summary

This PR implements hyphen-to-underscore key normalization at the tool boundary, converting Clojure-style :was-improved to Elixir-style "was_improved". The implementation is clean and well-tested. However, there are documentation issues and code duplication that need addressing.

What's Good

• Clean implementation: The new KeyNormalizer module is well-documented with doctests and proper typespecs
• Consistent application: Normalization is applied at all the right boundaries (tool arguments, return values in loop/single-shot/JSON modes, validator lookups)
• Comprehensive test updates: All test assertions correctly updated from atom keys to string keys
• Breaking change documented: PR description clearly notes the breaking change with before/after examples

Issues (Must Fix)

1. Code duplication - normalize_key/1 is implemented in two places

   • Location: lib/ptc_runner/lisp/eval.ex:601-602 and lib/ptc_runner/sub_agent/key_normalizer.ex:55-57
   • Problem: The same hyphen-to-underscore logic is duplicated with slightly different implementations
   • Impact: Maintenance burden, risk of divergence
   • Fix: Have eval.ex use KeyNormalizer.normalize_key/1 instead of its own private normalize_key/1
   • Complexity: Mechanical - add alias and replace function calls

2. Additional duplication in validator.ex - validator.ex:139

   • Location: lib/ptc_runner/sub_agent/signature/validator.ex:139
   • Problem: Inline String.replace(field_name, "-", "_") duplicates KeyNormalizer.normalize_key/1
   • Fix: Use KeyNormalizer.normalize_key/1 instead of inline replacement
   • Complexity: Mechanical

3. Missing documentation updates - Breaking change requires doc updates

   • Problem: Multiple documentation files show atom-key access patterns that no longer work:
     • docs/guides/subagent-getting-started.md: step.return.sentiment, step.return.score, etc.
     • docs/guides/subagent-testing.md: step.return.max, step.return.count
     • docs/specs/json-output-mode.md: Multiple step.return == %{key: value} assertions
   • Impact: Documentation is now incorrect for a breaking change
   • Fix: Update all documentation to use step.return["key"] syntax
   • Complexity: Mechanical find-replace

4. Missing doctest runner for KeyNormalizer

   • Problem: KeyNormalizer has doctests but no test file includes doctest PtcRunner.SubAgent.KeyNormalizer
   • Impact: Doctests won't run in CI, may drift from implementation
   • Fix: Add doctest PtcRunner.SubAgent.KeyNormalizer to an appropriate test file (or create test/ptc_runner/sub_agent/key_normalizer_test.exs)
   • Complexity: Trivial

Suggestions (Optional)

1. File size - Several files flagged exceed 800 lines (eval.ex, sub_agent.ex, loop.ex, test files)

   • This PR only adds minor lines to already-large files
   • No action needed for this PR, but worth tracking for future refactoring

2. Validator test fix - The change from 5000 to 1000 in validator_test.exs:32 appears to fix a pre-existing bug (doc says 1000, struct said 5000)

   • The PR description mentions this as "Fixed pre-existing validator test for default timeout value"
   • Nice catch, but note this is unrelated to the core PR scope

Security

No concerns - this is a data transformation change with no security implications.

Documentation

MUST FIX: Documentation must be updated to reflect the breaking change from atom keys to string keys. See Issue #3 above for specific files.

Verdict

Request Changes - The implementation is correct and well-tested, but the code duplication and missing documentation updates must be addressed before merge. All fixes are mechanical and straightforward.

🤖 Generated with Claude Code

[claude]

Auto-Triage Summary

Issue	Decision	Action
Code duplication: normalize_key/1 in eval.ex (line 601-602)	FIX_NOW	In fix request
Code duplication: inline String.replace in validator.ex (line 139)	FIX_NOW	In fix request
Missing documentation updates for breaking change (atom → string keys)	FIX_NOW	In fix request
Missing doctest runner for KeyNormalizer module	FIX_NOW	In fix request

Total: 4 issues found

• FIX_NOW: 4 (batched in single comment)
• DEFER_ISSUE: 0
• IGNORE: 0

Context: The initial code duplication issue (normalize_return_keys/1 across 3 files) was successfully fixed by creating the KeyNormalizer module. The latest review identified residual duplication in eval.ex and validator.ex, plus missing documentation updates for the breaking change from atom to string keys.

[claude]

PR Review: Normalize hyphenated keys to underscores at tool boundary

Summary

This PR introduces key normalization at the tool boundary, converting Clojure-style hyphenated keywords (e.g., :was-improved) to Elixir-style underscored string keys (e.g., "was_improved"). The implementation is clean with a well-documented KeyNormalizer module. However, there's a test bug and missing documentation updates.

What's Good

• Clean, focused implementation with a single-purpose KeyNormalizer module
• Good doctests with clear examples
• Recursive handling of nested maps and lists
• Consistent normalization at all boundaries (tool args, return values, JSON mode)
• Comprehensive test updates covering the new behavior

Issues (Must Fix)

1. Incorrect test assertion - test/ptc_runner/sub_agent/validator_test.exs:32

   • Problem: Changed assert agent.timeout == 5000 to assert agent.timeout == 1000, but the actual struct default in lib/ptc_runner/sub_agent.ex:220 is still timeout: 5000
   • Impact: This test will fail because the assertion doesn't match the actual default value
   • Fix: Revert to assert agent.timeout == 5000 - this change appears unrelated to the PR's purpose
   • Complexity: Mechanical (single line revert)

2. Missing documentation updates for breaking change

   • Problem: The PR description correctly identifies this as a breaking change (atom keys → string keys), but documentation still shows atom key access
   • Impact: Users following the docs will write step.return.sentiment instead of step.return["sentiment"]
   • Locations needing updates:
     • docs/guides/subagent-getting-started.md:59-60: step.return.sentiment → step.return["sentiment"]
     • docs/guides/subagent-getting-started.md:76-77: step.return.name, step.return.age
     • docs/guides/subagent-getting-started.md:96-97: step.return.name, step.return.price
     • docs/guides/subagent-testing.md:58: step.return.max
     • docs/guides/subagent-testing.md:74: step.return.count
     • docs/guides/subagent-testing.md:144: step.return.subject
     • docs/guides/subagent-concepts.md:132: step.return example shows atom keys
     • docs/specs/json-output-mode.md:557,575,663-664: Test examples use atom keys
   • Fix: Update all documentation examples to use string key access
   • Complexity: Mechanical (consistent pattern application)

Suggestions (Optional)

1. File size warning - Several files exceed the 800-line threshold (noted in review task), but the PR only adds minor changes to already-large files. Not actionable in this PR.

2. Consider adding a migration note - Nice-to-have: Add a note in the module doc or a CHANGELOG entry explaining how to migrate existing code from atom keys to string keys.

Security

No concerns - this is a data transformation at the boundary with no security implications.

Documentation

MUST FIX: Documentation examples throughout the guides need updating to reflect the breaking change from atom keys to string keys. See "Issues" section above for specific locations.

Verdict

Request Changes - The implementation is solid, but the test bug must be fixed and documentation must be updated before merge. Both issues are mechanical and straightforward to resolve.

[claude]

PR Review: feat(lisp): Normalize hyphenated keys to underscores at tool boundary

Summary

This PR implements key normalization at the tool boundary, converting Clojure-style hyphenated keywords (e.g., :was-improved) to Elixir-style underscored string keys (e.g., "was_improved"). The implementation is clean, well-tested, and addresses all previously identified issues. This is a breaking change that is properly documented.

What's Good

• Clean KeyNormalizer module: Well-documented with doctests, proper typespecs, and a dedicated test file that runs the doctests
• No code duplication: All files now use the shared KeyNormalizer module - eval.ex, loop.ex, json_mode.ex, sub_agent.ex, and validator.ex all alias and call KeyNormalizer.normalize_key/1 or KeyNormalizer.normalize_keys/1
• Comprehensive documentation updates: All guide docs (subagent-getting-started.md, subagent-testing.md, signature-syntax.md) updated to use string key access (step.return["key"])
• Test coverage: All test assertions properly updated from atom keys to string keys
• Breaking change properly documented: PR description includes clear before/after examples

Issues (Must Fix)

None found.

Suggestions (Optional)

1. File size - Several files exceed the 800-line threshold:

   • lib/ptc_runner/sub_agent.ex: 1180 lines (+5 in this PR)
   • lib/ptc_runner/sub_agent/loop.ex: 917 lines (+10 in this PR)

   The PR only adds minor changes to already-large files, so this is out of scope. Worth tracking for future refactoring.

Security

No concerns - this is a string transformation at data boundaries with no security implications.

Documentation

All relevant documentation has been updated to reflect the breaking change from atom keys to string keys. The spec file changes in docs/specs/json-output-mode.md are legitimate updates to the test examples (not weakening of requirements).

Spec Change Analysis

The changes to docs/specs/json-output-mode.md update test example assertions from atom keys (%{message: "hello"}) to string keys (%{"message" => "hello"}). This is a legitimate spec update that aligns the documented test examples with the new behavior. No requirements were weakened.

Verdict

Approve - The implementation is correct, well-tested, properly documented, and all previously identified issues (code duplication, missing docs) have been resolved. Ready to merge.

🤖 Generated with Claude Code

[claude]

Auto-Triage Summary

Issue	Decision	Action
No issues found in latest review	N/A	Added ready-to-merge label

Total: 0 issues found

• FIX_NOW: 0
• DEFER_ISSUE: 0
• IGNORE: 0

Context: All previously identified issues (code duplication, test bugs, missing documentation updates) have been resolved in subsequent commits. The latest review approved the PR with no issues found. The PR is mergeable and ready for merge.