PR-12: parity geometry-GF (#483)#512
PR-12: parity geometry-GF (#483)#512charliecreates[bot] wants to merge 1 commit intocharlie/issue-454-v2-pr12-geometry-classic-group11from
Conversation
|
orrery preview: https://pr-512.orrery-c4f.pages.dev/ |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The main risk in this diff is maintainability and cleanup correctness in the new cspice_runner.c geometry-gf cases, especially gfrepi, where the current goto done structure makes resource ownership fragile. The tspice dispatch additions are functionally fine but would benefit from explicit arity checks (for clearer errors) and some helper extraction to reduce index-heavy boilerplate. No obvious functional mismatches between the TS dispatch and the native runner were found for the added methods.
Additional notes (3)
- Readability |
packages/parity-checking/native/src/cspice_runner.c:893-911
gfrefnnow accepts boolean arguments viajsmn_parse_boolean, but other parts of the runner useparse_result-style return codes for richer error reporting (PARSE_INVALIDvsPARSE_RANGE, etc.). Returning onlytrue/falseloses detail and makes debugging malformed inputs harder (e.g., non-primitive token vs primitive-but-not-true/false).
Also, the function currently only accepts lowercase true/false. That’s fine for strict JSON, but if upstream ever sends True/False (non-JSON but sometimes seen), the error will be generic. If strict JSON is the contract, keep it strict, but then error messages should clearly indicate it expects JSON booleans.
- Readability |
packages/parity-checking/src/runners/tspiceRunner.ts:1590-1614
In the tspice dispatchers forgeometry-gf.gfsstp,geometry-gf.gfstep,geometry-gf.gfstol, andgeometry-gf.gfrefn, you validate types but do not validate the argument count. If a spec or caller passes fewer args,args[0]etc. will beundefined, which will correctly fail the type assertions—but the resulting error message will be about type mismatch rather than "missing arg".
This is minor, but it makes failures harder to triage, especially in a parity engine context where missing args often indicate spec bugs.
- Maintainability |
packages/parity-checking/src/runners/tspiceRunner.ts:1643-1737
The newgfsep/gfdistdispatchers are very repetitive (9 string checks + numeric checks + two window recipes + handle prep). This is manageable now, but it’s already a lot of boilerplate and will get worse as more geometry-gf methods are added.
Given this PR is part of a migration chain, the repetition increases the chance of subtle copy/paste bugs (wrong index, wrong error label, wrong recipe index).
Summary of changes
What changed
- Enabled
geometry-gfparity coverage by removing allgeometry-gf.*entries from the parity denylist:packages/parity-checking/catalogs/parity-denylist.jsonnow[]packages/parity-checking/catalogs/parity-denylist.tsnow exports an emptyPARITY_DENYLIST
- Added CSPICE runner support for Geometry Finder APIs in
packages/parity-checking/native/src/cspice_runner.c:- New JSON primitive parser:
jsmn_parse_boolean() - New call IDs and dispatch for:
geometry-gf.gfsstp,geometry-gf.gfstep,geometry-gf.gfstol,geometry-gf.gfrefn,geometry-gf.gfrepi,geometry-gf.gfrepf,geometry-gf.gfsep,geometry-gf.gfdist
- New JSON primitive parser:
- Added tspice runner dispatch entries for the same
geometry-gf.*methods inpackages/parity-checking/src/runners/tspiceRunner.ts, including argument validation and window handle lifecycle management. - Added method specs (v2) for the new
geometry-gfmethods underpackages/parity-checking/specs/methods/geometry-gf/*@v2.yml. - Added shared legacy workflow:
packages/parity-checking/workflows/legacy/geometry-gf.basic@v1.yml. - Updated completeness baselines & tests to reflect full coverage:
- Canonical method coverage:
165 → 173 - Denylist size:
8 → 0 - Updated assertions in integration/e2e/spec coverage tests.
- Canonical method coverage:
| case CALL_GEOMETRY_GF_GFREPI: { | ||
| if (tokens[argsTok].size < 3) { | ||
| write_error_json_ex("invalid_args", "geometry-gf.gfrepi expects args[0]=windowRecipe args[1]=begmss args[2]=endmss", NULL, | ||
| NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| int windowTok = jsmn_get_array_elem(tokens, argsTok, 0, tokenCount); | ||
| int begmssTok = jsmn_get_array_elem(tokens, argsTok, 1, tokenCount); | ||
| int endmssTok = jsmn_get_array_elem(tokens, argsTok, 2, tokenCount); | ||
|
|
||
| if (begmssTok < 0 || begmssTok >= tokenCount || tokens[begmssTok].type != JSMN_STRING) { | ||
| write_error_json_ex("invalid_args", "geometry-gf.gfrepi expects args[1] to be a string", NULL, | ||
| NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
| if (endmssTok < 0 || endmssTok >= tokenCount || tokens[endmssTok].type != JSMN_STRING) { | ||
| write_error_json_ex("invalid_args", "geometry-gf.gfrepi expects args[2] to be a string", NULL, | ||
| NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| RunnerCellRecipe windowRecipe; | ||
| char detail[256] = {0}; | ||
| if (!parse_cells_windows_recipe(input, tokens, tokenCount, windowTok, | ||
| &windowRecipe, detail, sizeof(detail))) { | ||
| write_error_json_ex("invalid_args", | ||
| "geometry-gf.gfrepi expects args[0] to be a cell/window recipe", | ||
| detail[0] ? detail : NULL, NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
| if (windowRecipe.kind != RUNNER_CELL_RECIPE_WINDOW) { | ||
| write_error_json_ex("invalid_args", | ||
| "geometry-gf.gfrepi expects args[0] to be [\"window\",maxIntervals]", | ||
| NULL, NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| SpiceCell *window = NULL; | ||
| bool isWindow = false; | ||
| if (!runner_alloc_cell_from_recipe(&windowRecipe, &window, &isWindow, detail, | ||
| sizeof(detail))) { | ||
| write_error_json_ex("invalid_args", | ||
| "geometry-gf.gfrepi window allocation failed", | ||
| detail[0] ? detail : NULL, NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
| (void)isWindow; | ||
|
|
||
| char *begmss = NULL; | ||
| char *endmss = NULL; | ||
|
|
||
| strDetail[0] = '\0'; | ||
| jsmn_strdup_err_t begmssErr = | ||
| jsmn_strdup(input, &tokens[begmssTok], &begmss, strDetail, sizeof(strDetail)); | ||
| if (begmssErr != JSMN_STRDUP_OK) { | ||
| runner_free_allocated_cell(window); | ||
| if (begmssErr == JSMN_STRDUP_INVALID) { | ||
| write_error_json_ex("invalid_request", "Invalid JSON string escape", | ||
| strDetail[0] ? strDetail : NULL, NULL, NULL, NULL); | ||
| } else { | ||
| write_error_json("Out of memory", NULL, NULL, NULL); | ||
| } | ||
| goto done; | ||
| } | ||
|
|
||
| strDetail[0] = '\0'; | ||
| jsmn_strdup_err_t endmssErr = | ||
| jsmn_strdup(input, &tokens[endmssTok], &endmss, strDetail, sizeof(strDetail)); | ||
| if (endmssErr != JSMN_STRDUP_OK) { | ||
| free(begmss); | ||
| runner_free_allocated_cell(window); | ||
| if (endmssErr == JSMN_STRDUP_INVALID) { | ||
| write_error_json_ex("invalid_request", "Invalid JSON string escape", | ||
| strDetail[0] ? strDetail : NULL, NULL, NULL, NULL); | ||
| } else { | ||
| write_error_json("Out of memory", NULL, NULL, NULL); | ||
| } | ||
| goto done; | ||
| } | ||
|
|
||
| gfrepi_c(window, begmss, endmss); | ||
| free(begmss); | ||
| free(endmss); | ||
| runner_free_allocated_cell(window); | ||
|
|
||
| if (failed_c() == SPICETRUE) { | ||
| char shortMsg[1841]; | ||
| char longMsg[1841]; | ||
| char traceMsg[1841]; | ||
| capture_spice_error(shortMsg, sizeof(shortMsg), longMsg, sizeof(longMsg), traceMsg, | ||
| sizeof(traceMsg)); | ||
| write_error_json("SPICE error in gfrepi", shortMsg, longMsg, traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| fputs("{\"ok\":true,\"result\":null}\n", stdout); | ||
| goto done; | ||
| } |
There was a problem hiding this comment.
geometry-gf.gfrepi allocates a SpiceCell *window via runner_alloc_cell_from_recipe, but on the success path it is freed before checking failed_c(). If gfrepi_c() signals a SPICE failure, the function jumps to done after printing the error, but the allocated cell is already freed, so that part is fine.
The actual issue is the error paths before the call: you do free window when begmss duplication fails and when endmss duplication fails, but any earlier goto done after runner_alloc_cell_from_recipe (e.g., after (void)isWindow; there are no more early gotos until duplication) is currently safe. However, this pattern is fragile and easy to break when adding new validation later; the goto done style across this case makes resource ownership hard to reason about.
More importantly: this case uses a different cleanup strategy than gfsep/gfdist (which clean up in a single block), increasing the odds of future leaks/double-frees when this grows.
Suggestion
Refactor this case to use a single cleanup: label and local ownership flags/pointers (initialized to NULL) so every goto after allocation goes through one cleanup path. That makes future edits much safer.
Example pattern:
case CALL_GEOMETRY_GF_GFREPI: {
SpiceCell *window = NULL;
char *begmss = NULL;
char *endmss = NULL;
// ... validation ...
if (!runner_alloc_cell_from_recipe(..., &window, ...)) {
write_error_json_ex(...);
goto cleanup;
}
if (jsmn_strdup(...) != JSMN_STRDUP_OK) { ...; goto cleanup; }
if (jsmn_strdup(...) != JSMN_STRDUP_OK) { ...; goto cleanup; }
gfrepi_c(window, begmss, endmss);
cleanup:
free(begmss);
free(endmss);
if (window) runner_free_allocated_cell(window);
if (failed_c() == SPICETRUE) { ... }
fputs("{\"ok\":true,\"result\":null}\n", stdout);
goto done;
}Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this refactor.
| const BASELINE_CANONICAL_METHOD_COVERAGE = 173; | ||
| const MAX_BASELINE_DENYLIST_SIZE = 0; |
There was a problem hiding this comment.
The completeness baselines were bumped (BASELINE_CANONICAL_METHOD_COVERAGE = 173, MAX_BASELINE_DENYLIST_SIZE = 0). This makes the guard much stricter and means any temporary denylisting will fail the guard. If denylisting is still expected as an escape hatch during migration chains, MAX_BASELINE_DENYLIST_SIZE = 0 may be too rigid and cause noisy failures for future PR stacks.
If the project intends to eliminate denylists entirely, this is fine—but then you should also ensure there’s a documented process for handling flaky/unsupported methods without denylisting (e.g., marking cases as skipped by capability detection).
Suggestion
If denylists are still a supported workflow during staged migrations, consider setting MAX_BASELINE_DENYLIST_SIZE to a small non-zero number (e.g., 2) and relying on tests to enforce it trends down over time, or introduce a separate guard that requires justification metadata for each denylisted method.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit adjusting the guard to allow a small denylist budget and/or require justification strings per entry.
1 participant
Stack step PR-12 in the issue #454 migration chain.