PR-11: parity geometry classic (#482)

[charliecreates]

Stack step PR-11 in the issue #454 migration chain.

• Issue: Parity denylist group 11: Geometry surface/intercept/illumination (9 methods) #482
• Head: charlie/issue-454-v2-pr12-geometry-classic-group11
• Base: charlie/issue-454-v2-pr11-ephemeris-spk-group10

[github-actions]

orrery preview: https://pr-511.orrery-c4f.pages.dev/

[charliecreates]

The native runner additions are functional but have two high-risk correctness/robustness problems: (1) repeated memory leaks on CSPICE failure paths due to goto done after failed_c() without freeing allocations, and (2) unsafe reliance on SpicePlane struct layout by directly reading/writing plane.normal and plane.constant, which can break across CSPICE versions. Separately, the new v2 method specs define empty result schemas (properties: {}), reducing the ability of parity to catch output-shape regressions. Addressing these will make the new geometry coverage much more stable and maintainable.

Additional notes (1)

• Maintainability | packages/parity-checking/src/runners/tspiceRunner.ts:120-120
assertNumberArg only checks typeof value === "number", but many SPICE calls will behave poorly (or fail in surprising ways) with NaN/Infinity. This diff adds several new geometry entry points that accept et and other numeric inputs; accepting non-finite numbers increases the risk of hard-to-debug parity failures.

You already enforce Number.isFinite in assertSpkPackedDescriptor; geometry should be held to the same standard.

Summary of changes

What changed

• Enabled classic geometry method parity coverage by removing these methods from the parity denylist:
  • geometry.subpnt, geometry.subslr, geometry.sincpt, geometry.ilumin, geometry.illumg, geometry.illumf, geometry.occult, geometry.nvc2pl, geometry.pl2nvc
• Extended the native CSPICE runner (packages/parity-checking/native/src/cspice_runner.c):
  • Added new CallId enum entries and parse_call_id mappings for the geometry calls.
  • Implemented JSON arg parsing, CSPICE invocation, and JSON result serialization for each new geometry call.
• Added v2 method specs under packages/parity-checking/specs/methods/geometry/*@v2.yml for the new geometry APIs.
• Added a legacy workflow definition: workflows/legacy/geometry.basic@v1.yml.
• Updated completeness baselines & tests to reflect increased coverage:
  • Canonical covered methods: 156 → 165
  • Denylist size: 17 → 8
  • Updated assertions in validateCompleteness and multiple test suites.
• Updated tspiceRunner dispatch to support new geometry methods and added argument validators (assertPlane4).

[charliecreates]

CALL_GEOMETRY_NVC2PL serializes a SpicePlane by reading plane.normal[] and plane.constant directly. In CSPICE, SpicePlane is an opaque-ish type whose internal representation is not part of the public contract; accessing fields directly risks ABI/implementation breakage across CSPICE versions/build flags.

This is especially brittle because you later reconstruct a SpicePlane in pl2nvc by writing to those same fields, doubling down on the assumption of layout.

Suggestion

Avoid direct struct field access for SpicePlane. Prefer CSPICE helper APIs to convert between representations:

• For nvc2pl: you already have normal and konst; just return those instead of the struct internals, or use a supported accessor (e.g., pl2nvc_c) on the produced plane to extract normal/konst for serialization.
• For pl2nvc: accept { normal, konst } (or two args) and call nvc2pl_c then pl2nvc_c, or (better) update the wire format so you never have to materialize a plane from undocumented internals.

Concretely, safest is to define the JSON plane representation as { "normal": [..], "konst": n } and use only nvc2pl_c/pl2nvc_c to cross the boundary.

Reply with "@CharlieHelps yes please" if you'd like me to add a commit that changes the serialization format and updates the TS runner/specs accordingly.

[charliecreates]

The method specs declare contract.result with type: object and properties: {} for all newly added geometry calls. That effectively removes any schema-level checking of the result shape and can hide regressions (e.g., missing fields like found, spoint, etc.). Given these are parity tests, being explicit about expected result structure is valuable.

At minimum, include the top-level shape and required properties for stable outputs (and handle conditional outputs like sincpt's found).

Suggestion

Tighten the contract.result schemas for the new geometry specs. Example for sincpt:

contract:
  result:
    oneOf:
      - type: object
        required: [found]
        properties:
          found: { const: false }
      - type: object
        required: [found, spoint, trgepc, srfvec]
        properties:
          found: { const: true }
          spoint:
            type: array
            minItems: 3
            maxItems: 3
            items: { type: number }
          trgepc: { type: number }
          srfvec:
            type: array
            minItems: 3
            maxItems: 3
            items: { type: number }

Do similarly for subpnt/subslr (object with spoint, trgepc, srfvec), ilumin/illumg/illumf (numbers + arrays + booleans for illumf), occult (integer), and plane methods.

Reply with "@CharlieHelps yes please" if you'd like me to add a commit updating the YAML contracts for these methods.

[charliecreates]

For geometry.pl2nvc, the runner accepts a length-4 array and treats it as a SpicePlane (normal[3] + constant). SPICE plane structures often assume the normal is a unit vector (or at least non-zero); passing a zero normal or denormalized vector can produce undefined/degenerate results.

Since the TS side now has assertPlane4, it’s a good place to prevent obviously-invalid planes before invoking native code.

Suggestion

Harden assertPlane4 by rejecting a zero normal vector (and optionally non-finite elements):

function assertPlane4(value: unknown, label: string): asserts value is Plane4 {
  // existing checks...
  const [nx, ny, nz] = value as unknown as number[];
  const n2 = nx * nx + ny * ny + nz * nz;
  if (!Number.isFinite(n2) || n2 === 0) {
    invalidArgs(`${label} expects a non-zero normal vector (got ${formatValue(value)})`);
  }
}

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.