fix(auth): persist stable --account alias on QR login - #53
Conversation
Multi-account setups bind readable accountIds (leader/jinjin/…) while iLink only returns hex@im.bot. Write both credential files and keep both during stale-userId cleanup so alias-based config resolves without hand-copying. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Secondary port to Tencent: Tencent/openclaw-weixin#248 (same change set). |
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates Weixin QR-login persistence to optionally store credentials under a stable --account alias in addition to the server ilink_bot_id, improving multi-account setups that bind config to human-readable account IDs.
Changes:
- Added
persistWeixinLoginAccounts+resolveLoginAccountAliasand updated stale-account cleanup to accept a keep-list (primary + alias). - Updated channel login flows to use the new persistence helper and log primary/alias mapping when applicable.
- Added/updated tests and documentation/changelog entries describing alias persistence behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/channel.ts | Switches QR-login persistence to persistWeixinLoginAccounts and logs alias mapping. |
| src/channel.test.ts | Updates mocks to match the new accounts API. |
| src/auth/accounts.ts | Adds alias resolution + dual-write persistence; updates stale cleanup to accept keep-list. |
| src/auth/account-store.test.ts | Adds tests for keep-list stale cleanup and new alias/persist helpers. |
| README_EN.md | Documents using --account aliases and dual credential files. |
| README.md | Chinese docs for stable alias usage and dual credential files. |
| CHANGELOG_EN.md | Notes fix for persisting stable --account aliases on QR login. |
| CHANGELOG.md | Chinese changelog entry for stable alias persistence fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NewFuture
left a comment
There was a problem hiding this comment.
The alias persistence flow still has four correctness issues; see the inline comments.
ericcaiwx-star
left a comment
There was a problem hiding this comment.
Collaborator note (author hat off — no merge from me)
Agree with @NewFuture’s four correctness blockers; they match how our multi-account gateway actually boots. I would not merge this shape as-is.
Confirming the blockers (prod lens)
- Dual
registerWeixinAccountId(primary + alias) →listAccountIdswould spawn two monitors on one bot token (split sync-buffer / context-token / replay-dedupe). That reintroduces the double-delivery class of bugs we just spent time killing. Canonical runtime ID must be one (prefer explicit alias when present); bot-hash file can remain a lookup/compat credential, not a second indexed runtime account. "default"sentinel asrequestedAccountIdon barechannels loginwould invent a spuriousdefaultalias + dual index even when the user never passed--account. Need to exclude host DEFAULT / only treat explicitly supplied aliases.alreadyConnected/binded_redirectpath skipping alias persistence makes the migration a no-op for the hash-only installs that need it most — must migrate unambiguous stored creds or fail loudly.- Clear-stale before index publish is a crash-window data loss risk; publish canonical index first (or transactional rollback) + fault-injection test.
Suggested reshape (for a follow-up push)
- Runtime index: alias XOR primary, never both.
- Credential files: may still write bot-hash JSON for tooling, but only the canonical id is registered for gateway tasks.
- Login entry tests:
accountId: "default"must not create an alias;--account leaderon already-bound hash must migrate or error. - Keep changelog under
Unreleased(already good); no version bump.
I’ll rework #53 along those lines in a later commit unless @NewFuture prefers a different canonical-id rule. Merge remains with the owner.
…grate Address NewFuture review on NewFuture#53: index only one runtime account (alias preferred), ignore the host default sentinel, migrate unambiguous hash-only bindings on binded_redirect, and publish the index before stale cleanup with a fault-injection regression. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pushed a follow-up addressing the four review blockers:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/auth/accounts.ts:193
- If this alias already belongs to a different token,
saveWeixinAccountoverwrites only its credentials while retaining alias-scoped cursor, context-token, replay-dedupe, andallowFromstate. The new bot can therefore inherit authorization grants and protocol state from an unrelated account. Detect a differing existing token before writing and either reject the alias collision or explicitly clear all account-owned state before rebinding it.
const aliasId = resolveLoginAccountAlias(params.requestedAccountId, primaryId);
const canonicalId = aliasId ?? primaryId;
if (aliasId) {
saveWeixinAccount(aliasId, creds);
src/auth/accounts.ts:269
- This migration path can also overwrite an existing unindexed alias that holds a different token, leaving that alias's old account-scoped state and authorization file in place. Apply the same token-collision validation/destructive-reset policy here before saving the source credentials onto the alias.
saveWeixinAccount(aliasId, creds);
src/channel.ts:578
- The new
alreadyConnectedadapter behavior is untested:src/channel.test.tsonly adds mocks and never asserts migration, reload, error propagation, or the newconnected: trueresult. Add adapter-level regression cases for an alias migration, a no-op/default account, and a migration failure so the changed channel contract is covered.
} else if (result.alreadyConnected) {
try {
const migrated = migrateBoundAccountToAlias({
requestedAccountId: params.accountId,
onClearContextTokens: clearContextTokensForAccount,
|
@NewFuture 按你上次标的四点都改完了(
当前 CI 全绿。请再扫一眼还有没有问题;没问题的话麻烦你这边 squash 合并(发版仍走你的 release 流程)。谢谢。 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (10)
src/auth/accounts.ts:239
- When the alias and its companion bot-hash ID are both indexed, this early return leaves both entries in
accounts.json, so the gateway can still start two monitors with the same token—the condition this change is intended to repair. Republish the alias as canonical and drop the discovered companion before returning.
const aliasEntry = indexedWithToken.find((entry) => entry.id === aliasId);
if (aliasEntry) {
const aliasToken = aliasEntry.data.token?.trim() ?? "";
const companion = (aliasToken ? findCompanionBotAccountId(aliasToken, aliasId) : null) ?? aliasId;
return { primaryId: companion, aliasId, canonicalId: aliasId };
src/auth/accounts.ts:285
- This new diagnostic persists the alias and source account ID verbatim. Avoid recording account identifiers in logs; the migration event itself is sufficient for diagnosis.
logger.info(`migrateBoundAccountToAlias: canonical=${aliasId} from source=${source.id}`);
src/channel.ts:420
- Both the user-facing output and persistent info log include the alias/source account IDs verbatim. Confirm the migration without emitting either identifier so captured diagnostics do not disclose account-identifying data.
log(`\n已将已绑定账号迁移为稳定别名 ${migrated.canonicalId}(凭证兼存 ${migrated.primaryId})。`);
logger.info(
`auth.login: migrated already-connected bot to alias=${migrated.canonicalId} from=${migrated.primaryId}`,
);
src/channel.ts:569
- This diagnostic logs the canonical and primary account IDs verbatim. Preserve the branch distinction without including identifier values.
logger.info(
aliasId
? `loginWithQrWait: saved account data canonical=${canonicalId} primary=${primaryId}`
: `loginWithQrWait: saved account data for accountId=${canonicalId}`,
);
src/channel.ts:584
- The migration log records both account IDs verbatim. Log only that the already-connected credential was migrated, not the identifying values.
logger.info(
`loginWithQrWait: migrated already-connected bot to alias=${migrated.canonicalId} from=${migrated.primaryId}`,
);
src/auth/accounts.ts:254
- The ambiguity error embeds every indexed account ID and is subsequently copied to the CLI and error logs by both callers. Since the IDs can include normalized server bot identifiers, report the count and remediation without enumerating them.
const ids = indexedWithToken.map((entry) => entry.id).join(", ");
throw new Error(
`weixin: already connected, but multiple bound accounts are ambiguous (${ids}). ` +
`Re-login with force for a single account, or remove the extra credentials before migrating to --account ${aliasId}.`,
);
src/auth/accounts.ts:70
writeFileSyncopens and truncates the existing index before writing, so a real ENOSPC/I/O failure can leaveaccounts.jsonempty or partial; the new test only simulates an exception before the underlying write. This does not provide the documented guarantee that a failed publish preserves the previous index. Write a sibling temporary file and atomically replace the index (with cleanup) instead.
/** Replace the persistent account index in a single write. */
function writeAccountIndex(accountIds: string[]): void {
const dir = resolveWeixinStateDir();
fs.mkdirSync(dir, { recursive: true });
fs.writeFileSync(resolveAccountIndexPath(), JSON.stringify(accountIds, null, 2), "utf-8");
src/channel.ts:578
- The adapter now turns
alreadyConnectedinto success and invokes alias migration, butchannel.test.tsonly adds mocks and has no assertions for either login entry point. Add regression cases that verify the requested alias is forwarded, migration/reload occurs,connectedis true foralreadyConnected, and migration failures reject; repository guidance requires tests for every behavior change.
} else if (result.alreadyConnected) {
try {
const migrated = migrateBoundAccountToAlias({
requestedAccountId: params.accountId,
onClearContextTokens: clearContextTokensForAccount,
src/auth/accounts.ts:194
- This writes both the stable alias and normalized server bot ID verbatim to persistent logs. Those identifiers can identify an account when logs are collected for support; keep the event but omit or redact the values.
This issue also appears in the following locations of the same file:
- line 250
- line 285
logger.info(`persistWeixinLoginAccounts: wrote alias=${aliasId} alongside primary=${primaryId}`);
src/channel.ts:400
- The success output exposes the normalized server bot ID (
primaryId), which is account-identifying data and may be retained in terminal/installer logs. The operator only needs confirmation that the companion credential was saved.
This issue also appears in the following locations of the same file:
- line 417
- line 565
- line 582
log(
aliasId
? `\n已将此 OpenClaw 连接到微信(账号 ${canonicalId},凭证兼存 ${primaryId})。`
: `\n已将此 OpenClaw 连接到微信。`,
);
| ); | ||
| } | ||
|
|
||
| const source = indexedWithToken[0]; |
|
@ericcaiwx-star 感谢 我结合 OpenClaw 2026.7.1 的 channels 实现又走了一遍真实流程,还有一个设计层面的顾虑想和你确认:当前实现实际上是把 hash accountId 在线重命名成 alias,而不只是保存显示别名。
在不修改 OpenClaw core 的前提下,我觉得有两个相对小且安全的方向:
如果这个 PR 的核心需求必须覆盖存量 hash-only 用户,我更倾向方案 2;如果只解决新登录时稳定命名,方案 1 更简单。想听下你对目标使用场景和这两个取舍的看法,我们再决定是否需要继续调整实现。 |
|
@NewFuture 感谢把宿主侧真实生命周期也摊开了,这个顾虑成立。 我们这边更倾向方案 2(插件内 alias → hash 逻辑映射)。 使用场景是单 gateway、多个人微:名册/bindings 用稳定别名(如
方案 1 对「只解决新登录命名」更干净,但覆盖不了我们这种多账号 roster + 存量 hash 的场景。 若你认可方案 2,我按这个方向改一版(primary hash 唯一进 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Suppressed comments (10)
src/auth/accounts.ts:254
- The thrown error includes every bound account ID and is subsequently logged verbatim by both login paths. This exposes sensitive account IDs contrary to
AGENTS.md:34-37; report the candidate count and recovery action without listing IDs or the requested alias.
if (indexedWithToken.length > 1) {
const ids = indexedWithToken.map((entry) => entry.id).join(", ");
throw new Error(
`weixin: already connected, but multiple bound accounts are ambiguous (${ids}). ` +
`Re-login with force for a single account, or remove the extra credentials before migrating to --account ${aliasId}.`,
);
src/auth/accounts.ts:285
- This diagnostic persists the canonical alias and source account ID verbatim. Account IDs are sensitive under
AGENTS.md:34-37; log only that migration completed, or redact both values.
logger.info(`migrateBoundAccountToAlias: canonical=${aliasId} from source=${source.id}`);
src/channel.ts:420
- Both the runtime message and persistent diagnostic include raw alias/source account IDs. These IDs are sensitive under
AGENTS.md:34-37; report migration success without interpolating either value.
log(`\n已将已绑定账号迁移为稳定别名 ${migrated.canonicalId}(凭证兼存 ${migrated.primaryId})。`);
logger.info(
`auth.login: migrated already-connected bot to alias=${migrated.canonicalId} from=${migrated.primaryId}`,
);
src/channel.ts:568
- This persistent diagnostic logs the canonical alias and bot ID verbatim, which violates the account-ID handling rule in
AGENTS.md:34-37. Use a value-free success message and narrow the earlier destructuring accordingly.
logger.info(
aliasId
? `loginWithQrWait: saved account data canonical=${canonicalId} primary=${primaryId}`
: `loginWithQrWait: saved account data for accountId=${canonicalId}`,
src/channel.ts:584
- This logs both migrated account IDs into the gateway log. Account IDs are sensitive under
AGENTS.md:34-37; retain only the migration event, without the raw values.
logger.info(
`loginWithQrWait: migrated already-connected bot to alias=${migrated.canonicalId} from=${migrated.primaryId}`,
);
src/auth/accounts.ts:109
- The keep set protects the current alias and primary, but stale cleanup still enumerates only indexed IDs. After
bot-old + alias-oldis persisted,bot-oldis deliberately unindexed; logging in the same user under a new bot removes onlyalias-oldand leaves the old bot-hash credential, context tokens, and allow-list usable. Enumerate/track companion credential files during stale cleanup and remove stale companions while preserving every current keep ID.
const keep = new Set(
(Array.isArray(keepAccountIds) ? keepAccountIds : [keepAccountIds]).map((id) => id.trim()).filter(Boolean),
);
if (keep.size === 0) return;
src/auth/accounts.ts:194
- This writes both account IDs verbatim to the persistent gateway log. Account IDs are classified as sensitive by
AGENTS.md:34-37; keep this diagnostic value-free (or use an account-ID redaction helper) instead.
This issue also appears in the following locations of the same file:
- line 249
- line 285
logger.info(`persistWeixinLoginAccounts: wrote alias=${aliasId} alongside primary=${primaryId}`);
src/channel.ts:399
- The new success message sends the alias and server bot ID through
runtime.log, exposing sensitive account IDs contrary toAGENTS.md:34-37. Keep the status message generic and destructure onlyaliasIdif the other result fields are no longer needed.
This issue also appears in the following locations of the same file:
- line 417
- line 565
- line 582
log(
aliasId
? `\n已将此 OpenClaw 连接到微信(账号 ${canonicalId},凭证兼存 ${primaryId})。`
: `\n已将此 OpenClaw 连接到微信。`,
src/channel.ts:593
- The plugin-level behavior changes are not exercised in
channel.test.ts: the account helpers are mocked, but no test drives successful alias persistence,alreadyConnectedmigration, error propagation, or this newconnected: trueresult. Add mocked channel tests for both login entry points so the host-facing contract is covered, as required for behavior changes by the repository testing rules.
connected: result.connected || Boolean(result.alreadyConnected),
src/auth/account-store.test.ts:254
- This opaque Weixin-shaped user ID is not clearly synthetic, unlike the surrounding
user-a@im.wechatfixtures. Repository privacy rules require obviously synthetic identifiers in tests (AGENTS.md:34-37); replace it in both the input and assertion with a labeled synthetic value.
userId: "o9cq80zLSSEWjtr2UODlOgvt3pO4@im.wechat",
| function writeAccountIndex(accountIds: string[]): void { | ||
| const dir = resolveWeixinStateDir(); | ||
| fs.mkdirSync(dir, { recursive: true }); | ||
| fs.writeFileSync(resolveAccountIndexPath(), JSON.stringify(accountIds, null, 2), "utf-8"); |
| const aliasEntry = indexedWithToken.find((entry) => entry.id === aliasId); | ||
| if (aliasEntry) { | ||
| const aliasToken = aliasEntry.data.token?.trim() ?? ""; | ||
| const companion = (aliasToken ? findCompanionBotAccountId(aliasToken, aliasId) : null) ?? aliasId; | ||
| return { primaryId: companion, aliasId, canonicalId: aliasId }; | ||
| } |
Keep listAccountIds/monitors/state on the bot-hash id and store a 1:1 alias→hash map for bindings/outbound, so host start(alias) cannot spawn a second transport after QR login. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@NewFuture 按你提的方案 2改完了( 设计
验证
请再审一眼这个方向是否符合你走宿主生命周期后的预期。若 OK,麻烦你这边 squash 合并(发版仍走你的 release 流程)。谢谢。 |
| if (account.accountId !== primaryId) { | ||
| logger.info("gateway.startAccount: skipping alias lifecycle task (transport owned by primary)"); | ||
| ctx.setStatus?.({ accountId: account.accountId, running: false }); | ||
| return; |
There was a problem hiding this comment.
OpenClaw 2026.7.1 treats a fulfilled startAccount() as an unexpected channel exit. After this return, server-channels.ts records channel exited without an error and schedules up to 10 auto-restarts unless the runtime has terminalDisconnect: true. Since login explicitly starts the requested alias, reload-off/failure turns this intended no-op into a backoff loop. Mark this lifecycle terminal (or keep it pending until abort) and cover the host lifecycle behavior.
There was a problem hiding this comment.
Rechecked against the minimum supported OpenClaw 2026.6.1: terminalDisconnect is not consulted in that version, so setting it alone would leave the restart loop there. The smallest cross-version fix is to make config.isEnabled return false when the resolved request is an alias (accountId !== primaryId). listAccountIds() already exposes only primary IDs, and explicit outbound sends still resolve the alias without this lifecycle check, while channels.start(alias) is rejected before any task is created. The early-return branch can remain as defensive behavior for direct adapter callers.
| if (aliasId !== source.primary) { | ||
| clearWeixinAccount(aliasId); | ||
| } | ||
| bindWeixinAccountAlias(aliasId, source.primary); |
There was a problem hiding this comment.
When requestedAccountId is the existing primary hash, resolveLoginAccountAlias(..., "") initially treats it as an alias. After the sole source is resolved, aliasId === source.primary, so this call throws (alias must differ from the primary bot id). Therefore rerunning channels login --account <existing-hash> on binded_redirect now fails instead of succeeding as a no-op. Detect equality after source resolution and skip alias binding.
There was a problem hiding this comment.
Still confirmed. The minimal fix is an equality no-op after resolving the source: if aliasId === source.primary, return null before calling bindWeixinAccountAlias(), then cover explicit hash relogin with a regression test. No migration machinery is needed for this case.
| const aliasId = resolveLoginAccountAlias(params.requestedAccountId, primaryId); | ||
| if (aliasId) { | ||
| // Drop leftover alias-scoped credential/state from the prior rename design. | ||
| adoptAccountStateNamespace(aliasId, primaryId); |
There was a problem hiding this comment.
This moves sync/context/allow-list state before proving that the leftover alias belongs to the newly confirmed primary. If leader still represents bot A from the prior online-rename state and a fresh login reuses leader for bot B, bot B inherits A's authorization/context and A's alias state is removed. Check the existing alias credential/mapping first and only adopt state when its token/companion proves the same bot; otherwise reject the collision (or rebind without inheriting state).
There was a problem hiding this comment.
To avoid overdesign here: badcc80 was never included in a release or tag; released users are hash-only, so there is no production alias namespace that must be migrated. The safer minimal fix is to remove adoptAccountStateNamespace() (and the companion fallback used only for that intermediate design), keep existing state on the primary hash, and reject an alias if an existing alias credential/map points to a different token/primary. Do not move sync, context, or pairing allowlist state between identities.
Disable alias accounts in config.isEnabled so start(alias) never creates a restarting lifecycle task; treat hash relogin as a no-op; reject conflicting alias credentials without moving state namespaces. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@NewFuture 按你标的三点最小修法改完了(最新 commit):
相关单测已覆盖。请再扫一眼;没问题的话麻烦 squash 合并。谢谢。 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/auth/accounts.ts:71
- This helper also writes
account-aliases.json, which contains stable aliases and primary account IDs, but the temporary/final file is created with the process-default permissions. Restrict the file at creation time, as the credential store does, so another local user cannot read account identifiers during or after the rename.
fs.writeFileSync(tmpPath, JSON.stringify(value, null, 2), "utf-8");
src/auth/account-store.test.ts:265
- The PR description and manual test plan require both
<alias>.jsonand<botHash>.json, but this assertion explicitly enforces that the alias credential file does not exist. Either implement the documented dual-file persistence or update the PR description/test plan to describe the logical alias-map design so the acceptance criterion is unambiguous.
// Alias is logical only — no second credential / state namespace.
expect(loadWeixinAccount("collin")).toBeNull();
src/messaging/process-message.ts:153
- Add a focused regression test that passes distinct primary and
routeAccountIdvalues and verifiesresolveAgentRoutereceives the alias. The current process-message tests exercise only the fallback where both IDs are identical, so the new binding behavior can regress even though the channel-level test still confirms the option was forwarded to the monitor.
const routeAccountId = deps.routeAccountId?.trim() || deps.accountId;
const route = deps.channelRuntime.routing.resolveAgentRoute({
cfg: deps.config,
channel: "openclaw-weixin",
accountId: routeAccountId,
| saveWeixinAccount(primaryId, creds); | ||
|
|
||
| const aliasId = resolveLoginAccountAlias(params.requestedAccountId, primaryId); | ||
| if (aliasId) { | ||
| assertAliasCredentialCompatible(aliasId, params.token); |
Summary
--accountalias and the normalizedilink_bot_id, so multi-account configs (leader/jinjin/ …) resolve without hand-copying JSON files.userIdcleanup keeps the primary + alias credential files.Motivation
openclaw channels login --channel openclaw-weixin --account collincurrently only writesnormalizeAccountId(ilink_bot_id).json. Operators who bind peers by readable accountId then need a manualcpof the credential file. This PR makes login write both paths.Test plan
npx vitest run src/auth/account-store.test.ts src/channel.test.ts(31 pass)channels login --account <alias>→ both<alias>.jsonand<botHash>.jsonexist under credentials dirNotes
Tencent/openclaw-weixinand cross-link.Made with Cursor