[From PR #75] Test runner regenerates random data causing validation inconsistency

[andreasronge]

Context

From PR #75 review - the demo's test runner has an architectural issue with data consistency.

Problem

PtcDemo.TestRunner.get_last_result/0 (lines 226-230) regenerates fresh random data by calling SampleData functions.

However, the Agent already has datasets loaded in state at init time (agent.ex:70-75) and executes programs against those datasets (agent.ex:110).

This means test validation runs against DIFFERENT data than what the program was executed against, potentially causing false positives/negatives in test results.

Suggested Fix

1. Add last_result field to Agent state struct
2. Store the execution result alongside last_program in agent.ex:146
3. Change get_last_result/0 to fetch the stored result from Agent state instead of re-executing
4. Or alternatively, add a public API like Agent.last_result/0 to retrieve the stored result

Impact

• Low severity (tests use constraint-based assertions which are somewhat resilient)
• Affects demo reliability and test accuracy
• Could cause confusing test failures when random data diverges

Files Affected

• demo/lib/ptc_demo/agent.ex (state management)
• demo/lib/ptc_demo/test_runner.ex (get_last_result/0)

[andreasronge]

This issue affects only the demo code (not the core library) and has low severity. Deferring until after #72 (CHANGELOG) is completed, as:

1. Issue [Phase 5] Add CHANGELOG.md for Hex package release #72 is already approved and ready for implementation
2. This tech debt is isolated to the demo's test runner
3. The demo tests still pass with constraint-based assertions
4. No functional impact on the core PtcRunner library

Adding needs-review label so this issue goes through proper review workflow.

[andreasronge]

Issue Review: Test runner regenerates random data causing validation inconsistency

Summary: This is a valid bug in the demo's test runner. TestRunner.get_last_result/0 regenerates fresh random data instead of using the datasets already stored in Agent state, causing validation to run against different data than execution.

Analysis:

1. Should This Be Done?: Yes. The issue identifies a real consistency bug. While constraint-based assertions mask the problem in practice, this is architectural debt that makes the test runner unreliable and could cause confusing failures.

2. Simplifications: The proposed solution is already simple. Adding a last_result field or exposing the datasets is a minimal change.

3. Codebase Research: Verified the issue accurately describes the code:

   • Agent.init/1 (lines 70-75): Loads datasets ONCE at startup with random generation
   • Agent.handle_call({:ask, _}, ...) (line 110): Executes against state.datasets
   • TestRunner.get_last_result/0 (lines 226-230): Regenerates FRESH random data via SampleData.products(), etc.
   • The SampleData module uses :rand.uniform and Enum.random - each call produces different data

4. Reuse Opportunities: The Agent already has last_program field pattern - last_result follows the same pattern.

5. Refactoring Needs: None. The change is additive.

6. Edge Cases:

   • What if last_result/0 is called before any query? Should return nil like last_program/0
   • Memory: Storing the result is fine - results are typically small aggregations, not full datasets

7. Issue Scope: Appropriate size for a single PR. Two files, minimal changes.

8. Test Plan: The issue mentions constraint-based assertions are currently masking the problem. No new tests are strictly needed since the existing test runner will continue to work. However, the fix makes the tests more reliable.

9. Solution Outline: The issue provides two options. I recommend Option 2 (storing last_result in Agent state) as it's cleaner:

   Implementation steps:

   1. Add last_result field to Agent struct (agent.ex:21): defstruct [:model, :context, :datasets, :last_program, :last_result, :mode]
   2. Store result in state after execution (agent.ex:146): %{state | context: new_context, last_program: program_json, last_result: result}
   3. Add Agent.last_result/0 public API alongside last_program/0
   4. Add handle_call(:last_result, ...) callback
   5. Update TestRunner.get_last_result/0 to call Agent.last_result/0 instead of re-running
   6. Clear last_result on reset (agent.ex:161)

Technical Issues Found:

• None. The issue correctly identifies the problem and proposes valid solutions.

Minor Improvement to Issue:

• The suggested fix should include clearing last_result on Agent.reset/0 (similar to last_program)

Recommended Labels:

• ready-for-implementation - The issue is well-specified with clear implementation path
• bug - This is a correctness issue, not an enhancement

Verdict: Ready for implementation. The issue correctly diagnoses the problem, identifies the affected files, and proposes a reasonable solution. A junior developer could implement this.

[andreasronge]

@claude Please implement this issue:

1. First, read the review comments on this issue
2. If the review identified issues (inconsistencies, missing details, etc.), update the issue description to address them
3. Implement the issue following the (updated) specification
4. Create a PR with your implementation

Make sure your implementation addresses any concerns raised in the review.

[andreasronge]

Implementation Complete

Summary: Fixed the test runner validation inconsistency bug by storing execution results in Agent state.

What was done:

1. Added last_result field to Agent struct (agent.ex:21)
2. Stored result in Agent state after successful execution (agent.ex:147)
3. Added Agent.last_result/0 public API (agent.ex:41-43)
4. Added handle_call(:last_result, ...) callback (agent.ex:175-178)
5. Updated Agent.reset/0 to clear last_result (agent.ex:167)
6. Simplified TestRunner.get_last_result/0 to use stored result instead of regenerating data (test_runner.ex:220-228)

Result: The test runner now validates against the exact same data used during execution. All 318 tests + 3 doctests pass with 0 failures.

PR: #79