fix(compare): add type discriminator to reliability metrics output

[EdwardIrby]

Summary

• Adds type: 'run' discriminator to ReliabilityMetricsSchema for capture result comparisons
• Adds type: 'trial' discriminator to TrialsReliabilityMetricsSchema for trial comparisons
• Enables consumers to use Zod z.discriminatedUnion() for type-safe parsing of reliability metrics

Breaking Change

Consumers with strict Zod schemas will need to add the type field:

• type: z.literal('run') for ReliabilityMetrics
• type: z.literal('trial') for TrialsReliabilityMetrics

Test plan

• bun run check passes
• bun test passes (499 tests)
• Verified type discriminator appears in comparison output

Closes #42

🤖 Generated with Claude Code

[claude]

Pull Request Review: Add type discriminator to reliability metrics output

Summary

This PR adds type: 'run' and type: 'trial' discriminators to ReliabilityMetricsSchema and TrialsReliabilityMetricsSchema respectively. This is a breaking change that enables consumers to use z.discriminatedUnion() for type-safe parsing when handling both types of reliability metrics.

Code Quality ✅

Strengths:

• Clean, focused implementation with minimal changes
• Follows existing patterns in the codebase (see lines 306-311 in schemas.ts where discriminated unions are already used for TrajectoryStepSchema)
• Type discriminators are properly added to both schema definitions and runtime objects
• Consistent use of literal types (z.literal('run') and z.literal('trial'))

Adherence to conventions:

• ✅ Uses type for schema definitions (schemas.ts:691, 880)
• ✅ Arrow functions used throughout
• ✅ TSDoc comments remain accurate
• ✅ No any types introduced
• ✅ Follows existing discriminated union pattern

Test Coverage ✅

Excellent test coverage:

• Tests verify the type discriminator appears in output (compare-statistical.spec.ts:106-107, compare-trials.spec.ts:111-112, 289)
• Both 'run' and 'trial' discriminators are tested
• Tests cover both weighted and statistical strategies
• Integration tests verify JSON output structure
• Markdown output tests ensure discriminator is handled correctly

Test quality:

• Uses descriptive test names
• Proper setup/teardown with beforeAll/afterAll
• Tests actual file I/O operations
• Verifies both positive and edge cases

Breaking Change Considerations ⚠️

Well-documented breaking change:

• PR description clearly explains the breaking change
• Breaking change is justified: enables type-safe discriminated unions
• Consumers will get clear Zod validation errors if they don't update

Migration path:

// Consumers need to update schemas:
- type: z.literal('run')      // for ReliabilityMetrics
- type: z.literal('trial')    // for TrialsReliabilityMetrics

Recommendation: Consider adding a migration guide or example to the PR description showing how consumers can use z.discriminatedUnion() after this change.

Potential Issues 🔍

Minor observations:

1. Schema consistency (compare.ts:467, compare-trials.ts:205): The type field is hardcoded in two places. This is acceptable but could benefit from a comment explaining why it's set at runtime rather than inferred from schema.

2. No version bump mentioned: This is a breaking change that should trigger at least a minor version bump (or major if following semver strictly). Ensure this is tracked in your release process.

Security Concerns ✅

No security concerns identified:

• No user input validation changes
• No new attack surfaces introduced
• Type discriminators are safe literal values
• No SQL injection, XSS, or other OWASP risks

Performance Considerations ✅

Minimal performance impact:

• Adding a literal field has negligible memory overhead
• No additional computation required
• Discriminated unions may actually improve parsing performance by enabling fast-path type narrowing

Suggestions for Improvement

1. Add usage example to PR description:

import { z } from 'zod'
import { ReliabilityMetricsSchema, TrialsReliabilityMetricsSchema } from './schemas'

const UnifiedReliabilitySchema = z.discriminatedUnion('type', [
  ReliabilityMetricsSchema,
  TrialsReliabilityMetricsSchema,
])

// Type-safe parsing with automatic narrowing
const result = UnifiedReliabilitySchema.parse(data)
if (result.type === 'run') {
  // TypeScript knows this is ReliabilityMetrics
  console.log(result.toolErrors)
} else {
  // TypeScript knows this is TrialsReliabilityMetrics
  console.log(result.avgPassExpK)
}

2. Documentation update: Consider updating any API documentation or README that mentions these schemas.

3. Changelog entry: Ensure CHANGELOG.md is updated with this breaking change if you maintain one.

Verdict ✅ APPROVED

This is a well-implemented, properly tested change that improves type safety. The breaking change is justified, clearly documented, and follows best practices for discriminated unions. The code quality is excellent and aligns with the repository's conventions.

Recommendations before merge:

• Add usage example to PR description (optional but helpful)
• Ensure version bump is planned for next release
• Update CHANGELOG.md if maintained

Great work on maintaining test coverage and following the existing patterns! 🎉

[claude]

Pull Request Review: Add type discriminator to reliability metrics output

Summary

This PR adds type: 'run' and type: 'trial' discriminators to ReliabilityMetricsSchema and TrialsReliabilityMetricsSchema. The implementation enables consumers to use z.discriminatedUnion() for type-safe parsing when handling both types of reliability metrics.

Code Quality ✅

Strengths:

• Clean, focused implementation with minimal changes
• Follows existing discriminated union pattern (schemas.ts:306-311 for TrajectoryStepSchema)
• Type discriminators properly added to both schema definitions and runtime objects
• Consistent use of literal types (z.literal('run') and z.literal('trial'))
• TSDoc comments are accurate and follow project standards

Adherence to conventions:

• ✅ Uses type for schema definitions (core.md convention)
• ✅ Arrow functions used throughout
• ✅ No any types introduced
• ✅ No interface usage (follows type-over-interface convention)
• ✅ No @ts-ignore directives
• ✅ TSDoc inline comments follow documentation standards

Test Coverage ✅

Excellent test coverage:

• compare-statistical.spec.ts:106-107 verifies type: 'run' for both runs
• compare-trials.spec.ts:111-112, 289 verifies type: 'trial' for trial metrics
• Tests cover both weighted and statistical strategies
• Integration tests verify JSON output structure
• Tests use test() not it() (follows testing.md convention)
• Proper test organization with describe() blocks

Coverage checklist:

• ✅ Happy path (basic comparison with discriminators)
• ✅ Edge cases (empty results handled in compare-trials.ts:198)
• ✅ Both discriminator values tested
• ✅ Integration with existing features (confidence intervals, markdown output)

Breaking Change Considerations ⚠️

Well-documented breaking change:

• PR description clearly explains the breaking change
• Breaking change is justified: enables type-safe discriminated unions
• Consumers will get clear Zod validation errors if they don't update schemas
• Excellent documentation added to SKILL.md with usage example (lines 755-780)

Migration path is clear:

// Consumers need to add type discriminator:
type: z.literal('run')      // for ReliabilityMetrics
type: z.literal('trial')    // for TrialsReliabilityMetrics

Recommendation: Ensure this triggers appropriate semantic versioning (minor or major bump).

Potential Issues 🔍

No significant issues found. Minor observations:

1. Hardcoded discriminators (compare.ts:467, compare-trials.ts:205, 210): The type field is set at runtime. This is the correct approach for discriminated unions, as the discriminator should be set by the producer. No changes needed.

2. Documentation completeness: The new usage example in SKILL.md (lines 755-780) is excellent and clearly demonstrates the benefit of this change.

Security Concerns ✅

No security concerns identified:

• No user input validation changes
• No new attack surfaces introduced
• Type discriminators are safe literal values
• No SQL injection, XSS, command injection, or other OWASP risks
• Follows principle of least privilege

Performance Considerations ✅

Minimal performance impact:

• Adding a literal field has negligible memory overhead (~8 bytes per object)
• No additional computation required
• Discriminated unions may actually improve parsing performance by enabling fast-path type narrowing in Zod

Architecture & Design ✅

Follows best practices:

• Discriminated unions are a TypeScript/Zod best practice for tagged union types
• Aligns with existing patterns in the codebase (TrajectoryStepSchema)
• Maintains backward compatibility in structure (only adds a required field)
• Schemas remain composable and extensible

Documentation ✅

Excellent documentation:

• SKILL.md updated with comprehensive usage example (lines 755-780)
• Shows practical benefit of the change (type narrowing)
• comparison-graders.md reference updated with example output
• TSDoc comments added for new discriminator fields
• PR description includes clear breaking change notice

Suggestions for Improvement

1. Version bump tracking: Ensure CHANGELOG.md is updated (if maintained) and semantic version is bumped appropriately for this breaking change.

2. Migration guide (optional): Consider adding a migration guide in SKILL.md or README for existing consumers, though the error messages from Zod will be clear enough.

3. Type guard utilities (optional future enhancement): Consider exporting type guard functions:

export const isRunReliabilityMetrics = (m: unknown): m is ReliabilityMetrics =>
  ReliabilityMetricsSchema.safeParse(m).success

Verdict ✅ APPROVED

This is a well-implemented, properly tested change that improves type safety and developer experience. The breaking change is justified, clearly documented, and follows best practices for discriminated unions. Code quality is excellent and fully aligns with repository conventions.

Recommendations before merge:

• Ensure semantic version bump is planned (suggest minor or major)
• Update CHANGELOG.md if maintained
• Consider updating any external API documentation

Great work on:

• Following existing discriminated union patterns
• Comprehensive test coverage (499 tests passing!)
• Excellent documentation with practical examples
• Clean, focused implementation

Closes #42 ✅

---

Review conducted following .plaited/rules conventions: core.md, testing.md, accuracy.md, documentation.md