Skip to content

umbrella: gateway lifecycle, runtime packaging, and Tailscale ingress - #60

Closed
hannesrudolph wants to merge 12 commits into
openclaw:mainfrom
hannesrudolph:fix/gateway-aware-restarts
Closed

umbrella: gateway lifecycle, runtime packaging, and Tailscale ingress#60
hannesrudolph wants to merge 12 commits into
openclaw:mainfrom
hannesrudolph:fix/gateway-aware-restarts

Conversation

@hannesrudolph

@hannesrudolph hannesrudolph commented Jul 26, 2026

Copy link
Copy Markdown
Member

Related: #102
Related: #103

Additional instructions

MUST: Keep Allow edits from maintainers enabled for this PR so maintainers
can help update the branch when needed.

What Problem This Solves

The original PR accumulated gateway lifecycle, environment cleanup, local runtime packaging, and automatic named Tailscale Service migration in one review surface. Although the remaining 12 commits are still unique relative to upstream main, they are not one independently reviewable product change.

The Tailscale portion is also unsafe to merge as written: it can replace unrelated handlers on the same Service; another local process can forge the trusted identity; environment destroy and prune do not own removal of the proxy, files, and route; and a later failure during a multi-Service rewrite can leave earlier routes changed without recoverable rollback state.

Why This Change Was Made

Keep this PR as a draft umbrella for provenance and redesign input while moving the mergeable work into focused branches:

The earlier rebases already removed behavior that upstream implemented independently, including targeted registry reconciliation, gateway readiness, source-extension packaging, source build profiles, full package-tree verification, cold checkpoints, durable-state preservation, and upgrade recovery.

A safe ingress redesign must preserve or explicitly reject mixed Service handlers, capture rollback state before any route mutation, reverse partial updates, authenticate the proxy boundary with an unforgeable mechanism, own proxy and route cleanup across environment lifecycle commands, and avoid silently changing routing and administrative authorization during an ordinary upgrade without an approved policy contract.

User Impact

Reviewers can evaluate and merge the lifecycle and runtime improvements independently without accepting the Tailscale authentication and routing risks. Operators receive no behavior change from this draft umbrella itself.

The unsafe ingress implementation remains available for analysis, but draft status and the split PRs prevent it from being treated as a merge candidate.

Evidence

  • Rebased onto exact upstream main at 768714ae3491fb44c99837994141bdea7ee32b3a; current umbrella head is 8a3d25725b1273a8319aa1d006c449ee93152ea9.
  • git cherry -v origin/main HEAD reports all 12 remaining commits as unique.
  • The five upstream commits after the common base overlap the umbrella implementation only in documentation files.
  • Rebased focused validation: 183 passed, 0 failed across supervisor, cleanup, runtime, service, and upgrade suites.
  • Hosted umbrella CI passed on macOS, Ubuntu, Windows, formatting, Rust 1.88, and security checks.
  • feat(runtime): package commit-matched companion plugins #102 passed 58 isolated focused tests and its complete hosted CI matrix.
  • fix: harden gateway restart, cleanup, and child temp dirs #103 passed 126 isolated focused tests and its complete hosted CI matrix.
  • Both split diffs were checked to contain no identity-proxy, named-Service, trusted-proxy, same-host, or administrative-scope code.
  • No OCM binary was installed locally, and no resident daemon or managed gateway was restarted.

@hannesrudolph hannesrudolph changed the title feat(service): drain active work before restart feat(service): recover active work across immediate restart Jul 27, 2026
@hannesrudolph
hannesrudolph marked this pull request as ready for review July 27, 2026 21:26
@hannesrudolph hannesrudolph changed the title feat(service): recover active work across immediate restart feat(service): recover active work and reap restarted gateways Jul 30, 2026
@hannesrudolph hannesrudolph changed the title feat(service): recover active work and reap restarted gateways fix(service): make gateway restarts recovery-aware, isolated, and process-safe Jul 31, 2026
@hannesrudolph
hannesrudolph force-pushed the fix/gateway-aware-restarts branch from 87db572 to b03511e Compare August 3, 2026 19:44
@hannesrudolph hannesrudolph changed the title fix(service): make gateway restarts recovery-aware, isolated, and process-safe fix(service): make gateway restarts, snapshots, and rollback safe Aug 3, 2026
@hannesrudolph
hannesrudolph force-pushed the fix/gateway-aware-restarts branch from a9be06b to c630263 Compare August 9, 2026 18:06
@hannesrudolph hannesrudolph changed the title fix(service): make gateway restarts, snapshots, and rollback safe fix: make gateway lifecycle and local source runtimes safe Aug 9, 2026
@hannesrudolph
hannesrudolph force-pushed the fix/gateway-aware-restarts branch from 4d8d245 to 795543f Compare August 10, 2026 16:09
@hannesrudolph
hannesrudolph force-pushed the fix/gateway-aware-restarts branch from 795543f to 5c82a19 Compare August 13, 2026 02:58
@hannesrudolph
hannesrudolph force-pushed the fix/gateway-aware-restarts branch 2 times, most recently from 1d86314 to cd10bdb Compare August 19, 2026 20:29
@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 19, 2026
@clawsweeper

clawsweeper Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codex review: found issues before merge. Reviewed August 21, 2026, 3:21 PM ET / 19:21 UTC.

ClawSweeper review

What this changes

This draft combines safer gateway lifecycle handling, local runtime companion packaging, and automatic migration of named Tailscale Service routes through an OCM identity proxy.

Merge readiness

Blocked by patch quality or review findings - 14 items remain

Keep this member-authored draft open only as provenance: its lifecycle and packaging work is now split into focused PRs, while the remaining automatic ingress migration has four unresolved P1 blockers.

Priority: P0
Reviewed head: 8a3d25725b1273a8319aa1d006c449ee93152ea9
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) PR readiness rating was derived from proof quality, review findings, security review, and reviewer confidence.
Proof confidence 🌊 off-meta tidepool Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Patch quality 🧂 unranked krab (1/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.
Evidence reviewed 7 items Current branch scope: The branch differs from current main across 31 files and includes new identity-proxy and Tailscale routing modules, so its central ingress behavior is not implemented on main.
Service rewrite loses handler-level state: Route discovery iterates individual handlers, but rewriting groups them by Service and invokes one service-wide replacement target; the retained backup contains only one original proxy.
Loopback identity is forgeable: The proxy accepts any loopback caller with forwarded headers, while the migration trusts loopback and grants the selected identity operator-admin scope.
Findings 4 actionable findings [P1] Preserve every handler in a migrated Service
[P1] Authenticate the loopback proxy boundary
[P1] Restore routes changed before a later rewrite fails
Security Needs attention Loopback callers can forge trusted identity headers: The proxy accepts forwarded identity data from any local process, then converts it to the headers trusted by the gateway.
Upgrade grants administrative scope automatically: The config migration grants the selected identity operator-admin and enables device auto-approval while changing the gateway trust model.

How this fits together

OCM upgrades and supervises local OpenClaw gateway environments. This branch changes runtime assembly and gateway lifecycle, then adds an upgrade-time path that rewrites Tailscale routing and gateway authentication.

flowchart LR
A[Upgrade request] --> B[OCM upgrade transaction]
B --> C[Gateway runtime and config]
B --> D[Ingress migration decision]
D --> E[Identity proxy]
E --> F[Tailscale Service route]
F --> C
Loading

Decision needed

Question Recommendation
Should OCM offer any automatic named Tailscale Service-to-trusted-proxy migration, or should routing remain operator-managed until a deliberate ingress contract is approved? Retain this as a draft-only design record: Do not merge code from this umbrella branch; land only the focused lifecycle and packaging work after independent review.

Why: This is a new authentication, routing, and administrative-authorization policy, not a mechanical repair that can be selected safely by review automation.

Before merge

  • Preserve every handler in a migrated Service (P1) - Handler discovery is per-route, but the grouped rewrite emits one service-wide tailscale serve --service replacement and stores only one proxy as backup. A Service with an unrelated host or path handler can therefore lose it; reject mixed Services or update a complete handler-level configuration losslessly.
  • Authenticate the loopback proxy boundary (P1) - Any localhost process can supply the forwarded headers accepted here, while the migration trusts loopback and grants the selected identity operator.admin. Require an unforgeable proxy-to-gateway credential or a transport boundary that prevents a local caller from impersonating a Tailscale user.
  • Restore routes changed before a later rewrite fails (P1) - If an earlier Service rewrite succeeds and a later one fails, this function returns before exposing its local backups. The caller then removes only the proxy, leaving earlier routes changed; capture all state before mutation and reverse successful rewrites on failure.
  • Remove ingress when its environment is removed (P1) - The proxy is persisted independently under OCM_HOME/ingress, but destroy and prune do not own that state or the rewritten Service route. Removing an environment can leave a running proxy and a route to a deleted gateway; integrate cleanup into environment lifecycle commands.
  • Resolve security concern: Loopback callers can forge trusted identity headers - The proxy accepts forwarded identity data from any local process, then converts it to the headers trusted by the gateway.
  • Resolve security concern: Upgrade grants administrative scope automatically - The config migration grants the selected identity operator-admin and enables device auto-approval while changing the gateway trust model.
  • Resolve merge risk (P1) - A mixed named Service can lose unrelated handlers, and a failed multi-Service rewrite can leave earlier routes changed.
  • Resolve merge risk (P1) - Any local process can forge the forwarded identity expected by the proxy and receive the automatically granted administrative scope.
  • Resolve merge risk (P1) - Environment destroy or prune can leave a proxy service and Service route pointing at a removed gateway.
  • Resolve merge risk (P1) - The upgrade silently changes existing routing, authentication, and authorization policy.
  • Complete next step (P2) - A maintainer must decide the ingress security and product boundary before any further implementation; this is not a safe automated repair.
  • Improve patch quality - Resolve the security review concern or explain why the changed path is safe.
  • Improve patch quality - Address the highest-priority review finding and re-run the changed-surface validation.

Findings

  • [P1] Preserve every handler in a migrated Service — src/tailscale.rs:364-377
  • [P1] Authenticate the loopback proxy boundary — src/identity_proxy.mjs:141-153
  • [P1] Restore routes changed before a later rewrite fails — src/tailscale.rs:371-377
  • [high] Loopback callers can forge trusted identity headers — src/identity_proxy.mjs:141
  • [high] Upgrade grants administrative scope automatically — src/store/openclaw_config.rs:399
Agent review details

Security

Needs attention: The new ingress path creates a forgeable local authentication boundary and automatically broadens administrative authorization.

Review metrics

Metric Value Why it matters
Changed surface 31 files affected; +4,919 / -269 lines The branch combines several independently risky subsystems rather than presenting one reviewable change.
Implementation versus tests implementation +3,554, tests +1,272, docs +93 Most growth is production routing, proxy, upgrade, and packaging code, so focused split review materially reduces merge risk.

Merge-risk options

Maintainer options:

  1. Keep the umbrella draft-only (recommended)
    Do not merge this branch; evaluate feat(runtime): package commit-matched companion plugins #102 and fix: harden gateway restart, cleanup, and child temp dirs #103 as the isolated paths for the non-ingress work.
  2. Redesign ingress before merge
    Any ingress proposal must preserve mixed Service handlers, authenticate the proxy boundary, reverse partial rewrites, and remove proxy state and routes during environment cleanup.

Technical review

Best possible solution:

Keep this branch as analysis only, review #102 and #103 independently, and require a separately approved ingress design with explicit lifecycle ownership and a non-forgeable trust boundary.

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

Yes, from source: a mixed Service, a second rewrite failure, or a local loopback caller with forged forwarded headers follows directly through the changed code; no real Tailscale setup was run in this read-only review.

Is this the best way to solve the issue?

No: splitting lifecycle and packaging work is safer, and automatic ingress migration needs an explicit security and lifecycle contract before implementation.

Full review comments:

  • [P1] Preserve every handler in a migrated Service — src/tailscale.rs:364-377
    Handler discovery is per-route, but the grouped rewrite emits one service-wide tailscale serve --service replacement and stores only one proxy as backup. A Service with an unrelated host or path handler can therefore lose it; reject mixed Services or update a complete handler-level configuration losslessly.
    Confidence: 0.99
  • [P1] Authenticate the loopback proxy boundary — src/identity_proxy.mjs:141-153
    Any localhost process can supply the forwarded headers accepted here, while the migration trusts loopback and grants the selected identity operator.admin. Require an unforgeable proxy-to-gateway credential or a transport boundary that prevents a local caller from impersonating a Tailscale user.
    Confidence: 0.99
  • [P1] Restore routes changed before a later rewrite fails — src/tailscale.rs:371-377
    If an earlier Service rewrite succeeds and a later one fails, this function returns before exposing its local backups. The caller then removes only the proxy, leaving earlier routes changed; capture all state before mutation and reverse successful rewrites on failure.
    Confidence: 0.99
  • [P1] Remove ingress when its environment is removed — src/identity_proxy.rs:196-204
    The proxy is persisted independently under OCM_HOME/ingress, but destroy and prune do not own that state or the rewritten Service route. Removing an environment can leave a running proxy and a route to a deleted gateway; integrate cleanup into environment lifecycle commands.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 768714ae3491.

Labels

Label justifications:

  • P0: The proposed loopback trust boundary can allow a local caller to impersonate an administrative Tailscale identity.
  • merge-risk: 🚨 compatibility: Upgrade rewrites existing Service routing and gateway authentication and authorization settings.
  • merge-risk: 🚨 security-boundary: The new proxy treats caller-controlled loopback headers as trusted identity input.
  • merge-risk: 🚨 availability: Partial route rewrites and stale ingress services can leave a gateway unreachable.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: Real behavior proof is not required for maintainer- or bot-authored pull requests.

Evidence

Security concerns:

  • [high] Loopback callers can forge trusted identity headers — src/identity_proxy.mjs:141
    The proxy accepts forwarded identity data from any local process, then converts it to the headers trusted by the gateway.
    Confidence: 0.99
  • [high] Upgrade grants administrative scope automatically — src/store/openclaw_config.rs:399
    The config migration grants the selected identity operator-admin and enables device auto-approval while changing the gateway trust model.
    Confidence: 0.98

What I checked:

  • Current branch scope: The branch differs from current main across 31 files and includes new identity-proxy and Tailscale routing modules, so its central ingress behavior is not implemented on main. (src/tailscale.rs:360, 8a3d25725b12)
  • Service rewrite loses handler-level state: Route discovery iterates individual handlers, but rewriting groups them by Service and invokes one service-wide replacement target; the retained backup contains only one original proxy. (src/tailscale.rs:364, 8a3d25725b12)
  • Loopback identity is forgeable: The proxy accepts any loopback caller with forwarded headers, while the migration trusts loopback and grants the selected identity operator-admin scope. (src/identity_proxy.mjs:141, 8a3d25725b12)
  • Partial route updates are not reversed: If a later Service rewrite fails, the local backup vector is discarded before the caller can restore earlier successful rewrites; the error path removes only the proxy. (src/cli/upgrade.rs:3653, 8a3d25725b12)
  • Ingress lacks lifecycle ownership: Proxy state is persisted under OCM_HOME/ingress/<environment>, but environment removal calls only environment and supervisor cleanup; no lifecycle path references the ingress owner. (src/identity_proxy.rs:191, 8a3d25725b12)
  • Focused successor branches: The PR body and related-item context identify open focused branches for lifecycle work and companion packaging, leaving named Tailscale ingress only on this draft.

Likely related people:

  • hannesrudolph: Prior merged supervisor and restart commits, plus the current draft, connect this contributor to the lifecycle boundary. (role: recent supervisor and restart contributor; confidence: high; commits: aea507db5396, 07cf0d48fb53; files: src/supervisor/mod.rs, src/service/manage.rs)
  • fuller-stack-dev: Recent merged work owns the upgrade transaction and local-runtime boundaries this draft extends. (role: recent upgrade and runtime contributor; confidence: high; commits: 381a5ed2ab23, c34c7072bf28; files: src/cli/upgrade.rs, src/store/runtimes.rs)
  • Vincent Koc: History includes focused runtime dependency and service lifecycle work on the adjacent owners. (role: adjacent runtime and service contributor; confidence: medium; commits: 85cf26bcfff1, 8bc65caaab70; files: src/store/runtimes.rs, src/service/manage.rs)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (9 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-19T23:47:42.186Z sha 558d6aa :: found issues before merge. :: [P1] Preserve every named-Service handler during migration
  • reviewed 2026-08-20T00:03:17.075Z sha 6ee4de4 :: found issues before merge. :: [P1] Preserve every named-Service handler during migration | [P1] Do not trust forwarded identity from any loopback caller
  • reviewed 2026-08-21T17:09:03.773Z sha 824c589 :: found issues before merge. :: [P1] Preserve every named-Service handler during migration | [P1] Authenticate the loopback proxy boundary
  • reviewed 2026-08-21T17:30:41.584Z sha 4eb1742 :: found issues before merge. :: [P1] Preserve every named-Service handler during migration | [P1] Authenticate the loopback proxy boundary
  • reviewed 2026-08-21T17:39:28.564Z sha 4eb1742 :: found issues before merge. :: [P1] Preserve every handler in a migrated Service | [P1] Authenticate the loopback proxy boundary
  • reviewed 2026-08-21T17:58:36.565Z sha 611b1a2 :: found issues before merge. :: [P1] Preserve every handler in a migrated Service | [P1] Authenticate each loopback caller at the proxy boundary
  • reviewed 2026-08-21T19:05:41.454Z sha 8a3d257 :: found issues before merge. :: [P1] Preserve every handler in a migrated Service | [P1] Authenticate the loopback proxy boundary | [P1] Roll back routes already changed after a later rewrite fails | [P1] Own ingress removal outside upgrade rollback
  • reviewed 2026-08-21T19:12:44.746Z sha 8a3d257 :: found issues before merge. :: [P1] Preserve every handler in a migrated Service | [P1] Authenticate the loopback proxy boundary | [P1] Restore routes changed before a later rewrite fails | [P1] Own ingress removal outside upgrade rollback

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Aug 19, 2026
@hannesrudolph
hannesrudolph force-pushed the fix/gateway-aware-restarts branch from 558d6aa to 6ee4de4 Compare August 19, 2026 23:58
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Aug 20, 2026
@hannesrudolph hannesrudolph changed the title fix: make gateway lifecycle and local source runtimes safe fix: make gateway lifecycle, cleanup, and local source runtimes safe Aug 21, 2026
@hannesrudolph hannesrudolph changed the title fix: make gateway lifecycle, cleanup, and local source runtimes safe fix: harden gateway lifecycle, cleanup, temp dirs, and local runtimes Aug 21, 2026
@clawsweeper clawsweeper Bot added P0 Emergency: data loss, security bypass, crash loop, or unusable core runtime. and removed P1 Urgent regression or broken agent/channel workflow affecting real users now. labels Aug 21, 2026
@hannesrudolph
hannesrudolph force-pushed the fix/gateway-aware-restarts branch from 611b1a2 to 8a3d257 Compare August 21, 2026 18:59
@hannesrudolph hannesrudolph changed the title fix: harden gateway lifecycle, cleanup, temp dirs, and local runtimes fix: harden gateway lifecycle, runtime packaging, and Tailscale ingress Aug 21, 2026
@hannesrudolph hannesrudolph changed the title fix: harden gateway lifecycle, runtime packaging, and Tailscale ingress umbrella: gateway lifecycle, runtime packaging, and Tailscale ingress Aug 21, 2026
@hannesrudolph
hannesrudolph marked this pull request as draft August 21, 2026 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P0 Emergency: data loss, security bypass, crash loop, or unusable core runtime. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant