Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31113a8968
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await installIpcMocks(electronApp, { | ||
| gatewayStatus: { state: 'running', port: 18789 }, | ||
| }); |
There was a problem hiding this comment.
Overriding only gateway:status here does not make the Models page consider the gateway running: useGatewayStore.init() reads status via hostApiFetch('/api/gateway/status') and uses gateway:status IPC only as a 30s reconciliation fallback (src/stores/gateway.ts), while E2E mode explicitly skips gateway auto-start (electron/main/index.ts). In this test that means status remains stopped, /models never loads token usage rows, and the count() === 2 poll can time out. Mock the Host API status response (or emit gateway:status-changed) so the store transitions to running immediately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eeb64d5bb1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| import { join } from 'node:path'; | ||
| import type { Page } from '@playwright/test'; | ||
| import { completeSetup, expect, test } from './fixtures/electron'; | ||
| import { completeSetup, expect, installIpcMocks, test } from './fixtures/electron'; |
There was a problem hiding this comment.
After switching the gateway-row test to test.skip, installIpcMocks is no longer used, but @typescript-eslint/no-unused-vars is configured as an error. Since the check workflow runs pnpm run lint, this creates a deterministic CI failure that blocks the PR from merging until the unused binding is removed (or intentionally ignored with the configured _ convention).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b97cdb4ce
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (isOwnProcess) { | ||
| this.restartController.recordRestartCompleted(); |
There was a problem hiding this comment.
onConnectedToExistingGateway is invoked for any attach-to-existing path in runGatewayStartupSequence (both true in-process restarts and ordinary reconnects to an already-running owned process), but this block always calls recordRestartCompleted() when isOwnProcess is true. Because deferred restarts are queued while state is reconnecting (restart() + shouldDeferRestart) and later resolved by flushDeferredRestart, this timestamp can make a deferred restart look already satisfied and get dropped even though no restart occurred, leaving config changes made during reconnect unapplied. Restrict this timestamp update to cases where a real restart completion is actually confirmed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02d16ac783
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (const key of Object.keys(section)) { | ||
| if (!VALID_TOP_LEVEL_KEYS.has(key) && !extraAllowedTopLevelKeys.has(key)) { | ||
| delete section[key]; |
There was a problem hiding this comment.
The new strict-channel cleanup removes any top-level key not in VALID_TOP_LEVEL_KEYS, which now deletes channels.openclaw-weixin.routeTag whenever accounts exists. That regresses the WeChat QR flow because loadWeChatRouteTag still reads the top-level routeTag when no accountId is passed, and /api/channels/wechat/start -> waitForWeChatLoginSession currently follows that no-accountId path, so the SKRouteTag header silently disappears after sanitize for routed deployments. Please exempt openclaw-weixin.routeTag (or pass accountId through that flow) before deleting unknown keys.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f641b17943
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| migrateLegacyChannelConfigToAccounts(section, DEFAULT_ACCOUNT_ID); | ||
| const accounts = getChannelAccountsMap(section); | ||
| if (!accounts?.[accountId]) continue; | ||
| if (ownedChannelAccounts && !ownedChannelAccounts.has(`${channelType}:${accountId}`)) { | ||
| if (!accounts?.[accountId] || (ownedChannelAccounts && !ownedChannelAccounts.has(`${channelType}:${accountId}`))) { |
There was a problem hiding this comment.
deleteAgentChannelAccounts now runs migrateLegacyChannelConfigToAccounts before it knows whether the current channel/account will be deleted. For strict-schema channels (like dingtalk), that migration removes top-level credential keys and stores them under accounts.default; then this new early-continue path skips strict channels without restoring the top-level mirror. If any other channel in the same loop is actually deleted, the final writeOpenClawConfig persists the mutated strict channel in an invalid shape, so deleting an unrelated agent can silently break an existing dingtalk config.
Useful? React with 👍 / 👎.
| if ('accounts' in section) { | ||
| delete section['accounts']; | ||
| modified = true; |
There was a problem hiding this comment.
In sanitizeOpenClawConfig, the strict-channel branch deletes accounts/defaultAccount immediately. If a dingtalk config is stored only under accounts.default (legacy or externally generated configs), this removes the only credential copy because this branch never mirrors nested values to top-level first. The sanitize write then permanently strips dingtalk credentials and the channel cannot reconnect after upgrade.
Useful? React with 👍 / 👎.
1 participant
Summary
Upgrade openclaw to 4.8
Type of Change
Validation
Checklist