[freeqaz]

Problem

Two low findings on the model endpoint settings (1292, 1293):

• An OpenShell openai credential source base_url and Kubernetes Compute runtime.codexModelBaseUrl skipped the 10-13 save-time URL rules (fix(presets): require two slashes after an http(s) baseUrl scheme and refuse userinfo in mcp env and plugin proxies #2351, fix(presets): refuse url userinfo in every env map and any env value with a scheme #2353, fix(presets): say the // is missing when a baseUrl scheme lacks it #2375). normalizeOpenAiBaseUrl is the WHATWG parser plus its own checks, so it silently rewrote https://@host/v1, https://:@host/v1, https:host/v1, https:/host/v1, https:///host/v1, and values with surrounding spaces or an embedded tab, newline or NUL. The source stored the raw string, and GET echoed it, while the profile, manifest and config.toml used the rewritten URL.
• The openai source type accepted hosts its sibling bearer-token and OAuth2 types refuse as "must be an exact DNS name": IP literals in every spelling the parser accepts (127.0.0.1, 169.254.169.254, 0x7f000001, 2130706433, 127.1), and labels with _ or an edge -. On the OpenShell path that widened egress for the member's own Sandbox.

Change

• openai-endpoint.ts: normalizeOpenAiBaseUrl first refuses a space or control character anywhere, an http(s) scheme followed by one or no slash, three or more slashes, and an @ before the host. The messages match the shared baseUrl rule (must start with https:// (the // after the scheme is missing)) and the auth.baseUrl rule (must not contain user info or an @ before the host). Both the Compute setting and the source use it.
• Store the normalized value rather than refuse anything normalization would change. validateSourceConfig may now return the configuration in the form the gateway uses. OCC stores, returns and registers that form, so the record, GET, and the profile agree. The other option would have refused benign spellings that Support OpenAI-compatible Codex model endpoints #913 deliberately accepts and tests (https://MODELS.example.test:443/api/./v1/, IDN hosts, a trailing slash). It would also have made members write punycode. The contract change is backward compatible: a Driver that returns nothing keeps the written config.
• OpenShell openai sources now apply the bearer-token exact-DNS-name host rule, at creation (The openai base_url host must be an exact DNS name.) and at registration or attachment. An existing IP-host source fails its next deployment with the endpoint requirements message, which now names IP addresses.
• runtime.codexModelBaseUrl stays operator-only. Its host rule is unchanged: IP and IPv6 hosts are still accepted there.
• Docs: a 2026-10-14 breaking-change notice (the oldest notice moves to the archive to stay under the word limit), plus the Credential Gateway contract, the OpenShell gateway, credential source and Compute references, and the lifecycle flow.

localhost and in-cluster names such as kubernetes.default.svc still pass, as they do for bearer-token hosts. Refusing them is a separate design question. No docs or tests show a real use of an IP endpoint; https://192.0.2.1/v1 appeared only as a normalization case and is now a refusal case.

Verification

• New and changed cases in tests/integration/credential-source-api.test.mjs (real Fastify app, IAM, OCC and OpenShell Driver) and tests/conformance/kubernetes-compute.test.mjs:
  • rewritten spellings refused with their messages, and no record kept;
  • parser-rewritten spellings (HTTPS://, :00443, full-width letters, backslashes, /./, uppercase host, :443, trailing slash) stored and returned normalized;
  • IP and malformed-label hosts refused with no gateway effect;
  • an existing IP-host source refused at attachment.
• Each new case fails against the main source and passes with the fix. 20 runs of the touched tests pinned to 2 CPUs on the final head: 20/20 pass.
• Ran once: credential-source-occ, occ-api-security, openshell-gateway-wire, occ-api, sandbox-driver-startup, secret-api, service-api-keys, plugin-compute and kubernetes-compute (846/846 pass). Also tsc --build, plus eslint and prettier on the touched files.
• Independent review: no bugs; probes of full-width, octal, hex and trailing-dot IPs and full-width @, / and : found no bypass. Its notes are applied: the returned config is checked against the catalog fields, the deploy-time Codex endpoint refusal uses the spelling message, and the notice covers deployments and Secret updates of existing sources.
• Not run: PostgreSQL and k3d suites (CI covers PostgreSQL).

Risks

• Breaking for openai sources with an IP host and for the refused spellings; the notice gives the steps.
• validateSourceConfig gains an optional return value. Every in-repo Driver and test double still type-checks.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge.

What this changes

This PR rejects ambiguous endpoint spellings, restricts OpenShell OpenAI sources to DNS hosts, and stores normalized endpoint URLs.

Example: Create an OpenShell source with base_url https://MODELS.example.test:443/api/./v1/.

• Before: The source retains the submitted spelling while its provider profile uses the normalized endpoint.
• After: The source and GET response contain https://models.example.test/api/v1, matching the provider profile.

Review scores

Measure	Result	What it means
Overall readiness	🧂 unranked krab (1/6)	The production repair is focused and source-consistent, but an existing regression-test defect remains and supplied evidence does not establish the changed authority boundary.
Proof confidence	🧂 unranked krab (1/6)	Real behavior proof is necessary before merge. See Before merge.
Patch quality	🦐 gold shrimp (3/6)	Security review found an item that needs attention.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: A saved endpoint can differ from the endpoint used by the gateway, and OpenAI sources admit host forms rejected by sibling credential types.
Reason: The repair makes stored configuration agree with execution and applies a stated host boundary; the supplied maintainer-authorship context establishes direction for the contract extension and intentional refusals.

Merge readiness

⛔ Blocked before merge - 5 items remain

Keep open for maintainer triage: this is a useful endpoint-admission repair, but the previously reported lifecycle-test defect remains.

Priority: P2
Reviewed head: c5169467954f647e1b23df99b8613a48754b00cf

Before merge

• Add real behavior proof - Authority-chain proof required: endpoint configuration passes from authorized OCC creation into persisted state, provider profiles, and credential injection. Supplied verification exercises real Fastify/IAM/OCC and the OpenShell Driver with gateway storage doubles. Provide redacted production-path output for an allowed normalized endpoint, a foreign-Namespace source rejected before gateway effects, and an existing ready source with a refused endpoint rejected before profile mutation or new attachment; preserve the existing withdrawal boundary. Also demonstrate replacement from the latest applicable released state with supported settings and data preserved. Related runtime evidence predates this change. Redact credentials, IPs, and non-public endpoints. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Use a ready source for the pre-upgrade attachment regression (P2) - The earlier finding remains: seedInvalid returns a source in registering state after marking its persisted row deleting, and this case then calls attachForRevision directly. OCC binding admission and Kubernetes Compute both reject non-ready sources before reaching endpoint validation, so the test bypasses the supported lifecycle and does not protect the claimed existing-source deployment behavior. Seed a realistic ready source and exercise its attachment through the supported deployment caller, asserting refusal before profile mutation or Sandbox attachment.
• Resolve security review attention item - No concrete credential disclosure or authorization bypass was found in the reviewed diff.
• Resolve merge risk - The changed persisted destination feeds provider-profile writes and credential injection, but its final-effect authorization boundary remains unestablished.
• Add data-model compatibility proof - The review found that existing stored data may not work after upgrade. Show that existing data still loads and works with this change.

Findings

• [P2] Use a ready source for the pre-upgrade attachment regression — tests/integration/credential-source-api.test.mjs:421

Tests

• Missing end-to-end proof: No supplied shipped-entry-point run exercises normalized endpoint delivery and forbidden-source refusal, and the reported base/head regression results were not independently executed.

Agent review details

How this fits together

OCC authorizes credential-source creation and stores its nonsecret configuration; the OpenShell Credential Gateway turns that configuration into provider profiles and revision attachments, while Kubernetes Compute consumes the selected model endpoint.

flowchart LR
  A[Credential source request] --> B[OCC authorization]
  B --> C[Endpoint validation]
  C --> D[Stored source configuration]
  D --> E[OpenShell provider profile]
  D --> F[Kubernetes Compute]
  E --> G[Sandbox credential injection]
  F --> G

Loading

Technical review

Best possible solution:

Keep the shared endpoint validation and canonical storage, repair the ready-source deployment regression, and establish the credential boundary and existing-state replacement behavior.

Do we have a high-confidence way to reproduce the issue?

Current-main source shows parser normalization occurring before spelling checks and raw configuration being retained; the added API cases cover those inputs, but no target code was executed during this read-only review.

Is this the best way to solve the issue?

Shared validation and storing the configuration actually consumed by the gateway address the mismatch without rejecting deliberately supported IDN, case, port, and path normalization.

Full review comments:

• [P2] Use a ready source for the pre-upgrade attachment regression — tests/integration/credential-source-api.test.mjs:421
  The earlier finding remains: seedInvalid returns a source in registering state after marking its persisted row deleting, and this case then calls attachForRevision directly. OCC binding admission and Kubernetes Compute both reject non-ready sources before reaching endpoint validation, so the test bypasses the supported lifecycle and does not protect the claimed existing-source deployment behavior. Seed a realistic ready source and exercise its attachment through the supported deployment caller, asserting refusal before profile mutation or Sandbox attachment.
  Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.95

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against b749f9e2b992.

Merge-risk options

Maintainer options:

1. Verify the credential boundary (recommended)
   Supply production-path evidence that normalized allowed endpoints work and forbidden source ownership or rejected persisted endpoints stop before profile mutation, attachment, or credential injection.
2. Pause landing
   Keep the PR open until the credential-boundary evidence is available.

Provenance checked

• Shared endpoint normalization and OpenShell host admission changes intended behavior with a stated reason (669cf7a: Support OpenAI-compatible Codex model endpoints #913 added custom HTTPS Responses endpoints and deliberately normalized supported URL forms.)
• OCC validation and configuration storage changes intended behavior with a stated reason (669cf7a: Pre-storage validation prevented invalid endpoints from leaving retained records; this PR explicitly extends that hook to return the canonical configuration.)
• Sibling credential types' DNS-host validation keeps the original intent (8753a21: Non-model credential sources bind credentials to an exact protected endpoint; the extracted predicate preserves that existing validation.)
• Source-owned profiles and replacement notice keeps the original intent (0232c95: fix(openshell): give every credential source its own provider profile #2261 established per-source profiles and an explicitly accepted replacement cutover; this PR preserves its notice in the existing archive.)

Testing

Proof path: in-process harness.

Security

Needs attention: No concrete credential disclosure or authorization bypass was found in the reviewed diff.

Evidence

What I checked:

• Pinned introduced change: The verified main-to-head diff adds shared spelling checks, DNS-host admission, normalized-config storage, regression cases, and owning documentation. (apps/controller/src/drivers/openai-endpoint.ts:16, c5169467954f)
• Current main still has the reported behavior: Main parses before checking endpoint spellings and stores submitted source configuration unchanged; custom endpoint support came through Support OpenAI-compatible Codex model endpoints #913. (apps/controller/src/drivers/openai-endpoint.ts:2, b749f9e2b992)
• Earlier finding remains unfixed: The added attachment case still calls seedInvalid, which returns a registering object after storing it as deleting; the test file is unchanged since the earlier reviewed head. (tests/integration/credential-source-api.test.mjs:421, c5169467954f)
• Supported deployment requires ready sources: OCC binding admission and Kubernetes Compute require ready source records before the production attachment call, so the added fixture cannot represent the claimed pre-upgrade deployment. (apps/controller/src/drivers/compute/kubernetes/index.ts:12490, c5169467954f)
• Accepted compatibility direction: The supplied maintainer-authorship context establishes direction; the PR explicitly accepts rejecting existing IP-host sources and refused spellings, preserves Compute IP support, and marks live replacement instructions untested. (docs/guides/deploy/breaking-changes.md:16, c5169467954f)
• Related runtime evidence has a different scope: test(openshell): prove the credential route and refuse untrusted upstreams #2374 records valuable real OpenShell route, TLS, and withdrawal observations, but its revision predates this patch and its runs explicitly use the default endpoint rather than a custom provider. (954910a3d845)

Likely related people:

• Mrunal Patel: Raw commit d4f544c adds apps/controller/src/drivers/credential-gateway/openshell.ts:62 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: d4f544c4eee8; files: apps/controller/src/drivers/credential-gateway/openshell.ts)
• Sally O'Malley: Raw commit 669cf7a adds apps/controller/src/drivers/credential-gateway/openshell.ts:314 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 669cf7a81118; files: apps/controller/src/drivers/credential-gateway/openshell.ts)
• freeqaz: Raw commit 031936b adds apps/controller/src/drivers/credential-gateway/openshell.ts:578 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 031936b65021; files: apps/controller/src/drivers/credential-gateway/openshell.ts)

Review metrics

Metric	Value	Why it matters
Introduced line changes	Production +125/-17; tests +110/-1; documentation +81/-49.	Growth is concentrated in shared endpoint checks and canonical configuration propagation; much of the documentation movement preserves an existing notice in its archive.

Labels

Label changes:

No label changes.

Label justifications:

• P2: This is a focused admission and stored-configuration consistency repair on the optional Credential Gateway path.
• merge-risk: 🚨 compatibility: Previously accepted source endpoints can fail deployment or Secret updates, and Compute spellings can fail startup; the maintainer-authorship context accepts that direction, while existing-state verification remains incomplete.
• merge-risk: 🚨 security-boundary: Persisted endpoints determine provider-profile destinations and credential injection, requiring allowed and forbidden final-effect verification.
• rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (1 earlier review cycle)

• reviewed 2026-10-14T02:49:49.439Z sha 3b2bfa6 :: needs real behavior proof before merge. :: [P2] Use a ready source for the pre-upgrade attachment regression

Reviewed October 13, 2026, 11:01 PM ET / October 14, 2026, 03:01 UTC (Revision 2).