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 into
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
openclaw-barnacle
Bot
added
channel: telegram
![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: 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.
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 👍 / 👎.
Tianworld
changed the title
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.
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.
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
openclaw-barnacle
Bot
added
gateway
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.
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.
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 👍 / 👎.
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.
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.
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 👍 / 👎.
|
Thanks for the context here. I did a careful shell check against current Current main already has the central useful Windows test stabilization and Telegram setup-promotion behavior, while this older PR is conflicting, lacks external real behavior proof, and carries stale broad churn that should not be used as the landing path. So I’m closing this older PR as already covered on Review detailsBest possible solution: Keep current main's narrower implementation and close this stale branch; any remaining Telegram promotion-key expansion should come back as a fresh focused PR with real setup proof. Do we have a high-confidence way to reproduce the issue? Yes for the cleanup decision: current-main source shows the Windows test and Telegram promotion paths already covered, and the stale PR is live-conflicting. I did not rerun the original Windows CI flake or a live Telegram setup flow in this read-only review. Is this the best way to solve the issue? No, this PR is no longer the best way to solve the issue. Current main has a narrower implementation and the remaining branch should be closed or replaced with a focused current-main PR if a small Telegram key-list gap remains. Security review: Security review cleared: No concrete security or supply-chain regression was found beyond the functional availability and compatibility risks already called out for the stale branch. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against e7ee1c55b433; fix evidence: commit cae64ee360a8, main fix timestamp 2026-06-14T18:09:26-07:00. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
clawsweeper
Bot
added
P2
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
This pull request has been automatically marked as stale due to inactivity. |
clawsweeper
Bot
added
rating: 🌊 off-meta tidepool
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
ClawSweeper applied the proposed close for this PR.
|
1 participant
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