PR-07: parity time SCLK (#478)#507
Open
charliecreates[bot] wants to merge 1 commit intocharlie/issue-454-v2-pr07-ek-fast-write-group06from
Open
PR-07: parity time SCLK (#478)#507charliecreates[bot] wants to merge 1 commit intocharlie/issue-454-v2-pr07-ek-fast-write-group06from
charliecreates[bot] wants to merge 1 commit intocharlie/issue-454-v2-pr07-ek-fast-write-group06from
Conversation
|
orrery preview: https://pr-507.orrery-c4f.pages.dev/ |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
Contributor
Author
There was a problem hiding this comment.
The native CSPICE runner adds SCLK conversions, but scs2e/scencd currently leak sclkch when CSPICE reports an error due to early goto done paths. The new v2 method specs define contract.result as an empty object, which does not match the actual primitive result types returned by these methods (number/string), weakening or breaking contract validation. These issues should be addressed before relying on the new parity coverage.
Additional notes (1)
- Maintainability |
packages/parity-checking/native/src/cspice_runner.c:3159-3599
There is a lot of repetitive boilerplate across the newly addedCALL_TIME_*handlers (token count checks, parsingSpiceInt, parsingdouble,jsmn_strdup+ free,failed_c()handling). This increases maintenance burden and makes it easier for future handlers to accidentally diverge (e.g., inconsistent error codes/messages or missing frees on new branches).
Given this file is already large, adding another ~400 lines of mostly duplicated patterns is a long-term maintainability cost.
Summary of changes
What changed
Parity coverage updates
- Removed several
time.*SCLK conversion methods from the parity denylist:time.scs2e,time.sce2s,time.scencd,time.scdecd,time.sct2e,time.sce2c- Updated both
packages/parity-checking/catalogs/parity-denylist.jsonandpackages/parity-checking/catalogs/parity-denylist.ts.
CSPICE runner: new time/SCLK call support
- Extended the native runner (
packages/parity-checking/native/src/cspice_runner.c) to dispatch and execute six additional CSPICE calls:scs2e_c,sce2s_c,scencd_c,scdecd_c,sct2e_c,sce2c_c
- Added new
CallIdenum values, dispatch table entries, and full argument parsing / error handling per call.
Spec + workflow additions
- Added v2 method specs under
packages/parity-checking/specs/methods/time/:scs2e@v2.yml,sce2s@v2.yml,scencd@v2.yml,scdecd@v2.yml,sct2e@v2.yml,sce2c@v2.yml
- Added a legacy workflow:
packages/parity-checking/workflows/legacy/time.sclk.basic@v1.yml.
Guard/test baseline adjustments
- Updated completeness/parity baselines from 130 → 136 covered methods and denylist size from 43 → 37:
validateCompleteness.tscompletenessGuard.test.ts,parity.e2e.test.ts,parityScenarioCoverage.test.ts
tspiceRunner dispatch expansion
- Added dispatch handlers for the new
time.*methods inpackages/parity-checking/src/runners/tspiceRunner.ts.
Comment on lines
+3233
to
+3598
| fprintf(stdout, "{\"ok\":true,\"result\":%.17g}\n", (double)et); | ||
| goto done; | ||
| } | ||
|
|
||
| case CALL_TIME_SCE2S: { | ||
| if (tokens[argsTok].size < 2) { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.sce2s expects args[0]=number args[1]=number", | ||
| NULL, | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| int scTok = jsmn_get_array_elem(tokens, argsTok, 0, tokenCount); | ||
| int etTok = jsmn_get_array_elem(tokens, argsTok, 1, tokenCount); | ||
|
|
||
| SpiceInt sc = 0; | ||
| parse_result scParse = PARSE_INVALID; | ||
| if (scTok >= 0 && scTok < tokenCount) { | ||
| scParse = jsmn_parse_int(input, &tokens[scTok], &sc); | ||
| } | ||
| if (scTok < 0 || scTok >= tokenCount || scParse != PARSE_OK) { | ||
| if (scParse == PARSE_UNSUPPORTED) { | ||
| write_unsupported_spiceint_width_error(); | ||
| } else { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.sce2s expects args[0] to be an integer (SpiceInt range)", | ||
| scParse == PARSE_TOO_LONG ? "numeric literal too long" : NULL, | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| } | ||
| goto done; | ||
| } | ||
|
|
||
| SpiceDouble et = 0.0; | ||
| parse_result etParse = PARSE_INVALID; | ||
| if (etTok >= 0 && etTok < tokenCount) { | ||
| etParse = jsmn_parse_double(input, &tokens[etTok], &et); | ||
| } | ||
| if (etTok < 0 || etTok >= tokenCount || etParse != PARSE_OK) { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.sce2s expects args[1] to be a number", | ||
| etParse == PARSE_TOO_LONG | ||
| ? "numeric literal too long" | ||
| : (etParse == PARSE_OUT_OF_RANGE ? "numeric literal out of range" : NULL), | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| SpiceChar out[2048]; | ||
| out[0] = '\0'; | ||
| sce2s_c(sc, et, (SpiceInt)sizeof(out), out); | ||
|
|
||
| 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 sce2s", shortMsg, longMsg, traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| fputs("{\"ok\":true,\"result\":\"", stdout); | ||
| json_print_escaped(out); | ||
| fputs("\"}\n", stdout); | ||
| goto done; | ||
| } | ||
|
|
||
| case CALL_TIME_SCENCD: { | ||
| if (tokens[argsTok].size < 2) { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.scencd expects args[0]=number args[1]=string", | ||
| NULL, | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| int scTok = jsmn_get_array_elem(tokens, argsTok, 0, tokenCount); | ||
| int sclkchTok = jsmn_get_array_elem(tokens, argsTok, 1, tokenCount); | ||
|
|
||
| SpiceInt sc = 0; | ||
| parse_result scParse = PARSE_INVALID; | ||
| if (scTok >= 0 && scTok < tokenCount) { | ||
| scParse = jsmn_parse_int(input, &tokens[scTok], &sc); | ||
| } | ||
| if (scTok < 0 || scTok >= tokenCount || scParse != PARSE_OK) { | ||
| if (scParse == PARSE_UNSUPPORTED) { | ||
| write_unsupported_spiceint_width_error(); | ||
| } else { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.scencd expects args[0] to be an integer (SpiceInt range)", | ||
| scParse == PARSE_TOO_LONG ? "numeric literal too long" : NULL, | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| } | ||
| goto done; | ||
| } | ||
|
|
||
| if (sclkchTok < 0 || sclkchTok >= tokenCount || tokens[sclkchTok].type != JSMN_STRING) { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.scencd expects args[1] to be a string", | ||
| NULL, | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| char *sclkch = NULL; | ||
| strDetail[0] = '\0'; | ||
| jsmn_strdup_err_t sclkchErr = | ||
| jsmn_strdup(input, &tokens[sclkchTok], &sclkch, strDetail, sizeof(strDetail)); | ||
| if (sclkchErr != JSMN_STRDUP_OK) { | ||
| if (sclkchErr == 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; | ||
| } | ||
|
|
||
| SpiceDouble sclkdp = 0.0; | ||
| scencd_c(sc, sclkch, &sclkdp); | ||
| free(sclkch); | ||
|
|
||
| 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 scencd", shortMsg, longMsg, traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| fprintf(stdout, "{\"ok\":true,\"result\":%.17g}\n", (double)sclkdp); | ||
| goto done; | ||
| } | ||
|
|
||
| case CALL_TIME_SCDECD: { | ||
| if (tokens[argsTok].size < 2) { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.scdecd expects args[0]=number args[1]=number", | ||
| NULL, | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| int scTok = jsmn_get_array_elem(tokens, argsTok, 0, tokenCount); | ||
| int sclkdpTok = jsmn_get_array_elem(tokens, argsTok, 1, tokenCount); | ||
|
|
||
| SpiceInt sc = 0; | ||
| parse_result scParse = PARSE_INVALID; | ||
| if (scTok >= 0 && scTok < tokenCount) { | ||
| scParse = jsmn_parse_int(input, &tokens[scTok], &sc); | ||
| } | ||
| if (scTok < 0 || scTok >= tokenCount || scParse != PARSE_OK) { | ||
| if (scParse == PARSE_UNSUPPORTED) { | ||
| write_unsupported_spiceint_width_error(); | ||
| } else { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.scdecd expects args[0] to be an integer (SpiceInt range)", | ||
| scParse == PARSE_TOO_LONG ? "numeric literal too long" : NULL, | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| } | ||
| goto done; | ||
| } | ||
|
|
||
| SpiceDouble sclkdp = 0.0; | ||
| parse_result sclkdpParse = PARSE_INVALID; | ||
| if (sclkdpTok >= 0 && sclkdpTok < tokenCount) { | ||
| sclkdpParse = jsmn_parse_double(input, &tokens[sclkdpTok], &sclkdp); | ||
| } | ||
| if (sclkdpTok < 0 || sclkdpTok >= tokenCount || sclkdpParse != PARSE_OK) { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.scdecd expects args[1] to be a number", | ||
| sclkdpParse == PARSE_TOO_LONG | ||
| ? "numeric literal too long" | ||
| : (sclkdpParse == PARSE_OUT_OF_RANGE ? "numeric literal out of range" : NULL), | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| SpiceChar out[2048]; | ||
| out[0] = '\0'; | ||
| scdecd_c(sc, sclkdp, (SpiceInt)sizeof(out), out); | ||
|
|
||
| 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 scdecd", shortMsg, longMsg, traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| fputs("{\"ok\":true,\"result\":\"", stdout); | ||
| json_print_escaped(out); | ||
| fputs("\"}\n", stdout); | ||
| goto done; | ||
| } | ||
|
|
||
| case CALL_TIME_SCT2E: { | ||
| if (tokens[argsTok].size < 2) { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.sct2e expects args[0]=number args[1]=number", | ||
| NULL, | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| int scTok = jsmn_get_array_elem(tokens, argsTok, 0, tokenCount); | ||
| int sclkdpTok = jsmn_get_array_elem(tokens, argsTok, 1, tokenCount); | ||
|
|
||
| SpiceInt sc = 0; | ||
| parse_result scParse = PARSE_INVALID; | ||
| if (scTok >= 0 && scTok < tokenCount) { | ||
| scParse = jsmn_parse_int(input, &tokens[scTok], &sc); | ||
| } | ||
| if (scTok < 0 || scTok >= tokenCount || scParse != PARSE_OK) { | ||
| if (scParse == PARSE_UNSUPPORTED) { | ||
| write_unsupported_spiceint_width_error(); | ||
| } else { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.sct2e expects args[0] to be an integer (SpiceInt range)", | ||
| scParse == PARSE_TOO_LONG ? "numeric literal too long" : NULL, | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| } | ||
| goto done; | ||
| } | ||
|
|
||
| SpiceDouble sclkdp = 0.0; | ||
| parse_result sclkdpParse = PARSE_INVALID; | ||
| if (sclkdpTok >= 0 && sclkdpTok < tokenCount) { | ||
| sclkdpParse = jsmn_parse_double(input, &tokens[sclkdpTok], &sclkdp); | ||
| } | ||
| if (sclkdpTok < 0 || sclkdpTok >= tokenCount || sclkdpParse != PARSE_OK) { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.sct2e expects args[1] to be a number", | ||
| sclkdpParse == PARSE_TOO_LONG | ||
| ? "numeric literal too long" | ||
| : (sclkdpParse == PARSE_OUT_OF_RANGE ? "numeric literal out of range" : NULL), | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| SpiceDouble et = 0.0; | ||
| sct2e_c(sc, sclkdp, &et); | ||
|
|
||
| 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 sct2e", shortMsg, longMsg, traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| fprintf(stdout, "{\"ok\":true,\"result\":%.17g}\n", (double)et); | ||
| goto done; | ||
| } | ||
|
|
||
| case CALL_TIME_SCE2C: { | ||
| if (tokens[argsTok].size < 2) { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.sce2c expects args[0]=number args[1]=number", | ||
| NULL, | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| int scTok = jsmn_get_array_elem(tokens, argsTok, 0, tokenCount); | ||
| int etTok = jsmn_get_array_elem(tokens, argsTok, 1, tokenCount); | ||
|
|
||
| SpiceInt sc = 0; | ||
| parse_result scParse = PARSE_INVALID; | ||
| if (scTok >= 0 && scTok < tokenCount) { | ||
| scParse = jsmn_parse_int(input, &tokens[scTok], &sc); | ||
| } | ||
| if (scTok < 0 || scTok >= tokenCount || scParse != PARSE_OK) { | ||
| if (scParse == PARSE_UNSUPPORTED) { | ||
| write_unsupported_spiceint_width_error(); | ||
| } else { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.sce2c expects args[0] to be an integer (SpiceInt range)", | ||
| scParse == PARSE_TOO_LONG ? "numeric literal too long" : NULL, | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| } | ||
| goto done; | ||
| } | ||
|
|
||
| SpiceDouble et = 0.0; | ||
| parse_result etParse = PARSE_INVALID; | ||
| if (etTok >= 0 && etTok < tokenCount) { | ||
| etParse = jsmn_parse_double(input, &tokens[etTok], &et); | ||
| } | ||
| if (etTok < 0 || etTok >= tokenCount || etParse != PARSE_OK) { | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "time.sce2c expects args[1] to be a number", | ||
| etParse == PARSE_TOO_LONG | ||
| ? "numeric literal too long" | ||
| : (etParse == PARSE_OUT_OF_RANGE ? "numeric literal out of range" : NULL), | ||
| NULL, | ||
| NULL, | ||
| NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| SpiceDouble sclkdp = 0.0; | ||
| sce2c_c(sc, et, &sclkdp); | ||
|
|
||
| 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 sce2c", shortMsg, longMsg, traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| fprintf(stdout, "{\"ok\":true,\"result\":%.17g}\n", (double)sclkdp); | ||
| goto done; |
Contributor
Author
There was a problem hiding this comment.
time.scdecd and time.sct2e are comparing/printing SpiceDouble values with %.17g and using very tight tolerances (1e-12). SCLK conversions can be sensitive to kernel version/formatting and floating rounding; a cross-platform change in printf/libc behavior can cause occasional parity flakes even when the underlying value is effectively the same.
This is especially relevant for scencd/sce2c/scs2e which return floating values where representation can vary slightly across platforms/toolkit builds.
Suggestion
Consider standardizing numeric JSON output formatting for all double-returning calls to reduce platform variance, e.g. using json_print_double(et) helper (if one exists elsewhere in this file) or switching to a fixed format like %.17e consistently. Also consider relaxing default compare tolerances specifically for SCLK-double outputs if flakes are observed.
If you'd like, reply with "@CharlieHelps yes please" and I can add a commit that introduces a shared helper for printing doubles (and updates the new call sites to use it).
Comment on lines
+5
to
+12
| contract: | ||
| contractMethod: time.scs2e | ||
| canonicalMethod: time.scs2e | ||
| aliases: [] | ||
| result: | ||
| type: object | ||
| properties: {} | ||
| setup: |
Contributor
Author
There was a problem hiding this comment.
The new method specs set contract.result to type: object with empty properties: {} while the runner implementations return either a JSON number (for scs2e, scencd, sct2e, sce2c) or a JSON string (for sce2s, scdecd). An always-object schema here looks wrong and risks either (a) the harness not validating anything meaningful or (b) consumers misinterpreting expected shapes.
Suggestion
Update the contract.result schemas to match the actual result value types emitted by the runner.
For numeric-returning calls (scs2e, scencd, sct2e, sce2c):
contract:
...
result:
type: numberFor string-returning calls (sce2s, scdecd):
contract:
...
result:
type: stringIf the harness expects { ok: true, result: ... } shape at a higher level, consider moving this typing to the correct schema node (i.e., the schema field that corresponds specifically to result). Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
Comment on lines
+5
to
+12
| contract: | ||
| contractMethod: time.scencd | ||
| canonicalMethod: time.scencd | ||
| aliases: [] | ||
| result: | ||
| type: object | ||
| properties: {} | ||
| setup: |
Contributor
Author
There was a problem hiding this comment.
The new method specs define contract.result as an empty object (type: object, properties: {}) while the actual legacy calls return primitives (string or number) in the JSON response under result.
Even if your harness treats expect.ok: true as sufficient, this contract shape is misleading and reduces the value of the spec as documentation/validation.
Suggestion
Update each new method spec’s contract.result to reflect the actual return type:
time.scs2e,time.scencd,time.sct2e,time.sce2c→type: numbertime.sce2s,time.scdecd→type: string
If you'd like, reply with "@CharlieHelps yes please" and I can add a commit updating all 6 specs accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack step PR-07 in the issue #454 migration chain.