[ericcaiwx-star]

Closes #2384

Problem

validateModelCredentialReferences accepts a proxy URL https:///v1, https:/v1, a channel proxy https:///slack, and an MCP server URL https:/mcp. https:///v1 and https:/v1 both parse as https://v1/ (host v1). Model baseUrl already refuses an empty authority and a missing //. These other URL fields do not.

Parent c28bd23933b33fcb7a726afd8049d770321b0b31.

Change

The shared URL check refuses an empty authority (https:///v1, including a backslash in place of a slash) or a missing // (https:/v1, https:v1) on proxy, channel, MCP, plugin, and proxy-environment URLs. The message names the field and says to write an absolute http:// or https:// URL and save again. Fields whose last segment is baseUrl are left to the model-provider check, which already has its own sentence. A Preset whose literal head is only http:, https:, http:/, or https:/, and still has an unfilled variable, is not a finished URL, so this check waits. The variable can supply //host or /host. https:///{{ vars.path }} still fails, because the empty authority is already written. The new conformance file is registered on the baseline lane.

Verification

Parent c28bd23933b33fcb7a726afd8049d770321b0b31 admitted https:///v1 and https:/v1 on proxy.proxyUrl, a channel proxy, an MCP server URL, and request.proxy.url.

Head fcd37fe350d1527708c98455cdcdd823dd36d6fc, which merges current main 8e64d914692505239d627e9519b5c27e35e9d3e1:

node --test tests/conformance/url-authority.test.mjs

Exit 0. 1 passed, 0 failed. Those spellings throw has an empty authority or a missing //, which parses as a different host. A proxy URL with a host does not throw. https:{{ vars.endpoint }} and https:/{{ vars.rest }}, with an unfilled variable after that literal head, do not throw. https:///{{ vars.path }} still throws. A model baseUrl of https:///v1 is not claimed by this check.

node scripts/ci/run-tests.mjs audit

Exit 0. Status passed on this head. No issues. tests/conformance/url-authority.test.mjs stays on the baseline lane.

node scripts/docs-site/word-count.mjs --max 2500

Exit 0. docs/guides/deploy/breaking-changes.md 2467, docs/guides/deploy/breaking-changes-archive.md 1702.

Risks

A Configuration or Preset that saved https:///... or https:/... on a proxy, channel, MCP, or plugin URL is rejected on the next save until the URL names a host. A Preset that leaves the host in a variable after https: or https:/ is still accepted here and checked again once filled. Model baseUrl messages are unchanged. The current page keeps the landed harness, observability, and OpenShell gateway notices.

[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 proxy, channel, MCP, plugin, and proxy-environment URLs whose HTTP scheme slashes cause the parser to read another host.

Example: Save a Configuration with proxy.proxyUrl set to https:///v1.

• Before: The value is accepted although the URL parser reads v1 as its host.
• After: The save rejects the value and asks for an absolute URL naming the intended host.

Review scores

Measure	Result	What it means
Overall readiness	🧂 unranked krab (1/6)	The earlier Preset-template finding is fixed, but compensation can strand an existing Configuration and shipped-entry-point evidence is absent.
Proof confidence	🧂 unranked krab (1/6)	Real behavior proof is necessary before merge. See Before merge.
Patch quality	🦪 silver shellfish (2/6)	1 actionable review finding remain.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: A saved proxy or MCP URL can name a different host after URL parsing than its written slash structure suggests.
Reason: The existing feature owner explicitly supports extending fail-early admission beyond model baseUrl, resolving the earlier intentional model-only boundary.

Merge readiness

⛔ Blocked before merge - 3 items remain

Keep this PR open: the URL validation change has owner support, but rejecting previously saved URLs also prevents compensation from restoring them after a failed update.

Priority: P2
Reviewed head: a2bb27e65d2ec8b773aa6d4bbd7f06e395bb9bd6

Before merge

• Add real behavior proof - The full PR body supplies direct-validator test results and audit output; the discussion adds test-suite results, but neither shows the changed behavior through shipped Configuration or Preset APIs. Provide redacted terminal output or logs showing malformed-value refusal and successful deferred-template rendering, identifying the tested revision and environment; redact IPs, credentials, 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.
• Preserve compensation for previously saved URL values (P1) - An older Configuration may contain proxy.proxyUrl = "https:/v1" and still load because stored reads skip this check. If a PATCH correcting that URL reaches Kubernetes but its response is lost, OCC rolls back the metadata transaction and calls driver.update(previous) at packages/occ/src/index.ts:5418. KubernetesConfigurationDriver.update validates that previous document, so this new throw prevents restoration. The ConfigMap remains at generation N+1 while metadata returns to N; subsequent reads and updates fail exactConfiguration's generation check. Preserve restoration of the exact stored snapshot through the trusted compensation path, and cover this case with the bundled Driver rather than only the permissive test Driver.
• 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

• [P1] Preserve compensation for previously saved URL values — apps/controller/src/drivers/configuration/model-auth.ts:335

Tests

• Missing end-to-end proof: Extend bundled-Driver integration coverage and provide shipped-API evidence for malformed-value refusal, deferred Preset rendering, and correction of a previously saved value, including compensation after response loss.

Agent review details

How this fits together

Configuration and Preset admission pass native runtime settings through the selected Configuration Driver; validated documents enter filesystem or Kubernetes storage and later Agent deployment.

flowchart TB
A[Configuration or Preset request] --> B[OCC admission]
B --> C[Configuration Driver]
C --> D[Shared URL validation]
D --> E[Configuration storage]
E --> F[Agent deployment]

Loading

Technical review

Best possible solution:

Keep the owner-approved admission rule while allowing trusted compensation to restore the exact previously stored document without relaxing validation of user writes.

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

Current-main source accepts the reported literal proxy URLs, and the introduced validator rejects them; the compensation failure follows directly through OCC rollback and bundled-Driver validation, without executing target code.

Is this the best way to solve the issue?

Extending the existing shared validator is appropriately scoped and has feature-owner support, but write admission must remain compatible with restoration of an exact older snapshot.

Full review comments:

• [P1] Preserve compensation for previously saved URL values — apps/controller/src/drivers/configuration/model-auth.ts:335
  An older Configuration may contain proxy.proxyUrl = "https:/v1" and still load because stored reads skip this check. If a PATCH correcting that URL reaches Kubernetes but its response is lost, OCC rolls back the metadata transaction and calls driver.update(previous) at packages/occ/src/index.ts:5418. KubernetesConfigurationDriver.update validates that previous document, so this new throw prevents restoration. The ConfigMap remains at generation N+1 while metadata returns to N; subsequent reads and updates fail exactConfiguration's generation check. Preserve restoration of the exact stored snapshot through the trusted compensation path, and cover this case with the bundled Driver rather than only the permissive test Driver.
  Confidence: 0.97

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

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

Provenance checked

• Proxy and MCP URL admission changes intended behavior with a stated reason (fix(presets): require two slashes after an http(s) baseUrl scheme and refuse userinfo in mcp env and plugin proxies #2351: The earlier change deliberately restricted slash validation to model baseUrl and retained parser interpretation for proxy and MCP URLs as a provisional scope decision.)
• Breaking-change notice relocation keeps the original intent (fix(helm): validate executionCluster.apiCidrs #2230: The existing notice explains execution API CIDR refusals; this PR moves and shortens it to accommodate the new admission notice within the documentation length limit.)
• Baseline test registration keeps the original intent ([ci] Split test suite declarations by lane #343: Lane-owned manifests declare test selection; this PR registers its added conformance file in the existing owner.)

Testing

Proof path: unit tests only.

Security

None.

Evidence

Acceptance criteria:

• node --test tests/conformance/url-authority.test.mjs
• node --test tests/conformance/configuration-occ.test.mjs
• node --test tests/integration/configuration-controller.test.mjs
• pnpm lint
• pnpm check:workspace

What I checked:

• Introduced validation: The pinned introduced diff adds the slash rejection inside the shared URL validator, including the corrected exception for an unfilled Preset variable after https:/. (apps/controller/src/drivers/configuration/model-auth.ts:330, a2bb27e65d2e)
• Compensation restores through write validation: After a Configuration update applies but fails to return successfully, OCC rolls back its metadata and calls driver.update(previous). The Kubernetes Driver validates that previous document before restoring it, so the new rejection blocks restoration of an older https:/v1 value. (packages/occ/src/index.ts:5418, a2bb27e65d2e)
• Persistent generation mismatch: Kubernetes update invokes validation before replacement; OCC subsequently rejects reads whose stored generation differs from metadata, making failed restoration consequential beyond the original request. (apps/controller/src/drivers/configuration/kubernetes/index.ts:270, a2bb27e65d2e)
• Existing compensation contract: The existing Configuration conformance case explicitly protects rollback after an applied update loses its response, but uses a test Driver and does not cover a grandfathered URL rejected by the bundled Driver. (tests/conformance/configuration-occ.test.mjs:204, a2bb27e65d2e)
• Owner direction and accepted admission tradeoff: In fix(configuration): refuse proxy and MCP URLs that parse as another host #2389 (comment), freeqaz supports extending the earlier model-only rule to proxy and MCP URLs and acknowledges that stored values remain readable while subsequent saves reject them; merged feature history independently establishes this person's area ownership. (apps/controller/src/drivers/configuration/model-auth.ts:304, 27731e871703)
• Current main and canonical report: Fetched main retains only userinfo checks in this helper; the slash restriction remains unique work for Proxy and MCP URLs with an empty authority parse as a different host #2384, which this PR references for closure. (apps/controller/src/drivers/configuration/model-auth.ts:300, 9ab1c06e6416)

Likely related people:

• Free Wortley: Raw commit 27731e8 adds apps/controller/src/drivers/configuration/model-auth.ts:78 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 27731e871703; files: apps/controller/src/drivers/configuration/model-auth.ts)
• kevinlin-openai: Raw commit 107900e adds apps/controller/src/drivers/configuration/model-auth.ts:13 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: 107900e9551b; files: apps/controller/src/drivers/configuration/model-auth.ts)

Labels

Label changes:

No label changes.

Label justifications:

• P2: The admission improvement is useful, but its introduced failure requires a previously saved malformed URL and a failed mutation after storage changes.
• merge-risk: 🚨 compatibility: Previously admitted documents remain readable but cannot pass the write validation used to restore them during compensation.
• rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
• 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 (3 earlier review cycles)

• reviewed 2026-10-14T03:21:49.343Z sha 5ccb72c :: needs real behavior proof before merge. :: [P2] [P2] Preserve Preset variables that render valid URL delimiters | [P3] [P3] Tell users how to correct the rejected URL
• reviewed 2026-10-14T03:53:16.127Z sha 287fd58 :: needs real behavior proof before merge. :: [P2] [P2] Preserve Preset variables that render valid URL delimiters | [P3] [P3] Tell users how to correct the rejected URL
• reviewed 2026-10-14T04:34:13.151Z sha 087ded1 :: needs real behavior proof before merge. :: [P2] Preserve Preset variables that complete the slash pair

Reviewed October 14, 2026, 12:57 AM ET / 04:57 UTC (Revision 4).

[freeqaz]

Free's clanker here (an AI agent working on Free's behalf).

Thanks. I reviewed head 087ded113. The checks-baseline-1.json change only registers the new url-authority.test.mjs. The check runs on saves only (stored skips it), so existing Configurations still load. The new https:{{ vars.x }} carve-out looks right.

The one blocker is a conflict. #2385 just landed (9031ae3) and added its notice at the same spot in docs/guides/deploy/breaking-changes.md. Could you merge main? Once it's green on the merged head, it's ready to land from our side.

A note for whoever lands it: #2351 deliberately limited the slash rule to model baseUrl ("Proxy and MCP URLs keep the parser's reading"). That was a provisional, precedent-based call, and extending it to proxy and MCP URLs matches the fail-early direction since then. One small inconsistency remains: baseUrl still refuses https:{{ vars.h }}, while these fields now allow it.

url-authority.test.mjs fails on main and passes on the merged tree, and configuration-controller.test.mjs (including #2377's pinned save-time URL cases) passes 112/112 on the merge.

[freeqaz]

Free's clanker here (an AI agent working on Free's behalf).

Thanks, Eric. I reviewed head a2bb27e65. The main merge kept #2385's notice, and the new "single scheme slash" commit looks right. It only touches non-baseUrl fields, and only on a Preset save (tail.length > 0), where the filled Configuration is checked again. https:/// with an empty authority is still refused. It doesn't reopen anything that #2351, #2375 or #2377 pin: configuration-controller.test.mjs passes 112/112 on the merge with current main at 2 CPUs, and url-authority.test.mjs passes 1/1.

The one blocker is a conflict again, sorry. #2388 just landed (8ccb33b) and its notice sits at the same spot in docs/guides/deploy/breaking-changes.md (plus the archive). Could you merge main once more? Once it's green on the merged head, it's ready to land from our side.

[clawsweeper]

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(configuration): refuse proxy and MCP URLs that parse as another host This is item 1/1 in the current shard. Shard 0/1.

This temporary status tracks the active review worker. The completed review will appear in the durable ClawSweeper review comment.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

[ericcaiwx-star]

Merged main through 8e64d9146, keeping the observability notice. Head fcd37fe35. The URL authority check passes on this head.

[freeqaz]

Free's clanker here (an AI agent working on Free's behalf). Landed as 67bd8d6, thanks Eric. Your main merge kept every main-side breaking-changes entry and the archive, and the code matches what we reviewed.