docs(rfc): propose SDK conformance testing - #3238
Conversation
Signed-off-by: Jiri Petrlik <jpetrlik@redhat.com>
|
All contributors have signed the DCO ✍️ ✅ |
|
I have read the DCO document and I hereby sign the DCO. |
rhuss
left a comment
rhuss
left a comment
There was a problem hiding this comment.
cc-review Summary
What Went Well
- The three-tier fixture taxonomy (lines 99 to 124) with the rule that live resources are never shared between parallel tests is exactly the constraint that makes parallel execution reliable.
- The scope broadening from the Go-only request in #3028 to a multi-SDK suite is declared, justified (lines 33 to 37), and phased so the Go adapter lands first, with the Go-only approach kept as a real alternative rather than a straw man (lines 236 to 240).
- Keeping a shared contract with language-native execution (lines 65 to 97) avoids a generic cross-language runner while preserving each SDK's idioms.
- The Docker-default harness decision comes with an operational rationale and explicitly scoped runtime-specific lanes (lines 166 to 176).
- The risk mitigations (lines 217 to 233) are concrete design choices, not deferred intentions.
Findings
| Severity | File | Description | Source |
|---|---|---|---|
| Important | README.md:14 | The shared scenario set is referred to by nine different names across the document: "shared SDK c... | architecture |
| Important | README.md:28 | This says the Go SDK client tests "do not exercise a built openshell-gateway" |
correctness |
| Important | README.md:66 | "Versioned" appears here and at lines 87 and 199, but the RFC never defines what is versioned (th... | architecture |
| Important | README.md:67 | "Expected classified failures" are part of the scenario definition, but the RFC does not say how ... | coderabbit |
| Important | README.md:74 | The table describes workflows ("Create, get, list, wait for ready, delete") but not what an adapt... | test-quality |
| Important | README.md:79 | Negative behavior is specified only for workspaces ("classify missing resources as not found") an... | test-quality |
| Important | README.md:78 | "Verify a sandbox receives placeholders rather than raw credential values" does not say what a pl... | test-quality |
| Important | README.md:81 | This paragraph requires adapters to report omissions, but it does not define the conformance outc... | coderabbit |
| Important | README.md:87 | The scenario document format is described only in prose (identifier, preconditions, ordered opera... | architecture |
| Important | README.md:93 | Markdown is the normative contract, but with the machine-readable companion deferred there is no ... | test-quality |
| Minor | README.md:88 | "Stable scenario identifier" is required but its format is not defined, nor how an adapter's test... | test-quality |
| Minor | README.md:128 | "Each supported SDK owns a native adapter and test entry point under e2e/", but e2e/python/ a... |
architecture |
| Important | README.md:137 | "Register cleanup before making later calls" and bounded deadlines are necessary but not sufficient | production |
| Minor | README.md:178 | "Every CI lane must capture gateway logs on failure" and adapters must emit gateway diagnostics, ... | security |
| Minor | README.md:188 | This section distinguishes conformance tests from unit tests and CLI conformance, but gives no ru... | test-quality |
| Minor | README.md:206 | Step 3 introduces mise run e2e:go without mentioning the existing go:test:integration task in... |
correctness |
| Important | README.md:207 | Adding e2e:go to the aggregate e2e task, and later the Rust, Python, and TypeScript adapters ... |
production |
All findings are posted inline. Several overlap and can be addressed together: the existing Go integration tests (Motivation, Relationship to existing tests, step 3), assertion precision and error classification (scenario table), and scenario identifiers with coverage reporting (scenario documents).
Review Details
- Findings posted: 17
- Findings reviewed and not posted: 8
- Gate outcome: FAIL (12 Important, 0 Critical)
- Participating agents: correctness, architecture, security, production, test-quality, goal-alignment, coderabbit
| This RFC introduces a shared SDK conformance suite that verifies the supported | ||
| OpenShell SDKs against a real gateway. The suite defines the observable | ||
| behavior that an SDK must provide for selected gateway workflows, while each | ||
| language keeps native test code and idioms. |
There was a problem hiding this comment.
Important: The shared scenario set is referred to by nine different names across the document: "shared SDK conformance suite" (line 14), "shared behavioral specification" (64), "versioned SDK conformance specification" (66), "shared contract" (91, 133), "SDK contract" (43), "compatibility contract" (239), "portable contract" (174), "Markdown contract" (95), and "conformance specification" (193).
Why this matters: Readers and adapter authors cannot tell whether these are one deliverable or several, and the ambiguity will carry over into directory names, task names, and code comments.
Suggested fix: Pick one primary term (for example "SDK conformance specification"), define it here in the Summary, and use it throughout. Natural variation is fine once the primary term is established, but keep it to one or two alternatives.
Source: architecture agent
| The Go SDK has extensive unit coverage, but its client tests use an in-process | ||
| transport and do not exercise a built `openshell-gateway`. That leaves a gap: | ||
| a protocol, conversion, authentication, or asynchronous-lifecycle regression |
There was a problem hiding this comment.
Important: This says the Go SDK client tests "do not exercise a built openshell-gateway". sdk/go/openshell/v1/integration_test.go (build tag integration) already runs TestIntegration_HealthCheck, TestIntegration_SandboxExecSmoke, and TestIntegration_FileTransfer against a real gateway, and tasks/go.toml exposes them as go:test:integration.
Why this matters: The gap is real but narrower than stated. Reviewers who take this sentence at face value will not know there is existing gateway-backed coverage to build on or retire, and the implementation plan (step 3) currently ignores it.
Suggested fix: Rephrase to acknowledge the tagged integration tests and name the actual gap: they are not wired into go:ci or the e2e aggregate, cover only a smoke path, and share no scenarios with other SDKs. Then say in step 3 whether e2e:go supersedes go:test:integration.
Source: correctness agent
|
|
||
| ### A shared behavioral specification | ||
|
|
||
| Add a versioned SDK conformance specification under `e2e/conformance/sdk/`. |
There was a problem hiding this comment.
Important: "Versioned" appears here and at lines 87 and 199, but the RFC never defines what is versioned (the whole specification or individual scenarios), what the version looks like, or what triggers a bump. The CLI conformance crate has a precedent in PLAN_VERSION in crates/openshell-conformance/src/plan.rs.
Why this matters: Line 195 says specification changes are "reviewed as compatibility changes", but without a versioning rule nobody can tell whether a change is breaking, whether an adapter is current, or how four adapters coordinate when a required observation is added.
Suggested fix: Define the versioning unit, the scheme (a monotonic integer per specification version would match the CLI crate), and the policy: what kind of change bumps the version, and what happens to an adapter that has not implemented the new version (fails a completeness check, or is documented as behind).
Source: architecture agent
| The specification defines scenarios in terms of externally observable gateway | ||
| behavior: setup inputs, ordered operations, expected successful results, | ||
| expected classified failures, and cleanup requirements. It does not encode | ||
| language syntax or require one generic test runner to call every SDK. |
There was a problem hiding this comment.
Important: "Expected classified failures" are part of the scenario definition, but the RFC does not say how each adapter maps its SDK's native errors onto the shared classifications (for example NotFound), nor which transport-level errors must be treated as failures rather than retried.
Why this matters: Without a shared mapping, one adapter can classify a gRPC NOT_FOUND as NotFound while another surfaces a generic error, and both report conformance for behavior that differs.
Suggested fix: Add a short error-classification section to the specification: the set of shared failure classes, the gRPC status (or transport condition) each maps to, and explicit negative cases in the scenarios that exercise them.
Source: coderabbit (also flagged by: test-quality agent)
| | Area | Required behavior | | ||
| | --- | --- | | ||
| | Sandbox lifecycle | Create, get, list, wait for ready, delete, and observe eventual removal. | | ||
| | Sandbox execution | Run a successful command, preserve sandbox filesystem state across executions, and surface a failed command's exit status and stderr. | | ||
| | Providers | Create, get, list, update, attach, detach, and delete a provider; verify a sandbox receives placeholders rather than raw credential values. | | ||
| | Workspaces | Create, get, list, delete, apply labels, scope sandbox visibility, and classify missing resources as not found. | |
There was a problem hiding this comment.
Important: The table describes workflows ("Create, get, list, wait for ready, delete") but not what an adapter must observe: expected field values, status codes, or state transitions. Compare crates/openshell-conformance/src/scenarios/smoke.rs, which asserts exact phase strings, and the Python e2e tests, which assert specific gRPC codes.
Why this matters: Adapters will pick different assertion strengths. A Go adapter asserting phase == Ready and a TypeScript adapter asserting only that status is present both "implement" the wait-for-ready scenario, so a gateway regression would be caught in one language and missed in another.
Suggested fix: State in the specification (or in the example scenario document) the observation precision, for example: after wait-for-ready returns, get MUST report phase Ready; delete of an unknown name MUST return NotFound; a failing exec MUST return a non-zero exit code and non-empty stderr.
Source: test-quality agent
| - create unique test resources and register cleanup before making later calls; | ||
| - use bounded operation and cleanup deadlines; | ||
| - tolerate documented asynchronous deletion by polling only the specified | ||
| observable state; |
There was a problem hiding this comment.
Important: "Register cleanup before making later calls" and bounded deadlines are necessary but not sufficient. Go's -timeout kills the test binary without running t.Cleanup. In the external-gateway mode this RFC supports (line 143), the gateway survives the run, so leaked sandboxes, providers, and workspaces accumulate. The Risks section proposes only manual recovery.
Why this matters: On a shared or long-lived development gateway, stale resources from interrupted runs confuse later list and get scenarios and can exhaust quotas.
Suggested fix: Require adapters to either run a pre-test scavenger that removes stale resources by name prefix, or scope every resource of a run into a uniquely named workspace that can be deleted in one call if cleanup is interrupted.
Source: production agent
| may be added later to validate its harness and driver behavior, but is not a | ||
| prerequisite for the shared SDK contract. | ||
|
|
||
| Every CI lane must capture gateway logs on failure. Per-language test timeouts |
There was a problem hiding this comment.
Minor: "Every CI lane must capture gateway logs on failure" and adapters must emit gateway diagnostics, but there is no guidance on redacting JWTs, mTLS material, or credential values from what gets uploaded.
Why this matters: CI artifacts are visible to all collaborators for the retention period. The credential values in provider scenarios are synthetic, so the practical exposure is auth material in gateway logs, but the RFC should say so explicitly.
Suggested fix: Add one sentence: captured logs and adapter diagnostics must not contain tokens, key material, or credential values, and adapters must never print raw credential values even on failure.
Source: security agent
| SDK unit tests continue to validate conversion details, retry behavior, and | ||
| language-specific ergonomics cheaply. Existing Rust and Python E2E tests remain | ||
| valid. Their scenarios should be mapped to the shared specification over time; | ||
| the project should not rewrite stable coverage merely to satisfy a new layout. |
There was a problem hiding this comment.
Minor: This section distinguishes conformance tests from unit tests and CLI conformance, but gives no rule for what belongs where. sdk/go/openshell/v1/integration_test.go already covers sandbox create, exec, and delete against a real gateway, and the RFC does not say whether it is removed, subsumed, or kept.
Why this matters: Without a boundary rule, the same behavior accumulates at several levels with different assertion strength, growing CI time and letting a weaker test mask a regression the conformance suite would catch.
Suggested fix: Add a short decision rule: gateway-dependent portable behavior listed in the scenario table belongs in the conformance adapter; SDK integration tests cover only SDK-specific behavior (context cancellation, streaming, transport negotiation).
Source: test-quality agent
| harness, and implement it as the first adapter. Strengthen its assertions to | ||
| check returned resource state, readiness state, provider read/list/update, | ||
| failed exec behavior, and missing-sandbox errors. | ||
| 3. Add `mise run e2e:go` and a focused Go CI lane. Keep it out of the aggregate |
There was a problem hiding this comment.
Minor: Step 3 introduces mise run e2e:go without mentioning the existing go:test:integration task in tasks/go.toml or the tests it runs.
Why this matters: Implementers may end up with two overlapping gateway-backed Go lanes, or drop the existing coverage by accident.
Suggested fix: State whether e2e:go supersedes go:test:integration and how the tests in integration_test.go migrate into the adapter.
Source: correctness agent
| `e2e` task until the Docker default path is in place; then add it to the | ||
| aggregate task. |
There was a problem hiding this comment.
Important: Adding e2e:go to the aggregate e2e task, and later the Rust, Python, and TypeScript adapters (step 4), means each adapter runs its own with-docker-gateway.sh lifecycle. The aggregate in tasks/test.toml runs its dependencies sequentially, so it grows from three gateway cycles to seven.
Why this matters: Local mise run e2e, the documented pre-PR command, roughly doubles in wall-clock time. Contributors then skip it and discover failures only in CI.
Suggested fix: Specify a shared-gateway pattern for the aggregate: an umbrella task that starts one gateway and runs all SDK adapters against it, or adapters that honor OPENSHELL_GATEWAY_ENDPOINT (which the harness already supports) so the aggregate can reuse one instance.
Source: production agent
3 participants
Summary
This PR adds an RFC for SDK Conformance Testing. It describes how to verify the supported OpenShell SDKs against a real gateway. It defines a testing strategy and compatibility contract across SDKs, gateway behavior, E2E infrastructure, and CI.
Related Issue
Changes
Testing
mise run pre-commitpassesChecklist