Skip to content

fix(deploy): refuse a codex network proxy that blocks the repository broker at save and admission - #3726

Merged
freeqaz merged 9 commits into
mainfrom
fix-2350
Oct 26, 2026
Merged

freeqaz merged 9 commits into
mainfrom
fix-2350

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

An Agent with repository bindings on a dedicated Codex Harness (or on embedded OpenClaw with the Codex Plugin Driver and plugins) can have a Configuration whose plugins.entries.codex.config.appServer.networkProxy blocks the repository credential broker: enabled: false, an unknown mode, a non-boolean allowLocalBinding, malformed domains, or a deny of the broker host. That Agent passed Configuration save, Agent save and deploy admission. Kubernetes Compute refused it only in preparation (codexRepositoryBrokerNetworkPolicy). By then a dedicated deploy had already stopped its predecessors, so the Agent went offline because of a configuration error. Board finding 2350; sibling of #3517 (finding 2348).

Change

  • One rule in packages/contracts: codexRepositoryBrokerNetworkProxy parses the setting, and codexProxiesRepositoryBroker says which Harness sends broker traffic through Codex. Kubernetes Compute preparation now calls that rule, so there is no second parser, and keeps its refusal as the backstop.
  • OCC applies the rule at Agent create and update, at provisioning (both the request and the stored record) and at deploy admission next to admitRepositoryCredentials. It runs when the Agent has repository bindings, its Harness sends broker traffic through Codex, and no Sandbox with its own repository path is selected. The refusal is 400 INVALID_REQUEST. It names the dotted setting and what to change, and preparation uses the same text, for example:
    Configuration setting plugins.entries.codex.config.appServer.networkProxy.enabled is false, so Codex cannot reach the repository credential broker. Set it to true or delete it, or remove the repository bindings.
  • Configuration save does not change. A Configuration does not know which Agents use it, the same as in fix(plugins): refuse plugins the agent's harness cannot run at save and admission #3517.
  • A deny of the broker host itself is still refused only in preparation. Only the prepared session material names the broker host, and getting that host at admission would need a Driver contract change. That refusal leaves the host out of its text: the worker drops a whole worker.compute-prepare-failed line whose message is over 256 characters, and a long Namespace name made the host long enough to cross it.
  • Docs: a Kubernetes Compute failure condition, a flow step, and a breaking-change notice (it also names the empty-host refusal and the preparation-only broker deny). Three older 2026-10-24 notices moved to the earlier page to stay under the word limit with the fix(api): name repository, standing approval and credential source field refusals #3725 notice beside this one.

Verification

  • Repro on main d7fe9676e5: all six shapes passed Configuration save, Agent save and admission, and were refused only in preparation.
  • tests/integration/repository-credentials-admission.test.mjs (new tests):
    • Each shape is refused at create, update and deploy with the exact text from tests/helpers/refusal-text.mjs.
    • After a refused deploy, the running revision is still the Agent's only revision and the Agent stays running, so the worker has nothing to stop it for.
    • A limited proxy with its own allowlist deploys.
    • Clearing the bindings lets the same setting save and deploy.
    • Embedded OpenClaw is refused only with the Codex Plugin Driver.
    • Provisioning is refused before queueing.
  • tests/conformance/repository-runtime-material.test.mjs: the preparation backstop now asserts the shared text. The broker-host deny case uses the host of a 63-character Namespace (the Kubernetes maximum) and checks the Compute diagnostic message fits the worker's 256-character cap; it fails on the previous head (the old text was over the cap).
  • tests/integration/postgres-agent-provisioning.test.mjs (new test): a provisioning request admitted while a Sandbox Driver with its own repository path is selected, after which the Installation drops that Sandbox Driver. The worker refuses the stored record before it creates the Agent, and retry answers the setting's 400. With the stored-record check disabled (if (false && ...)) the worker creates the Agent and the test fails.
  • Mutants, each killed by at least one test: drop the deploy admission call, drop the Agent save call, drop the provisioning call, widen the Codex Plugin Driver condition.
  • Both test files passed 20/20 at 2 CPUs on a remote test VM, again after the review fixes, and the new PostgreSQL test passed 20/20 at 2 CPUs there. The static and checks-baseline-2 lanes passed there. tsc, eslint and prettier are clean.

Risks

  • Breaking: these Agents are now refused at save and deploy. They could not deploy before either. A notice is added.
  • An Agent update that keeps a pair that is now invalid is refused until the Configuration or the bindings change. Other topology checks behave the same way.
  • No Driver contract or IAM change.

…broker at save and admission

Agent save, provisioning and deploy admission now apply the Codex network
proxy rule Kubernetes Compute preparation used for repository bindings
(disabled proxy, unknown mode, non-boolean allowLocalBinding, malformed
domains), through one shared rule in contracts, so a dedicated deploy no
longer stops its predecessors before refusing. Preparation keeps the check
as the backstop and for a deny of the broker host, which only the prepared
material names. Finding 2350.
…d a breaking-change notice

Adds a Kubernetes Compute failure condition naming the refused networkProxy
settings, a flow step at deploy admission, and a 2026-10-26 notice. Moves the
two oldest 2026-10-24 notices to the earlier page to keep the notices page
under the word limit.
@freeqaz
freeqaz requested a review from a team as a code owner October 26, 2026 05:27
@clawsweeper

clawsweeper Bot commented Oct 26, 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 clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. 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. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 26, 2026
@clawsweeper

clawsweeper Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR rejects incompatible Codex repository-proxy settings during Agent save, provisioning, and deployment admission instead of waiting for preparation.

Example: A dedicated Codex Agent has repository bindings and networkProxy.enabled is false.

  • Before: Deployment is admitted, and preparation rejects the configuration after the worker stops predecessor revisions.
  • After: Admission returns 400 INVALID_REQUEST before creating a replacement revision.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Useful, focused repair with meaningful admission coverage and no identified defect; demonstrated behavior is limited to an in-process harness.
Proof confidence 🦐 gold shrimp (3/6) The ordinary contributor proof gate does not apply to this collaborator-authored PR; supplied verification reports real Fastify admission tests with in-memory persistence, not a live worker or Kubernetes runtime demonstration.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: A configuration error can take a running dedicated Agent offline before its replacement is rejected.
Reason: Earlier rejection prevents unnecessary downtime, reuses the existing preparation rule, and follows direction established by the collaborator-authored PR.

Merge readiness

✅ Ready for maintainer review

Keep this PR open: it addresses a source-confirmed deployment failure with a focused shared check, and no concrete introduced defect was found.

Priority: P2
Reviewed head: 1b133e5a4b57244d0652f4890490408ff9b989f8

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: Supplied verification does not show a shipped API/worker deployment preserving a live predecessor or an upgrade from a released installation; desiredRuntimeState assertions do not establish workload readiness.
Agent review details

How this fits together

OCC validates Agent configuration and repository bindings before queueing revisions; Kubernetes Compute repeats the shared proxy check when preparing runtime material.

flowchart TB
A[Agent request] --> B[OCC admission]
B -->|Invalid proxy| C[400 refusal]
B -->|Accepted| D[Revision queue]
D --> E[Worker and Compute]
E -->|Preparation backstop| C
E -->|Valid material| F[Codex runtime]
Loading

Technical review

Best possible solution:

Use the existing admission owners and one shared parser, retaining preparation checks for broker-host-specific denies.

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

Current-main source shows admission lacks this check while exclusive replacement stops predecessors before the preparation check; no failing run was performed during this review.

Is this the best way to solve the issue?

The shared parser is a focused solution that preserves preparation semantics and the Sandbox repository-path exemption; exact broker-host denies deliberately remain preparation-only.

AGENTS.md: found and applied where relevant.

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

Provenance checked

  • Codex repository broker policy parsing keeps the original intent (aa36f8a: Enable broker connectivity while preserving explicit network restrictions and fail-closed policy parsing, as recorded in the supplied provenance for Fix repository broker sandbox policy #444.)
  • Dedicated and embedded Codex broker selection keeps the original intent (849b2b2: Support stock Codex broker access while retaining domain restrictions and separate Kubernetes and repository authorization controls, as recorded in Use stock Codex for repository broker access #445.)
  • Breaking-change notices keeps the original intent (c66531d: Tell operators which previously accepted settings are refused after upgrading; existing notices are moved intact and the earlier admission refusal is documented.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Likely related people:

  • Free Wortley: Raw commit fc503b7 adds packages/occ/src/index.ts:13278 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: fc503b755735; files: packages/occ/src/index.ts)

Labels

Label changes:

  • add P2: The fix prevents a configuration-dependent deployment failure on repository-bound Codex Agents.
  • add merge-risk: 🚨 compatibility: Previously saved invalid Agents now reject updates earlier; the maintainer-authored change explicitly accepts and documents this behavior.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P2: The fix prevents a configuration-dependent deployment failure on repository-bound Codex Agents.
  • merge-risk: 🚨 compatibility: Previously saved invalid Agents now reject updates earlier; the maintainer-authored change explicitly accepts and documents this behavior.
  • 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.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Show an existing repository-bound Agent rejecting the invalid deployment through the shipped API while its live predecessor remains available.

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.

Reviewed October 26, 2026, 1:41 AM ET / 05:41 UTC.

The worker drops a whole worker.compute-prepare-failed line when the
message is over 256 characters. With the broker host in the text, a
Namespace name over 19 characters pushed the deny refusal past that, in
the one case only preparation catches. The conformance case now uses a
63-character Namespace host and checks the diagnostic message fits.
…he broker

The worker and retry repeat the broker proxy check for a stored
provisioning record; no test covered it. The record is admitted while a
Sandbox Driver with its own repository path is selected, then the
Installation drops it and the worker refuses the record before it creates
the Agent; retry answers the setting's 400.
# Conflicts:
#	docs/guides/deploy/breaking-changes.md
#	tests/integration/repository-credentials-admission.test.mjs
@freeqaz
freeqaz merged commit 37f6602 into main Oct 26, 2026
39 of 41 checks passed
@freeqaz
freeqaz deleted the fix-2350 branch October 26, 2026 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. 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