PR-20: remove v1 runtime (#454)#520
PR-20: remove v1 runtime (#454)#520charliecreates[bot] wants to merge 1 commit intocharlie/issue-454-v2-pr20-migrate-ephemeris-core-v2from
Conversation
|
orrery preview: https://pr-520.orrery-c4f.pages.dev/ |
There was a problem hiding this comment.
The v2-only migration is mostly clean, but there are a few sharp edges: schema validation now fails on missing schemaVersion with a generic message, the parity summary silently hard-codes aliasGuardValidatedCount: 0 after removing the alias guard, and the runner legacy-downgrade path (invokeLegacyCall) could use clearer intent/safety to prevent accidental routing through legacy dispatch. None of these are type/lint issues, but they affect migration ergonomics and API/reporting clarity.
Additional notes (4)
- Maintainability |
packages/parity-checking/src/dsl/schemaValidate.ts:705-713
parseMethodSpecAny()now rejects any file whereschemaVersionis missing. Previously, missingschemaVersionimplicitly meant legacy v1, which was intentional back-compat. If there are still real-world method YAMLs withoutschemaVersionin the repo or downstream usage, this change will fail fast with a less actionable error than "missing schemaVersion" and may break users unexpectedly.
Given the PR goal (remove v1 runtime), failing is fine—but the error should clearly distinguish between (a) missing and (b) wrong value, so migrations are straightforward.
- Compatibility |
packages/parity-checking/src/engine/parityEngine.ts:111-111
runParityEngine()now returnsaliasGuardValidatedCount: 0unconditionally by deleting the dispatch-alias parity guard. That’s a behavior change in the engine summary: consumers may interpret0as "ran and validated none" rather than "guard no longer applicable".
If ParityEngineSummary is used for reporting/CI gating, this can silently reduce coverage signal. At minimum, the summary should make it explicit that alias-guard validation is disabled/removed in v2-only mode, or the field should become optional/removed (depending on public API expectations).
- Maintainability |
packages/parity-checking/src/runners/cspiceRunner.ts:14-14
You’ve removedRunCaseInputV1and narrowedRunCaseInputto v2 only, butinvokeRunner()now accepts a union withLegacyInvokeInput(a local ad-hoc shape).
This reintroduces a “v1-like” payload into the call chain without a discriminant, which makes it easier for other call sites to accidentally pass legacy-shaped data (especially since LegacyInvokeInput is structurally compatible with many objects). It also makes the runner protocol less explicit.
If the goal is to keep the binary interface legacy while the internal engine is v2, prefer making the legacy conversion step private and type-safe (e.g. separate function invokeRunnerLegacy() or a tagged type) so legacy payloads can’t leak across boundaries.
- Maintainability |
packages/parity-checking/src/runners/tspiceRunner.ts:2048-2061
IntspiceRunner, the flow now prefers executing the legacy DISPATCH function whentoLegacyInvokeInput()succeeds, and otherwise falls back toexecuteV2CaseWithBackend(). This effectively givesinvokeLegacyCallhigher priority than any future v2-native execution for single-step workflows.
That’s fine if invokeLegacyCall is meant to remain a first-class escape hatch, but it should be intentional: otherwise a future v2 workflow that happens to match the "single step" shape could unexpectedly route through legacy dispatch (depending on how toLegacyInvokeInput() is implemented).
Summary of changes
Summary
This PR removes remaining v1 runtime paths from the parity-checking stack and standardizes execution on v2.
DSL validation
- Tightened
parseMethodSpecAny()andparseCrossCuttingSpecAny()to requireschemaVersion: 2and throw otherwise. - Removed legacy v1 parsing/compat behavior for both method and cross-cutting specs.
Parity engine
- Simplified
runParityEngine()to only execute v2 method specs viaexecuteMethodSpecParityV2(). - Removed v1 include-resolution / workflow-index path and the dispatch-alias parity guard execution.
- Deleted the now-unused
withTspiceRunner()helper.
Runners
- Updated
RunCaseInputto be v2-only. - Kept a compatibility shim by introducing an internal
LegacyInvokeInputshape so runners can still executeinvokeLegacyCallworkflows by converting v2 workflow input into the legacy{ call, args }invoke format. - Updated both
cspiceRunnerandtspiceRunnerto attempttoLegacyInvokeInput(input)unconditionally (since input is now always v2).
| type LegacyInvokeInput = { | ||
| setup?: RunCaseInput["setup"]; | ||
| call: string; | ||
| args: unknown[]; | ||
| }; | ||
|
|
There was a problem hiding this comment.
LegacyInvokeInput introduces args: unknown[], but the v2 runner input already has contract and structured args: unknown. The conversion path now allows a v2 workflow to downgrade into a legacy { call, args[] } invocation, but there’s no validation here that the legacy args being passed match what the legacy dispatcher expects.
This is probably intentional for invokeLegacyCall, but it’s easy for new v2 scenarios to accidentally hit this path (one-step workflow with invokeLegacyCall) and bypass v2 contract/arg handling semantics.
Suggestion
To make the downgrade path safer and harder to trigger accidentally:
- Consider requiring an explicit marker in the workflow step (beyond
op === "invokeLegacyCall") such as alegacy: trueboolean, or - At least add a runtime assertion that
step.argsis an array (if that’s the source) and fail with a clear message when it’s not.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit that adds a narrow runtime assertion + improved error message in toLegacyInvokeInput.
Stack step PR-20 in the issue #454 migration chain.