docs(rfc): propose SDK conformance testing #3238
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,289 @@ | ||
| --- | ||
| authors: | ||
| - "@jiripetrlik" | ||
| state: draft | ||
| links: | ||
| - https://github.com/NVIDIA/OpenShell/issues/3028 | ||
| - https://github.com/jiripetrlik/OpenShell/pull/1 | ||
| --- | ||
|
|
||
| # RFC 0015 - SDK Conformance Testing | ||
|
|
||
| ## Summary | ||
|
|
||
| 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. | ||
|
|
||
| The first implementation is a Go adapter based on the exploratory work in the | ||
| related pull request. The prototype currently uses a Podman-backed gateway; | ||
| this RFC proposes a Docker-backed gateway as the portable default conformance | ||
| environment. The first adapter covers sandbox lifecycle and execution, | ||
| providers, and workspaces. Rust, Python, and TypeScript adapters follow | ||
| incrementally. | ||
|
|
||
| ## Motivation | ||
|
|
||
| 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 | ||
|
Comment on lines
+28
to
+30
Contributor
There was a problem hiding this comment. Important: This says the Go SDK client tests "do not exercise a built 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 Source: correctness agent |
||
| can pass SDK unit tests while breaking SDK consumers in a deployed system. | ||
|
|
||
| Adding a Go-only E2E suite would improve that situation, but would leave the | ||
| project with independently selected scenarios, assertions, harnesses, and CI | ||
| requirements for every SDK. The result would be uneven compatibility coverage | ||
| and no durable statement of which gateway behavior all official SDKs promise to | ||
| support. | ||
|
|
||
| OpenShell already has reusable CLI conformance scenarios in | ||
| `openshell-conformance`, and language-specific E2E suites for Rust and Python. | ||
| Those establish useful patterns, but CLI conformance cannot validate typed SDK | ||
| interfaces, conversions, error classification, or SDK conveniences such as | ||
| readiness waiting. A shared SDK contract is needed alongside them. | ||
|
|
||
| This deserves an RFC because it defines a testing strategy and compatibility | ||
| contract across SDKs, gateway behavior, E2E infrastructure, and CI. It also | ||
| sets an extension boundary that future SDKs should follow. | ||
|
|
||
| ## Non-goals | ||
|
|
||
| - Replacing unit tests, generated-protobuf checks, or each SDK's language-native | ||
| integration tests. | ||
| - Defining every public SDK method as conformant in the initial release. | ||
| - Replacing the existing `openshell-conformance` CLI runner or changing its | ||
| scenarios and plan format. | ||
| - Requiring every scenario to run against every compute driver, identity mode, | ||
| or operating system on every pull request. | ||
| - Standardizing language APIs, error types, package layouts, or async models. | ||
| - Providing cross-version compatibility testing against released gateway and SDK | ||
| versions in the first implementation. | ||
|
|
||
| ## Proposal | ||
|
|
||
| ### A shared behavioral specification | ||
|
|
||
| Add a versioned SDK conformance specification under `e2e/conformance/sdk/`. | ||
|
Contributor
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 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. | ||
|
Comment on lines
+67
to
+70
Contributor
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 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) |
||
|
|
||
| The initial specification version covers these workflows: | ||
|
|
||
| | 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. | | ||
|
Contributor
There was a problem hiding this comment. Important: "Verify a sandbox receives placeholders rather than raw credential values" does not say what a placeholder looks like or how the adapter must check it. The existing Python e2e test checks for the Why this matters: An adapter that only asserts Suggested fix: Make the placeholder format a conformance fixture shared by all adapters and require a positive match against that pattern, not just inequality with the raw value. Source: test-quality agent |
||
| | Workspaces | Create, get, list, delete, apply labels, scope sandbox visibility, and classify missing resources as not found. | | ||
|
Comment on lines
+74
to
+79
Contributor
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 Why this matters: Adapters will pick different assertion strengths. A Go adapter asserting 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
Contributor
There was a problem hiding this comment. Important: Negative behavior is specified only for workspaces ("classify missing resources as not found") and exec (failed command). Sandbox lifecycle and providers have no error-path scenarios, although Why this matters: A suite with only happy paths cannot detect an SDK that wraps typed errors into generic ones. Consumers writing retry logic would then treat permanent failures as transient. Suggested fix: Add explicit error scenarios per area with the expected classification: sandbox (invalid spec, get or delete of an unknown name), providers (unknown profile type, type change on update, attach of an unknown provider), and keep the existing workspace not-found case. Source: test-quality agent (also flagged by: coderabbit) |
||
|
|
||
| A scenario may explicitly mark an operation as unsupported for an SDK only when | ||
| the SDK's published API does not expose that capability. The adapter must report | ||
| the omission in its results; it must not silently skip it. Adding an SDK feature | ||
| that makes an omitted scenario available requires enabling that scenario in the | ||
| same change. | ||
|
Comment on lines
+81
to
+85
Contributor
There was a problem hiding this comment. Important: This paragraph requires adapters to report omissions, but it does not define the conformance outcome for an omitted scenario: does the adapter fail, is the SDK marked non-conformant, or is the omission an allowed pass? It also does not require any approval for omitting a scenario in an SDK that otherwise supports the area. Why this matters: As written, an adapter could mark every required operation as unsupported, report the omissions, and still succeed. Suggested fix: Define the runner outcome for omissions, require a recorded capability exception (reviewed like a compatibility change, as line 195 already suggests) for each one, and state that omitting all required operations in an area cannot pass. Source: coderabbit |
||
|
|
||
| The specification is a set of versioned Markdown scenario documents. Each | ||
| document gives a stable scenario identifier, preconditions, ordered operations, | ||
|
Contributor
There was a problem hiding this comment. Minor: "Stable scenario identifier" is required but its format is not defined, nor how an adapter's test functions reference it. Why this matters: Without a defined identifier scheme, answering "does the TypeScript adapter implement provider attach/detach?" means reading every test file. Suggested fix: Define the identifier format here (for example Source: test-quality agent |
||
| expected observable results, cleanup expectations, and permitted SDK-specific | ||
| variations. Markdown is the normative contract because it keeps the behavior | ||
| reviewable without introducing a parser or a general gateway workflow language. | ||
|
Comment on lines
+87
to
+91
Contributor
There was a problem hiding this comment. Important: The scenario document format is described only in prose (identifier, preconditions, ordered operations, expected results, cleanup, permitted variations). There is no example, although the scenario document is the primary interface adapter authors implement against and the RFC template recommends examples when they ease review. Why this matters: Reviewers cannot judge whether the described structure is sufficient, and the first adapter author will invent the de facto format regardless of what this RFC intended. Suggested fix: Add one short example scenario document (15 to 20 lines) for a scenario from the initial set, for example the sandbox create, wait-for-ready, get, delete lifecycle, showing the identifier and the expected observations. Source: architecture agent |
||
|
|
||
| Adapters translate the Markdown requirements into native test code. A future | ||
| machine-readable companion may reduce duplicated fixture data or produce | ||
| coverage reports, but it must derive from the Markdown contract and is not | ||
| required for the initial implementation. | ||
|
Comment on lines
+93
to
+96
Contributor
There was a problem hiding this comment. Important: Markdown is the normative contract, but with the machine-readable companion deferred there is no mechanism to verify that each adapter implements every scenario. Enforcement is code review only. The CLI conformance crate avoids this with a scenario registry that rejects unknown names. Why this matters: A scenario added to the specification can be implemented in Go and forgotten in TypeScript with no CI signal. The "report omissions" rule above covers intentional gaps, not accidental ones. Suggested fix: Require each adapter to emit a small machine-readable coverage report (scenario IDs run, skipped with reason, not implemented) and add a CI step that diffs it against the scenario manifest. This is far lighter than a DSL and closes the drift risk named in the Risks section. Source: test-quality agent |
||
|
|
||
| ### Fixture ownership and isolation | ||
|
|
||
| In this RFC, a fixture is predefined test data or setup that makes a scenario | ||
| repeatable. Fixtures have three ownership levels: | ||
|
|
||
| | Fixture type | Owner | Examples | Reuse rule | | ||
| | --- | --- | --- | --- | | ||
| | Conformance fixture | Shared scenario documents | A standard sandbox policy, an exec command, expected exit code, placeholder expectation, or expected `NotFound` result. | Shared by every SDK adapter that implements the scenario. | | ||
| | Adapter fixture | One SDK adapter | Go `TestMain`, a configured Go client, context deadlines, helper functions, and language-native assertions. | Local to that SDK because its runtime and testing idioms differ. | | ||
| | Live test resource | One test invocation | A sandbox, provider, workspace, credential value, or temporary file created while a test runs. | Never shared between parallel tests; each test uses a unique name and cleans it up. | | ||
|
|
||
| The shared Markdown scenario is authoritative for conformance fixtures. It | ||
| states the inputs and observations each SDK must use or verify, such as a | ||
| failing command's exit code and stderr content. It does not prescribe how Go, | ||
| Python, Rust, or TypeScript construct a client or express assertions. | ||
|
|
||
| Adapter fixtures remove repeated language-specific setup without crossing SDK | ||
| boundaries. They may share a gateway connection within one test process only | ||
| when the adapter can do so safely. They must not cause tests to share mutable | ||
| gateway objects or credentials. | ||
|
|
||
| Live resources are deliberately isolated. Every test creates its own uniquely | ||
| named sandbox, provider, and workspace as needed; it registers bounded cleanup | ||
| before later operations can fail. A scenario may define a synthetic credential | ||
| value for placeholder testing, but the adapter creates the provider that holds | ||
| it for that invocation and must verify that the raw value is never returned to | ||
| the sandbox. | ||
|
|
||
| ### Native SDK adapters | ||
|
|
||
| Each supported SDK owns a native adapter and test entry point under `e2e/`. | ||
|
Contributor
There was a problem hiding this comment. Minor: "Each supported SDK owns a native adapter and test entry point under Why this matters: The first Python or Rust adapter author will make a structural decision that this RFC should make. Suggested fix: Add a sentence stating the intended layout (for example Source: architecture agent |
||
| An adapter loads the shared scenarios, performs the corresponding SDK calls, | ||
| and checks the standardized observations using that language's normal test | ||
| framework. It may add language-specific assertions where they validate the SDK | ||
| surface, such as Go context cancellation or TypeScript typing, without changing | ||
| the shared contract. | ||
|
|
||
| The adapters have a common responsibility: | ||
|
|
||
| - 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; | ||
|
Comment on lines
+137
to
+140
Contributor
There was a problem hiding this comment. Important: "Register cleanup before making later calls" and bounded deadlines are necessary but not sufficient. Go's 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 |
||
| - emit enough resource identity and gateway diagnostics to make CI failures | ||
| actionable; | ||
| - run only when the harness explicitly supplies a reachable gateway. | ||
|
|
||
| The Go adapter lives in `e2e/go/` as a separate Go module with a local | ||
| `replace` directive to `sdk/go/`. Its files use the `e2e` build tag. The related | ||
| draft PR demonstrates this shape and currently uses the Podman gateway harness, | ||
| but it must be rebased, moved to the Docker default lane, and adjusted to the | ||
| shared scenarios before it is accepted. | ||
|
|
||
| ```mermaid | ||
| flowchart LR | ||
| SPEC[Versioned SDK scenarios] --> GO[Go adapter] | ||
| SPEC --> PY[Python adapter] | ||
| SPEC --> TS[TypeScript adapter] | ||
| SPEC --> RS[Rust SDK adapter] | ||
| GO --> GW[Real gateway] | ||
| PY --> GW | ||
| TS --> GW | ||
| RS --> GW | ||
| CLI[CLI conformance runner] --> GW | ||
| ``` | ||
|
|
||
| ### Gateway harness and runtime coverage | ||
|
|
||
| The default SDK conformance lane starts one Docker-backed gateway through the | ||
| existing `e2e/with-docker-gateway.sh` harness. This gives all SDK adapters one | ||
| repeatable local command and prevents the general `mise run e2e` task from | ||
| implicitly requiring both Docker and Podman. | ||
|
|
||
| Runtime-specific SDK scenarios are allowed when the expected result depends on | ||
| a compute driver or deployment mode. They belong in explicitly named tasks and | ||
| CI jobs, such as `e2e:go:podman`; they do not redefine the portable contract. | ||
| The initial Go suite therefore runs in the Docker default lane. A Podman lane | ||
| 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 | ||
|
Contributor
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 |
||
| must be shorter than the job timeout and should report the last observed state | ||
| instead of relying on a global test-framework timeout. | ||
|
|
||
| ### Relationship to existing tests | ||
|
|
||
| `openshell-conformance` remains the portable CLI conformance suite. It invokes | ||
| the `openshell` binary and validates command-line behavior, so it is not an SDK | ||
| adapter and does not consume SDK scenarios. | ||
|
|
||
| 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. | ||
|
Comment on lines
+188
to
+191
Contributor
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. 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 |
||
|
|
||
| The conformance specification is the source of truth for portable SDK behavior. | ||
| Its scenario identifiers, required observations, and capability exceptions are | ||
| reviewed as compatibility changes. | ||
|
|
||
| ## Implementation plan | ||
|
|
||
| 1. Add the version-one Markdown scenario documents and a short contributor | ||
| guide under `e2e/conformance/sdk/`. Start with sandbox, exec, provider, and | ||
| workspace workflows defined above. | ||
| 2. Rebase the Go prototype onto current `main`, move it to the Docker gateway | ||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Minor: Step 3 introduces 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 Source: correctness agent |
||
| `e2e` task until the Docker default path is in place; then add it to the | ||
| aggregate task. | ||
|
Comment on lines
+207
to
+208
Contributor
There was a problem hiding this comment. Important: Adding Why this matters: Local 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 Source: production agent |
||
| 4. Add adapters for the Rust SDK, Python SDK, and TypeScript SDK. Migrate or | ||
| map existing E2E coverage where it matches a scenario, retaining | ||
| language-specific tests where it does not. | ||
| 5. Add runtime-specific lanes only for scenarios whose expected behavior varies | ||
| by runtime. Document supported capability exceptions and their rationale. | ||
| 6. Update the relevant architecture and contributor documentation when the | ||
| suite is accepted and implemented. | ||
|
|
||
| ## Risks | ||
|
|
||
| - A broad DSL could become a second orchestration framework. Markdown keeps the | ||
| contract descriptive, while adapters retain control flow in native code. | ||
| - The contract could flatten meaningful language differences. Shared scenarios | ||
| define gateway-visible outcomes, while each SDK retains its own type, error, | ||
| and cancellation assertions. | ||
| - Running every SDK against every runtime would make CI too slow and flaky. The | ||
| default lane uses Docker and adds specialized lanes only for | ||
| runtime-dependent behavior. | ||
| - Asynchronous cleanup can leave leaked sandboxes or providers after failures. | ||
| Adapters must register bounded cleanup early and failure diagnostics must | ||
| identify resources for manual recovery. | ||
| - A scenario may accidentally codify a gateway implementation detail. Reviews | ||
| should specify observable behavior and avoid asserting storage internals, | ||
| polling intervals, or transport implementation details. | ||
|
|
||
| ## Alternatives | ||
|
|
||
| ### Keep a Go-only E2E suite | ||
|
|
||
| This delivers value quickly and the related PR is a useful starting point. | ||
| However, it does not establish a shared SDK compatibility contract, leaving | ||
| other bindings to independently choose what they test. It is retained as the | ||
| first implementation phase, but not as the final design. | ||
|
|
||
| ### Put all E2E tests inside each SDK directory | ||
|
|
||
| Keeping tests beside SDK code simplifies ownership, but makes a shared scenario | ||
| catalog and common gateway-harness conventions harder to discover and reuse. | ||
| The separate `e2e/` adapters make the real-gateway boundary explicit while SDK | ||
| unit and integration tests remain near their implementation. | ||
|
|
||
| ### Extend the existing CLI conformance runner | ||
|
|
||
| The runner already has reusable plans and lifecycle scenarios, but it executes | ||
| the CLI. Adding language SDK invocation would couple it to multiple language | ||
| runtimes and would still not validate SDK-native APIs. Shared scenario concepts | ||
| can align, while the runners remain separate. | ||
|
|
||
| ### Use only generated-protobuf compatibility checks | ||
|
|
||
| Schema checks detect wire changes, but cannot verify SDK conversions, readiness | ||
| helpers, credential-placeholder safety, cleanup behavior, or error | ||
| classification against a running gateway. | ||
|
|
||
| ## Prior art | ||
|
|
||
| - `crates/openshell-conformance` provides a versioned plan and registered | ||
| scenario model for CLI conformance. This RFC adopts the idea of explicit, | ||
| reviewable scenarios while retaining SDK-native execution. | ||
| - The existing `e2e/with-docker-gateway.sh` harness supplies a repeatable real | ||
| gateway lifecycle for Rust and Python E2E tests. It is the portable default | ||
| for the SDK suite. | ||
| - The Go SDK's fake client and bufconn tests demonstrate the complementary | ||
| unit-test layer. They remain appropriate for fast, isolated tests that do | ||
| not need a real gateway. | ||
| - The related Go E2E draft demonstrates parallel-safe resource naming, | ||
| placeholder assertions, and eventual-deletion handling that the first adapter | ||
| should preserve. | ||
|
|
||
| ## Open questions | ||
|
|
||
| - When, if ever, should repeated fixture data or coverage reporting justify a | ||
| machine-readable companion to the Markdown scenarios? | ||
| - Which SDKs are in the initial supported-adapter set? This RFC assumes Go, | ||
| Rust, Python, and TypeScript, subject to confirmation of release support. | ||
| - Should independently released SDKs run the same scenarios against the | ||
| latest gateway only, a release compatibility range, or both? | ||
| - Which provider type can provide deterministic placeholder coverage without | ||
| requiring a live external credential service in every CI lane? | ||
| - Should scenario results be emitted as a machine-readable report for release | ||
| qualification, or are native test reports sufficient initially? | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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