PR-14: migrate kernel-pool to v2 (#454)#514
Conversation
|
orrery preview: https://pr-514.orrery-c4f.pages.dev/ |
There was a problem hiding this comment.
The v2 migration is structurally consistent, but the new specs largely stop validating returned payloads by using an empty contract.result and only asserting expect.ok: true, which can allow silent regressions. Consider adding minimal result-shape validation (and/or key-field expectations) for methods that return data so the parity suite actually guards behavior, not just successful invocation.
Additional notes (1)
- Maintainability |
packages/parity-checking/specs/methods/kernel-pool/dtpool@v2.yml:12-17
These v2 specs hardcodesetup.kernelsto$FIXTURES/kernel-pool-parity.tpceverywhere. If any kernel-pool method requires additional kernels, ordering, or a different fixture, this uniform setup could cause false positives (tests passing because the fixture happens to contain enough) or false negatives (missing needed data).
At minimum, it would be good to confirm each method’s intent matches this single shared fixture and that the fixture is stable for all cases.
Summary of changes
Summary
Migrated kernel-pool parity-checking method specs from v1 to v2 across multiple methods.
Key changes
- Replaced legacy v1 spec format (
id,kind,uses) with v2 structure:schemaVersion: 2manifestblock (id,kind)contractblock (contractMethod,canonicalMethod,aliases,resultschema)setup.kernelspointing to$FIXTURES/kernel-pool-parity.tpcworkflow.stepsusingop: invokeLegacyCall
- Added new v2 spec files for:
cvpool,dtpool,expool,gnpool,pcpool,pdpool,pipool,swpool. - Renamed existing specs to v2 for:
gcpool,gdpool,gipool. - Preserved the existing
cases(ids/args/expectations) while moving them under the v2 schema.
| contract: | ||
| contractMethod: kernel-pool.gcpool | ||
| canonicalMethod: kernel-pool.gcpool | ||
| aliases: [] | ||
| result: | ||
| type: object | ||
| properties: {} | ||
| setup: |
There was a problem hiding this comment.
All migrated v2 specs declare an empty result schema (result: { type: object, properties: {} }). For several of these methods (e.g., gcpool, gdpool, gipool) the call returns data and not just { ok: true }. Leaving the result unconstrained can hide regressions where the legacy call returns malformed/partial payloads while tests still pass.
If the parity framework supports it, you should assert at least minimal shape (e.g., required fields / array presence) or a snapshot of key fields for one representative case per method.
Suggestion
Tighten contract.result for methods that return values (e.g., enforce presence/type of a value/data field or whatever the runner emits), and optionally add expect assertions beyond ok: true for one case per method.
For example (adjust field names to your actual output):
- In
contract.result, addrequiredand basic property types. - In a case, add
expectchecks for returned payload fields.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit that updates one or two kernel-pool specs with a stricter result schema and corresponding expect assertions as a pattern for the rest.
| contract: | ||
| contractMethod: kernel-pool.cvpool | ||
| canonicalMethod: kernel-pool.cvpool | ||
| aliases: [] | ||
| result: | ||
| type: object | ||
| properties: {} |
There was a problem hiding this comment.
contract.result is declared as an empty object schema (properties: {}) across these v2 specs. That makes the contract effectively unconstrained and may hide regressions where the legacy call starts returning unexpected fields or changes shape.
If these methods have a known return shape (even if minimal, e.g. { ok: boolean } or whatever the harness returns), it’s better to encode it here so the spec actually validates something beyond success status.
Suggestion
Tighten contract.result to match the actual expected output structure for this method (and apply consistently across the kernel-pool specs). For example, if the runner returns a standard envelope, encode that explicitly:
contract:
...
result:
type: object
required: [ok]
properties:
ok:
type: booleanIf the result is truly void, consider an explicit sentinel schema (or a comment/flag supported by the framework) rather than an unconstrained object. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion across all affected specs.
| workflow: | ||
| steps: | ||
| - op: invokeLegacyCall |
There was a problem hiding this comment.
The migrated workflow.steps uses only op: invokeLegacyCall and removes the explicit v1 uses: workflows/legacy/kernel-pool.basic@v1 linkage.
That’s fine if invokeLegacyCall implicitly provides the same behavior (agent wiring, env, kernel loading semantics, etc.). But if kernel-pool.basic@v1 did anything extra (pre/post steps, normalization), this migration could subtly change the parity contract while still returning ok: true for these limited cases.
Suggestion
Verify parity equivalence by ensuring invokeLegacyCall encapsulates everything previously provided by workflows/legacy/kernel-pool.basic@v1. If not, add the missing steps explicitly in the v2 workflow (or create a v2 shared workflow that mirrors the v1 behavior). Reply with "@CharlieHelps yes please" if you'd like me to add a commit scaffolding a shared v2 workflow for kernel-pool methods and updating these specs to reference it.
Stack step PR-14 in the issue #454 migration chain.