Skip to content

fix(openshell): reject duplicate sandbox network policy names - #2215

Merged
freeqaz merged 10 commits into
openclaw:mainfrom
ericcaiwx-star:codex/batch-d-4-oct11
Oct 13, 2026
Merged

freeqaz merged 10 commits into
openclaw:mainfrom
ericcaiwx-star:codex/batch-d-4-oct11

Conversation

@ericcaiwx-star

@ericcaiwx-star ericcaiwx-star commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Closes #2214

Duplicate policy names silently collapsed via Object.fromEntries, dropping egress rules. A policy named workspace-node-enrollment is reserved and is refused as well. That name starts today, and a user rule with it is replaced when a workspace-node binding exists.

Change

Startup refuses duplicate policy.networkPolicies names and the reserved name workspace-node-enrollment, with OPENSHELL_NETWORK_POLICY_NAME_REJECTED and reason rename-or-merge. Recovery is to rename or merge the duplicate names, and to rename a policy called workspace-node-enrollment. docs/guides/deploy/breaking-changes.md records both refusals on 2026-10-13, ahead of the #2248 demo port notice.

See commits on ericcaiwx-star:codex/batch-d-4-oct11.

Verification

Exact head 8eeb4973873186dc247a1937d27f20fb8687a362. That commit merges upstream/main (07590c64e27ac61c9ad60d9ab61424b70281011a). The rejection check, startup diagnostic, and regression test match e55a3abae41a17fc931f37e0c044222c1fbb27bc. Behavioral proof below is from that commit unless a parent is named. Node v26.7.0, macOS.

Corrected startup used an installation file with two policies, model-egress and model-egress-backup, sharing the fixture endpoint host api.openai.com port 443 and binary /app/bin/model-client. Kubernetes authentication was kubeconfig against a local single-node k3s API published only on loopback. The development auth secret is the repository placeholder and is not printed.

A fresh disposable PostgreSQL database was migrated with proof head e55a3abae41a17fc931f37e0c044222c1fbb27bc. Output: {"event":"migration.completed","history":"empty"}. Development bootstrap wrote installation ins_1491ce6c-c193-49fe-a675-1ce592fbff53.

NODE_ENV=development node apps/controller/src/server.mjs on that proof head logged:

{"event":"listening","host":"loopback","phasesMs":{"composition":414,"configuration":713,"listen":4,"modules":721,"ready":164},"port":18091,"service":"occ-api","severity":"INFO","startupMs":2015}

The process was stopped with SIGTERM and logged shutdown.completed. stderr was empty. The shipped loader still reported both policy names, model-egress and model-egress-backup.

Upgrade. gh release list previously returned no releases, and this checkout has no v* tags. The installed revision is merge-base d2fe579cd51f53bb979f0cb0f9ad8785b5f4585a. That proof head changes no migration, lockfile, package manifest, or platform-state files relative to that revision. The same database and the same installation file, started with that revision's server.mjs, logged:

{"event":"listening","host":"loopback","phasesMs":{"composition":375,"configuration":193,"listen":4,"modules":549,"ready":168},"port":18092,"service":"occ-api","severity":"INFO","startupMs":1288}

That process also logged shutdown.completed after SIGTERM. After both processes exited, the stored installation id was still ins_1491ce6c-c193-49fe-a675-1ce592fbff53, with 2 identities and 2 bindings.

Invalid names on that proof head: node --test --test-name-pattern='shipped startup names a rejected OpenShell policy' tests/integration/sandbox-driver-startup.test.mjs passed 1, failed 0, exit 0. A duplicate planted policy name and the reserved name workspace-node-enrollment each make shipped server.mjs exit 1 with startup-error code OPENSHELL_NETWORK_POLICY_NAME_REJECTED and reason rename-or-merge. Neither name appeared in stdout or stderr.

Loader and earlier server results below are from parent 763fd98e30001e78a50104d38c5274e21eda9b2b. macOS, Node v26.7.0.

Shipped entry: NODE_ENV=development OCC_PORT=18081 node apps/controller/src/server.mjs, with OCC_CONFIG_PATH pointing at an Installation file. OCC_HOST is loopback. The development auth secret is the repository's 52-character development placeholder and is not printed. Each file sets gateway.readiness.timeoutMs=120000 and gatewayTrustedProxyCidrs to one documentation TEST-NET address. The first policy is model-egress with endpoint host api.openai.com port 443 and binary /app/bin/model-client.

second policy name model-egress
server exit=1
{"event":"startup-error","code":"STARTUP_FAILED"}
loadInstallationConfiguration: REJECTED
OpenShell network policy name model-egress must be unique.
second policy name workspace-node-enrollment
server exit=1
{"event":"startup-error","code":"STARTUP_FAILED"}
loadInstallationConfiguration: REJECTED
OpenShell network policy name workspace-node-enrollment is reserved.
second policy name model-egress-backup (binaries and endpoints kept)
server exit=1
{"event":"startup-error","code":"DATABASE_CONFIGURATION_INVALID"}
loadInstallationConfiguration: ACCEPTED

Duplicate and reserved names fail inside Installation loading. The parent run stopped the corrected name at an unset database URL. The proof-head run above continues that corrected file through listening.

Risks

None identified beyond the stricter validation described above.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ericcaiwx-star
ericcaiwx-star marked this pull request as ready for review October 11, 2026 15:36
@ericcaiwx-star
ericcaiwx-star requested a review from a team as a code owner October 11, 2026 15:36
@clawsweeper

clawsweeper Bot commented Oct 11, 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.

@ericcaiwx-star

Copy link
Copy Markdown
Contributor Author

Maintainer comment pass (author self-review):

  • One topic; owner-boundary fix only in the named module.
  • Regression test fails on pre-fix behavior at current main.
  • No config surface expansion, no secrets, no CHANGELOG edit.
  • Ready for ClawSweeper when CI attaches to the pushed head.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Oct 11, 2026
@clawsweeper

clawsweeper Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR rejects duplicate and reserved OpenShell network policy names during startup and reports how to recover.

Example: An Installation contains two network policies named model-egress.

  • Before: Startup accepts them, but Object.fromEntries retains only the last rule.
  • After: Startup refuses the configuration with OPENSHELL_NETWORK_POLICY_NAME_REJECTED and reason rename-or-merge.

Review scores

Measure Result What it means
Overall readiness 🦞 diamond lobster (5/6) A focused, owner-approved repair with shipped startup evidence, meaningful regression coverage, verified unchanged proof code, and no remaining findings.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (logs): Supplied terminal evidence exercises server.mjs with rejected policy names and corrected configuration on macOS with Node v26.7.0, disposable PostgreSQL, and local loopback k3s; corrected startup reaches listening, and the runtime diff matches the proof revision.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Configured sandbox egress rules silently disappear when policy names collide.
Reason: The repair prevents policy loss, retains the existing owner and wire contract, and satisfies the recorded owner requests for reserved-name disclosure and startup recovery.

Merge readiness

✅ Ready for maintainer review

This PR fixes a source-confirmed policy-loss bug that remains on current main; the recovery documentation and diagnostic are present, and no actionable patch defect remains.

Priority: P2
Reviewed head: d66160d9eb0bcb73c254cc6fcef67947ee0f0524

Before merge

None.

Findings

None.

Provenance

  • Name-keyed network policy serialization: original intent not found (unknown: Scoped history and blame reach a boundary commit for Object.fromEntries; the original reason for admitting duplicate names is unknown, while the current wire contract requires a map.)
Agent review details

How this fits together

Installation loading validates the selected OpenShell Sandbox Driver before construction; accepted policies later become the name-keyed SandboxSpec sent through the OpenShell gateway.

flowchart TB
  A[Installation configuration] --> B[Driver validation]
  B -->|Rejected name| C[Startup recovery diagnostic]
  B -->|Valid names| D[OpenShell Sandbox Driver]
  D --> E[Name keyed SandboxSpec]
  E --> F[OpenShell gateway]
Loading

Technical review

Best possible solution:

Land the focused validation and recovery diagnostic while preserving every valid policy’s binaries and endpoints.

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

Current-main source shows duplicate names collapsing in Object.fromEntries and the reserved name being replaced by the enrollment rule; no target code was executed during this read-only review.

Is this the best way to solve the issue?

Validation belongs in the existing Sandbox Driver admission owner, where it prevents ambiguous policies before construction without adding another configuration surface.

AGENTS.md: found and applied where relevant.

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

Provenance checked

Testing

Proof path: shipped entry point.

Security

None.

Evidence

What I checked:

Likely related people:

  • freeqaz: Raw commit 7ad7fbb adds apps/controller/src/startup-failure.ts:7 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 7ad7fbb740f5; files: apps/controller/src/startup-failure.ts)
  • Derek Carr: Raw commit dd2b058 adds apps/controller/src/drivers/sandbox/openshell.ts:1249 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: dd2b05871579; files: apps/controller/src/drivers/sandbox/openshell.ts)
  • Mrunal Patel: Raw commit 3165d51 adds apps/controller/src/drivers/sandbox/openshell.ts:631 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 3165d51e6bf8; files: apps/controller/src/drivers/sandbox/openshell.ts)

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #2214
Summary: This PR repairs the duplicate-name policy loss reported by the linked issue.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: The fix prevents silent loss of operator-configured sandbox egress rules within the optional OpenShell development integration.
  • merge-risk: 🚨 compatibility: Previously accepted duplicate or reserved names now stop startup; the owner accepted this stricter validation with documented rename-or-merge recovery.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.
  • proof: sufficient: Contributor real behavior proof is sufficient.

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 (9 earlier review cycles; latest 8 shown)
  • reviewed 2026-10-11T19:31:01.802Z sha 763fd98 :: needs real behavior proof before merge. :: none
  • reviewed 2026-10-11T23:16:20.438Z sha 763fd98 :: needs real behavior proof before merge. :: [P2] Expose a safe recovery diagnostic for rejected policy names
  • reviewed 2026-10-12T00:03:50.187Z sha e55a3ab :: needs real behavior proof before merge. :: none
  • reviewed 2026-10-12T01:26:13.832Z sha e55a3ab :: needs maintainer review before merge. :: none
  • reviewed 2026-10-12T23:07:24.852Z sha 8eeb497 :: needs maintainer review before merge. :: none
  • reviewed 2026-10-13T05:48:00.945Z sha 37cae85 :: needs maintainer review before merge. :: none
  • reviewed 2026-10-13T07:00:03.124Z sha c8b7125 :: needs maintainer review before merge. :: none
  • reviewed 2026-10-13T07:38:23.575Z sha 706a7e8 :: needs maintainer review before merge. :: none

Reviewed October 13, 2026, 4:12 AM ET / 08:12 UTC (Revision 10).

@freeqaz

freeqaz commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator

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

The check is correct, and the code looks good to me. The pinned OpenShell (v0.1.3-pre.2) keys network_policies by name as a map, so duplicate names can never reach it, and today Object.fromEntries keeps only the last rule. A user rule named workspace-node-enrollment is also silently replaced whenever a workspace-node binding exists. CI is green, and the branch still merges cleanly with main.

I'm not landing it yet, for one reason: it refuses Installations that start today. A config with duplicate policy names, or with a policy named workspace-node-enrollment and no workspace-node binding, currently starts (with a rule dropped). After this change, that controller refuses to start. Our rule for a check that refuses previously working input is to add an entry to docs/guides/deploy/breaking-changes.md (What breaks / Who is affected / How to tell / Steps). For example: "rename or merge duplicate policy.networkPolicies names; rename a policy called workspace-node-enrollment".

Two small asks:

With those in place and CI green, this can land.

Keep the network-policy breaking-change notice ahead of the notices that landed on main.
@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Oct 11, 2026
Startup was logging only STARTUP_FAILED, so operators could not tell a duplicate or reserved policy name from the process log. The event now carries a fixed code and rename-or-merge reason, without the policy name.
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Oct 12, 2026
Keep the OpenShell network-policy breaking-change notice ahead of the demo port notice from openclaw#2248.
@freeqaz

freeqaz commented Oct 13, 2026

Copy link
Copy Markdown
Collaborator

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

This PR is reviewed and ready to land, but after today's landings it now conflicts with main in docs/guides/deploy/breaking-changes.md. Could you merge main into the branch (a merge is fine, no rebase needed)? We'll land it on the next head where CI Required is green.

Heads-up on breaking-changes.md: several notices landed today, and the page is now at 2,495 of its 2,500 words, so keep main's entries above yours and expect Static Checks to ask for room. Moving the oldest notices into breaking-changes-archive.md works, but that page is near its limit too; if it's easier, leave the cap failure and we'll make room on main.

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Oct 13, 2026
@freeqaz

freeqaz commented Oct 13, 2026

Copy link
Copy Markdown
Collaborator

Free's clanker here (an AI agent working on Free's behalf): #2217 just landed and moved the notice list in docs/guides/deploy/breaking-changes.md; one more main merge and this lands.

@freeqaz

freeqaz commented Oct 13, 2026

Copy link
Copy Markdown
Collaborator

Free's clanker here (an AI agent working on Free's behalf). #2230 just landed, so this conflicts with main again in docs/guides/deploy/breaking-changes.md. Could you merge main once more? It lands once CI Required is green on the new head. Heads-up: with #2213 and #2215 both on main, docs/flows/openshell-sandbox-provisioning.md comes to 2512 words (cap 2500), so whichever of the two lands second needs a short trim there.

@freeqaz

freeqaz commented Oct 13, 2026

Copy link
Copy Markdown
Collaborator

Free's clanker here (an AI agent working on Free's behalf). #2249 just landed, so this conflicts with main again in docs/guides/deploy/breaking-changes.md. Could you merge main once more? It lands once CI Required is green on the new head. Reminder: with #2215 and #2213 both on main, docs/flows/openshell-sandbox-provisioning.md comes to 2513 words (cap 2500), so whichever of the two lands second needs a short trim there.

@freeqaz

freeqaz commented Oct 13, 2026

Copy link
Copy Markdown
Collaborator

Free's clanker here (an AI agent working on Free's behalf). #2213 just landed (d3ae099), so this conflicts with main again, in docs/guides/deploy/breaking-changes.md and docs/flows/openshell-sandbox-provisioning.md. Could you merge main once more? With both changes in, docs/flows/openshell-sandbox-provisioning.md comes to about 2513 words, over the 2500-word docs cap, so please trim that page to at most 2500 words in the same merge. It lands once CI Required is green on the new head.

@clawsweeper

clawsweeper Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(openshell): reject duplicate sandbox network policy names 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.

@freeqaz
freeqaz merged commit ff020a4 into openclaw:main Oct 13, 2026
39 checks passed
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. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. 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.

OpenShell drops duplicate sandbox network policy names at startup

2 participants