test: support hermetic local API reference - #316
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the scripts/mock helper to prefer a locally available, generated OpenAPI reference (when present) for starting the SDK mock server, while keeping the existing URL-based behavior as a fallback.
Changes:
- Add detection of a local transformed OpenAPI spec (
api_reference/openapi.transformed.yml) plus generation metadata (.castiron.stats.yml). - Validate the local spec against the expected hash before using it.
- Preserve the existing behavior of reading
openapi_spec_urlfrom.stats.ymlwhen no local reference is available.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [[ -z "$EXPECTED_HASH" || "$ACTUAL_HASH" != "$EXPECTED_HASH" ]]; then | ||
| echo "Error: Local OpenAPI specification does not match generation metadata" >&2 | ||
| exit 1 | ||
| fi |
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Thermo-nuclear code-quality review: no high-confidence maintainability findings. The change keeps local-spec selection and validation together in the existing flow; scripts/mock remains 61 lines and no file approaches the 1,000-line threshold. Approved.
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Reviewed this exact head against the Castiron checked-in API-reference and metadata contracts, source-checkout test invocation, existing network fallback, and gem packaging. Found one P1 contract mismatch that prevents the proposed hermetic-local-reference path from working; details and fix direction are inline. Reproduced both failure modes and confirmed the explicit-argument and legacy-fallback behavior with seven isolated focused checks.
| if [[ -n "$1" && "$1" != '--'* ]]; then | ||
| URL="$1" | ||
| shift | ||
| elif [[ -f .castiron.stats.yml && -f api_reference/openapi.transformed.yml ]]; then |
There was a problem hiding this comment.
[P1] Consume the actual Castiron local-reference contract. The Castiron producer checks in .castiron/api-definition.yaml and .castiron/generation-inputs.json; the descriptor's api_reference records the canonical repo-relative path and a SHA-256 hash. See https://github.com/openai/openai/blob/23bc2f67b559457998cb0facddccb8b9763ed9f4/api/sdk-worker/sdk_worker/api_ref_artifacts/castiron_repo_inputs.py#L16-L20 and https://github.com/openai/openai/blob/23bc2f67b559457998cb0facddccb8b9763ed9f4/api/sdk-worker/sdk_worker/api_ref_artifacts/castiron_repo_inputs.py#L175-L206. .castiron.stats.yml has only schema_version, generation_id, openapi_spec_hash, config_hash, and codegen_sha; its parser explicitly rejects additional fields, so openapi_transformed_spec_hash cannot be added to make this work: https://github.com/openai/openai/blob/23bc2f67b559457998cb0facddccb8b9763ed9f4/api/sdk-worker/sdk_worker/api_ref_artifacts/castiron_candidate_flow.py#L106-L133. Consequently, a real generated checkout never enters this branch and instead downloads the stale/network spec, breaking offline hermetic runs; if api_reference/openapi.transformed.yml is present alongside valid Castiron stats, EXPECTED_HASH is always empty and scripts/mock/scripts/test exit before starting Steady. I reproduced both behaviors against this exact script. Please discover the API reference from .castiron/generation-inputs.json, require its canonical .castiron/api-definition.yaml path, verify the descriptor's SHA-256 against the file, and preserve the existing .stats.yml URL fallback only when no checked-in Castiron reference is present.
Allow the SDK mock server to use a locally available API reference when present, while preserving the existing network-based fallback.