Skip to content

feat(config): the model-registry provider connection block - #122

Closed
gurnben wants to merge 2 commits into
mainfrom
feat/model-registry-connections
Closed

gurnben wants to merge 2 commits into
mainfrom
feat/model-registry-connections

Conversation

@gurnben

@gurnben gurnben commented Nov 6, 2026

Copy link
Copy Markdown
Member

What

The model-registry schema (config/v1/model-registry.schema.json) now declares the provider connection block, so provider routing declarations validate. The block is optional — registries without one load unchanged (backward compat is tested).

Why

sci#287 (source-code-intelligence) delivers provider routing from the model-registry's provider connection blocks. Its Go executor parses the block leniently (yaml.Unmarshal into its own structs), but traust-core's config-completeness audit — run by the engine at startup — validates the registry against this repo's config/v1/model-registry.schema.json, whose providers carried additionalProperties: false with only data_handling and models. A registry carrying a connection block therefore failed startup (live on the dev cluster, 2026-11-06; a dev-only Containerfile patch currently works around it — this PR makes it permanent):

ERROR: startup failed (ConfigError): cannot audit config home /etc/traust: deployment config: 1 issue(s); first: providers/enmaas: Additional properties are not allowed ('connection' was unexpected)

The shape

The authoritative semantics are sci#287's Go structs (internal/traustcore/model.go); the schema mirrors the executor's validate() exactly:

field required shape notes
type yes anthropic | anthropic-compatible | gemini | openai | openai-compatible The wire and credential format, not the model family; a gateway serving both wires appears as two providers. Registered as provider_connection_type. gemini/openai are known vocabulary — the executor refuses them at delivery until the worker grows their lanes, which is worker state, not registry shape.
base_url on the compatible types string The gateway endpoint. Required on anthropic-compatible/openai-compatible (they exist for endpoint overrides); empty on the proprietary types, whose default endpoint serves (an empty string reads as absent, as the executor's validate() does).
harness no string The harness a gateway connection fronts its wire with (omp, claude); empty keeps the default. The executor passes it through unvalidated, so the schema documents the values rather than pinning them.
credential yes object {source: env | secret, name?, key?} — a reference, never the credential itself. source: secret requires the Secret's name and key (if/then). Registered as credential_source.

The block is routing data only — never the credentials themselves (the registry is policy, the deployment is delivery, as the component README puts it). additionalProperties: false on both the block and the credential refuses anything else, including inline credentials.

Tests

New tests/test_model_registry_connections.py:

  • valid blocks, including the dev cluster's enmaas shape (an openai-compatible gateway, omp harness, secret credential) and an empty base_url on a proprietary type reading as absent;
  • refusals mirroring the schema's failure modes: an unknown type, a secret credential without name+key, a base_url on each proprietary type, the compatible types' missing endpoint, an unknown credential source, and inline credentials / stray keys;
  • a provider without a connection still loads.

No existing tests were changed. The typed sections stay permissive by design (the schema is the authority), so ProviderSpec keeps the block as an extra field — no Python model change.

Downstream

traust-core/engine pin bumps pick this up; a parallel engine PR notes the dependency. Version 0.63.0 (minor: an optional property added to a closed object — the compat gate's allowed class).

💘 Generated with Crush

sci#287 (source-code-intelligence) routes the runner pool's model
delivery through a provider's connection block: the wire it speaks, its
endpoint, its harness and where its credential lives. The Go executor
parses the block leniently, but the engine's config-completeness audit
validates the registry against config/v1/model-registry.schema.json,
whose providers carried additionalProperties: false — so a registry
declaring a connection failed startup (live on the dev cluster,
2026-11-06: "providers/enmaas: Additional properties are not allowed
('connection' was unexpected)"; a dev-only Containerfile patch worked
around it).

The block is optional and mirrors the executor's validate() exactly:

- type (required): anthropic, anthropic-compatible, gemini, openai,
  openai-compatible — the wire and credential format, not the model
  family. Registered as provider_connection_type.
- base_url: required on the compatible types (they exist for endpoint
  overrides), empty on the proprietary types (an empty string reads as
  absent, as the executor does).
- harness: the harness a gateway connection fronts its wire with
  (omp, claude); empty keeps the default.
- credential (required): {source: env|secret, name?, key?} — a
  reference, never the credential itself; source secret requires the
  Secret's name and key. Registered as credential_source.

The block is routing data only, never credentials: any other key on it
is refused. Providers without one load unchanged (backward compat);
traust-core/engine pin bumps pick this up.

Tests: tests/test_model_registry_connections.py — valid blocks
(including the dev cluster's enmaas shape), unknown type, secret
credential without name/key, base_url on a proprietary type, and the
no-connection backward-compat case.

💘 Generated with Crush

Assisted-by: Crush:rits/zai-org/glm-5-3
@gurnben
gurnben requested a review from a team as a code owner November 6, 2026 19:13
@openshift-ci

openshift-ci Bot commented Nov 6, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gurnben

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2026
@qodo-code-review

qodo-code-review Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Empty secret references pass validation ✓ Resolved

Resolving commit: 85257ba

Resolution reason
Added minLength: 1 constraints on the credential's name and key in the registry schema, so empty
Secret names or keys fail validation; added loader tests asserting empty name and empty key are
refused, and updated the changelog to say non-empty.
Description
The credential schema requires name and key for source: secret but gives both fields only a
string type, so empty strings satisfy the requirement. A registry with name: "" or key: ""
passes the config loader's schema check even though it does not identify a Secret or a key within
one.

Grey Divider

Context sources
Review mode: Auto: ⚖️ Balanced: Focus changes a schema contract for secret credential references, a designated high-risk area.
✅ Fix-loop round: fix loop

Grey Divider

Tip of the day
💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous reviews

Review updated until commit 85257ba ⚖️ Balanced

Results up to commit f899484 ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Empty secret references pass validation ✓ Resolved

Resolving commit: 85257ba

Resolution reason
Added minLength: 1 constraints on the credential's name and key in the registry schema, so empty
Secret names or keys fail validation; added loader tests asserting empty name and empty key are
refused, and updated the changelog to say non-empty.
Description
The credential schema requires name and key for source: secret but gives both fields only a
string type, so empty strings satisfy the requirement. A registry with name: "" or key: ""
passes the config loader's schema check even though it does not identify a Secret or a key within
one.

Grey Divider

Qodo Logo

Qodo's review of the connection block caught a divergence from the
executor's validate(): the schema required the secret credential's
name and key to be present but not non-empty, so name: "" or key: ""
passed the engine's audit while the executor refuses them at dispatch
(sci#287's internal/traustcore/model.go: "a %q credential needs the
Secret's name and key" — Name == "" || Key == "").

Mirror the Go check exactly: minLength 1 on both, inside the
source: secret then-branch — the executor ignores name/key entirely
on source: env, so the schema must not grow stricter there (the same
conditional idiom the base_url rules use). Two new negative tests
cover the empty name and the empty key; 0.63.0 is unreleased, so its
CHANGELOG bullet now says non-empty.

💘 Generated with Crush

Assisted-by: Crush:rits/zai-org/glm-5-3
@gurnben

gurnben commented Nov 6, 2026

Copy link
Copy Markdown
Member Author

Addressed in 85257ba (fix(config): refuse empty secret references in provider connections).

Real finding — the executor's validate() refuses empty strings, not just absent fields (sci#287, internal/traustcore/model.go: Name == "" || Key == "" → "a %q credential needs the Secret's name and key"), so the schema's presence-only required was more lenient than the Go side.

Fixed by mirroring the Go check exactly: minLength: 1 on name and key, placed inside the source: secret then-branch — the executor ignores name/key entirely on source: env, so the schema must not grow stricter there (same conditional idiom the base_url rules use). Two negative tests added (empty name, empty key); the env-with-empty-name case was verified to stay valid, matching Go.

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 85257ba

@gurnben

gurnben commented Nov 6, 2026

Copy link
Copy Markdown
Member Author

Closing in favor of #121 — the stack-consistent connection schema this duplicated.

Context: this PR and #121 were opened in parallel (a dispatch-timing miss on my side; #121 is the sci#287 stack's own contracts piece, already consumed by core#386 and referenced by the foundry plan). The design resolution favors #121's open vocabulary — type and credential.source as free strings with the semantics in descriptions ("the consumer's closed set, like tier names") — over this PR's closed enums.

Worth cherry-picking from this PR into #121 if the lane wants it (does not conflict with the open-vocabulary choice):

  1. The conditional requirement source: secret → name + key required (mirrors the Go executor's Name == "" || Key == "" refusal; feat(config): provider connection blocks in the model registry #121's unconditional minLength: 1 catches empty strings but not absent keys on a secret-source credential).
  2. The negative tests — unknown type, empty secret references, base_url on a proprietary type, stray keys (23 tests in tests/test_model_registry_connections.py; feat(config): provider connection blocks in the model registry #121 carries its own coverage, these are the fail-closed complements).

The qodo finding on this PR (empty secret references pass validation) is already covered by #121's minLength: 1 on name/key.

@gurnben gurnben closed this Nov 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant