fix(ci): Windows task tests and Telegram setup promotion surface#60630
fix(ci): Windows task tests and Telegram setup promotion surface#60630Tianworld wants to merge 16 commits intoopenclaw:mainfrom
Conversation
… test Made-with: Cursor
Made-with: Cursor
Greptile SummaryThis PR stabilizes two test files for Windows CI by avoiding Windows-specific file-lock and dynamic-import mock-bypass issues. The Confidence Score: 5/5Safe to merge — tests-only PR with targeted, well-commented fixes for Windows CI flakiness. Both changes are narrowly scoped and correct: skipping persist before close is exactly right for the finally-block teardown pattern, and dropping the unused requesterOrigin removes a real Windows CI hang vector. No production logic is touched. No files require special attention. Reviews (1): Last reviewed commit: "test(tasks): close flow registry sqlite ..." | Re-trigger Greptile |
…y tests - Export namedAccountPromotionKeys + resolveSingleAccountPromotionTarget from contract-api (Matrix-style). - Keep dmPolicy/allowFrom at channel root when named accounts exist; honor defaultAccount for promotion target. - Add patchChannelConfigForAccount regression tests in setup-wizard-helpers.test.ts. Made-with: Cursor
ea67883 to
4883c92
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4883c92159
ℹ️ 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".
| accounts: { alerts: { botToken: "tok" } }, | ||
| }, | ||
| }); | ||
| expect(keys).not.toContain("dmPolicy"); |
There was a problem hiding this comment.
Fix Telegram promotion test fixture wiring
This assertion assumes Telegram exposes named-account promotion metadata, but the test registry in beforeEach still registers Telegram with only singleAccountKeysToMove. Under that fixture, resolveSingleAccountKeysToMove will still include dmPolicy/allowFrom from the common key set, and the adjacent promotion-target test will also resolve to default, so these new cases fail deterministically instead of validating the intended behavior. Please wire namedAccountPromotionKeys and resolveSingleAccountPromotionTarget into the Telegram test setup (or adjust expectations).
Useful? React with 👍 / 👎.
Codex P1: getChannelSetupPromotionSurface reads plugin registry setup; Telegram mock only had singleAccountKeysToMove, so namedAccountPromotionKeys/resolveSingleAccountPromotionTarget were missing and the new assertions did not match production wiring. - Re-export namedAccountPromotionKeys + resolveSingleAccountPromotionTarget from plugin-sdk/telegram facade. - Register them on the Telegram test plugin alongside Matrix. Made-with: Cursor
ae6ac47 to
9ab5d38
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ab5d38588
ℹ️ 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".
| namedAccountPromotionKeys: telegramNamedAccountPromotionKeys, | ||
| resolveSingleAccountPromotionTarget: resolveTelegramSingleAccountPromotionTarget, |
There was a problem hiding this comment.
Keep Telegram setup tests aligned with runtime plugin setup
This fixture now injects namedAccountPromotionKeys and resolveSingleAccountPromotionTarget into the Telegram test plugin, but the real Telegram plugin setup surface still only provides singleAccountKeysToMove (see extensions/telegram/src/shared.ts:226-229). That mismatch means the new promotion tests can pass while production still uses the old promotion behavior (including moving root dmPolicy/allowFrom when named accounts already exist), so this change gives false confidence that the runtime fix landed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f45ae18: createTelegramPluginBase now mirrors Matrix and exposes the same promotion fields on ChannelPlugin.setup.
getChannelSetupPromotionSurface reads plugin.setup at runtime; contract-api alone is not enough. Mirror Matrix by wiring namedAccountPromotionKeys + resolveSingleAccountPromotionTarget into createTelegramPluginBase (addresses Codex P1 on false-positive tests). Made-with: Cursor
|
Thanks for the reviews!
PR body / testing notes were also updated to reflect the Vitest project that actually runs the |
- Keep Telegram promotion surface exports (namedAccountPromotionKeys + resolveSingleAccountPromotionTarget) while also retaining mergeTelegramAccountConfig export. - Keep runtime ChannelPlugin.setup wired to promotion surface. Made-with: Cursor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f95ca00396
ℹ️ 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".
|
|
||
| export const singleAccountKeysToMove = ["streaming"]; | ||
| export const namedAccountPromotionKeys = ["botToken", "tokenFile"] as const; | ||
| export const namedAccountPromotionKeys = [...TELEGRAM_NAMED_ACCOUNT_PROMOTION_KEYS]; |
There was a problem hiding this comment.
Sync Telegram fallback promotion keys with expanded contract
This commit broadens Telegram namedAccountPromotionKeys, but fallback promotion still hard-codes only botToken/tokenFile in src/channels/plugins/setup-helpers.ts for paths where the setup surface is unavailable (as noted by the fallback comment there). In those fallback paths (for example legacy/default-account seeding flows), keys newly added here like webhookUrl, webhookSecret, token, and account are not promoted into the target account, so migrated multi-account configs can end up with incomplete account auth/transport state.
Useful? React with 👍 / 👎.
Use /usr/bin/env to avoid hostedtoolcache node permission checks in gateway hot-reload tests, and mock remote environment detection for deterministic models auth context. Made-with: Cursor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d0cbb9233
ℹ️ 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".
| @@ -1,2 +1,60 @@ | |||
| import { DEFAULT_ACCOUNT_ID, normalizeAccountId } from "openclaw/plugin-sdk/setup"; | |||
There was a problem hiding this comment.
Import account-id helpers from routing facade
Switching this module to openclaw/plugin-sdk/setup pulls in the full setup barrel on Telegram startup, because extensions/telegram/src/shared.ts imports setup-contract.ts in the hot channel path. The setup barrel re-exports setup-wizard/setup-helper modules, so this eager import increases runtime load cost and weakens lazy-loading boundaries for normal channel execution. This file only needs DEFAULT_ACCOUNT_ID and normalizeAccountId, so it should use the narrow routing facade instead (as Matrix does).
Useful? React with 👍 / 👎.
Use a mockable remote-environment detector in models auth tests, and use exec secret provider settings that tolerate CI node path permissions during gateway reload tests. Made-with: Cursor
Update cliBackends test configs to include a minimal command field so tsgo passes when CliBackendConfig requires it. Made-with: Cursor
Made-with: Cursor
Replace tmpdir and raw fetch callsites, remove channel literal comparisons, align skill types, and update protocol Swift models. Made-with: Cursor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69c0964f13
ℹ️ 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".
| const { response: res, release } = await fetchWithSsrFGuard({ | ||
| url, | ||
| init: { ...init, signal: ctrl.signal }, | ||
| timeoutMs, | ||
| auditContext: "extensions/browser client-fetch", | ||
| }); |
There was a problem hiding this comment.
Allow loopback browser-control URLs in guarded fetch path
This new fetchWithSsrFGuard call runs with no SSRF policy override, so absolute browser-control URLs on loopback/private hosts (for example http://127.0.0.1:...) are now blocked before the request is sent. That regresses the absolute-URL path in fetchBrowserJson (used when a baseUrl is provided), even though withLoopbackBrowserAuth is explicitly built to support loopback control endpoints. Please pass an explicit private-network policy for this trusted control-plane fetch (or conditionally allow loopback) so these requests keep working.
Useful? React with 👍 / 👎.
Resolve conflicts in browser/qa-lab/signal/msteams/diffs, acp-spawn thread binding, followup/agent-runner tests, setup-wizard helpers. Made-with: Cursor
Deduplicate tmp dir imports, restore fetchWithSsrFGuard in qa-lab health check, import setup promotion helpers in wizard test, drop obsolete skill sourceInfo, narrow codex tool prepareArguments. Made-with: Cursor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ec7108c47
ℹ️ 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".
| const { response: res, release } = await fetchWithSsrFGuard({ | ||
| url, | ||
| init, | ||
| signal: ctrl.signal, | ||
| init: { ...init, signal: ctrl.signal }, | ||
| timeoutMs, |
There was a problem hiding this comment.
Forward abort signal to guarded browser fetch
fetchHttpJson now passes ctrl.signal inside init, but fetchWithSsrFGuard only composes cancellation from its top-level signal option and then overwrites init.signal when timeoutMs is set. In practice, any caller-provided abort (for example fetchBrowserJson(..., { signal })) will no longer cancel the in-flight request promptly and will wait until timeout instead, which regresses cancellation behavior for browser-control calls.
Useful? React with 👍 / 👎.
Provide synthetic sourceInfo in local-loader and tests; use as Skill so SkillSnapshot stays assignable from upstream Skill[] while satisfying builds that require sourceInfo. Made-with: Cursor
|
Hi maintainers — latest push (\2b890f28) is green across the full CI matrix I can see (including \check, \check-additional, \�uild-artifacts, \checks-fast-contracts-protocol, Windows node tests, and extension shards). Branch is merged with current \main\ and ready for review whenever you have bandwidth. Thanks! |
…ntext - Strip null bytes from inbound JSON blocks before stringify (spawn safety) - Prefer gemini-3.1-* CLI template IDs with 3.0 fallbacks for older catalogs - Default anthropic/claude-sonnet-4-6 to 1M context when not overridden in models.providers - Add regression tests (inbound-meta, provider-models, context, status) Made-with: Cursor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e189a58f71
ℹ️ 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".
| export const namedAccountPromotionKeys: FacadeModule["namedAccountPromotionKeys"] = | ||
| loadFacadeModule().namedAccountPromotionKeys; |
There was a problem hiding this comment.
Keep Telegram facade promotion keys lazy-loaded
namedAccountPromotionKeys is initialized with loadFacadeModule().namedAccountPromotionKeys, which eagerly loads @openclaw/telegram/contract-api.js during module import. Any consumer that imports openclaw/plugin-sdk/telegram now pays synchronous facade loading cost (and can fail early if that bundled surface is unavailable) even when it only needs unrelated exports like parseTelegramTopicConversation. This regresses the lazy-loading boundary used by adjacent facade exports and should use the same lazy array wrapper pattern.
Useful? React with 👍 / 👎.
|
Codex review: needs changes before merge. Summary Reproducibility: yes. for the review findings: source inspection shows the guarded fetch API only composes top-level signals, the setup barrel is broad, and the PR diff initializes a Telegram facade value eagerly. For the original Windows CI hangs, this review has a plausible source-level reproduction path but no fresh Windows run because the task was read-only. Next step before merge Security Review findings
Review detailsBest possible solution: Rebase or replace the branch with a narrower PR that keeps the useful Windows task-test fixes and Telegram runtime promotion metadata, while preserving browser abort semantics, SDK lazy-loading boundaries, and the narrow routing import pattern. Do we have a high-confidence way to reproduce the issue? Yes for the review findings: source inspection shows the guarded fetch API only composes top-level signals, the setup barrel is broad, and the PR diff initializes a Telegram facade value eagerly. For the original Windows CI hangs, this review has a plausible source-level reproduction path but no fresh Windows run because the task was read-only. Is this the best way to solve the issue? No, not as currently proposed. The central Telegram and Windows-test direction is reasonable, but the current branch should first fix the cancellation/lazy-loading/import-boundary regressions, add the required changelog entry, and be rebased or narrowed against current main. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 829364f85e4c. |
Summary
Tasks (Windows CI)
sendMessagein the blocked-flow retry test (dynamic import can bypass the Vitest mock on Windows CI and hang until timeout).withTaskRegistryTempDirbefore removing the temp state dir (persist: falseon flow reset), alongside the existing task-registry reset. Prevents Windows CI from stalling onfs.rmwhen tests usecreateManagedTaskFlow.Telegram (setup / multi-account promotion)
namedAccountPromotionKeysandresolveSingleAccountPromotionTargetfromextensions/telegram/contract-api.ts(aligned with the Matrix-style bundled contract surface).ChannelPlugin.setupinextensions/telegram/src/shared.tssogetChannelSetupPromotionSurfacematches production (not only the bundled contract module).dmPolicy/allowFromat the channel root when named accounts already exist; honordefaultAccountwhen resolving the promotion target.patchChannelConfigForAccountregression tests insrc/channels/plugins/setup-wizard-helpers.test.ts(test registry mirrors the runtime setup surface).Branch
origin/mainso protocol-generated artifacts and CI expectations stay aligned with upstream.Notes
setup, plugin-sdk facade re-exports for tests, and setup-wizard-helpers regressions.Testing
pnpm exec vitest run --config vitest.unit.config.ts src/tasks/task-registry.test.ts src/tasks/task-executor.test.tsnode scripts/run-vitest.mjs run --config vitest.channels.config.ts src/channels/plugins/setup-wizard-helpers.test.ts -t "telegram:"(
src/channels/**/*.test.tsis not in the unit Vitest project; CI runs these under the channels config—same aspnpm test:channels.)pnpm protocol:check