feat: separate user-facing schema (write/read) from internal model [INFP-234]#9814
feat: separate user-facing schema (write/read) from internal model [INFP-234]#9814dgarros wants to merge 42 commits into
Conversation
dgarros
added
type/feature
github-actions
Bot
added
type/documentation
Merging this PR will improve performance by 10.71%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_get_schema |
374.9 ms | 338.6 ms | +10.71% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing dga/user-schema-infp-234-gqj6d (65cde1b) with develop (ae0ff85)
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would require human review. Introduces breaking API change to schema load, modifies core schema definitions and validation logic, and requires coordinated SDK release. High impact, requires human review.
Re-trigger cubic
There was a problem hiding this comment.
3 issues found and verified against the latest diff
Confidence score: 3/5
- In
tasks/backend.py,backend.validate-generateddoes not diff-checkpython_sdk/infrahub_sdk/schema/generated, so generated SDK schema drift can slip through CI and ship stale or inconsistent SDK artifacts — add that path to the validation diff before merging. - In
PRD-user-facing-schema-separation.md, FR-008 requires removing hand-written SDK models, but the PR scope explicitly defers that, creating a spec-to-implementation mismatch that can confuse release expectations and acceptance decisions — either align the PR scope with FR-008 or update FR-008/Non-goals to clearly stage this work. - In
dev/specs/002-user-facing-schema/plan.md, the Constitution Check still claims hand-written SDK model removal while the PR defers it, which can cause incorrect sign-off against Principle VII — update the plan’s check criteria to match the staged rollout before merge approval.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dev/specs/002-user-facing-schema/plan.md">
<violation number="1" location="dev/specs/002-user-facing-schema/plan.md:31">
P3: Plan's Constitution Check (Principle VII) states 'the SDK's hand-written schema models are removed (net reduction of parallel definitions)', but the PR description explicitly lists 'No removal of hand-written SDK models in this PR' as a deferred non-goal. The claimed net reduction benefit won't be realized in this scope; the plan should reflect the actual scope boundary so it doesn't mislead reviewers about what this PR delivers.</violation>
</file>
<file name="tasks/backend.py">
<violation number="1" location="tasks/backend.py:385">
P2: Generated SDK schema files can drift unnoticed because the generation step now writes `python_sdk/infrahub_sdk/schema/generated`, but `backend.validate-generated` does not diff-check that path. Adding that path to the validation diff keeps CI aligned with the new generator output.</violation>
</file>
<file name="PRD-user-facing-schema-separation.md">
<violation number="1" location="PRD-user-facing-schema-separation.md:41">
P2: FR-008 requires replacing the hand-written SDK models and leaving "no third parallel definition," but this is excluded from the current PR's scope (the Non-goals section of the PR description explicitly defers removal). The PRD's own Out of Scope section doesn't list FR-008, creating a contradiction: a reader of the PRD would expect old model classes to be removed as part of this work, while the actual PR scope preserves them. Either FR-008 should be moved to the Out of Scope / deferred section, or its verify criteria should be dropped until a follow-up.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| - **FR-005**: The read model MUST include `read`-level fields and exclude `internal` fields. *Verify:* GET returns `inherited`; it never returns internal bookkeeping fields. | ||
| - **FR-006**: The write and read models MUST be generated into the Python SDK and be importable with only the SDK installed (no server, no backend package). *Verify:* import and validate a payload in an SDK-only environment. | ||
| - **FR-007**: The backend MUST validate write submissions and serialise read responses using the SDK-hosted models, not a backend-local copy. *Verify:* a rule change in the generated model changes both server and SDK behaviour. | ||
| - **FR-008**: The SDK's existing hand-written schema models MUST be replaced by the generated write/read models, leaving no third parallel definition. *Verify:* old model classes are removed; SDK callers use the generated ones. |
There was a problem hiding this comment.
P2: FR-008 requires replacing the hand-written SDK models and leaving "no third parallel definition," but this is excluded from the current PR's scope (the Non-goals section of the PR description explicitly defers removal). The PRD's own Out of Scope section doesn't list FR-008, creating a contradiction: a reader of the PRD would expect old model classes to be removed as part of this work, while the actual PR scope preserves them. Either FR-008 should be moved to the Out of Scope / deferred section, or its verify criteria should be dropped until a follow-up.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At PRD-user-facing-schema-separation.md, line 41:
<comment>FR-008 requires replacing the hand-written SDK models and leaving "no third parallel definition," but this is excluded from the current PR's scope (the Non-goals section of the PR description explicitly defers removal). The PRD's own Out of Scope section doesn't list FR-008, creating a contradiction: a reader of the PRD would expect old model classes to be removed as part of this work, while the actual PR scope preserves them. Either FR-008 should be moved to the Out of Scope / deferred section, or its verify criteria should be dropped until a follow-up.</comment>
<file context>
@@ -0,0 +1,126 @@
+- **FR-005**: The read model MUST include `read`-level fields and exclude `internal` fields. *Verify:* GET returns `inherited`; it never returns internal bookkeeping fields.
+- **FR-006**: The write and read models MUST be generated into the Python SDK and be importable with only the SDK installed (no server, no backend package). *Verify:* import and validate a payload in an SDK-only environment.
+- **FR-007**: The backend MUST validate write submissions and serialise read responses using the SDK-hosted models, not a backend-local copy. *Verify:* a rule change in the generated model changes both server and SDK behaviour.
+- **FR-008**: The SDK's existing hand-written schema models MUST be replaced by the generated write/read models, leaving no third parallel definition. *Verify:* old model classes are removed; SDK callers use the generated ones.
+
+## Key Entities
</file context>
|
|
||
| **Scale/Scope**: Four schema families (node, generic, attribute, relationship) × ~20–30 fields each; one generator template; two API endpoints; one SDK schema module. | ||
|
|
||
| ## Constitution Check |
There was a problem hiding this comment.
P3: Plan's Constitution Check (Principle VII) states 'the SDK's hand-written schema models are removed (net reduction of parallel definitions)', but the PR description explicitly lists 'No removal of hand-written SDK models in this PR' as a deferred non-goal. The claimed net reduction benefit won't be realized in this scope; the plan should reflect the actual scope boundary so it doesn't mislead reviewers about what this PR delivers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/002-user-facing-schema/plan.md, line 31:
<comment>Plan's Constitution Check (Principle VII) states 'the SDK's hand-written schema models are removed (net reduction of parallel definitions)', but the PR description explicitly lists 'No removal of hand-written SDK models in this PR' as a deferred non-goal. The claimed net reduction benefit won't be realized in this scope; the plan should reflect the actual scope boundary so it doesn't mislead reviewers about what this PR delivers.</comment>
<file context>
@@ -0,0 +1,99 @@
+
+**Scale/Scope**: Four schema families (node, generic, attribute, relationship) × ~20–30 fields each; one generator template; two API endpoints; one SDK schema module.
+
+## Constitution Check
+
+*GATE: evaluated pre-research and re-checked post-design.*
</file context>
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tasks/backend.py">
<violation number="1" location="tasks/backend.py:440">
P2: Offline SDK validation can accept a schema payload that `/api/schema/load` still rejects because the generated write root makes `version` optional while the server load model requires it. In opsmill/infrahub-sdk-python#1135/infrahub_sdk/schema/generated/write.py:360-363, making `InfrahubSchemaWrite.version` required would keep the documented server/SDK write-contract parity.</violation>
<violation number="2" location="tasks/backend.py:446">
P3: `InfrahubSchemaWrite` is missing `extra="forbid"` on its `model_config`, allowing unknown root-level fields to be silently ignored instead of rejected. The nested write models (NodeSchemaWrite, GenericSchemaWrite, etc.) all set `extra="forbid"`, so this is an inconsistency at the root level. While the SDK's `validate_schema()` function validates items individually and is unaffected, `InfrahubSchemaWrite` is exported as a public model (in `__init__.py`'s `__all__`), so users or future code paths that validate a payload with `InfrahubSchemaWrite.model_validate(...)` directly would not catch a misspelled root key like `"verson"` or an extraneous field like `"metadata"`. Consider adding `model_config = ConfigDict(extra="forbid")` to the write-variant root class, either by threading `model_config_args` into the template's root-class block or by adding it conditionally in `_root()`.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ] | ||
|
|
||
| def _root(suffix: str, with_version: bool) -> dict[str, Any]: | ||
| version_field = ["version: str | None = None"] if with_version else [] |
There was a problem hiding this comment.
P2: Offline SDK validation can accept a schema payload that /api/schema/load still rejects because the generated write root makes version optional while the server load model requires it. In opsmill/infrahub-sdk-python#1135/infrahub_sdk/schema/generated/write.py:360-363, making InfrahubSchemaWrite.version required would keep the documented server/SDK write-contract parity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tasks/backend.py, line 440:
<comment>Offline SDK validation can accept a schema payload that `/api/schema/load` still rejects because the generated write root makes `version` optional while the server load model requires it. In opsmill/infrahub-sdk-python#1135/infrahub_sdk/schema/generated/write.py:360-363, making `InfrahubSchemaWrite.version` required would keep the documented server/SDK write-contract parity.</comment>
<file context>
@@ -412,36 +412,50 @@ def _generate_schemas_sdk(context: Context) -> None:
]
+ def _root(suffix: str, with_version: bool) -> dict[str, Any]:
+ version_field = ["version: str | None = None"] if with_version else []
+ fields = [
+ *version_field,
</file context>
| version_field = ["version: str | None = None"] if with_version else [] | |
| version_field = ["version: str"] if with_version else [] |
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 5 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tasks/backend.py">
<violation number="1" location="tasks/backend.py:418">
P2: The `computed_attribute_fields` list duplicates the schema of the canonical `ComputedAttribute` model from `computed_attribute.py`. Because `internal.py` only references `ComputedAttribute` as an opaque `internal_kind`, the generator has to hardcode the sub-fields here. That creates a second source of truth: if the canonical model changes (new fields, updated descriptions, or metadata), SDK generation will silently drift from backend behavior and reintroduce the server/SDK contract mismatch this PR aims to eliminate. Consider deriving these fields programmatically from the `ComputedAttribute` model or representing them as a proper schema node in `internal.py` so the template can consume them directly.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| # The computed_attribute block is a value model of its own (kind + template/transform); render it | ||
| # as a dedicated data model so the write contract is explicit instead of an opaque mapping. | ||
| computed_attribute_fields = [ |
There was a problem hiding this comment.
P2: The computed_attribute_fields list duplicates the schema of the canonical ComputedAttribute model from computed_attribute.py. Because internal.py only references ComputedAttribute as an opaque internal_kind, the generator has to hardcode the sub-fields here. That creates a second source of truth: if the canonical model changes (new fields, updated descriptions, or metadata), SDK generation will silently drift from backend behavior and reintroduce the server/SDK contract mismatch this PR aims to eliminate. Consider deriving these fields programmatically from the ComputedAttribute model or representing them as a proper schema node in internal.py so the template can consume them directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tasks/backend.py, line 418:
<comment>The `computed_attribute_fields` list duplicates the schema of the canonical `ComputedAttribute` model from `computed_attribute.py`. Because `internal.py` only references `ComputedAttribute` as an opaque `internal_kind`, the generator has to hardcode the sub-fields here. That creates a second source of truth: if the canonical model changes (new fields, updated descriptions, or metadata), SDK generation will silently drift from backend behavior and reintroduce the server/SDK contract mismatch this PR aims to eliminate. Consider deriving these fields programmatically from the `ComputedAttribute` model or representing them as a proper schema node in `internal.py` so the template can consume them directly.</comment>
<file context>
@@ -412,11 +413,42 @@ def _generate_schemas_sdk(context: Context) -> None:
+ # The computed_attribute block is a value model of its own (kind + template/transform); render it
+ # as a dedicated data model so the write contract is explicit instead of an opaque mapping.
+ computed_attribute_fields = [
+ SchemaAttribute(
+ name="kind",
</file context>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dev/specs/002-user-facing-schema/plan.md">
<violation number="1" location="dev/specs/002-user-facing-schema/plan.md:31">
P3: Plan's Constitution Check (Principle VII) states 'the SDK's hand-written schema models are removed (net reduction of parallel definitions)', but the PR description explicitly lists 'No removal of hand-written SDK models in this PR' as a deferred non-goal. The claimed net reduction benefit won't be realized in this scope; the plan should reflect the actual scope boundary so it doesn't mislead reviewers about what this PR delivers.</violation>
</file>
<file name="tasks/backend.py">
<violation number="1" location="tasks/backend.py:385">
P2: Generated SDK schema files can drift unnoticed because the generation step now writes `python_sdk/infrahub_sdk/schema/generated`, but `backend.validate-generated` does not diff-check that path. Adding that path to the validation diff keeps CI aligned with the new generator output.</violation>
<violation number="2" location="tasks/backend.py:418">
P2: The `computed_attribute_fields` list duplicates the schema of the canonical `ComputedAttribute` model from `computed_attribute.py`. Because `internal.py` only references `ComputedAttribute` as an opaque `internal_kind`, the generator has to hardcode the sub-fields here. That creates a second source of truth: if the canonical model changes (new fields, updated descriptions, or metadata), SDK generation will silently drift from backend behavior and reintroduce the server/SDK contract mismatch this PR aims to eliminate. Consider deriving these fields programmatically from the `ComputedAttribute` model or representing them as a proper schema node in `internal.py` so the template can consume them directly.</violation>
<violation number="3" location="tasks/backend.py:440">
P2: Offline SDK validation can accept a schema payload that `/api/schema/load` still rejects because the generated write root makes `version` optional while the server load model requires it. In opsmill/infrahub-sdk-python#1135/infrahub_sdk/schema/generated/write.py:360-363, making `InfrahubSchemaWrite.version` required would keep the documented server/SDK write-contract parity.</violation>
</file>
<file name="PRD-user-facing-schema-separation.md">
<violation number="1" location="PRD-user-facing-schema-separation.md:41">
P2: FR-008 requires replacing the hand-written SDK models and leaving "no third parallel definition," but this is excluded from the current PR's scope (the Non-goals section of the PR description explicitly defers removal). The PRD's own Out of Scope section doesn't list FR-008, creating a contradiction: a reader of the PRD would expect old model classes to be removed as part of this work, while the actual PR scope preserves them. Either FR-008 should be moved to the Out of Scope / deferred section, or its verify criteria should be dropped until a follow-up.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 6 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 6 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 5 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 6 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 5 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 5 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 5 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dev/specs/002-user-facing-schema/plan.md">
<violation number="1" location="dev/specs/002-user-facing-schema/plan.md:31">
P3: Plan's Constitution Check (Principle VII) states 'the SDK's hand-written schema models are removed (net reduction of parallel definitions)', but the PR description explicitly lists 'No removal of hand-written SDK models in this PR' as a deferred non-goal. The claimed net reduction benefit won't be realized in this scope; the plan should reflect the actual scope boundary so it doesn't mislead reviewers about what this PR delivers.</violation>
</file>
<file name="tasks/backend.py">
<violation number="1" location="tasks/backend.py:385">
P2: Generated SDK schema files can drift unnoticed because the generation step now writes `python_sdk/infrahub_sdk/schema/generated`, but `backend.validate-generated` does not diff-check that path. Adding that path to the validation diff keeps CI aligned with the new generator output.</violation>
<violation number="2" location="tasks/backend.py:418">
P2: The `computed_attribute_fields` list duplicates the schema of the canonical `ComputedAttribute` model from `computed_attribute.py`. Because `internal.py` only references `ComputedAttribute` as an opaque `internal_kind`, the generator has to hardcode the sub-fields here. That creates a second source of truth: if the canonical model changes (new fields, updated descriptions, or metadata), SDK generation will silently drift from backend behavior and reintroduce the server/SDK contract mismatch this PR aims to eliminate. Consider deriving these fields programmatically from the `ComputedAttribute` model or representing them as a proper schema node in `internal.py` so the template can consume them directly.</violation>
<violation number="3" location="tasks/backend.py:440">
P2: Offline SDK validation can accept a schema payload that `/api/schema/load` still rejects because the generated write root makes `version` optional while the server load model requires it. In opsmill/infrahub-sdk-python#1135/infrahub_sdk/schema/generated/write.py:360-363, making `InfrahubSchemaWrite.version` required would keep the documented server/SDK write-contract parity.</violation>
</file>
<file name="PRD-user-facing-schema-separation.md">
<violation number="1" location="PRD-user-facing-schema-separation.md:41">
P2: FR-008 requires replacing the hand-written SDK models and leaving "no third parallel definition," but this is excluded from the current PR's scope (the Non-goals section of the PR description explicitly defers removal). The PRD's own Out of Scope section doesn't list FR-008, creating a contradiction: a reader of the PRD would expect old model classes to be removed as part of this work, while the actual PR scope preserves them. Either FR-008 should be moved to the Out of Scope / deferred section, or its verify criteria should be dropped until a follow-up.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 6 unresolved issues from previous reviews.
Re-trigger cubic
bd0af8c to
b64c79a
Compare
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 4 unresolved issues from previous reviews.
Re-trigger cubic
ajtmccarty
left a comment
ajtmccarty
left a comment
There was a problem hiding this comment.
looks pretty good. I think some refactoring of where the code-generation logic lives is in order. that's the only significant issue though
| # ``kind`` is a computed field on the generated read model (derived from namespace+name); | ||
| # only ``hash`` needs to be supplied here since it is computed by the server. |
There was a problem hiding this comment.
I don't think this comment is necessary
| router = APIRouter(prefix="/schema") | ||
|
|
||
|
|
||
| class APISchemaMixin: |
There was a problem hiding this comment.
not created by this PR, but Mixins are a code smell and this one looks pretty easy to remove. it's only called in get_schema() and would be better as a private helper method
| # The user-facing "write" contract lives in the SDK; each submitted node/generic is | ||
| # validated against it so that fields the user may not set (read-level, internal, or | ||
| # unknown) and out-of-range constrained values are rejected at the boundary with a | ||
| # field-level message, before the payload is coerced into the richer internal models. |
There was a problem hiding this comment.
I think this comment is unnecessary. There's no information here that is not covered by either where validate_write_schema is imported from or the names of the functions.
| # The published request contract only exposes user-settable fields; the processing | ||
| # engine operates on the richer internal SchemaRoot. Building it here surfaces | ||
| # internal-only invariants (e.g. reserved keywords) as request-validation errors, | ||
| # and lets the endpoints feed the pipeline without re-deriving the conversion. |
There was a problem hiding this comment.
4-line comment is overkill
# Build the internal model here to surface validation errors at construction
| internal_schema = schema.internal_schema | ||
| candidate_schemas.append(internal_schema) | ||
| errors += internal_schema.validate_namespaces() | ||
| errors += internal_schema.validate_reserved_suffixes() |
There was a problem hiding this comment.
looks like these two should be one function on SchemaRoot
| } | ||
|
|
||
|
|
||
| def _sdk_enum_specs() -> tuple[dict[str, list[tuple[str, str]]], dict[str, str], dict[str, str]]: |
There was a problem hiding this comment.
a dataclass response might make this more readable
There was a problem hiding this comment.
this new code looks like it should be its own class or module. it more than doubles the size of this file
There was a problem hiding this comment.
I agree, I'm planning to do a refactoring of this code in another PR
| Path(f"{generated}/__init__.py").write_text(init_content, encoding="utf-8") | ||
|
|
||
|
|
||
| def _generate_schemas_sdk(context: Context) -> None: |
There was a problem hiding this comment.
this function has 8 functions defined inside of it. should probably be a class that defines methods
88cc757 to
e7d0487
Compare
…ion kind [INFP-234] An invalid attribute kind fails the discriminated-union tag and short-circuits per-variant field checks, so it can't be reported alongside co-located fields. Use a valid kind so the read-level/unknown fields are the asserted rejections; out-of-enum kind stays covered by the dedicated enum tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the inline Literal rendering of constrained fields in the generated SDK write/read models with dedicated (str, Enum) classes emitted into a new generated/enums.py. Add an enum_class marker on SchemaAttribute so the SDK generator maps each constrained field to its enum, add the enums template, and give every generated model use_enum_values=True. The internal backend schema generation is unchanged (byte-identical output). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…NFP-234] Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…NFP-234] Enum-backed fields now reference shared enum component schemas instead of repeating inline literal enums. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…NFP-234] Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lues check [INFP-234] The SDK schema generation now emits dedicated Enum subclasses for non-discriminator constrained fields (e.g. branch: BranchSupportType) instead of Literal aliases. Generalize the allowed-values helper to recognise both a Literal and an Enum subclass so the write-model visibility test reflects the intended contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dels [INFP-234] Emit kind as a computed_field and hash as a plain field on the generated read base node model (template gains computed_fields support and a conditional computed_field import). The backend API schema models drop their hand-added api_kind/hash and the set_kind validator; from_schema now only populates hash since kind self-derives. Regenerate openapi.json (kind readOnly, hash nullable) and the frontend REST types, and bump the python_sdk pointer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…NFP-234] The generator now emits `kind` on write node models as a plain property and on read node models as a serialized `@computed_field`. A per-computed-field `serialize` flag in the SDK template renders `@computed_field` only for the read variant, so `kind` gives attribute access everywhere but only enters the payload on read — the write/load contract (`extra="forbid"`) stays clean on round-trip. Bumps the SDK submodule for the regenerated write model. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ir [INFP-234] Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…NFP-234] `backend.validate-generated` already regenerates the user-facing schema models and protocols into the python_sdk submodule (from the same internal definitions) but only diffed the backend copies. Add diffs for the submodule's generated schema dir and protocols.py so CI catches a committed SDK copy that drifts from the backend source — the SDK protocols had no such guard before. The diffs run with `git -C python_sdk` because a superproject `git diff` only tracks the submodule pointer, not the files inside it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…INFP-234] Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ible [INFP-234] The develop rebase introduced a new `ordered` attribute-schema field. Without a visibility it defaulted to internal and was dropped from the user-facing SDK write/read models, even though it is user-settable. Classify it write-visible and regenerate the SDK models, OpenAPI, and frontend types accordingly. Bumps the SDK submodule for the regenerated models/protocols. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ypes [INFP-234] The read schema OpenAPI contract changed: attributes are now a per-kind discriminated union, computed attributes a union, and several components gained Read/Write suffixes (DropdownChoiceRead, RelationshipSchemaRead, *AttributeParametersRead). Update the frontend consumers accordingly: - Point the entities/schema type aliases at the current component names; model AttributeSchema/ComputedAttribute as unions. - Introduce a minimal FilterFieldSchema for the global event filters, which pass synthetic field descriptors (e.g. `{ kind: "Dropdown", choices }`) and real schemas through the same input; drop the unused fieldSchema prop from GlobalKindFilter. - Handle the now-optional read `hash` in the object-table schema selector. Locks in the resulting TypeScript-error improvement in .betterer.results. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
16468b5 to
33643fe
Compare
…on [INFP-234] The SDK client now projects schema.load()/check() payloads onto the write contract, so tests and callers can load a schema read back from Infrahub or a full model dump without stripping read-only/internal fields by hand. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
96f2db1 to
55fe3d4
Compare
Catches the SDK up to current infrahub-develop (X-Priority headers and per-request header refactors), aligning client request behavior with what the backend's develop-synced git/credential component tests expect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…NFP-234] A submitted schema may carry read-only/internal fields (a schema read back from Infrahub, or a full internal dump). SchemaLoadAPI now validates the payload against the internal schema first — which accepts those fields but still rejects genuinely unknown or context-invalid ones — then projects it onto the write contract, dropping the read-only/internal fields before the strict write models run. This replaces the reverted client-side stripping, keeping rejection of real mistakes on the server. Bumps the SDK submodule for that revert. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ndary [INFP-234] Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
4 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/infrahub/api/schema.py">
<violation number="1" location="backend/infrahub/api/schema.py:133">
P2: Schema-load requests that explicitly send `extensions: null` now fail validation even though `InfrahubSchemaWrite` accepts that value. Normalizing a null extension to an omitted/empty extension before validating with `SchemaRoot` would preserve the public write contract.</violation>
<violation number="2" location="backend/infrahub/api/schema.py:133">
P1: Malformed schema attributes missing `kind` or `name` can now produce an unhandled `KeyError` rather than a 422 validation response because the internal validation call invokes direct dictionary indexing and the handler catches only `PydanticValidationError`. Keeping this preflight validation from propagating non-Pydantic exceptions would retain the API's validation-error behavior.</violation>
</file>
<file name="changelog/+infp-234-schema-load-read-only.fixed.md">
<violation number="1" location="changelog/+infp-234-schema-load-read-only.fixed.md:1">
P2: The generated release notes will tell users both that read-only/internal fields are tolerated and that they are rejected, including opposite instructions for schema-load clients. Please reconcile `changelog/+infp-234.changed.md` with this corrected behavior, retaining rejection only for genuinely unknown or context-invalid fields.</violation>
</file>
<file name="python_sdk">
<violation number="1" location="python_sdk:1">
P2: Offline `validate_schema()` reports a schema without `version` as valid, but the corresponding `/api/schema/load` request is rejected because `SchemaLoadAPI` requires that field; this makes the advertised offline validation an incomplete server-side contract. The generated write root should make `version` required, matching opsmill/infrahub-sdk-python#1135/infrahub_sdk/schema/generated/write.py:582 with the load model.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| # field on an extension), drawing the tolerate/reject line a uniform extra="forbid" cannot. | ||
| if isinstance(data, dict): | ||
| try: | ||
| SchemaRoot.model_validate(data) |
There was a problem hiding this comment.
P1: Malformed schema attributes missing kind or name can now produce an unhandled KeyError rather than a 422 validation response because the internal validation call invokes direct dictionary indexing and the handler catches only PydanticValidationError. Keeping this preflight validation from propagating non-Pydantic exceptions would retain the API's validation-error behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/api/schema.py, line 133:
<comment>Malformed schema attributes missing `kind` or `name` can now produce an unhandled `KeyError` rather than a 422 validation response because the internal validation call invokes direct dictionary indexing and the handler catches only `PydanticValidationError`. Keeping this preflight validation from propagating non-Pydantic exceptions would retain the API's validation-error behavior.</comment>
<file context>
@@ -122,11 +124,21 @@ class SchemaLoadAPI(InfrahubSchemaWrite):
+ # field on an extension), drawing the tolerate/reject line a uniform extra="forbid" cannot.
if isinstance(data, dict):
+ try:
+ SchemaRoot.model_validate(data)
+ except PydanticValidationError as exc:
+ messages = [f"{'.'.join(str(part) for part in err['loc'])}: {err['msg']}" for err in exc.errors()]
</file context>
| # field on an extension), drawing the tolerate/reject line a uniform extra="forbid" cannot. | ||
| if isinstance(data, dict): | ||
| try: | ||
| SchemaRoot.model_validate(data) |
There was a problem hiding this comment.
P2: Schema-load requests that explicitly send extensions: null now fail validation even though InfrahubSchemaWrite accepts that value. Normalizing a null extension to an omitted/empty extension before validating with SchemaRoot would preserve the public write contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/api/schema.py, line 133:
<comment>Schema-load requests that explicitly send `extensions: null` now fail validation even though `InfrahubSchemaWrite` accepts that value. Normalizing a null extension to an omitted/empty extension before validating with `SchemaRoot` would preserve the public write contract.</comment>
<file context>
@@ -122,11 +124,21 @@ class SchemaLoadAPI(InfrahubSchemaWrite):
+ # field on an extension), drawing the tolerate/reject line a uniform extra="forbid" cannot.
if isinstance(data, dict):
+ try:
+ SchemaRoot.model_validate(data)
+ except PydanticValidationError as exc:
+ messages = [f"{'.'.join(str(part) for part in err['loc'])}: {err['msg']}" for err in exc.errors()]
</file context>
| @@ -0,0 +1 @@ | |||
| The `/api/schema/load` endpoint now tolerates read-only and internal fields on a submitted schema (for example a schema read back from Infrahub, or an exported schema), stripping them before applying the write contract. Fields that are genuinely unknown or invalid for their context are still rejected. | |||
There was a problem hiding this comment.
P2: The generated release notes will tell users both that read-only/internal fields are tolerated and that they are rejected, including opposite instructions for schema-load clients. Please reconcile changelog/+infp-234.changed.md with this corrected behavior, retaining rejection only for genuinely unknown or context-invalid fields.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At changelog/+infp-234-schema-load-read-only.fixed.md, line 1:
<comment>The generated release notes will tell users both that read-only/internal fields are tolerated and that they are rejected, including opposite instructions for schema-load clients. Please reconcile `changelog/+infp-234.changed.md` with this corrected behavior, retaining rejection only for genuinely unknown or context-invalid fields.</comment>
<file context>
@@ -0,0 +1 @@
+The `/api/schema/load` endpoint now tolerates read-only and internal fields on a submitted schema (for example a schema read back from Infrahub, or an exported schema), stripping them before applying the write contract. Fields that are genuinely unknown or invalid for their context are still rejected.
</file context>
| @@ -1 +1 @@ | |||
| Subproject commit 25cf9394da583b43ffd124f2bcbcd748a4ec86b7 | |||
| Subproject commit d15ce11be0d0466d169f5fbfe3fccb49b283095e | |||
There was a problem hiding this comment.
P2: Offline validate_schema() reports a schema without version as valid, but the corresponding /api/schema/load request is rejected because SchemaLoadAPI requires that field; this makes the advertised offline validation an incomplete server-side contract. The generated write root should make version required, matching opsmill/infrahub-sdk-python#1135/infrahub_sdk/schema/generated/write.py:582 with the load model.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python_sdk, line 1:
<comment>Offline `validate_schema()` reports a schema without `version` as valid, but the corresponding `/api/schema/load` request is rejected because `SchemaLoadAPI` requires that field; this makes the advertised offline validation an incomplete server-side contract. The generated write root should make `version` required, matching opsmill/infrahub-sdk-python#1135/infrahub_sdk/schema/generated/write.py:582 with the load model.</comment>
<file context>
@@ -1 +1 @@
-Subproject commit 07579bb9c738daa32d1ea5024eeb46d8916c378d
+Subproject commit d15ce11be0d0466d169f5fbfe3fccb49b283095e
</file context>
…NFP-234] pydantic's FieldInfo.discriminator is typed str | Discriminator | None; the projection only handles string discriminators, so coerce non-str to None to satisfy mypy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 11 unresolved issues from previous reviews.
Re-trigger cubic
…es [INFP-234] Reject genuinely unknown/forbidden fields with a bracketed dotted path and the received value (matching the write-contract error style), and update the extension test: a read-level field is now tolerated (stripped), only the unknown field is rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 11 unresolved issues from previous reviews.
Re-trigger cubic
…INFP-234] The early internal-schema check now rejects only extra/unknown fields; constraint and enum violations and reserved keywords flow through to the write-contract check and build_internal_schema, preserving their original error messages. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/infrahub/api/schema.py">
<violation number="1" location="backend/infrahub/api/schema.py:160">
P2: `POST /api/schema/load` still cannot accept the `/api/schema` response that this boundary is intended to tolerate: the response-only root and computed fields are classified as `extra_forbidden` and rejected before projection. Validating/stripping the known read-contract fields before the unknown-field check would preserve round-tripping while continuing to reject fields unknown to both contracts.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| # Only reject genuinely unknown/forbidden fields here. Constraint and enum | ||
| # violations, reserved keywords, and other invariants are surfaced with their | ||
| # original messages by the write-contract check below or by build_internal_schema. | ||
| unknown = [error for error in exc.errors() if error["type"] == "extra_forbidden"] |
There was a problem hiding this comment.
P2: POST /api/schema/load still cannot accept the /api/schema response that this boundary is intended to tolerate: the response-only root and computed fields are classified as extra_forbidden and rejected before projection. Validating/stripping the known read-contract fields before the unknown-field check would preserve round-tripping while continuing to reject fields unknown to both contracts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/api/schema.py, line 160:
<comment>`POST /api/schema/load` still cannot accept the `/api/schema` response that this boundary is intended to tolerate: the response-only root and computed fields are classified as `extra_forbidden` and rejected before projection. Validating/stripping the known read-contract fields before the unknown-field check would preserve round-tripping while continuing to reject fields unknown to both contracts.</comment>
<file context>
@@ -154,10 +154,15 @@ def validate_write_contract(cls, data: Any) -> Any:
+ # Only reject genuinely unknown/forbidden fields here. Constraint and enum
+ # violations, reserved keywords, and other invariants are surfaced with their
+ # original messages by the write-contract check below or by build_internal_schema.
+ unknown = [error for error in exc.errors() if error["type"] == "extra_forbidden"]
+ if unknown:
+ raise ValueError("; ".join(_format_schema_errors(unknown))) from exc
</file context>
…kind [INFP-234] The SDK read models dropped the deprecated `String` attribute kind. The git component/credential tests mock a schema response from these fixtures, and the client parses it before any node query — so `kind: "String"` made schema parsing fail and the expected repository query was never sent. Use `Text` instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 12 unresolved issues from previous reviews.
Re-trigger cubic
2 participants
Why
The user-facing schema was a direct dump of Infrahub's internal model: it advertised internal-only fields (e.g.
inherited) as settable and described constrained fields (e.g. attributekind) as free-form strings. Users — and especially LLM agents — produced technically-invalid schemas as a result.Goal: generate a distinct write contract (exactly what a user may submit) and read contract (adds visible-but-not-settable fields) from the single source of truth, reject non-settable/out-of-enum input at the load boundary, and let clients validate offline via the SDK.
Non-goals (this PR): full removal of the SDK's hand-written models (FR-009) and publishing the write contract in the REST OpenAPI (FR-004) — both documented as follow-ups in
specs/002-user-facing-schema/opsmill-implement-followups.md.Closes INFP-234
What changed
Behavioral
POST /api/schema/loadnow rejects fields a user may not set (read/internal) and out-of-enum values (including within schemaextensions) with field-level errors — breaking for payloads that previously carried those fields.Implementation
Visibilityaxis on schema field definitions; every field classified write/read/internal from the single sourceinternal.py.generate_schema_sdk.j2); internal models unchanged (byte-identical output).Unchanged: GET
/api/schemaresponse shape; stored-schema read-back; no database or migration changes.Suggested review order
backend/infrahub/core/schema/definitions/internal.py— theVisibilityaxis + per-field classification.tasks/backend.py+backend/templates/generate_schema_sdk.j2— write/read generation into the SDK.backend/infrahub/api/schema.py— themodel_validator(mode="before")boundary gate.How to review
Focus on the boundary gate and the visibility classification. Companion SDK PR (land first): opsmill/infrahub-sdk-python#1135.
How to test
uv run pytest backend/tests/unit/core/schema/test_generated_visibility.py uv run pytest backend/tests/functional/api/test_load_schema.py # requires Neo4j testcontainers uv run pytest backend/tests/component/api/test_40_schema.pyFull functional load suite: 17 passed locally (Neo4j testcontainers).
Impact & rollout
/api/schema/loadpayloads carrying non-settable fields (inherited,used_by,hierarchy, derivedkind, …); changelog fragment + upgrade note included; API round-trip via GET→edit→POST now requires stripping read-only fields.Checklist
🤖 Generated with Claude Code
Summary by cubic
Separates the user-facing schema into SDK-generated write/read contracts, validates
/api/schema/loadagainst the write contract (tolerating read/internal fields by stripping them), and returns field‑level errors with dotted paths.GET /api/schemanow returns SDK read models (computedkind,hash); OpenAPI and frontend types reflect the new shapes; addresses INFP‑234.New Features
infrahub_sdkmodels:InfrahubSchemaWrite/InfrahubSchemaReadwith a discriminatedAttributeunion, typedcomputed_attribute, dropdownchoices, and shared enum classes; read models computekindand includehash.GET /api/schemaserves SDK read models;POST /api/schema/{load,check}declares the write model and validates viainfrahub_sdk.schema.validate_schema()after projecting payloads onto the write contract;extensionsare gated; CI checks SDK schema/protocol drift.Migration
/api/schema/loadaccepts schemas with read/internal fields (e.g., a read‑back/export) and strips them before enforcing the write contract. Unknown fields still fail with field‑level errors.infrahub_sdk.schema.validate_schema()or the SDK’sschema.load()/check()(both project to the write contract). Direct REST clients may submit read‑backs unchanged but must avoid unknown/non‑write fields.Stringis removed from SDK read models and OpenAPI; useTextinstead (fixtures and tests updated accordingly).Written for commit 65cde1b. Summary will update on new commits.