feat: support device identity pairing

[plutoless]

No description provided.

[github-actions]

PR Review: feat: support device identity pairing

This PR adds Ed25519-based device identity pairing to the OpenClaw gateway extension and a corresponding frontend dialog. The overall design is sound and the UX flow is well thought out. Here are my findings:

---

Bugs

1. Untracked asyncio.create_task calls risk silent task loss

In extension.py, two fire-and-forget tasks are created without storing a reference:

asyncio.create_task(
    self._reemit_startup_pairing_notice(...)   # in _check_gateway_ready_on_start
)
asyncio.create_task(self._send_connect_background())   # in _handle_message

CPython can garbage-collect tasks that have no strong references before they complete. The recommended pattern is to keep a set and discard completed tasks:

task = asyncio.create_task(...)
self._background_tasks.add(task)
task.add_done_callback(self._background_tasks.discard)

2. extractApproveCommand is duplicated

ChatCard.tsx and MessageList.tsx each define their own extractApproveCommand function with slightly different null-guards ((text || "") vs text). Extract this into a shared utility to avoid drift.

3. Stored device_id in the identity JSON is ignored on load

_load_or_create_device_identity saves device_id to disk but _load_device_identity ignores the stored value and recomputes it from the public key. The stored field is dead data, which can mislead operators who inspect the file. Either remove it from the stored format or use it (with a consistency check).

---

Security

4. Private key stored unencrypted; silent chmod failure

The identity file is written with NoEncryption() and then chmod 600 is applied in a bare try/except OSError: pass. On some platforms or filesystems the chmod can silently fail, leaving the private key world-readable. Consider logging a warning on chmod failure rather than swallowing it:

try:
    os.chmod(identity_path, 0o600)
except OSError as e:
    self.ten_env.log_warn(f"[openclaw] could not restrict permissions on identity file: {e}")

5. Token included in signature payload

_build_device_auth_payload incorporates the raw auth token in the signed string. This is presumably intentional (binds the token to the signed claim), but it means rotating the token invalidates all persisted signatures and may surprise operators. A comment explaining why the token is part of the payload would clarify intent.

---

Code Quality

6. Text-based pairing detection is fragile

The backend _build_pairing_required_payload already sets a structured pairing_required: True flag, but the frontend isPairingRequiredMessage still checks for the substring "pairing is required" in the free-text message body. If the gateway changes its wording, the dialog silently stops appearing. It would be more robust for _onTextChanged to check text.pairing_required === true instead (i.e. pass the structured flag through the RTM message, not just the human-readable text).

7. exit_after_notify is dead code

_build_pairing_required_payload always returns "exit_after_notify": True and the field is declared in manifest.json, but nothing in the extension or frontend acts on it. Either implement the intended behaviour or remove the field for now to avoid confusion.

8. Synchronous file I/O in on_start (async context)

_load_or_create_device_identity does blocking file I/O (including key generation, JSON serialisation, and Path.write_text) and is called directly from the async on_start handler without await asyncio.to_thread(...). This blocks the event loop for the duration of the call. For the common case (file already exists) this is fast, but key generation can be measurable. Consider wrapping in asyncio.to_thread.

9. Manifest type name: "bool" vs "boolean"

The new pairing_required and exit_after_notify fields use "type": "bool". Confirm this matches the TEN framework's type registry; the rest of the manifest uses "string" and "int", so the short name should be consistent with how the framework spells the boolean type.

---

Missing Tests

There are no tests for:

• _load_or_create_device_identity / _load_device_identity (happy path, corrupt file, wrong key type)
• _build_device_auth_payload / _build_device_payload signature correctness
• _build_pairing_required_payload detection logic (especially the heuristic string checks)
• Frontend helper functions isPairingRequiredMessage / extractApproveCommand

Even a small unit-test file covering the pure functions would significantly increase confidence in the pairing flow.

---

Minor

• The _reemit_startup_pairing_notice docstring/parameter is named attempts but calls max(attempts, 0), so passing 0 silently does nothing. A guard or assertion would make this more explicit.
• _describe_ws_state calls self.ws.exception(); on some aiohttp versions this may return None even when the connection is alive—make sure this is safe to call unconditionally.

---

Overall: The feature is well-structured and the UX for guiding users through gateway pairing is clear. Addressing the task-reference bug (#1) and the text-based detection fragility (#6) would be the highest-priority fixes before merge.

[github-actions]

PR Review: feat: support device identity pairing

Overall this is a well-structured feature. The Ed25519 identity approach, background-task lifecycle management, and frontend pairing dialog are all sensible. A few issues worth addressing before merge:

---

Bugs

1. Wrong field used for pairing_required code check

In openclaw_gateway_tool_python/extension.py, _build_pairing_required_payload:

details: dict[str, Any] = {}
if isinstance(exc, GatewayRequestError):
    details = exc.details          # nested payload dict
...
code = str(details.get("code", "")).strip().lower()   # looks inside details
is_pairing_required = (
    ...
    or code == "pairing_required"
)

The error code is stored in exc.code (set from error["code"] in _handle_message), not inside exc.details. details.get("code") looks one level too deep and will always be empty for a normal gateway error response. The check should be:

code = (exc.code if isinstance(exc, GatewayRequestError) else "").lower()

2. _send_connect_background and _reemit_startup_pairing_notice can race with each other

When startup fails with a pairing error, _check_gateway_ready_on_start emits once immediately, then schedules _reemit_startup_pairing_notice for 2 more emissions. Separately, _handle_message also schedules _send_connect_background when connect.challenge arrives, and that also calls _emit_pairing_required_event on failure. The frontend could receive 3–5 identical pairing events within seconds, all opening/reopening the dialog. Consider deduplicating at the emission level (e.g., track the last emitted pairing event hash).

---

Security

3. Silent failure on chmod for private key file

try:
    os.chmod(identity_path, 0o600)
except OSError:
    pass

If this fails (e.g., on certain filesystems or container mounts), the private key file is silently left world-readable. At minimum log a warning:

except OSError as e:
    self.ten_env.log_warn(f"[openclaw] could not restrict identity file permissions: {e}")

---

Schema / Type System

4. manifest.json uses "type": "bool" instead of "boolean"

The TEN Framework type system uses "boolean" (see other manifests in the codebase). The new fields use "bool":

"pairing_required": { "type": "bool" },
"exit_after_notify": { "type": "bool" }

This may cause runtime schema validation failures or silent ignoring of these fields. Should be "boolean" to match the rest of the schema.

---

Dead Code / Unreachable Logic

5. Copy command button in MessageList.tsx is unreachable for pairing messages

ChatCard.tsx intercepts any openclaw_result containing "pairing is required", opens the modal dialog, and returns early — so pairing messages are never dispatched to the Redux store and never appear in MessageList. The extractApproveCommand / onCopyCommand logic added to OpenclawMessageCard in MessageList.tsx would therefore never fire for pairing messages.

If the intent is to handle pairing messages that arrive via a different code path, document it. Otherwise this code is dead and adds noise.

6. exit_after_notify: True is never consumed

The payload sets exit_after_notify: True, it appears in the manifest schema, but neither _handle_openclaw_reply_event in main_python/extension.py nor the frontend code reads or acts on it. Either implement the auto-exit/retry behavior or remove the field.

---

Code Quality / Minor

7. Unused import asyncio in main_python/extension.py

The new code in main_python/extension.py doesn't use the asyncio module directly (the await calls don't need an explicit import). Per the AGENTS.md YAGNI guideline, remove it unless it's actually needed.

8. No tests for cryptographic core

_load_or_create_device_identity, _load_device_identity, _build_device_payload, and _build_device_auth_payload contain the security-critical logic of this PR. There are no new tests covering key generation, persistence/reload, signature construction, or pairing detection. These functions are pure/deterministic and straightforward to unit-test — worth adding given the security impact.

9. Dialog deduplication key uses timestamp

const dialogKey = `${approveCmd}|${Number(text.ts ?? 0)}`;

If ts is missing or 0 (e.g., during startup re-emissions), different pairing events with the same approve command collapse to the same key and the dialog won't reopen. Given the re-emission logic in item 2 above, it may be more reliable to use a per-event UUID or sequence number.

---

Summary

Severity	Item
Bug	details.get("code") should be exc.code (#1)
Bug	Multiple concurrent pairing event emissions (#2)
Security	Silent chmod failure on identity file (#3)
Schema	"bool" → "boolean" in manifest.json (#4)
Dead code	Copy command button in MessageList unreachable (#5)
Incomplete	exit_after_notify field not acted upon (#6)
Minor	Unused asyncio import (#7)
Minor	No tests for crypto functions (#8)
Minor	Dialog dedup key fragile with missing ts (#9)

[github-actions]

PR Review: feat: support device identity pairing

Overall this is a solid feature implementation. The Ed25519 device identity flow, challenge-nonce handshake, and frontend pairing dialog are well-structured. A few concrete issues worth addressing before merging.

---

Bug: _reemit_startup_pairing_notice retries are always suppressed

In extension.py (openclaw_gateway_tool_python), _check_gateway_ready_on_start creates a retry task with attempts=2, interval_seconds=1.0:

self._create_background_task(
    self._reemit_startup_pairing_notice(
        exc=exc, task_id="", summary="", attempts=2, interval_seconds=1.0
    )
)

But _is_duplicate_pairing_event suppresses any re-emission within 5 seconds of the last emission. The 2 retries at 1-second intervals both fire at t=1s and t=2s — well within the 5-second window — so they are always suppressed by deduplication. The retry loop is effectively a no-op.

Options:

• Remove _reemit_startup_pairing_notice entirely if the first emission is sufficient.
• Or raise the retry interval above 5 seconds (e.g., interval_seconds=6.0) so retries actually fire.
• Or reduce the dedup window to shorter than the retry interval.

---

Security: chmod failure is silently swallowed

try:
    os.chmod(identity_path, 0o600)
except OSError:
    pass

If chmod fails (e.g., unsupported filesystem, read-only volume, or running as a different user than the file owner), the private key file may be world-readable with no indication. This should at minimum log a warning:

except OSError as e:
    self.ten_env.log_warn(
        f"[openclaw_gateway_tool_python] could not restrict identity file permissions: {e}"
    )

---

Potential issue: pipe-delimited signature payload has no field escaping

_build_device_auth_payload uses | as a delimiter:

return "|".join([
    "v2", device_id, client_id, client_mode, role, ",".join(scopes),
    str(signed_at_ms), token or "", nonce,
])

device_id (SHA-256 hex) and nonce (server-controlled) cannot contain |. But client_id, client_mode, token, and scopes all come from user config. A client_id or token containing | would shift field boundaries and produce a signature over a different payload than the gateway expects. Consider either:

• Validating that these fields do not contain |, or
• Using a structured/length-prefixed encoding rather than a bare delimiter.

---

Code quality: on_start does synchronous file I/O

async def on_start(self, ten_env: AsyncTenEnv) -> None:
    ...
    self._device_identity = self._load_or_create_device_identity()

_load_or_create_device_identity does synchronous reads, writes, and key generation on the event loop thread. For a small JSON file this is unlikely to cause a problem in practice, but it is technically incorrect for an async context. Low priority, but worth noting.

---

UX: pairing dialog cannot be reopened after close

Once the user closes the pairing dialog in ChatCard.tsx, there is no way to recover the approve command without triggering another pairing event. The lastPairingDialogKeyRef deduplication prevents reopening on the same event. Worth considering either:

• Adding the pairing message to chat history (the return before dispatch(addChatItem(...)) currently prevents this), or
• Making the dialog re-openable (e.g., a persistent banner alongside the dialog).

---

Minor: clipboard failure is silent

const copyPairingCommand = async () => {
  try {
    await navigator.clipboard.writeText(pairingApproveCmd);
    setPairingCopied(true);
  } catch (_error) {
    setPairingCopied(false);
  }
};

The catch silently resets the button state with no feedback. Users on older browsers or non-HTTPS contexts will see the button flash back to "Copy command" with no explanation. A brief error message would improve UX.

---

Missing tests

No tests are added for:

• openclawPairing.ts (isPairingRequiredMessage, extractApproveCommand, extractListCommand) — these are pure functions and easy to unit test.
• _build_device_auth_payload, _load_or_create_device_identity, _build_pairing_required_payload in Python — these handle the security-sensitive identity and signing logic.

Given AGENTS.md's guidance on extensions having a tests/ directory, at least basic tests for the signing payload format and the identity load/create round-trip would be valuable here.

---

Nits

• _reemit_startup_pairing_notice can likely be removed once the dedup/retry conflict above is resolved — the first emission should be sufficient.
• _describe_ws_state is a useful addition for debugging.
• Resetting _connect_sent = False and _connect_nonce = "" in _open_connection correctly handles reconnect scenarios.

[github-actions]

PR Review: feat: support device identity pairing

Overall this is a solid feature that adds Ed25519-based device identity and a clean pairing approval UX. The architecture is reasonable and the deduplication logic for pairing events is thoughtful. Below are issues ranging from security-critical to minor.

---

Security

[Critical] Race condition / TOCTOU in private key file creation

In _load_or_create_device_identity:

identity_path.write_text(json.dumps(payload, indent=2) + "\n")
try:
    os.chmod(identity_path, 0o600)
except OSError:
    pass

The file is written with default permissions, then chmod is called. Between these two operations another process could read the private key. The chmod failure is also silently swallowed with no log warning.

Suggested fix — write to a temp file with restricted permissions and atomically rename:

import tempfile
tmp_fd, tmp_path = tempfile.mkstemp(dir=identity_path.parent)
try:
    os.chmod(tmp_path, 0o600)
    with os.fdopen(tmp_fd, "w") as f:
        f.write(json.dumps(payload, indent=2) + "\n")
    os.replace(tmp_path, identity_path)
except Exception:
    os.unlink(tmp_path)
    raise

[Medium] Delimiter injection in _build_device_auth_payload

The signature payload is assembled by joining fields with |:

return "|".join([
    "v2", device_id, client_id, client_mode, role,
    ",".join(scopes), str(signed_at_ms), token or "", nonce,
])

If any field contains | (e.g. a custom gateway_client_id), the signed payload would be ambiguous or structurally different from what the gateway verifies. Consider validating that no field contains |, or switching to a length-prefixed or JSON encoding:

return json.dumps(
    ["v2", device_id, client_id, client_mode, role, scopes, signed_at_ms, token or "", nonce],
    separators=(",", ":"), ensure_ascii=True,
)

---

Design

[Medium] Frontend should use the structured pairing_required flag, not text parsing

openclawPairing.ts detects pairing by substring-matching the text message:

export function isPairingRequiredMessage(text: string): boolean {
  return text.toLowerCase().includes("pairing is required");
}

But the backend already sets pairing_required: True in the openclaw_result payload, and it is declared in manifest.json. The frontend should check this boolean directly instead of parsing text:

if ("pairing_required" in text && text.pairing_required) { ... }

Text-based detection breaks silently if the backend message wording ever changes. The helpers in openclawPairing.ts for extracting command strings from the message text are still useful, but the trigger should be the boolean flag.

[Minor] Polling loop in _ensure_connected adds unnecessary complexity

The original asyncio.wait_for approach was simpler. The replacement polls every 200 ms with manual deadline tracking, which is harder to reason about and adds up to 200 ms of extra latency on the happy path. Consider restoring:

await asyncio.wait_for(
    self._hello_event.wait(),
    timeout=max(self.config.connect_timeout_ms, 1000) / 1000,
)

If the goal is to detect a closed socket mid-wait, having the recv loop cancel the future or set an error event would be cleaner.

---

Compatibility

[Minor] Python 3.10+ union syntax used throughout

New code uses X | Y type annotations (e.g. DeviceIdentity | None, dict[str, Any] | None). If the project targets Python 3.9 (common in Docker AI agent images), these raise TypeError at runtime for variable annotations — not just at type-check time. Either add from __future__ import annotations at the top of the file, or use Optional[...] from typing.

---

Code Quality

[Minor] Re-emit interval is tightly coupled to the dedup window

retry_interval_s = self._pairing_emit_dedupe_window_s + 1.0

Deriving the retry interval from the dedup window is fragile. If _pairing_emit_dedupe_window_s is tuned, this arithmetic could accidentally suppress or duplicate retries. Consider making the retry interval an independent constant.

[Minor] No user feedback on clipboard copy failure

} catch (_error) {
  setPairingCopied(false);
}

When navigator.clipboard.writeText fails (e.g. on non-HTTPS origins or when permission is denied), the user sees no indication that the copy failed. Consider showing a brief inline error or falling back to selecting the <pre> text.

[Minor] _reemit_startup_pairing_notice does not stop when pairing is resolved

The background retry loop re-emits pairing notices for a fixed number of attempts even if the user has already approved pairing and the agent reconnected. This can cause stale pairing banners to reappear. Consider checking _hello_event state before each re-emit, or cancelling the retry task when connection is restored.

---

Positive notes

• The deduplication logic for pairing events (_is_duplicate_pairing_event) is well-designed and prevents banner spam from concurrent startup/connect paths.
• Graceful fallback to --latest when requestId is absent is a good UX touch.
• manifest.json correctly declares the new pairing_* output properties.
• Background task lifecycle cleanup in on_stop is properly handled.
• The lastPairingDialogKeyRef dedup in the frontend correctly prevents duplicate dialogs from the same event.

🤖 Generated with Claude Code

[github-actions]

PR Review: feat: support device identity pairing

Overall this is a well-structured addition. The Ed25519 device identity flow, background task lifecycle management, and deduplication logic are all solid. The comments below cover issues ranging from a design concern to minor nits.

---

Security

[Minor] Private key stored in plaintext PEM
extension.py saves the private key with NoEncryption(). chmod 600 reduces exposure, but a key stored unencrypted on disk is readable by anyone with root or the same UID. For a device identity key this is likely an acceptable trade-off, but it is worth documenting explicitly (e.g. a note in README or a log warning on key creation) so operators are aware.

---

Potential Bugs

[Bug] _connect_nonce can be overwritten before the background task reads it

In _handle_message:

self._connect_nonce = self._extract_connect_nonce(payload)
self._create_background_task(self._send_connect_background())

If the gateway sends a second connect.challenge before the background task has run (e.g. during a reconnect), _connect_nonce is overwritten in-place. The first background task then uses the second nonce. Since _connect_sent prevents double-sends this does not cause a double connect, but the nonce used for signing may be from a different challenge than intended.

Safer: pass the nonce directly to the background task so each closure captures its own value:

nonce = self._extract_connect_nonce(payload)
self._create_background_task(self._send_connect_background(nonce))

Then update _send_connect_background / _send_connect to accept the nonce as a parameter instead of reading from self._connect_nonce.

[Bug] Synchronous file I/O in on_start blocks the event loop

_load_or_create_device_identity() is a synchronous method that calls Path.mkdir(), Path.exists(), Path.read_text(), and Path.write_text(). It is invoked directly from on_start without asyncio.get_event_loop().run_in_executor(...). For small identity files this is unlikely to be noticeable, but it is inconsistent with the async-first pattern of the rest of the extension.

---

Design Concerns

[Design] Frontend detects pairing by parsing human-readable text instead of using the structured flag

The openclaw_reply_event output schema now includes a pairing_required boolean, and main_python/extension.py correctly reads it from the structured payload. However, the RTM message forwarded to the frontend only carries text:

await self._send_rtm_message({
    "data_type": "openclaw_result",
    "text": self._build_pairing_required_message(payload),
    "ts": ts,
})

The frontend (ChatCard.tsx) then detects pairing by checking isPairingRequiredMessage(text):

return text.toLowerCase().includes("pairing is required");

This means the frontend depends on a specific phrase being present in human-readable text, creating an invisible coupling between _build_pairing_required_message in Python and isPairingRequiredMessage in TypeScript. Any change to the wording would silently break detection.

The RTM message could carry the structured flag directly:

await self._send_rtm_message({
    "data_type": "openclaw_result",
    "pairing_required": True,
    "pairing_approve_cmd": payload.get("pairing_approve_cmd", ""),
    "pairing_list_cmd": payload.get("pairing_list_cmd", ""),
    "text": self._build_pairing_required_message(payload),
    "ts": ts,
})

The frontend would then check text.pairing_required === true instead of string-matching.

[Minor] Inconsistent hint text

_build_pairing_required_payload in extension.py sets:

"pairing_hint": "Run these commands on the gateway host, then restart this agent."

But _build_pairing_required_message in main_python/extension.py ends with:

"Approve pairing, then retry your request."

One says "restart this agent", the other says "retry your request". The latter is more accurate (no restart required), so the hint in _build_pairing_required_payload should be aligned.

---

Breaking Change

[Note] Default CLIENT_ID / CLIENT_MODE values changed

The defaults changed from webchat-ui/webchat to openclaw-control-ui/ui. Any existing deployment that relied on the old defaults without explicitly setting OPENCLAW_GATEWAY_CLIENT_ID / OPENCLAW_GATEWAY_CLIENT_MODE will silently connect with different identity values after upgrading. This is documented in the README, but a comment in property.json or a migration note in the PR description would make the impact more visible.

---

Minor / Nits

Frontend clipboard failure is silent

In copyPairingCommand, a clipboard failure resets pairingCopied to false but gives no visual feedback to the user (the button just stays as "Copy command"). A small inline error message (e.g. "Copy failed – see console") would help in non-HTTPS contexts where navigator.clipboard is unavailable.

_ensure_connected polling interval

The polling loop sleeps up to 0.2s per iteration. This is fine, but the log on _recv_loop close now fires every time a connection drops (not just on error), which could increase log noise during expected reconnects. Consider a guard like if not self._stopped: before the warn.

No tests added

The openclawPairing.ts functions are small and pure — they are ideal candidates for unit tests. Likewise, the Python _build_pairing_required_payload, _build_device_auth_payload, and _is_duplicate_pairing_event methods are testable in isolation. Adding at least basic tests would guard against the fragile text-matching coupling mentioned above.

---

Summary

Area	Status
Ed25519 key gen & signing	✅ Correct
Background task lifecycle	✅ Clean
Deduplication logic	✅ Sound
Nonce ownership (race)	⚠️ Fix recommended
Sync I/O in async context	⚠️ Minor
Structured vs. text-based pairing detection	⚠️ Design improvement
Hint text consistency	⚠️ Minor
Breaking default change	📝 Noted
Test coverage	📝 Missing

The nonce ownership issue and the structured-vs-text detection design concern are the most important items to address before merge.

[github-actions]

PR Review: feat: support device identity pairing

The overall design is solid — Ed25519 device identity with gateway-challenge/nonce signing, a dedup-aware pairing event pipeline, and a frontend dialog to guide users through approval. Below are issues worth addressing before merge.

---

Bugs

1. Synchronous file I/O blocks the event loop in async on_start

_load_or_create_device_identity() is a synchronous method called directly from async on_start. It performs blocking filesystem operations (mkdir, exists, read_text, write_text). These will stall the event loop, potentially delaying other extensions. Wrap it in asyncio.get_event_loop().run_in_executor(None, ...) or use aiofiles.

# extension.py – on_start
self._device_identity = self._load_or_create_device_identity()  # blocks event loop

---

2. Silent chmod failure — security implication, no log warning

When os.chmod(0o600) fails (e.g. running as root, certain bind-mount filesystems), the exception is swallowed with pass and no log entry is written. The private key file may remain world-readable without any indication.

try:
    os.chmod(identity_path, 0o600)
except OSError:
    pass  # No warning logged

This should at minimum emit a log_warn so operators know to take action.

---

3. Dialog deduplication key is fragile when ts is absent or zero

In ChatCard.tsx:

const dialogKey = `${approveCmd}|${Number(text.ts ?? 0)}`;

When ts is missing or 0, every pairing message with the same command shares the same key, so a second distinct pairing attempt (e.g. after a reconnect) is silently ignored by lastPairingDialogKeyRef. A monotonically incrementing counter or a per-message UUID would be more reliable.

---

4. copyPairingCommand swallows clipboard errors silently

} catch (_error) {
    setPairingCopied(false);
}

If the clipboard API fails (permission denied, non-secure context), the button just resets with no indication that the copy did not work. A brief error label ("Copy failed — select manually") would help users.

---

5. Pairing dialog permanently inaccessible after "Dismiss"

Clicking "Dismiss" sets pairingBannerVisible = false. After that, neither the banner nor the dialog can be reopened without a new pairing event. If the user dismisses by mistake and the gateway is still awaiting approval, they have no way to retrieve the command. Consider keeping a minimal "Show pairing instructions" button or restoring visibility on the next pairing message regardless of the banner state.

---

Security

6. Pipe-delimited signature payload is vulnerable to field injection

return "|".join([
    "v2",
    device_id,          # SHA-256 hex – safe
    client_id,          # env-var controlled – can contain "|"
    client_mode,        # env-var controlled – can contain "|"
    role,               # hardcoded "operator"
    ",".join(scopes),   # env-var controlled – can contain "|"
    str(signed_at_ms),
    token or "",        # env-var controlled – can contain "|"
    nonce,
])

If any operator-controlled field (client_id, client_mode, a scope value, or token) contains |, the boundary between fields shifts and the gateway verifies a structurally different payload than intended. The practical risk is limited to misconfigured or adversarial deployments, but the protocol design is unnecessarily brittle. A canonical JSON encoding or percent-encoding of individual fields would eliminate this class of issue.

---

7. Private key stored unencrypted — needs clearer documentation

The identity file stores the Ed25519 private key as a plaintext PEM string inside JSON. The README mentions "ensure the path is writable" but says nothing about protecting the file. In Docker/K8s deployments where identity volumes may be shared or snapshotted, this warrants an explicit security note.

---

Code Quality

8. _emit_pairing_required_event return value is ambiguous

The function returns True in both the "event was emitted" and "event was suppressed as a duplicate" cases. Callers that need to distinguish between them (the retry loop, the startup handler) cannot. Consider a dedicated return type or two separate boolean flags.

---

9. Missing tests

• openclawPairing.ts exports three pure functions (isPairingRequiredMessage, extractApproveCommand, extractListCommand). These are straightforward to unit-test and the regex-based extraction is exactly the kind of logic that benefits from a test suite.
• _build_device_auth_payload and the identity file round-trip (_load_or_create_device_identity / _load_device_identity) have no test coverage. The signing logic especially warrants a test given its security relevance.

---

Minor Notes

• The pairing_hint field says "restart this agent", but the retry mechanism (_reemit_startup_pairing_notice) means restarting is not required — just approving the pairing request. The hint text should reflect that.

• The Telnyx/Plivo changes in this PR are purely cosmetic (trailing newline fixes, code formatting). These are fine changes but would be cleaner as a separate commit or PR to keep the feature diff focused.

• Automated review comments mentioning details.get("code") and "bool" vs "boolean" do not appear to match the current diff — exc.code is used correctly in _build_pairing_required_payload and manifest.json already uses "boolean". Worth double-checking those were pre-diff observations.

[github-actions]

PR Review: feat: support device identity pairing

Overall this is a well-structured feature that adds Ed25519-based device identity and a pairing approval UX. The crypto primitives are used correctly and the background task lifecycle management is solid. Below are findings grouped by priority.

---

Security

[Medium] chmod failure is silently swallowed without logging

try:
    os.chmod(identity_path, 0o600)
except OSError:
    pass

If this fails (e.g. on a FAT filesystem, certain Docker volume mounts, or restrictive containers), the private key file will be left world-readable with no indication. At minimum this should emit a log_warn:

except OSError as e:
    self.ten_env.log_warn(
        f"[openclaw_gateway_tool_python] could not restrict identity file permissions: {e}"
    )

[Low] Auth token included in the signing payload

_build_device_auth_payload binds the token to the signature. This is likely intentional to prevent replay attacks, but it means rotating the gateway token silently invalidates the device signature, leading to a pairing_required error that will look identical to a new-device pairing request. Worth a comment explaining the design intent.

---

Potential Bugs

[High] Backward compatibility: gateways that don't send connect.challenge

Previously _send_connect() was called immediately after opening the WebSocket. Now it's gated on receiving a connect.challenge frame:

# _handle_message
if event_name == "connect.challenge":
    self._connect_nonce = self._extract_connect_nonce(payload)
    self._create_background_task(self._send_connect_background())
    return

If a gateway version doesn't send connect.challenge, _send_connect is never called, _hello_event is never set, and _ensure_connected will time out with a confusing message. Is there a version check or fallback for older gateways? The README/troubleshooting section should mention this requirement.

[Medium] isPairingRequiredMessage uses fragile text matching in the frontend

openclawPairing.ts:

export function isPairingRequiredMessage(text: string): boolean {
  return text.toLowerCase().includes("pairing is required");
}

The backend already sets pairing_required: true in the payload and the frontend message is built by _build_pairing_required_message. Rather than parsing the rendered text, the _onTextChanged handler could check text.pairing_required directly from the structured RTM payload before it becomes a plain string — or at minimum, the text matching should look for the same marker string consistently. The current approach is brittle if the copy ever changes.

[Medium] _reemit_startup_pairing_notice re-emits a stale exception

async def _reemit_startup_pairing_notice(self, *, exc: Exception, ...):
    for _ in range(max(attempts, 0)):
        if self._stopped:
            return
        await asyncio.sleep(max(interval_seconds, 0.1))
        await self._emit_pairing_required_event(exc, ...)

After the sleep, if the user has already approved pairing and a reconnection attempt succeeded, this will still emit the stale "pairing required" notice. Consider checking whether the connection is now healthy before re-emitting:

if self._hello_event.is_set():
    return  # already connected, skip retry

[Low] _build_pairing_required_payload detection heuristic is over-broad

is_pairing_required = (
    "pairing required" in message_lower
    or "missing scope: operator.write" in message_lower  # <-- too broad
    or code == "pairing_required"
)

"missing scope: operator.write" could theoretically occur if an account simply doesn't have that scope configured at all (not just unpaired). In that case, the user would receive the misleading "approve this device" dialog when the actual fix is to update gateway permissions. Preferring code == "pairing_required" and only falling back to text matching when no code is provided would be more precise.

---

Breaking Changes (not documented in PR description)

The defaults for gateway_client_id and gateway_client_mode changed:

• webchat-ui → openclaw-control-ui
• webchat → ui

Users who deployed with the old defaults (without setting the env vars explicitly) will silently get different values after upgrading. This should be called out in the PR description and ideally in a changelog or migration note.

---

Code Quality

Background task cleanup in on_stop

The cleanup loop is:

for task in list(self._background_tasks):
    task.cancel()
self._background_tasks.clear()

The clear happens before awaiting cancellation, so any task callbacks (like task.add_done_callback(self._background_tasks.discard)) will fire on an already-empty set — that's harmless but slightly confusing. Consider awaiting the cancellations if the runtime allows it, or add a comment that teardown is fire-and-forget.

_build_device_auth_payload version string

The canonical string starts with "v2". There's no v1 in this codebase and no comment explaining the version history. A short comment would help future maintainers understand whether v1 must still be supported or can be ignored.

Unrelated formatting changes in the same PR

A large portion of the diff (Telnyx, Plivo, SIP voice-assistant examples) is purely cosmetic reformatting — trailing newlines, line-length adjustments, trailing commas. These are fine individually but mixing them with a security-sensitive feature makes review harder and obscures the functional diff. Consider a follow-up cleanup PR or keep the formatter changes separate.

---

Test Coverage

There are no tests added for:

• Key generation and round-trip loading (_load_or_create_device_identity / _load_device_identity)
• Signature construction (_build_device_payload, _build_device_auth_payload)
• Pairing detection logic (_build_pairing_required_payload)
• Deduplication logic (_is_duplicate_pairing_event)
• Frontend helpers (isPairingRequiredMessage, extractApproveCommand, extractListCommand)

Given that the signing logic is security-critical and the pairing detection relies on string heuristics, unit tests would significantly improve confidence here.

---

Minor / Nits

• The pairing hint "Run these commands on the gateway host, then restart this agent." — based on reading _check_gateway_ready_on_start, restarting may not always be necessary if the extension can reconnect. If reconnection is automatic after approval, the hint should say "retry your request" instead.
• property.json default path ~/.openclaw/identity/device.json — inside a container this may resolve to /root/.openclaw/... or another user's home. The .env.example uses an absolute path /data/openclaw/device_identity.json which is a better container-safe default. Consider making the property.json default match the example.
• _describe_ws_state is useful for diagnostics; consider also logging it on _open_connection entry (at DEBUG level) for observability.