PR-08: parity error-state API (#479)#508
PR-08: parity error-state API (#479)#508charliecreates[bot] wants to merge 1 commit intocharlie/issue-454-v2-pr08-time-sclk-group07from
Conversation
|
orrery preview: https://pr-508.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 biggest issue is that error.chkout is being made deterministic by implicitly calling chkin inside the method implementation (both TS and native), which changes semantics and can mask real parity differences. The new native/TS handlers are also inconsistent about rejecting extra arguments (several accept >= 1 rather than == 1). Finally, the new v2 method specs use overly-generic result schemas that don’t match the actual return shapes (boolean/string/null), reducing contract signal.
Additional notes (1)
- Maintainability |
packages/parity-checking/src/guards/validateCompleteness.ts:7-8
The completeness baselines (BASELINE_CANONICAL_METHOD_COVERAGE,MAX_BASELINE_DENYLIST_SIZE) are updated as raw numbers. This pattern tends to create churn and provides limited signal when the catalog changes (it forces updating constants rather than deriving expectations).
Since you already compute the summary in tests, consider making the guard check derive from the contract catalog at runtime (or pin against a snapshot file) so changes are reviewable as data diffs instead of magic-number edits.
Summary of changes
What changed
✅ Added v2 method coverage for CSPICE error-state APIs
- New method specs under
packages/parity-checking/specs/methods/error/*@v2.ymlfor:error.failed,error.reset,error.getmsg,error.setmsg,error.sigerr,error.chkin,error.chkout
- New legacy workflow descriptor:
packages/parity-checking/workflows/legacy/error.basic@v1.yml
🧩 Enabled legacy runner support in native CSPICE harness
packages/parity-checking/native/src/cspice_runner.c- Added new
CallIdvariantsCALL_ERROR_* - Registered dispatch strings like
"error.getmsg"inparse_call_id() - Implemented switch cases to invoke corresponding CSPICE calls and emit JSON responses
- Added new
🔁 Wired up TSPICE runner dispatch for the new methods
packages/parity-checking/src/runners/tspiceRunner.ts- Added dispatch handlers for the
error.*methods, including deterministic priming viasetmsg()forreset,getmsg, andsigerr
- Added dispatch handlers for the
📉 Reduced denylist and updated baseline/test expectations
- Removed
error.*entries from:packages/parity-checking/catalogs/parity-denylist.jsonpackages/parity-checking/catalogs/parity-denylist.ts
- Updated completeness baselines and test assertions:
- Coverage
136 → 143 - Denylist size
37 → 30 - Files touched:
validateCompleteness.ts,completenessGuard.test.ts,parity.e2e.test.ts,parityScenarioCoverage.test.ts
- Coverage
| "error.getmsg": (backend, args) => { | ||
| if (typeof args[0] !== "string") { | ||
| invalidArgs(`error.getmsg expects args[0] to be a string (got ${formatValue(args[0])})`); | ||
| } | ||
| if (args[0] !== "SHORT" && args[0] !== "LONG" && args[0] !== "EXPLAIN") { | ||
| invalidArgs( | ||
| `error.getmsg expects args[0] to be "SHORT", "LONG", or "EXPLAIN" (got ${formatValue(args[0])})`, | ||
| ); | ||
| } | ||
|
|
||
| // Prime a stable long-message payload because getmsg() reads global buffers. | ||
| backend.setmsg("TSPICE parity getmsg long message"); | ||
| return backend.getmsg(args[0]); | ||
| }, |
There was a problem hiding this comment.
error.getmsg validates only args[0] but doesn’t reject extra arguments. In the native runner, CALL_ERROR_GETMSG only checks tokens[argsTok].size < 1, so extra args are silently accepted there too. This creates inconsistent API behavior versus other handlers like error.failed/error.reset which explicitly enforce zero args.
Given these are contract-level method specs, it’s better to be strict and fail fast on unexpected parameters to keep parity deterministic and avoid tests accidentally passing while sending junk inputs.
Suggestion
Tighten argument-count validation to match a strict contract surface. For example:
"error.getmsg": (backend, args) => {
if (args.length !== 1 || typeof args[0] !== "string") {
invalidArgs(
`error.getmsg expects args[0]=string and no extra args (got ${formatValue(args)})`,
);
}
if (args[0] !== "SHORT" && args[0] !== "LONG" && args[0] !== "EXPLAIN") {
invalidArgs(
`error.getmsg expects args[0] to be "SHORT", "LONG", or "EXPLAIN" (got ${formatValue(args[0])})`,
);
}
backend.setmsg("TSPICE parity getmsg long message");
return backend.getmsg(args[0]);
},Then mirror the same strictness in cspice_runner.c for CALL_ERROR_GETMSG (and similarly for SETMSG/SIGERR/CHKIN/CHKOUT) by rejecting tokens[argsTok].size != 1.
Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
| "error.chkout": (backend, args) => { | ||
| if (typeof args[0] !== "string") { | ||
| invalidArgs(`error.chkout expects args[0] to be a string (got ${formatValue(args[0])})`); | ||
| } | ||
|
|
||
| // `chkout` is deterministic after matching `chkin` within the same case. | ||
| backend.chkin(args[0]); | ||
| backend.chkout(args[0]); | ||
| return null; | ||
| }, |
There was a problem hiding this comment.
error.chkout is implemented by calling chkin_c(name) and then chkout_c(name) to force determinism. However, the API name suggests it should only perform chkout (and error if the call stack is unbalanced). Auto-priming inside the method changes semantics and can mask real contract behavior differences.
If the goal is determinism for the spec case, the priming should live in the workflow (pre-steps) or in the YAML case setup, not in the method implementation itself.
Suggestion
Move the chkin priming out of the error.chkout implementation and into a deterministic workflow/case setup.
Options:
-
Introduce a workflow for these error-state tests (e.g.
error.basic@v2) with steps:invokeLegacyCallforerror.chkininvokeLegacyCallforerror.chkout
-
If the framework supports it, add a
before/setup step to theerror.chkout.basiccase rather than altering the dispatch.
Then error.chkout dispatch becomes:
"error.chkout": (backend, args) => {
if (typeof args[0] !== "string") {
invalidArgs(`error.chkout expects args[0] to be a string (got ${formatValue(args[0])})`);
}
backend.chkout(args[0]);
return null;
},Mirror this in the native runner by removing the chkin_c(name) prime.
Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
| "error.reset": (backend) => { | ||
| // Prime mutable error buffers so reset() is exercised deterministically. | ||
| backend.setmsg("TSPICE parity reset prelude"); | ||
| backend.reset(); | ||
| return null; | ||
| }, |
There was a problem hiding this comment.
error.reset primes the long-message buffer via setmsg_c(...) but doesn’t verify that priming didn’t itself signal a SPICE error before calling reset_c(). You do check failed_c() after setmsg_c() in the native runner; the TS runner currently doesn’t. If backend.setmsg() can fail (and parity is specifically about error-state APIs), this can cause confusing downstream failures (e.g., reset failing due to a prior failure you didn’t surface).
This is especially relevant because error.reset is often used to clear state; if the prelude fails, you’re no longer testing reset() from a known precondition.
Suggestion
After priming via backend.setmsg(...), explicitly check/clear error state (depending on your backend API). For example, if the backend exposes failed() and/or throws on failure, enforce parity with the native behavior.
One possible approach:
"error.reset": (backend) => {
backend.setmsg("TSPICE parity reset prelude");
if (backend.failed()) {
// surface as a deterministic failure rather than continuing
throw new Error("SPICE error in setmsg (reset prelude)");
}
backend.reset();
return null;
},If your runner uses structured errors instead of throwing, adapt accordingly.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| case CALL_ERROR_GETMSG: { | ||
| if (tokens[argsTok].size < 1) { | ||
| write_error_json_ex("invalid_args", "error.getmsg expects args[0]=string", | ||
| NULL, NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| int whichTok = jsmn_get_array_elem(tokens, argsTok, 0, tokenCount); | ||
| if (whichTok < 0 || whichTok >= tokenCount || | ||
| tokens[whichTok].type != JSMN_STRING) { | ||
| write_error_json_ex("invalid_args", | ||
| "error.getmsg expects args[0] to be a string", NULL, | ||
| NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| char *which = NULL; | ||
| strDetail[0] = '\0'; | ||
| jsmn_strdup_err_t whichErr = | ||
| jsmn_strdup(input, &tokens[whichTok], &which, strDetail, | ||
| sizeof(strDetail)); | ||
| if (whichErr != JSMN_STRDUP_OK) { | ||
| if (whichErr == 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; | ||
| } | ||
|
|
||
| if (strcmp(which, "SHORT") != 0 && strcmp(which, "LONG") != 0 && | ||
| strcmp(which, "EXPLAIN") != 0) { | ||
| free(which); | ||
| write_error_json_ex( | ||
| "invalid_args", | ||
| "error.getmsg expects args[0] to be \"SHORT\", \"LONG\", or \"EXPLAIN\"", | ||
| NULL, NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| // Prime a stable long-message payload because getmsg() reads global buffers. | ||
| setmsg_c("TSPICE parity getmsg long message"); | ||
| 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)); | ||
| free(which); | ||
| write_error_json("SPICE error in setmsg (getmsg prime)", shortMsg, longMsg, | ||
| traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| SpiceChar msg[1841]; | ||
| msg[0] = '\0'; | ||
| getmsg_c(which, (SpiceInt)sizeof(msg), msg); | ||
| free(which); | ||
|
|
||
| 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 getmsg", shortMsg, longMsg, traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| fputs("{\"ok\":true,\"result\":\"", stdout); | ||
| json_print_escaped(msg); | ||
| fputs("\"}\n", stdout); | ||
| goto done; | ||
| } | ||
|
|
||
| case CALL_ERROR_SETMSG: { | ||
| if (tokens[argsTok].size < 1) { | ||
| write_error_json_ex("invalid_args", "error.setmsg expects args[0]=string", | ||
| NULL, NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| int msgTok = jsmn_get_array_elem(tokens, argsTok, 0, tokenCount); | ||
| if (msgTok < 0 || msgTok >= tokenCount || tokens[msgTok].type != JSMN_STRING) { | ||
| write_error_json_ex("invalid_args", | ||
| "error.setmsg expects args[0] to be a string", NULL, | ||
| NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| char *msg = NULL; | ||
| strDetail[0] = '\0'; | ||
| jsmn_strdup_err_t msgErr = | ||
| jsmn_strdup(input, &tokens[msgTok], &msg, strDetail, sizeof(strDetail)); | ||
| if (msgErr != JSMN_STRDUP_OK) { | ||
| if (msgErr == 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; | ||
| } | ||
|
|
||
| setmsg_c(msg); | ||
| free(msg); | ||
|
|
||
| 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 setmsg", shortMsg, longMsg, traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| fputs("{\"ok\":true,\"result\":null}\n", stdout); | ||
| goto done; | ||
| } | ||
|
|
||
| case CALL_ERROR_SIGERR: { | ||
| if (tokens[argsTok].size < 1) { | ||
| write_error_json_ex("invalid_args", "error.sigerr expects args[0]=string", | ||
| NULL, NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| int shortTok = jsmn_get_array_elem(tokens, argsTok, 0, tokenCount); | ||
| if (shortTok < 0 || shortTok >= tokenCount || | ||
| tokens[shortTok].type != JSMN_STRING) { | ||
| write_error_json_ex("invalid_args", | ||
| "error.sigerr expects args[0] to be a string", NULL, | ||
| NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| char *shortMsgArg = NULL; | ||
| strDetail[0] = '\0'; | ||
| jsmn_strdup_err_t shortErr = | ||
| jsmn_strdup(input, &tokens[shortTok], &shortMsgArg, strDetail, | ||
| sizeof(strDetail)); | ||
| if (shortErr != JSMN_STRDUP_OK) { | ||
| if (shortErr == 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; | ||
| } | ||
|
|
||
| // Preserve deterministic long-message content in the raised SPICE error. | ||
| setmsg_c("TSPICE parity sigerr long message"); | ||
| 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)); | ||
| free(shortMsgArg); | ||
| write_error_json("SPICE error in setmsg (sigerr prelude)", shortMsg, | ||
| longMsg, traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| sigerr_c(shortMsgArg); | ||
| free(shortMsgArg); | ||
|
|
||
| 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 sigerr", shortMsg, longMsg, traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| fputs("{\"ok\":true,\"result\":null}\n", stdout); | ||
| goto done; | ||
| } | ||
|
|
||
| case CALL_ERROR_CHKIN: { | ||
| if (tokens[argsTok].size < 1) { | ||
| write_error_json_ex("invalid_args", "error.chkin expects args[0]=string", | ||
| NULL, NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| int nameTok = jsmn_get_array_elem(tokens, argsTok, 0, tokenCount); | ||
| if (nameTok < 0 || nameTok >= tokenCount || tokens[nameTok].type != JSMN_STRING) { | ||
| write_error_json_ex("invalid_args", | ||
| "error.chkin expects args[0] to be a string", NULL, | ||
| NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| char *name = NULL; | ||
| strDetail[0] = '\0'; | ||
| jsmn_strdup_err_t nameErr = | ||
| jsmn_strdup(input, &tokens[nameTok], &name, strDetail, sizeof(strDetail)); | ||
| if (nameErr != JSMN_STRDUP_OK) { | ||
| if (nameErr == 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; | ||
| } | ||
|
|
||
| chkin_c(name); | ||
| free(name); | ||
|
|
||
| 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 chkin", shortMsg, longMsg, traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| fputs("{\"ok\":true,\"result\":null}\n", stdout); | ||
| goto done; | ||
| } | ||
|
|
||
| case CALL_ERROR_CHKOUT: { | ||
| if (tokens[argsTok].size < 1) { | ||
| write_error_json_ex("invalid_args", "error.chkout expects args[0]=string", | ||
| NULL, NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| int nameTok = jsmn_get_array_elem(tokens, argsTok, 0, tokenCount); | ||
| if (nameTok < 0 || nameTok >= tokenCount || tokens[nameTok].type != JSMN_STRING) { | ||
| write_error_json_ex("invalid_args", | ||
| "error.chkout expects args[0] to be a string", NULL, | ||
| NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| char *name = NULL; | ||
| strDetail[0] = '\0'; | ||
| jsmn_strdup_err_t nameErr = | ||
| jsmn_strdup(input, &tokens[nameTok], &name, strDetail, sizeof(strDetail)); | ||
| if (nameErr != JSMN_STRDUP_OK) { | ||
| if (nameErr == 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; | ||
| } | ||
|
|
||
| // `chkout` is deterministic after a matching `chkin` in this case. | ||
| chkin_c(name); | ||
| 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)); | ||
| free(name); | ||
| write_error_json("SPICE error in chkin (chkout prime)", shortMsg, longMsg, | ||
| traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| chkout_c(name); | ||
| free(name); | ||
|
|
||
| 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 chkout", shortMsg, longMsg, traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| fputs("{\"ok\":true,\"result\":null}\n", stdout); | ||
| goto done; | ||
| } |
There was a problem hiding this comment.
The CALL_ERROR_GETMSG branch accepts at least one arg but doesn’t reject extra args. That makes the legacy/native runner more permissive than it probably should be and risks parity drift if a caller mistakenly supplies additional parameters.
Related: several of the new error handlers (SETMSG, SIGERR, CHKIN, CHKOUT) similarly check < 1 but not != 1.
Suggestion
Make the argument count checks strict for the new error.* calls.
Example pattern:
if (tokens[argsTok].size != 1) {
write_error_json_ex("invalid_args", "error.getmsg expects exactly 1 argument", NULL,
NULL, NULL, NULL);
goto done;
}Apply similarly:
error.setmsg,error.sigerr,error.chkin,error.chkout→size != 1error.failed,error.reset→size != 0(already done)
Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
| case CALL_ERROR_CHKOUT: { | ||
| if (tokens[argsTok].size < 1) { | ||
| write_error_json_ex("invalid_args", "error.chkout expects args[0]=string", | ||
| NULL, NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| int nameTok = jsmn_get_array_elem(tokens, argsTok, 0, tokenCount); | ||
| if (nameTok < 0 || nameTok >= tokenCount || tokens[nameTok].type != JSMN_STRING) { | ||
| write_error_json_ex("invalid_args", | ||
| "error.chkout expects args[0] to be a string", NULL, | ||
| NULL, NULL, NULL); | ||
| goto done; | ||
| } | ||
|
|
||
| char *name = NULL; | ||
| strDetail[0] = '\0'; | ||
| jsmn_strdup_err_t nameErr = | ||
| jsmn_strdup(input, &tokens[nameTok], &name, strDetail, sizeof(strDetail)); | ||
| if (nameErr != JSMN_STRDUP_OK) { | ||
| if (nameErr == 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; | ||
| } | ||
|
|
||
| // `chkout` is deterministic after a matching `chkin` in this case. | ||
| chkin_c(name); | ||
| 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)); | ||
| free(name); | ||
| write_error_json("SPICE error in chkin (chkout prime)", shortMsg, longMsg, | ||
| traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| chkout_c(name); | ||
| free(name); | ||
|
|
||
| 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 chkout", shortMsg, longMsg, traceMsg); | ||
| goto done; | ||
| } | ||
|
|
||
| fputs("{\"ok\":true,\"result\":null}\n", stdout); | ||
| goto done; | ||
| } |
There was a problem hiding this comment.
Same semantic concern as in the TS runner: CALL_ERROR_CHKOUT primes with chkin_c(name) before calling chkout_c(name). That makes error.chkout succeed even when it should fail for an unbalanced call stack, meaning this implementation can’t detect parity issues around the actual chkout behavior.
Determinism should be achieved via explicit setup steps rather than by changing what the method does.
Suggestion
Remove the implicit chkin_c(name) priming from the CALL_ERROR_CHKOUT handler and instead ensure the parity case/workflow performs a error.chkin call before error.chkout.
Minimal change:
- Delete the
chkin_c(name)block (and its error handling) - Keep only
chkout_c(name) - Update
error/chkout@v2.ymlworkflow/case to include a priorerror.chkinstep if needed
Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
| schemaVersion: 2 | ||
| manifest: | ||
| id: methods/error/failed@v2 | ||
| kind: method | ||
| contract: | ||
| contractMethod: error.failed | ||
| canonicalMethod: error.failed | ||
| aliases: [] | ||
| result: | ||
| type: object | ||
| properties: {} | ||
| workflow: | ||
| steps: | ||
| - op: invokeLegacyCall | ||
| cases: | ||
| - id: error.failed.clean-state | ||
| args: [] | ||
| expect: | ||
| ok: true |
There was a problem hiding this comment.
The v2 specs define result as an object with empty properties for methods that clearly return null or a boolean/string (e.g., error.failed returns a boolean; error.getmsg returns a string; error.reset returns null). This weakens contract guarantees and makes it harder for the parity framework to validate output shapes.
Even if the engine treats legacy calls as loosely typed, the spec should reflect reality to prevent future regressions.
Suggestion
Update the contract.result schemas to match the actual outputs:
error.failed→ booleanerror.getmsg→ stringerror.reset,error.setmsg,error.sigerr,error.chkin,error.chkout→ null (or atype: "null"if supported by your schema dialect)
Example:
contract:
result:
type: booleanand
contract:
result:
type: stringand
contract:
result:
type: "null"Reply with "@CharlieHelps yes please" if you’d like me to add a commit with this suggestion.
1 participant
Stack step PR-08 in the issue #454 migration chain.