Skip to content

refactor(configuration): drop the dead ipv4 pattern and pin the deploy-time gateway password check - #2425

Merged
freeqaz merged 2 commits into
mainfrom
deslop-1014
Oct 14, 2026
Merged

freeqaz merged 2 commits into
mainfrom
deslop-1014

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

A mutation sweep (mutant-sweep-1014b) listed three spots that looked like code that never decides anything:

  1. strictDecimalIpv4 in openAiBaseUrlRewritesIpv4 (apps/controller/src/drivers/openai-endpoint.ts).
  2. The second refuseLiteralGatewayPassword call in the Configuration Driver's credential rules (validateModelCredentialReferences, apps/controller/src/drivers/configuration/model-auth.ts), which repeats the save rule in validateConfigurationSettings.
  3. The KiB branch of byteSize (packages/occ/src/errors.ts), whose only in-repo caller passes 1 MiB.

Change

  1. Removed. The function tests the parser's own hostname, and only after isIP(hostname) === 4. The URL parser serializes every IPv4 host as four decimal octets with no leading zeros, so the strict pattern can never fail there. written !== hostname alone refuses every rewritten spelling. The refusals and messages are unchanged; the doc comment now says why the comparison is enough.
  2. Kept, and now tested. It is not a duplicate. Deployment validates the stored document through the Driver (configurationDriver.validate) without running validateConfigurationSettings. This call is what refuses a literal gateway.auth.password that an older release stored. When the call is removed, deploying such a document returns 409 gateway.auth.mode must be trusted-proxy on Kubernetes Compute and 503 DEPENDENCY_UNAVAILABLE on other Compute, instead of the save message. Comments in both files now say this. A new integration test pins the stored document: it reads back unchanged, and both a re-save and a deploy are refused (400, same message, no revision, no echo of the value), for both kinds of Compute.
  3. Kept. ConfigurationSizeError(limitBytes) is the documented way any Configuration Driver reports its own limit (docs/reference/drivers/configuration.md, the validateSize contract), so other limits can reach byteSize. It stays a tiny general helper.

Verification

  • Probe of the URL parser's output for the spellings the pattern was meant to refuse: leading zeros (010.0.0.1, 10.00.0.1), octal (0300.0250.0.1, 017700000001), hex (0xC0.0xA8.0.1, 0x7f000001, 0x), short forms (192.168.1, 192.11010305, 3232235777, 00), a trailing dot, %31 escapes, a fullwidth digit and a backslash after the scheme. Each yields a canonical hostname that passes the pattern. The comparison with the written host refuses each one, and the old and new functions agree on every input. A fuzz run over 300k random digit, hex and dot hosts (26.5k parsed as IPv4) found no hostname that fails the pattern.
  • Gateway password: a probe test seeded a literal password past OCC and deployed it. With the call, both kinds of Compute returned 400 with the save message. Without it, Kubernetes Compute returned 409 and ready Compute returned 503. The new test fails without the call (409 instead of 400) and passed 20/20 at 2 CPUs.
  • Suites at 2 CPUs: configuration-controller 233/233, url-authority, kubernetes-compute 354/354, profile-renderer 67/67, configuration-occ 27/27. tsc --build is clean, and prettier and eslint pass on the touched files.
  • Mutant rerun on the new code: the remaining comparison (dropped, or ANDed with the old pattern), backslash and case handling, the spelling deferral, and the compute and renderer refusals are all killed. GP6 (the Gateway password call removed) is now killed. IP9 and SZ2 survive as before; both are equivalent.

Risks

Low. An opus review approved it with nits, all applied: comment wording, and the test also pins the read values and the re-save refusal. One condition is removed, and the probe and fuzz run show it cannot change a result. The rest is comments and one new test.

…y-time Gateway password check

openAiBaseUrlRewritesIpv4 tested the parser's own hostname against a strict
decimal pattern. The URL parser always serializes an IPv4 host as four
decimal octets without leading zeros, so the pattern never failed where
isIP(hostname) === 4; comparing the written host with the parsed one decides
every case alone. Same refusals, same messages.

The second refuseLiteralGatewayPassword call (in the Configuration Driver's
credential rules) is not a duplicate: deployment validates the stored
document through the Driver without running validateConfigurationSettings,
so this call is what refuses a literal password an older release stored.
Removing it turned that 400 into a 409 gateway.auth.mode conflict on
Kubernetes Compute and a 503 elsewhere. Keep it, say so in the comments, and
pin it with a test.

byteSize keeps its KiB branch: ConfigurationSizeError is the documented way
any Configuration Driver reports its own limit, so other limits can reach it.
@freeqaz
freeqaz requested a review from a team as a code owner October 14, 2026 13:24
@clawsweeper

clawsweeper Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
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 clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Oct 14, 2026
@clawsweeper

clawsweeper Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR removes a redundant IPv4 pattern and tests the existing deployment refusal for stored literal Gateway passwords.

Example: Configure a Codex endpoint as https://3221225985/v1.

  • Before: The endpoint is refused because its written host differs from the parsed 192.0.2.1.
  • After: The endpoint remains refused using the hostname comparison alone.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Focused and correct on inspection, with useful integration coverage; supplied execution evidence remains in-process.
Proof confidence 🦐 gold shrimp (3/6) This maintainer-authored, behavior-preserving refactor is outside the ordinary contributor proof gate; reported parser probes and real-controller integration runs support the change but do not establish shipped-server execution.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Refactor · Worth it: Yes · Fix scope: Complete
User problem: Future cleanup could remove the deployment password check because save-time validation appears to duplicate it.
Reason: The maintainer-authored direction preserves existing behavior and adds useful integration coverage without introducing another abstraction or option.

Merge readiness

✅ Ready for maintainer review

Keep open for maintainer triage: this focused refactor preserves existing behavior and adds useful deployment coverage; no introduced defect was found.

Priority: P3
Reviewed head: aaded26426cbf62d6305844af6bd8380424fddb6

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: Supplied execution evidence covers parser probes and in-process controller runs, without a shipped-server run; the added coverage protects existing behavior and is reported to fail when the retained guard is removed.
Agent review details

How this fits together

Endpoint validation checks profile and Compute inputs before storing normalized URLs; Configuration Drivers validate stored documents before deployment admits an Agent revision.

flowchart TD
  A[Profile input] --> B[Endpoint validation]
  B --> C[Compute admission]
  D[Stored Configuration] --> E[Configuration Driver validation]
  E --> C
  C --> F[Agent revision]
Loading

Technical review

Best possible solution:

Retain the simpler IPv4 comparison and the deployment refusal coverage under the existing Configuration Driver.

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

This PR addresses redundant code and missing coverage rather than a current product failure; source inspection supports its explanation, and this read-only review ran no tests.

Is this the best way to solve the issue?

The narrow simplification preserves the endpoint contract, and the added integration case exercises the real Driver check that existing save and Compute fixture tests do not protect.

AGENTS.md: found and applied where relevant.

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

Provenance checked

  • IPv4 endpoint rewrite detection keeps the original intent (b739ad0: Refuse written IPv4 spellings that Node normalizes to another address before Compute or profile setup stores them.)
  • Gateway password validation comments keeps the original intent (8e64d91: Require credential references in sensitive slots while keeping older stored documents readable for repair.)
  • Save-time validation description keeps the original intent (2c3627e: Run one Configuration content rule set across saves, provisioning and Preset admission.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Likely related people:

  • Free Wortley: Raw commit 8e64d91 adds apps/controller/src/drivers/configuration/model-auth.ts:741 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 8e64d9146925; files: apps/controller/src/drivers/configuration/model-auth.ts)
  • Eric Cai: Raw commit b739ad0 adds apps/controller/src/drivers/openai-endpoint.ts:96 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: b739ad0d2c79; files: apps/controller/src/drivers/openai-endpoint.ts)

Review metrics

Metric Value Why it matters
Diff allocation Production +14/-8 lines; integration tests +50 lines Production growth consists of explanatory comments around one redundant-condition removal; test growth protects the distinct deployment validation path.

Labels

Label changes:

No label changes.

Label justifications:

  • P3: Behavior-preserving cleanup and regression coverage with no demonstrated current user failure.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

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-14T13:27:16.004Z sha a5b6d51 :: needs maintainer review before merge. :: none

Reviewed October 14, 2026, 9:39 AM ET / 13:39 UTC (Revision 2).

…ssword comments

Name isIP's own canonical-form rule in the IPv4 comment, say that deployment
(not Compute admission) refuses a stored document under these rules, scope
the re-check claim to the bundled Configuration Drivers, and have the stored
Gateway password test also pin the read values and the re-save refusal.
@freeqaz
freeqaz merged commit d49a868 into main Oct 14, 2026
40 of 45 checks passed
@freeqaz
freeqaz deleted the deslop-1014 branch October 14, 2026 13:41

This branch had an error being deployed

1 failed deployment
integration-qa-pr — aaded264 Deployed Oct 14, 2026 by freeqaz via QA advisory (compose) #875
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant