docs(rfc): propose SDK conformance testing - #3238
jiripetrlik wants to merge 1 commit into
Conversation
|
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
There was a problem hiding this comment.
The RFC now:
- Defines SDK conformance specification as the normative versioned Markdown scenarios.
- Defines SDK conformance suite as the specification plus native adapters.
- Consistently uses the specification term for scenarios/contracts and adapter/lane terms for execution.
- Updates the diagram, fixture ownership table, risks, alternatives, and prior-art references.
There was a problem hiding this comment.
Confirmed. The Summary now defines "SDK conformance specification" and "SDK conformance suite" as distinct artifacts, and the rest of the document holds the distinction. Swept the file for the old variants: no stray "shared contract", "SDK contract", "compatibility contract", or "portable contract" remain.
| 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
There was a problem hiding this comment.
Updated to:
- Acknowledge go:test:integration and its gateway-backed health and sandbox-exec smoke tests.
- State that it requires a supplied gateway and is excluded from go:ci and aggregate e2e.
- Identify the actual gap: narrow smoke coverage with no specification-backed shared scenarios.
There was a problem hiding this comment.
Confirmed, and the revised text is accurate against the repo. go:test:integration exists at tasks/go.toml:12, the tests read OPENSHELL_GATEWAY_ADDRESS, and go:ci (line 53) does not depend on it. The gap is now stated correctly as narrow smoke coverage with no specification-backed scenarios.
|
|
||
| ### 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
There was a problem hiding this comment.
Implemented the versioning policy in RFC.
There was a problem hiding this comment.
Confirmed. "Specification versioning" defines the unit (the whole normative scenario set), the scheme (monotonic integer), what triggers a bump, and what happens to an adapter behind the current version. That was everything the comment asked for.
| 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)
There was a problem hiding this comment.
The RFC now:
- Defines adapter-level failure classifications without standardizing public SDK error APIs.
- Requires v1 NotFound (NOT_FOUND) and AlreadyExists (ALREADY_EXISTS) scenarios for sandboxes, providers, and workspaces.
- Requires generic/unclassified errors to fail those scenarios.
- Treats connection, DNS, TLS, and unexpected gRPC failures—including UNAVAILABLE, DEADLINE_EXCEEDED, and CANCELLED—as scenario failures with no adapter-level retry.
There was a problem hiding this comment.
Confirmed, and I verified the four mappings against the gateway rather than just the text. ALREADY_EXISTS for sandbox, provider, and workspace (compute/mod.rs:1034, grpc/provider.rs:249, grpc/workspace.rs:198), INVALID_ARGUMENT for a provider type change (grpc/provider.rs:386), and FAILED_PRECONDITION for a missing provider on both create and attach (grpc/sandbox.rs:405 and :1096). The no-retry rule for transport failures is the right call.
| | 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
There was a problem hiding this comment.
The new section requires adapters to verify exact observable behavior, including:
- Requested names, stable returned identifiers, and list membership.
- READY after readiness waiting.
- NotFound plus list absence after deletion.
- Exact success output, exit code, and empty stderr.
- Persistent filesystem markers across executions.
- Nonzero exit plus a failure sentinel in stderr.
- Credential placeholders that never equal the raw secret.
- Exact workspace labels and visibility scope.
There was a problem hiding this comment.
The new "Observation precision" section is a real improvement and most of it is exactly what was missing. Two of the assertions are still not precise enough to implement identically across four SDKs, so I would like one more pass here.
First, "phase READY" is not a token that appears anywhere. The wire enum value is SANDBOX_PHASE_READY (proto/openshell.proto:1091), while the CLI conformance scenario asserts the rendered string "Ready" (crates/openshell-conformance/src/scenarios/smoke.rs:85). In the one section whose purpose is exactness, name the enum value and say how language-specific renderings map onto it.
Second, "stdout equals the scenario's success sentinel, and stderr is empty" is only well defined when the exec runs without a PTY. ExecSandboxRequest has a tty field (proto/openshell.proto:1475), and a PTY merges the two streams, so the assertion changes meaning depending on a flag the scenario never sets. No suite in the repo currently asserts an empty stderr; the Python and Rust tests only capture it for diagnostics. Please pin tty=false in the scenario and consider whether empty stderr is worth the flake risk compared to asserting exit code and stdout.
Third, a related race: SANDBOX_PHASE_COMPLETED and SANDBOX_PHASE_STOPPED exist, so a short-lived entrypoint can leave READY between the readiness wait and the follow-up get. The conformance fixture should pin a long-running entrypoint, or the observation should accept READY or a later terminal phase.
There was a problem hiding this comment.
- Replaced the ambiguous READY token with protobuf enum value SANDBOX_PHASE_READY. Adapters may assert their SDK-native representation, provided it maps to that wire value.
- Required the lifecycle fixture to remain running until cleanup. Reaching COMPLETED, STOPPED, or another terminal phase before the follow-up get now fails the scenario.
- Defined exec scenarios as non-interactive and non-PTY, with tty=false and no_login_shell=true, while allowing SDKs to express those semantics through their native APIs.
- Made output assertions deterministic: the success command emits only the exact stdout sentinel and exits 0; the failure command emits only the exact stderr sentinel and exits with the scenario’s specified nonzero code.
- Required adapters to compare collected output without trimming or normalization.
There was a problem hiding this comment.
Confirmed, all four points. The phase observation now names SANDBOX_PHASE_READY with an explicit rule that an SDK-native representation must map to the named wire value, which is the right shape. Exec scenarios are pinned to tty = false and no_login_shell = true, and I verified both are real fields (proto/openshell.proto:1586 defines bool no_login_shell = 10). Keeping the empty-stderr assertion is defensible now that the sentinels are fixed ASCII with no trailing newline and adapters must compare without trimming, and requiring empty stdout plus an exact exit code on the failure path is stronger than what I asked for. The terminal-phase rule closes the readiness race.
| - 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
There was a problem hiding this comment.
Implemented the interrupted-run recovery requirement. Adapters must now use run-prefixed resource names, scavenge stale adapter-owned resources before using a persistent external gateway, delete dependencies in safe order, and fail setup if eligible stale resources cannot be removed. The TTL must exceed the maximum lane duration to protect active concurrent runs.
There was a problem hiding this comment.
Confirmed. Run-prefixed names, a bounded pre-run scavenger, a TTL that must exceed the maximum lane duration, and a defined deletion order address the leak. One data point in favor: MAX_NAME_LEN is 253 (crates/openshell-server/src/grpc/mod.rs:132), so the prefix leaves plenty of room for scenario suffixes.
| 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
There was a problem hiding this comment.
Added the CI artifact redaction requirement: gateway logs and adapter diagnostics must exclude tokens, TLS key material, and credential values, including synthetic fixture secrets.
There was a problem hiding this comment.
Confirmed. Naming tokens, TLS key material, and credential values, and extending it to synthetic fixture secrets, covers what the comment asked for.
| 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
There was a problem hiding this comment.
Added the test-ownership rule.
There was a problem hiding this comment.
Confirmed. The rule now says portable gateway-dependent behavior belongs in the adapter and SDK-local tests keep context cancellation, streaming, authentication, and transport negotiation. That is the boundary that was missing.
| 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
There was a problem hiding this comment.
Made step 3 explicit: e2e:go initially complements go:test:integration, maps all three existing Go integration tests into corresponding conformance scenarios, and becomes the authoritative portable lane only after equivalent coverage exists.
There was a problem hiding this comment.
Confirmed. Step 3 maps all three existing Go integration tests onto scenarios and states that the adapter lane becomes authoritative only once equivalent coverage exists, so nothing is dropped by accident.
| `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
There was a problem hiding this comment.
Added the shared-gateway task pattern.
The RFC now requires:
- e2e::adapter tasks that never manage gateway lifecycle
- standalone e2e: wrappers for focused runs
- one e2e:sdk umbrella task that starts a Docker gateway once and runs all adapters sequentially
- aggregate e2e to depend on e2e:sdk, not each language task
There was a problem hiding this comment.
The umbrella task is the right shape, but my original comment gave you a wrong premise and I want to correct it before this is locked in.
I claimed the aggregate runs its dependencies sequentially. It does not. This repo sets jobs = 8, and mise run --dry-run e2e shows e2e:rust, e2e:python, and e2e:mcp starting concurrently, each against its own gateway on a dynamically chosen port (e2e_pick_port in e2e/with-docker-gateway.sh:463). So local mise run e2e does not double in wall-clock time today, and the rationale at the end of "Gateway harness and runtime coverage" should be rewritten.
The design still stands on its own merits: one gateway instead of four reduces CPU, memory, and port pressure, which matters more on CI runners than raw fan-out does. But as written, e2e:sdk runs four adapters strictly sequentially where mise would have parallelized them, which could make the aggregate slower rather than faster. Since the fixture rules already require uniquely named per-test resources and permit sharing a connection within a process, consider allowing adapters to run in parallel against the single shared gateway, and state the actual reason for the umbrella (bounded concurrent gateways) rather than wall-clock time.
There was a problem hiding this comment.
Corrected the shared-gateway execution model and rationale. e2e:sdk now runs adapter-only tasks concurrently against one shared gateway, waits for all adapters, and tears down the gateway afterward. Sharing limits concurrent gateway containers and reduces CPU, memory, and dynamic-port pressure while preserving adapter parallelism; it no longer assumes mise dependencies execute sequentially.
There was a problem hiding this comment.
Confirmed, and thank you for taking the correction cleanly. The rationale now rests on bounded concurrent gateway containers and port pressure rather than wall-clock time, and says explicitly that it does not depend on mise dependencies running sequentially. Running adapters concurrently against the one shared gateway, waiting for all of them, and tearing down only after they complete is exactly the shape I was hoping for.
1239f1f to
6950fc2
Compare
|
@rhuss Thank you for your review. I've tried to address your comments. |
|
Second review pass on Fourteen are properly addressed and I have resolved those threads. The error classification table and the credential placeholder matcher deserve specific credit: both match the implementation exactly, including Three threads remain open with follow-ups: observation precision (the phase token and the stdout/stderr assertions), the two open questions that now contradict the manifest and coverage-report requirement, and the shared-gateway rationale, where my original comment gave you an incorrect premise about how mise runs task dependencies. Three further points that do not map onto an existing thread:
Two process notes: the frontmatter still says markdownlint passes on the file. |
Signed-off-by: Jiri Petrlik <jpetrlik@redhat.com>
6950fc2 to
38c6e2d
Compare
|
Hello @rhuss ,
Can you please take a look again? |
|
Third pass on Two of the answers are better than what I asked for. On the gateway variable, rather than picking between Two small process items remain, both from the end of my previous comment:
Neither blocks the design. Once the state field is updated and the pull request is marked ready, this is in good shape from my side. |
|
This pull request has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity. |
github-actions
Bot
added
the
state:stale
This branch has not been deployed
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