Skip to content

improve(signal): make transport ownership account-scoped#108345

Closed
jesse-merhi wants to merge 1 commit into
mainfrom
jesse/signal-account-transport
Closed

improve(signal): make transport ownership account-scoped#108345
jesse-merhi wants to merge 1 commit into
mainfrom
jesse/signal-account-transport

Conversation

@jesse-merhi

@jesse-merhi jesse-merhi commented Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What Problem This Solves

Signal transport ownership was split across flat channel fields, per-account state, setup policy, and runtime auto-detection. That made multi-account managed-native setup vulnerable to port collisions and let external-native or container accounts fall into local-daemon assumptions.

This change gives every effective Signal account one concrete transport owned by that account. Runtime no longer detects or changes transport kind. Signal setup owns detection, probing, canonical writes, and managed-native port allocation.

Canonical Contract

{
  channels: {
    signal: {
      transport: { kind: "managed-native", httpPort: 8080 },
      accounts: {
        work: {
          account: "+15555550123",
          transport: {
            kind: "external-native",
            url: "http://signal-work:8080",
          },
        },
        alerts: {
          account: "+15555550124",
          transport: {
            kind: "container",
            url: "http://signal-container:8080",
          },
        },
      },
    },
  },
}

The root channels.signal.transport belongs only to the effective default account. Named accounts own channels.signal.accounts.<id>.transport and do not inherit the root transport.

Public Signal setup seams are exported from extensions/signal/api.ts and implemented in extensions/signal/src/setup-transport.ts:

  • detectSignalTransport: setup-only native/container detection.
  • probeSignalTransport: probe one explicit transport.
  • prepareSignalManagedNativeTransport: preserve the selected account's managed options while allocating a collision-free port across effective sibling endpoints.
  • writeSignalAccountTransport: validate and write a canonical account-owned transport.

Callers creating managed-native accounts prepare first, then write. Port allocation, host/URL validation, endpoint alignment, and collision policy stay inside Signal.

Migration And Recovery

openclaw doctor --fix migrates shipped flat Signal fields to the canonical transport union while preserving account identity, default-account ownership, configured paths/options, disabled accounts, and container account requirements.

Ambiguous offline auto-mode endpoints remain pending rather than being guessed. The exception is an explicitly gateway-owned legacy daemon (autoStart: true), which migrates to managed-native even while the update has that daemon stopped.

When an external plugin package or channel changes, update finalization reruns the updated plugin's doctor and strict config validation in the updated process before restart. Doctor, entrypoint, and validation failures are structured and fail closed; a previously stopped service is restored only when the updated schema accepts the resulting config.

Why Non-Signal Files Still Change

The earlier generic channel/plugin hooks have been removed. The remaining non-extensions/signal/** changes are bounded to existing owner seams:

  • canonical Signal config types, schema, tests, and generated config metadata in src/config/**;
  • one existing generic setup input field for explicit Signal transport selection;
  • update finalization sequencing required because Signal is an external plugin and its newly installed doctor/schema must run in a fresh process;
  • the Signal-specific QA Lab dependency adapter;
  • official external-channel catalog metadata, docs, and changelog.

There are no changes to generic channel add/CLI discovery, generic doctor flow, read-only plugin loading, official-plugin recovery capabilities, or src/cli/update-cli/update-command.ts.

Config surface counts change from core/channel/plugin 2851/3680/3537 to 2832/3693/3537: 13 canonical channel nodes are added, while 19 core nodes disappear with the retired flat Signal surface/schema extraction. Plugin config count is unchanged.

Evidence

  • Branch rebuilt from current main as one commit: 6a46dae24297cfba23acd0acedd9baad31d3afe1.
  • Final two-pass committed-branch review reported no actionable findings.
  • Supported remote Node runtime: 243 updater tests passed; core and core-test typechecks passed; targeted core lint passed.
  • Final focused Signal compatibility/setup proof: 86 tests passed after the last Signal policy changes; extension and extension-test typechecks plus full extension lint passed.
  • Earlier final-tree Signal suite: 691 tests passed across 42 files; subsequent accepted edge-case fixes each received focused regression tests.
  • QA transport tests, update-schema tests, generated config/docs/SDK checks, SDK surface check, repository guards, formatting, and git diff --check passed.

The local shell currently uses Node 25.2.1, whose embedded SQLite version is intentionally rejected by the repository's WAL-safety guard. The three affected update-sentinel tests pass on the supported remote runtime as part of the 243-test updater suite above.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation channel: signal Channel integration: signal commands Command implementations size: XL maintainer Maintainer-authored PR labels Jul 15, 2026
@jesse-merhi
jesse-merhi force-pushed the jesse/signal-account-transport branch from 0b55dc6 to f8d288b Compare July 15, 2026 21:08
@openclaw-barnacle openclaw-barnacle Bot added cli CLI command changes scripts Repository scripts labels Jul 15, 2026
@clawsweeper clawsweeper Bot added 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. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. labels Jul 16, 2026
@clawsweeper

clawsweeper Bot commented Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 21, 2026, 12:28 AM ET / 04:28 UTC.

Summary
The branch replaces flat Signal transport settings and runtime auto-detection with explicit account-owned managed-native, external-native, or container transports, including doctor migration, setup, update finalization, tests, generated metadata, and docs.

PR surface: Source +2097, Tests +2567, Docs +17, Config +4, Generated 0, Other +4. Total +4689 across 60 files.

Reproducibility: yes. for the current architectural problem: current main visibly resolves flat transport fields and performs runtime auto-detection, while the PR replaces those paths with account-owned concrete transports. A live multi-account Signal deployment is still needed to prove the migration and recovery behavior end to end.

Review metrics: 2 noteworthy metrics.

  • Signal config contract: 13 canonical channel nodes added; 19 retired core nodes removed. The branch replaces a shipped configuration shape rather than adding an isolated implementation detail, so upgrade behavior is a merge-critical concern.
  • Update recovery path: 1 fresh-process doctor/validation flow added. The new flow runs after plugin update and before restart, making plugin execution and recovery semantics material to merge review.

Stored data model
Persistent data-model change detected: migration/backfill/repair: CHANGELOG.md, migration/backfill/repair: extensions/signal/doctor-contract-api.ts, migration/backfill/repair: extensions/signal/src/config-compat.ts, migration/backfill/repair: extensions/signal/src/doctor.ts, migration/backfill/repair: extensions/signal/src/shared.ts, migration/backfill/repair: src/cli/update-cli/update-command-fresh-doctor.ts, and 8 more. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Obtain explicit Signal/config-owner approval of the migration and post-update doctor execution boundary.

Risk before merge

  • [P1] Existing installations using shipped flat Signal fields must complete openclaw doctor --fix successfully before normal strict validation resumes; maintainer approval should cover fresh-install and upgrade/recovery behavior for managed-native, external-native, container, disabled, and ambiguous offline configurations.
  • [P1] The update path now starts an updated entrypoint to run plugin-owned doctor repair before restart. That is a deliberate but security-sensitive execution and service-recovery boundary which needs explicit owner acceptance beyond unit and type-check evidence.

Maintainer options:

  1. Confirm upgrade and recovery contract (recommended)
    Before merge, have the Signal/config owner confirm that doctor migration and strict validation preserve supported managed-native, external-native, container, disabled, and ambiguous-offline upgrade paths.
  2. Accept the post-update execution boundary
    Merge with an explicit maintainer decision that updated plugin doctor code may run in a fresh process during update finalization and may fail closed before service restart.
  3. Pause the broad migration
    Keep the existing configuration contract until maintainers choose a narrower migration or update-recovery design.

Next step before merge

  • [P2] The remaining blocker is an explicit maintainer decision on configuration migration, update recovery, and plugin-doctor execution; it is not a narrow autonomous repair.

Maintainer decision needed

  • Question: Should OpenClaw approve the new post-plugin-update flow that executes the updated plugin’s doctor in a fresh process and blocks restart until canonical Signal configuration validates?
  • Rationale: The code makes a durable upgrade and execution-boundary choice: it migrates shipped user configuration while allowing an updated external plugin to run repair logic during update finalization. Automated review can identify the risk, but cannot choose the repository’s acceptable trust and recovery policy.
  • Likely owner: jesse-merhi — Recent shipped Signal work and authorship of the proposed migration make this the strongest available routing candidate; final acceptance remains a maintainer decision.
  • Options:
    • Approve the canonical migration boundary (recommended): Accept fresh-process plugin doctor execution during plugin updates, provided the owner confirms upgrade coverage for existing Signal configurations and intentional fail-closed restart behavior.
    • Keep migration but narrow update execution: Retain account-owned Signal transports while redesigning the update flow so migration can run without invoking the updated plugin’s doctor automatically.

Security
Needs attention: The diff introduces a deliberate security-sensitive update boundary by running updated plugin doctor logic in a fresh process before service restart; this requires maintainer approval, not an inferred pass.

Review details

Best possible solution:

Adopt one canonical account-owned Signal transport contract only after a Signal/config owner confirms the doctor migration and fresh-process update recovery preserve existing deployments and intentionally accept the plugin-doctor execution boundary.

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

Yes for the current architectural problem: current main visibly resolves flat transport fields and performs runtime auto-detection, while the PR replaces those paths with account-owned concrete transports. A live multi-account Signal deployment is still needed to prove the migration and recovery behavior end to end.

Is this the best way to solve the issue?

Unclear: the owner-local transport and doctor design fits the repository’s plugin-boundary policy, but maintainers must decide whether automatic fresh-process plugin doctor execution is the acceptable update-recovery mechanism.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a substantial but bounded Signal transport/configuration migration with limited channel-specific blast radius.
  • merge-risk: 🚨 compatibility: The PR retires shipped flat Signal config fields and changes setup, validation, doctor migration, and account inheritance behavior.
  • merge-risk: 🚨 security-boundary: Update finalization invokes updated plugin doctor logic in a fresh process before restart, creating an intentional code-execution and fail-closed recovery boundary.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The PR is from a repository MEMBER, so the external-contributor real-behavior-proof gate does not apply; the body nevertheless reports focused Signal, updater, typecheck, lint, and generated-contract validation.
Evidence reviewed

PR surface:

Source +2097, Tests +2567, Docs +17, Config +4, Generated 0, Other +4. Total +4689 across 60 files.

View PR surface stats
Area Files Added Removed Net
Source 34 2425 328 +2097
Tests 16 3213 646 +2567
Docs 5 58 41 +17
Config 1 4 0 +4
Generated 3 14 14 0
Other 1 4 0 +4
Total 60 5718 1029 +4689

Security concerns:

  • [medium] Approve updated-plugin doctor execution during update — src/cli/update-cli/update-command-fresh-doctor.ts:47
    The new fresh-process update flow invokes the newly installed entrypoint to run doctor --repair; an updated plugin can therefore execute its doctor migration before restart. The PR contains fail-closed handling, but maintainers must explicitly accept this trust and recovery boundary.
    Confidence: 0.91

What I checked:

  • Current-main behavior remains flat and auto-detected: Current main resolves httpHost, httpPort, httpUrl, and autoStart from merged Signal account config, so account-owned concrete transport routing is not present there. (extensions/signal/src/accounts.ts:1, be009ee5570f)
  • Current-main runtime still probes transport kind: Current main exposes SignalApiMode, caches auto-detected modes by URL, and chooses native/container at request time; the PR’s removal of this path is therefore a meaningful remaining change rather than obsolete churn. (extensions/signal/src/client-adapter.ts:1, be009ee5570f)
  • Current released contract uses retired fields: The current Signal type contract still declares apiMode, httpUrl, httpHost, httpPort, cliPath, and autoStart, establishing that this branch changes a shipped configuration surface requiring a migration. (src/config/types.signal.ts:1, be009ee5570f)
  • Proposed migration and recovery boundary: The PR head introduces canonical Signal transports, plugin-owned doctor migration, and fresh-process post-plugin doctor/validation before restart; this is the central compatibility and security-sensitive decision surface. (src/cli/update-cli/update-command-fresh-doctor.ts:1, 6a46dae24297)
  • Prior review continuity: The prior completed ClawSweeper cycle at cdb40b39e4a46fb29e5b9587ebada667e8e21074 recorded no actionable code findings and explicitly routed the permanent migration and recovery-security boundary to maintainer review; the new head is a rebuild at 6a46dae24297cfba23acd0acedd9baad31d3afe1. (6a46dae24297)
  • Signal-area history signal: The current v2026.7.1 release credits Jesse Merhi for shipped Signal status-reaction work, providing a separate recent Signal-area contribution beyond this proposed branch. (2d2ddc43d0dc)

Likely related people:

  • jesse-merhi: Authored the reviewed migration and is independently credited for shipped Signal status-reaction work in v2026.7.1; the available history signal supports routing Signal transport follow-up to this contributor, though the broader ownership trail is incomplete in this read-only environment. (role: recent Signal contributor; confidence: medium; commits: 6a46dae24297, 2d2ddc43d0dc; files: extensions/signal/src/accounts.ts, extensions/signal/src/client-adapter.ts, extensions/signal/src/setup-transport.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.
Review history (4 earlier review cycles)
  • reviewed 2026-07-16T03:40:51.771Z sha f8d288b :: found issues before merge. :: [P1] Keep disabled external plugins non-executable during doctor
  • reviewed 2026-07-20T03:27:43.368Z sha 4a4cf19 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-20T04:10:07.916Z sha 4a4cf19 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-20T05:27:15.411Z sha cdb40b3 :: needs maintainer review before merge. :: none

@jesse-merhi
jesse-merhi force-pushed the jesse/signal-account-transport branch from f8d288b to 4a4cf19 Compare July 20, 2026 03:23
@clawsweeper clawsweeper Bot added 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. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed 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. 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 Jul 20, 2026
@jesse-merhi
jesse-merhi force-pushed the jesse/signal-account-transport branch from 5e45a67 to 6a46dae Compare July 21, 2026 03:40
@openclaw-barnacle openclaw-barnacle Bot removed the commands Command implementations label Jul 21, 2026
@jesse-merhi jesse-merhi mentioned this pull request Jul 21, 2026
Merged
@steipete

steipete commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Superseded by #112176, which landed account-owned Signal transport as part of the repository-wide channel-owned setup contracts in 4a2a600.

That broader fix moves setup parsing, validation, and config mutation to channel owners instead of retaining a Signal-only core seam.

@steipete steipete closed this Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

channel: signal Channel integration: signal cli CLI command changes docs Improvements or additions to documentation extensions: qa-lab maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. scripts Repository scripts size: XL status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jesse-merhi @steipete @Patrick-Erichsen