Consolidate duplicate template expansion implementations

[andreasronge]

Context

PR #436 extracted shared placeholder extraction helpers to the Template module. However, there are still two identical expand_template/2 private functions that were not consolidated:

• lib/ptc_runner/sub_agent.ex:439-454
• lib/ptc_runner/sub_agent/loop.ex:409-424

Both are textually identical and perform the same "lenient" expansion (returning {{key}} unchanged on missing keys).

Problem

Code duplication between SubAgent.expand_template/2 and Loop.expand_template/2. Both functions:

• Use the same regex: ~r/\{\{\s*(\w+)\s*\}\}/
• Have identical logic for atom/string key fallback
• Return {{key}} unchanged when a key is missing

The existing Template.expand/2 returns {:error, {:missing_keys, [...]}} on missing keys (strict behavior), while these private functions need lenient behavior.

Solution

Add an on_missing option to Template.expand/2:

@spec expand(String.t(), map(), keyword()) :: {:ok, String.t()} | {:error, {:missing_keys, [String.t()]}}

# Default: strict - error on missing (current behavior)
Template.expand("{{name}}", %{})
# => {:error, {:missing_keys, ["name"]}}

# Opt-in: lenient - keep placeholders unchanged
Template.expand("{{name}}", %{}, on_missing: :keep)
# => {:ok, "{{name}}"}

Then replace the duplicate private functions with:

defp expand_template(prompt, context) do
  {:ok, result} = Template.expand(prompt, context, on_missing: :keep)
  result
end

Implementation Steps

1. Add on_missing option to Template.expand/2 (default :error for backward compatibility)
2. Add tests for the new :keep behavior
3. Replace expand_template/2 in SubAgent (line 439) with call to Template.expand/3
4. Replace expand_template/2 in Loop (line 409) with call to Template.expand/3
5. Verify all existing tests pass

Acceptance Criteria

• Template.expand/2 continues to error on missing keys (backward compatible)
• Template.expand("{{x}}", %{}, on_missing: :keep) returns {:ok, "{{x}}"}
• No duplicate expand_template/2 functions remain in SubAgent or Loop
• All existing tests pass
• Test at prompt_test.exs:57-65 continues to pass (handles missing variables gracefully)

Files to Modify

• lib/ptc_runner/sub_agent/template.ex - Add on_missing option
• lib/ptc_runner/sub_agent.ex - Replace private function
• lib/ptc_runner/sub_agent/loop.ex - Replace private function
• test/ptc_runner/sub_agent/template_test.exs - Add tests for :keep behavior

Notes

• The existing Prompt.expand_mission/2 already demonstrates the pattern for handling Template.expand/2 with lenient fallback - this consolidation will make that pattern reusable
• The private implementations don't support nested paths ({{user.name}}), but this is acceptable since those callers don't currently use nested paths

[claude]

Analysis

I've reviewed the three implementations and their usage:

1. **New **: Errors on missing keys (strict validation)
2. Old implementations (SubAgent:439, Loop:409): Return literal on missing keys (lenient)

Current Behavior is Intentional

The test at explicitly validates that missing template variables should be kept as literals:

test "handles missing template variables gracefully" do
  agent = SubAgent.new(prompt: "Find emails for {{user}}")
  context = %{}
  prompt = Prompt.generate(agent, context: context)
  # Should keep original template if expansion fails
  assert prompt =~ "Find emails for {{user}}"
end

This makes sense for the SubAgent use case where:

• The LLM sees the template placeholders in the system prompt
• Missing context is a valid state (user might provide it later)
• Showing {{user}} is more informative than an error

Recommendation: Option A (Add :on_missing option)

Add an :on_missing option to Template.expand/2:

# Default: error on missing (current behavior)
Template.expand("{{name}}", %{}) 
# => {:error, {:missing_keys, ["name"]}}

# Opt-in: keep placeholders
Template.expand("{{name}}", %{}, on_missing: :keep)
# => {:ok, "{{name}}"}

Then consolidate the duplicates by calling:

defp expand_template(prompt, context) do
  {:ok, result} = Template.expand(prompt, context, on_missing: :keep)
  result
end

Implementation Plan

1. Add on_missing option to Template.expand/2 (default :error)
2. Update tests to cover both behaviors
3. Replace duplicate expand_template/2 in SubAgent and Loop with calls to Template.expand/2
4. Verify all existing tests still pass

This maintains backward compatibility while eliminating duplication.

Ready to implement?

[claude]

Analysis

I've reviewed the three implementations and their usage:

1. New Template.expand/2: Errors on missing keys (strict validation)
2. Old implementations (SubAgent:439, Loop:409): Return literal {{key}} on missing keys (lenient)

Current Behavior is Intentional

The test at test/ptc_runner/sub_agent/prompt_test.exs:57-65 explicitly validates that missing template variables should be kept as literals:

test "handles missing template variables gracefully" do
  agent = SubAgent.new(prompt: "Find emails for {{user}}")
  context = %{}
  prompt = Prompt.generate(agent, context: context)
  # Should keep original template if expansion fails
  assert prompt =~ "Find emails for {{user}}"
end

This makes sense for the SubAgent use case where:

• The LLM sees the template placeholders in the system prompt
• Missing context is a valid state (user might provide it later)
• Showing {{user}} is more informative than an error

Recommendation: Option A (Add :on_missing option)

Add an :on_missing option to Template.expand/2:

# Default: error on missing (current behavior)
Template.expand("{{name}}", %{}) 
# => {:error, {:missing_keys, ["name"]}}

# Opt-in: keep placeholders
Template.expand("{{name}}", %{}, on_missing: :keep)
# => {:ok, "{{name}}"}

Then consolidate the duplicates by calling:

defp expand_template(prompt, context) do
  {:ok, result} = Template.expand(prompt, context, on_missing: :keep)
  result
end

Implementation Plan

1. Add on_missing option to Template.expand/2 (default :error)
2. Update tests to cover both behaviors
3. Replace duplicate expand_template/2 in SubAgent and Loop with calls to Template.expand/2
4. Verify all existing tests still pass

This maintains backward compatibility while eliminating duplication.

Ready to implement?

[claude]

Issue Review: Consolidate duplicate template expansion implementations

Summary: Clean tech-debt issue to consolidate two identical private expand_template/2 functions into the existing Template module by adding an on_missing option.

Pre-Review Checks

• Codebase health: ✅ Pass - All 2018 tests passing
• Higher-priority blockers: None - Other from-pr-review issues ([From PR #391] Consider String.to_existing_atom for signature field validation #392, [From PR #391] Extract shared type_name/describe_type helper to reduce duplication #393) are marked as deferred
• Issue claims verified: ✅ Confirmed
  • Duplicate functions exist at SubAgent:439-454 and Loop:409-424
  • They are textually identical
  • Template.expand/2 exists and uses strict error behavior

Analysis

1. Should This Be Done?: ✅ Yes - This is a natural follow-up to PR Extract shared placeholder extraction helpers #436. The duplication is real and the consolidation is straightforward.

2. Simplifications: The proposed solution is appropriately simple - a single option parameter rather than a new function.

3. Codebase Research: ✅ Verified

   • Template.expand/2 already exists with strict behavior (line 160-178)
   • Prompt.expand_mission/2 (line 519-523) already handles the lenient fallback pattern
   • Both private functions are identical and only used once each

4. Reuse Opportunities: ✅ Good - The solution reuses the existing Template.expand/2 function and its infrastructure.

5. Refactoring Needs: None - This IS the refactoring.

6. Edge Cases:

   • ✅ Backward compatibility: Default behavior remains :error
   • ✅ Existing test at prompt_test.exs:57-65 validates graceful handling
   • Note: Private implementations don't support nested paths ({{user.name}}), but callers don't use them

7. Issue Scope: ✅ Appropriate - Small, focused change. Single PR.

8. Test Plan: ✅ Clear - Add tests for :keep behavior, verify existing tests pass.

9. Solution Outline: ✅ Clear and actionable - Implementation steps are concrete.

Technical Issues Found

None. The proposal is sound.

Updates Made to Issue Body

• Restructured with clear sections (Context, Problem, Solution, Implementation Steps, Acceptance Criteria)
• Added specific line numbers for files to modify
• Added code examples showing the API design
• Added acceptance criteria checklist
• Noted the regex difference (private functions don't support nested paths) as acceptable

Recommended Labels

• ready-for-implementation - Issue is well-defined with clear solution
• enhancement - Already has appropriate labels (tech-debt, from-pr-review)

Verdict: ✅ Ready for implementation

[claude]

@claude Please implement this issue. Read the linked spec documents for context.