P3.1: establish public contract baseline - #2
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c71838dbe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "additionalProperties": false, | ||
| "required": ["command_id", "tenant_id", "corelink_device_id", "command_type", "payload", "status", "created_at", "updated_at"], | ||
| "properties": { |
There was a problem hiding this comment.
Because this schema sets additionalProperties to false but neither declares nor requires attempt_count, every command response conforming to components.schemas.Command in openapi/corelink-public-v1.yaml is rejected by the advertised reusable schema; responses containing the OpenAPI-defined timeout_at, acknowledged_at, or completed_at fields are rejected as well. Consumers validating generated API responses against schemas/command.schema.json therefore report valid commands as invalid, so the definitions should be shared or kept structurally identical.
Useful? React with 👍 / 👎.
| DeviceUpdate: | ||
| type: object | ||
| minProperties: 1 |
There was a problem hiding this comment.
When a patch body contains only a misspelled or unsupported field, such as {"stats":"retired"}, it satisfies minProperties: 1 because object schemas allow additional properties by default. Contract validators and mock servers therefore accept a request that updates none of the fields defined by DeviceUpdate; close the request schema to unknown properties or otherwise require at least one recognized update field.
Useful? React with 👍 / 👎.
| responses: | ||
| '200': |
There was a problem hiding this comment.
When a required dependency is unavailable, a readiness endpoint must expose a non-success response so probes can withdraw the instance, but this operation documents only 200 and the shared Health schema has no not-ready state. Generated mocks and contract tests consequently cannot represent the endpoint's essential failure case, and an actual 503 response would fall outside the documented contract and its Problem Details policy; add the not-ready response explicitly.
Useful? React with 👍 / 👎.
| payload: {$ref: '#/components/schemas/EventEnvelope'} | ||
| CommandStatus: | ||
| name: CommandStatus | ||
| payload: {$ref: '#/components/schemas/EventEnvelope'} |
There was a problem hiding this comment.
Both channel-specific messages reference the same unconstrained envelope, whose only event_type example is command.completed; generated documentation therefore presents that command event as the payload for device.lifecycle, and contract validation also accepts command events on the device channel (or arbitrary event types on either channel). Give each message an event-specific event_type constraint and example so routing mistakes are detectable.
Useful? React with 👍 / 👎.
| receiveDeviceLifecycle: | ||
| action: receive | ||
| channel: {$ref: '#/channels/device.lifecycle'} | ||
| receiveCommandStatus: | ||
| action: receive | ||
| channel: {$ref: '#/channels/command.status'} |
There was a problem hiding this comment.
For the outward public and partner deliveries described by this contract, action: receive declares that the application represented by the AsyncAPI document consumes messages from these channels rather than publishing them. Generators that honor operation direction will consequently produce consumer operations for CoreLink's lifecycle and status notifications; use send for the CoreLink publisher perspective or explicitly redefine the document as a consumer-side contract.
Useful? React with 👍 / 👎.
| `v1` is a public, supported contract. Its canonical source is this repository; | ||
| runtime implementation, SDKs, the CLI, documentation and the mock server must | ||
| be verified against it before release. |
There was a problem hiding this comment.
This policy labels v1 as a public, supported contract, while the README says the only available 1.0.0-draft is prerelease-only and explicitly not a release claim. A consumer reading the canonical policy can therefore reasonably rely on compatibility and deprecation guarantees that the repository says are not yet active; scope the promise to a released v1 version or mark the current draft unsupported consistently.
Useful? React with 👍 / 👎.
| - New optional fields, optional query parameters, new enum values and new | ||
| operations are additive changes. Consumers must ignore unknown response | ||
| fields and handle unknown enum values safely. |
There was a problem hiding this comment.
The policy classifies new enum values as additive, but the public schemas model DeviceStatus and CommandStatus with closed enum constraints. After adding a value, any consumer or generated SDK validating a response against the previous v1 schema rejects that value, so the promised change is observably breaking; either model these as extensible strings with documented known values or classify enum expansion as breaking.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 701e6932bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| operationId: createDevice | ||
| x-corelink-stability: stable | ||
| requestBody: |
There was a problem hiding this comment.
When a client retries createDevice after a timeout or lost response, this operation provides neither an idempotency key nor replay semantics, so the same request can create duplicate device records; unlike command creation, the body contains no client-controlled unique identifier that can safely deduplicate it. This also violates the repository rule in README.md that idempotency be defined for every operation, so the contract should specify a key and its replay behavior before SDKs consume it.
Useful? React with 👍 / 👎.
| '200': | ||
| description: Tenant-scoped device page. | ||
| content: {application/json: {schema: {$ref: '#/components/schemas/DevicePage'}}} | ||
| '401': {$ref: '#/components/responses/Unauthorized'} | ||
| '403': {$ref: '#/components/responses/Forbidden'} |
There was a problem hiding this comment.
When listDevices receives an invalid query or path value, such as limit=0, an unknown status, or a malformed UUID, validation must return a non-success response, but this operation advertises only 200, 401, and 403. Generated clients, mocks, and contract tests therefore cannot represent this routine failure path or verify the promised Problem Details payload; add the applicable validation response, consistent with the runtime-parity plan for 400 errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea88362156
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if candidate_major.to_i > base_major.to_i | ||
| warn "Breaking changes are permitted because contract major changed from #{base_major} to #{candidate_major}." |
There was a problem hiding this comment.
When a breaking PR changes info.version in openapi/corelink-public-v1.yaml from 1.x to 2.x, this branch permits every detected removal even though the workflow still compares and publishes the same v1 file. That overwrites the contract consumed by clients pinned to the v1 document, contrary to the policy requiring a separate v2 document; verify the major from the candidate filename or never waive changes to the v1 file.
Useful? React with 👍 / 👎.
| base_required = Array(base_schema["required"]) | ||
| candidate_required = Array(candidate_schema["required"]) | ||
| added_required = candidate_required - base_required | ||
| changes << "#{location}: fields made required (#{added_required.join(", ")})" unless added_required.empty? |
There was a problem hiding this comment.
When a response schema removes a field from required while leaving it in properties, this calculation produces no change and the gate passes, allowing the server to omit a value that existing generated clients require. Requiredness must be compared in the opposite direction for response schemas; the current direction is appropriate only for requests and also incorrectly rejects a response field becoming guaranteed.
Useful? React with 👍 / 👎.
| base_parameters = Array(base_path_item["parameters"]) + Array(base_operation["parameters"]) | ||
| candidate_parameters = Array(candidate_path_item["parameters"]) + Array(candidate_operation["parameters"]) | ||
| base_parameters.each do |parameter| |
There was a problem hiding this comment.
When a later v1 contract adds a new required query or header parameter, it is absent from base_parameters, so this base-only loop never examines it and reports the breaking change as compatible. Iterate candidate-only parameters as well and reject any newly introduced required parameter, otherwise existing clients cannot call the changed operation successfully.
Useful? React with 👍 / 👎.
| value | ||
| end | ||
|
|
||
| def schema_changes(base_spec, candidate_spec, base_schema, candidate_schema, location, changes) |
There was a problem hiding this comment.
When a request constraint is narrowed without changing its type or enum—for example, reducing CommandCreate.command_type.maxLength from 150 to 10—the recursive comparison reports compatibility even though previously valid requests are rejected. The policy explicitly classifies tighter validation as breaking, so this routine must compare bounds, lengths, patterns, and equivalent object/array constraints.
Useful? React with 👍 / 👎.
| base_operation.fetch("responses", {}).each do |status, base_response| | ||
| next unless status.match?(/^2/) |
There was a problem hiding this comment.
When a v1 operation deletes a documented error response such as 404, this filter skips it and the compatibility gate passes. That directly leaves the policy's promise that documented error codes are not removed unenforced and can remove generated SDK error variants; compare all documented response statuses rather than only 2xx responses.
Useful? React with 👍 / 👎.
| candidate_operation = candidate_path_item[method] | ||
| unless candidate_operation | ||
| changes << "operation removed: #{method.upcase} #{path}" | ||
| next | ||
| end |
There was a problem hiding this comment.
When a previously anonymous operation gains inherited bearerAuth by removing security: [], or an authenticated operation gains additional security requirements, this checker compares parameters, bodies, and responses but never effective security. The policy classifies authentication and authorization expansion as breaking, and existing callers will receive authentication failures, so compare operation security after applying root-level inheritance.
Useful? React with 👍 / 👎.
| paths: | ||
| - "openapi/corelink-public-v*.yaml" | ||
| - "asyncapi/**" |
There was a problem hiding this comment.
When a pull request changes only openapi/corelink-admin-v1.yaml or openapi/corelink-internal-v1.yaml, none of the configured path filters match, so the workflow does not run even though its parse step explicitly includes both files. A malformed administrative or internal contract can therefore merge without the advertised syntax check; include those OpenAPI paths in the trigger.
Useful? React with 👍 / 👎.
| candidate_operation = candidate_path_item[method] | ||
| unless candidate_operation | ||
| changes << "operation removed: #{method.upcase} #{path}" | ||
| next | ||
| end |
There was a problem hiding this comment.
When a later v1 change renames an operation's operationId while leaving its path and method intact, this checker accepts it because it never compares operation metadata. Generated SDKs commonly derive public method names from operationId, so the next generation removes the old method and breaks source compatibility; treat an operationId change as breaking within v1.
Useful? React with 👍 / 👎.
| - name: Reject unversioned breaking public changes | ||
| if: github.event_name == 'pull_request' | ||
| run: | | ||
| git show "origin/${{ github.base_ref }}:openapi/corelink-public-v1.yaml" > /tmp/base-public.yaml || true | ||
| ruby scripts/check_openapi_compatibility.rb /tmp/base-public.yaml openapi/corelink-public-v1.yaml |
There was a problem hiding this comment.
When a PR removes an AsyncAPI channel or narrows one of the reusable JSON Schemas, the workflow is triggered but merely parses those files; the only compatibility command compares the public OpenAPI document. Consumers generated from asyncapi/ or validating against schemas/ can therefore break while the advertised contract-compatibility job stays green, so diff those public artifacts as well.
Useful? React with 👍 / 👎.
| - name: Parse versioned contracts | ||
| run: | | ||
| ruby -e 'require "yaml"; %w[openapi/corelink-public-v1.yaml openapi/corelink-admin-v1.yaml openapi/corelink-internal-v1.yaml asyncapi/corelink-events-v1.yaml].each { |path| YAML.safe_load(File.read(path), permitted_classes: [], aliases: false); puts "parsed #{path}" }' |
There was a problem hiding this comment.
When the compatibility policy is followed by adding openapi/corelink-public-v2.yaml alongside v1, the path filter starts this workflow but this hard-coded parse command reads only v1; the compatibility command below also compares only v1. The entire new major contract can thus contain malformed YAML and still pass, so discover and validate every matching versioned public contract rather than enumerating only the current file.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fb0dc86a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "title": "CoreLink Device", | ||
| "type": "object", | ||
| "additionalProperties": false, | ||
| "required": ["corelink_device_id", "tenant_id", "device_model_id", "status", "created_at", "updated_at"], |
There was a problem hiding this comment.
When tooling consumes schemas/device.schema.json instead of the OpenAPI document, it accepts or generates a Device without metadata, even though components.schemas.Device requires that field for every API response. This makes the two advertised definitions of the same resource disagree and can let standalone contract tests pass payloads that API validators reject; add metadata to this required list or share one canonical schema.
Useful? React with 👍 / 👎.
| CommandStatus: | ||
| name: CommandStatus | ||
| payload: {$ref: '#/components/schemas/EventEnvelope'} |
There was a problem hiding this comment.
For a command.status delivery, this message accepts any object as data, including {}, because the shared envelope provides no command ID, status, or other channel-specific requirements; the device message has the same problem. Consequently generated consumers receive only an untyped object and contract validation cannot detect malformed event bodies, so each message should specialize data with its actual lifecycle or command-status payload schema.
Useful? React with 👍 / 👎.
| "name": "Validation example (missing token)", | ||
| "request": { | ||
| "method": "GET", | ||
| "url": "{{base_url}}/api/v1/tenants/{{tenant_id}}/devices?limit=0" | ||
| }, | ||
| "event": [{ "listen": "test", "script": { "exec": ["pm.test('returns a public problem response', function () { pm.expect([400, 401]).to.include(pm.response.code); });"] } }] |
There was a problem hiding this comment.
Because this request deliberately omits the token and treats 401 as success, an implementation that rejects authentication before validating limit=0 passes the test without exercising request validation at all. The test also never inspects the advertised Problem Details media type or body, so it cannot provide the validation coverage claimed by the collection; send a valid token and require the documented validation response.
Useful? React with 👍 / 👎.
| changes << "success response removed: #{method.upcase} #{path} #{status}" | ||
| next | ||
| end | ||
| content_schemas(base_spec, base_response).each do |media_type, schema| |
There was a problem hiding this comment.
When a v1 success response retains its status and body but removes a documented header, this loop never records a change because it compares only content schemas. Generated clients can depend on headers such as pagination metadata, Deprecation, or Sunset, and the lifecycle policy explicitly makes the latter headers part of the public deprecation contract, so response-header removals must also be checked.
Useful? React with 👍 / 👎.
| firmware_release_id: {type: [string, 'null'], format: uuid} | ||
| name: {type: [string, 'null'], maxLength: 255} | ||
| metadata: {type: object, additionalProperties: true} | ||
| DevicePage: {type: object, required: [items, total, limit, offset], properties: {items: {type: array, items: {$ref: '#/components/schemas/Device'}}, total: {type: integer}, limit: {type: integer}, offset: {type: integer}}} |
There was a problem hiding this comment.
A device page with total: -1, limit: 0, or a negative offset currently satisfies this response schema, even though those values cannot describe a valid page and the corresponding request parameters already require positive/nonnegative ranges. Contract validation therefore cannot catch pagination responses that can break client page-count or navigation logic; apply minimum: 0 to total and offset and minimum: 1 to limit, including the identical CommandPage fields.
Useful? React with 👍 / 👎.
| next | ||
| end | ||
|
|
||
| %w[get put post patch delete head options].each do |method| |
There was a problem hiding this comment.
When a v1 document defines an OpenAPI trace operation and a later candidate removes it, this hard-coded method list never visits the operation, so the compatibility gate reports success despite removing a public endpoint. Include trace alongside the other Path Item operation fields so every operation type supported by the declared OpenAPI version is protected.
Useful? React with 👍 / 👎.
1 participant
Implements the first contract-first deliverable for CoreLinkPlatform/platform#94.
Validation: JSON parse, YAML parse and
git diff --checkpassed. Redocly was not available because npm dependency retrieval did not complete in this environment.This PR intentionally does not claim SDK generation or sandbox completion; those stay gated on runtime parity and contract-diff CI.