Conversation
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
First review of the new agentscope-extensions-channel-weixin module (+3129 across 31 files). Overall quality is well above the average contribution here: the credential types redact themselves in toString() (and that is asserted in tests), all error paths log through safeMessage(...), the endpoint policy is a real allowlist rather than a permissive check, lease handling uses generation counters, the live smoke test is gated behind WEIXIN_LIVE_TEST=true, and both the aggregator pom and the BOM were updated. Nothing here is a correctness bug, so this is a COMMENT rather than REQUEST_CHANGES — the two things I would like settled before merge are the durability story of the state store and an explicit ToS/brand-compliance note for a personal-account channel.
Findings
- [Warning]
InMemoryWeixinStateStore.java:44— account entries are created on demand and never evicted (unbounded growth) - [Info]
InMemoryWeixinStateStore.java:32— cursor / peer tokens / lease are memory-only: restart replays or drops inbound messages and loses the conversation token - [Info]
WeixinEndpointPolicy.java:84— loopback is matched by hostname string, so a hostname resolving to 127.0.0.1 passes; a literalisLoopbackAddress()check would close it - [Info]
WeixinChannel.java:153— please add an explicit note that this drives a personal WeChat account over an unofficial iLink endpoint, so maintainers can judge the compliance risk
Notes
- CLA status was not found on head
16da1747; please make sure the CLA check is green — I cannot approve while it is unresolved. - The sibling PRs #3180/#3182 touch the same channel abstraction; a rebase check after those land would be worthwhile.
Automated review by github-manager-bot
31ba755 to
f3ab33c
Compare
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-checked the new head (f3ab33c7). The delta since the last round is test-only: one new WeixinChannelRuntimeTest covering six runtime paths the loopback fixture did not reach. That is a real improvement — but none of the three findings from the previous review were touched, so this PR is still not approvable as it stands.
What the new test does well
deliversWithTheContextTokenStoredForThePeer, dispatchesMessagesWithoutAProviderIdExactlyOnce, peerThrottleStopsDispatchAndSurfacesATransientFailure, pollingCredentialRejectionStopsTheChannel, reportsCredentialRejectionFromDeliveryAndStopsTheChannel and rejectsDeliveryWithoutAnActiveConsumer assert on the wire payload (to_user_id, context_token) rather than on internal state, and the throttle test pins the exact boundary (21st event must not dispatch) instead of "eventually failed". @Timeout(60) on the class plus a real HttpServer on an ephemeral port means these will not silently hang CI. Credential-rejection propagation to WeixinRuntimeListener was previously untested behaviour, so this closes a genuine gap.
Still open (unchanged from the previous round)
No main-source file changed, so all three remain:
WeixinEndpointPolicy.isLoopbackmatches on hostname string — a host namedlocalhost.*that resolves to127.0.0.1passes the SSRF gate. Resolve to an address and checkisLoopbackAddress()instead.InMemoryWeixinStateStore.accountsgrows per distinctaccountIdwith no eviction, and the map is also used as a validation path.- Inbox cursor, per-peer context tokens and the lease live only in this JVM, so a restart re-reads the personal-message backlog from a null cursor.
The first one is the blocker; the other two are acceptable if documented as single-instance scope, which they currently are not.
CI green (6/6 SUCCESS) and CLA signed on this head.
Automated review by github-manager-bot
|
Correction to the review comment above: I wrote that Automated review by github-manager-bot |
Follow-up to the review on agentscope-ai#3184. - `InMemoryWeixinStateStore` no longer creates accounts from read-only lookups (`loadCursor`, `loadContextToken`, `isLeaseCurrent`, `releaseLease`, claim validation), and every accepted batch now expires completed tombstones across all accounts and forgets accounts whose lease expired with an empty inbox. The javadoc states that this adapter is single-JVM state and not for production. - `WeixinChannel.fromProperties(...)` warns on construction that the standalone factory keeps credentials and runtime state inside that process. - The module README gains a "Compliance and scope" section (personal account, provider terms, host responsibility) and spells out what the in-memory store does and does not provide. The loopback claim in the review is not reproducible: loopback is matched on the literal host (`localhost`/`127.0.0.1`/`::1`), so `localhost.attacker.example` is rejected at `normalizeBaseUrl` and never reaches `validateProviderEndpoint`. Resolving the host instead would let such a name *into* the allowlist, so the literal match is kept and a regression test now pins the rejection. Also covers the remaining functional gaps: `WeixinLoginClient.verify(...)` (blank code, code in the status query, the verification challenge, untrusted redirect, provider failures) and the outbound `notifyStart`/`notifyStop` endpoints. Module line coverage is 93%.
|
Follow-up to the review above (commit d88e133). Addressed
Not changed, with reasoning
On your notes
Tests: 50 in the module (1 environment-gated live smoke test skipped), module line coverage 93%. |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-checked head d88e1330 (fix(weixin): bound in-memory state and document deployment limits). Two of the three findings from the previous round are properly fixed, one of my earlier findings was wrong and I want to correct it on the record, and the new commit surfaces one issue I would like addressed before this is approvable.
Correction — the SSRF finding was a false positive. I claimed WeixinEndpointPolicy.isLoopback matched on a hostname prefix, so localhost.attacker.example could pass the gate. Reading WeixinEndpointPolicy.java:82-84, the check is an exact match on localhost / 127.0.0.1 / ::1 — no suffix can sneak through. The new rejectsHostnamesThatOnlyLookLikeLoopback test pins exactly that, including the 127.0.0.1.attacker.example case. Please treat that earlier comment as retracted; my mistake.
Fixed as requested:
InMemoryWeixinStateStorenow splitsaccount(...)(mutating) fromexisting(...)(read-only lookups), soloadCursor/loadContextToken/isLeaseCurrent/releaseLeaseno longer materialise state, andreadOnlyLookupsDoNotRetainAccountsassertsretainedAccounts() == 0. Tombstone expiry moved into a singleprune(...)that also forgets idle accounts.- Single-instance scope is now documented rather than implied: the class javadoc says "Not for production", the README explains what a host must implement for restart recovery and horizontal scaling, and
fromPropertieslogs a startup warning. That is the outcome I was asking for.
New blocker (correctness, see inline): the eviction rule in prune removes the whole Account, cursor included, so a peer idle for longer than the lease TTL restarts reading from "". That reintroduces the backlog-replay path the cursor exists to prevent, and it conflicts with the README's "reference implementation" framing.
Security item to resolve (see inline): verify_code and qrcode travel as GET query parameters (WeixinLoginClient.observe), and the new test asserts that wire shape. Anything that logs request URIs — provider access logs, intermediaries, jdk.httpclient debug logging, an exception message carrying the URI — will capture a login secret. The related bot_token handling in the same method is worth a leak check on the error path.
CI is green on this head (Check License, Module Sync, ubuntu/windows builds, codecov/patch). I could not find a license/cla status context on d88e1330, so I am not treating CLA as satisfied — if your CLA is tracked another way here, please point me at it. Approving is blocked on the eviction item; the query-string item may be acceptable with a documented justification if the provider protocol forces GET.
Automated review by github-manager-bot
Follow-up to the second review round on agentscope-ai#3184. **Correctness — the eviction rule dropped the cursor.** `prune` forgot any account whose lease had expired, cursor included, so an account idle for longer than the lease TTL would have resumed from `""` and replayed the provider backlog. The automatic sweep now only forgets accounts that hold nothing at all (no cursor, no peer context, no inbox, no live lease) — the ids materialised by validation paths that the previous commit stopped creating in the first place. Retiring real state is now an explicit host action: `WeixinStateStore.removeAccount(accountId)` (default no-op, implemented by the in-memory store), so a host can drop an account without the store guessing. `keepsTheCursorWhenAnAccountGoesIdle` replaces the assertion that used to demand the opposite. **Security — the login transport is query-string based.** The provider protocol puts `qrcode` and `verify_code` in the GET status query, so the status URL is a credential; that is now stated in the README with the operational consequence (no URI-level logging behind this client). The module was verified not to echo anything itself: provider failures report only the status code, and both JSON parse paths (login and outbound) now report line/column instead of Jackson's default message, which quotes the offending input — a partially parsed login response contains `bot_token`, and an inbox response contains message payloads and context tokens. Three tests assert that an HTTP failure, an unparseable login body and a transport failure carry neither the verify code, the QR id, nor body content. Not changed: `fromProperties` still logs WARN on every construction — that is the intent, and `create(...)` with an explicit store is the silent path.
Follow-up to the second review round on agentscope-ai#3184. **Correctness — the eviction rule dropped the cursor.** `prune` forgot any account whose lease had expired, cursor included, so an account idle for longer than the lease TTL would have resumed from `""` and replayed the provider backlog. The automatic sweep now only forgets accounts that hold nothing at all (no cursor, no peer context, no inbox, no live lease) — the ids materialised by validation paths that the previous commit stopped creating in the first place. Retiring real state is now an explicit host action: `WeixinStateStore.removeAccount(accountId)` (default no-op, implemented by the in-memory store), so a host can drop an account without the store guessing. `keepsTheCursorWhenAnAccountGoesIdle` replaces the assertion that used to demand the opposite. **Security — the login transport is query-string based.** The provider protocol puts `qrcode` and `verify_code` in the GET status query, so the status URL is a credential; that is now stated in the README with the operational consequence (no URI-level logging behind this client). The module was verified not to echo anything itself: provider failures report only the status code, and both JSON parse paths (login and outbound) now report line/column instead of Jackson's default message, which quotes the offending input — a partially parsed login response contains `bot_token`, and an inbox response contains message payloads and context tokens. Three tests assert that an HTTP failure, an unparseable login body and a transport failure carry neither the verify code, the QR id, nor body content. Not changed: `fromProperties` still logs WARN on every construction — that is the intent, and `create(...)` with an explicit store is the silent path.
981a623 to
d4b8082
Compare
|
Follow-up to the second review round (commit d4b8082). Blocker fixed — the eviction rule was dropping the cursor. You were right and it was my bug: Security item addressed. The status URL is credential material by protocol ( On your CLA note — you were right and I was wrong. I said earlier the check was green on Tests: 56 in the module (1 environment-gated live smoke test skipped). CI on d4b8082 is running. |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
This PR adds a Personal Weixin iLink channel module with QR login, endpoint allowlisting, long-poll ingestion, cursor/inbox state, context-token replies, and account leases. Verdict: REQUEST_CHANGES, because the id-less message fallback can permanently drop repeated inbound messages; startup retry and provider-success validation also need correction. Strengths include careful credential redaction, endpoint restrictions, and a well-defined durable-state/lease contract.
Automated review by github-manager-bot
…utcomes Addresses the CHANGES_REQUESTED review on agentscope-ai#3184. - **`longPollTimeoutMs` did nothing.** Every request used `requestTimeoutMs`, so a deadline shorter than the provider's long-poll window aborted every `getupdates` call. The client now derives the deadline per call: `longPollTimeoutMs` plus a short grace for `getupdates`, `requestTimeoutMs` for the control calls. `longPollSettingKeepsASlowGetUpdatesAlive` drives a 900 ms response with a 300 ms request timeout and asserts it still succeeds. - **A failed `notifystart` left the account polling.** The catch reported a transient failure and fell through into the poll loop with `providerStarted == false`, so with the lease still renewing the session was never started again and no `notifystop` was ever owed. The session now leaves the lease, so the outer loop backs off, reacquires and retries startup. `providerSessionStartIsRetriedAfterATransientFailure` fails `notifystart` twice and asserts the third attempt happens. This fix also exposed that `WeixinChannelLoopbackTest`'s fake provider had never implemented `notifystart` — it was returning 404 and the old code swallowed it — so the fixture now answers it. - **A missing provider outcome counted as success.** `asInt(0)` accepted `{}` or any 2xx body without `ret`/`errcode` as a delivered message, after which the inbox claim completed and the reply was dropped. Both `updates(...)` and `assertSuccess(...)` now require an explicit provider result. `responsesWithoutAProviderOutcomeAreRejected` covers poll and send. Provider outcome and credential failures also keep their exception type instead of being wrapped in a generic `RuntimeException` by `sendWithContext`. - **Byte-identical id-less messages collapsed into one.** The fallback inbox id was the payload digest alone, so two identical messages in one batch became one claim while the cursor advanced — a user typing the same thing twice lost one message. The fallback id now includes the position in the batch, which keeps a re-delivered batch deduplicated while separating genuine duplicates. `byteIdenticalMessagesWithoutIdsAreBothDispatched` covers it. Test-design fix along the way: the runtime fixture now emulates a long poll (25 ms) instead of answering instantly, which was spinning the consumer loop and starving the rest of the suite; the lease-lifecycle suite dropped from 17.8 s to 7.0 s.
|
All four findings from the CHANGES_REQUESTED review are fixed in ea8c4fd. Every one of them was real, and two were bugs I introduced in the previous round.
Two things surfaced while fixing them, both worth the reviewer's attention:
Test-design fix along the way: the runtime fixture answered Module: 60 tests (1 environment-gated live smoke test skipped). Ready for re-review. |
|
Note on head The previous run failed on Windows in a module this PR does not touch:
|
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Thanks for the quick turnaround — this push resolves four of the issues from the previous review, each with a regression test: longPollTimeoutMs now actually bounds the getupdates request, a missing ret/errcode is no longer treated as a delivered message, a failed notifystart is retried instead of silently polling, and byte-identical id-less messages in one batch no longer collapse into a single inbox claim. The new commits look correct to me.
Findings
- [Warning]
WeixinChannel.java:289— startup retries run at a fixed 1000 ms becausebackoffis only escalated inside the polling loop; a long provider outage turns into an unbounded fast retry loop. - [Warning]
WeixinOutboundClient.java:89— the widened catch now also lets localIllegalStateExceptionsignals (fenced lease, non-2xx HTTP) escape unwrapped, and the redundantWeixinCredentialRejectedExceptionalternative in the union suggests the type distinction was not intended here. - [Info]
WeixinOutboundClient.java:104— the provider's advertisedlongpolling_timeout_msis parsed but never used, so a provider window longer than the local setting still gets aborted early.
Not touched by this push
The earlier findings on InMemoryWeixinStateStore (unbounded per-account map growth / cursor lost on restart), WeixinEndpointPolicy.isLoopback hostname matching, and the verification secrets carried in the WeixinLoginClient query string are still open — no need to fix them in this PR if they belong to the follow-up in this stack, but they should stay tracked.
Overall: good progress, the two warnings are worth a look before merge.
Automated review by github-manager-bot
| notifyListener( | ||
| "transient failure", | ||
| listener -> listener.onTransientFailure(p.accountId(), safeMessage(error))); | ||
| // The provider session never started: there is nothing to poll and nothing to stop. |
There was a problem hiding this comment.
Retrying notifystart is the right fix, and the providerStarted guard in the finally block correctly avoids stopping a session that never started. One gap left: this throw exits consume() before the polling loop ever raises backoff, so poll()'s catch keeps sleeping the same 1000 ms forever. A provider that is down for an hour costs ~3600 notifystart attempts and ~3600 warn lines, while the polling path behind it escalates to maxBackoffMs. Growing the backoff here (or in the poll() catch) would keep both startup and polling retries on the same escalating policy.
There was a problem hiding this comment.
You are right, and it is a real gap in what I wrote: backoff is escalated only in the polling loop's catch, so the startup throw routes to poll()'s catch which sleeps the field without growing it. A provider that is down during startup therefore retries at a flat 1 s — same cost profile as the loop before I moved the retry out of it. The fix is to escalate in the outer catch too (capped by maxBackoffMs), so startup and polling share one policy.
Tracking it as a follow-up item rather than pushing another head onto an approved PR; happy to land it here instead if you would prefer it before merge.
| post( | ||
| "/ilink/bot/sendmessage", | ||
| Map.of("msg", m, "base_info", baseInfo()), | ||
| beforeSend)); |
There was a problem hiding this comment.
Two things worth separating here. WeixinCredentialRejectedException extends RuntimeException, not IllegalStateException, so listing it in this union is a no-op — it was already excluded by the dedicated catch in consume(). The side effect that does change is that every IllegalStateException now escapes unwrapped, including the ones this class produces for local fencing: beforeSend.run() throws IllegalStateException("Weixin lease lost"), and post() throws IllegalStateException("iLink HTTP " + status). Those are not provider outcomes, so the commit message's intent ("provider outcome … keep their type") is only partly met, and drainInbox()'s catch (Exception) will call failMessage() for a claim whose lease has just been fenced. A dedicated exception type for "no ret/errcode" would let this catch stay narrow.
There was a problem hiding this comment.
Both points are correct.
WeixinCredentialRejectedException extends RuntimeException, so that alternative in the multi-catch is dead — it was matched by the earlier dedicated catch already. And the side effect you identified is the real problem: IllegalStateException("Weixin lease lost") from beforeSend.run() and IllegalStateException("iLink HTTP 503") from post() are local signals, not provider outcomes, and they now escape unwrapped. The intent was only to stop wrapping the provider-outcome failure, so a dedicated type (e.g. WeixinProviderRejection extends IllegalStateException) thrown by requireProviderResult(...)/assertSuccess(...), with this catch narrowed to that type plus credential rejection, is the right shape.
On the failMessage concern: validClaim(...) returns null once the lease is not current, so the fenced claim is not resurrected — but calling it at all on that path is still the wrong shape, agreed.
Tracking for the follow-up alongside the backoff item.
| } | ||
|
|
||
| /** Grace on top of the long-poll window before the request is abandoned. */ | ||
| private static final long LONG_POLL_GRACE_MS = 5_000L; |
There was a problem hiding this comment.
timeoutFor() derives the long-poll deadline from the configured longPollTimeoutMs plus a fixed 5 s grace, while the getupdates response already carries longpolling_timeout_ms (parsed into JsonNodeResponse at the bottom of this file and never read). If the provider advertises a window longer than the local setting, the client still aborts the poll early — exactly the failure mode this commit set out to remove — and the new test only covers the case where the local setting is the larger one. Taking max(local, advertised) once the response is known, or at least logging when the advertised value exceeds the configured one, would close that gap.
There was a problem hiding this comment.
Confirmed — longpolling_timeout_ms is parsed onto the response and never read, and my test only covers the case where the local setting is the larger one, so the gap you describe is untested as well as unfixed.
Of the two options, honouring it is the better one: remember the advertised window from the last response and take max(configured, advertised) + grace for the next poll, which is exactly the "provider asks for a longer window" case. Dropping the field would remove state with no effect but would also throw away the provider's own declaration.
Tracking it for the follow-up with a test where the advertised value exceeds the configured one.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-checked the fixes in ea8c4fdb at head 80cfaf37 (the head commit itself is an empty CI retrigger, as you noted). All four blocking findings from the previous round are genuinely closed, and the way they were closed holds up:
longPollTimeoutMsnow bounds thegetupdatesrequest with an explicit 5 s grace on top of the provider window (WeixinOutboundClient:172-177), so the long poll is no longer abandoned mid-window.requireProviderResult(...)on the response path means a missing/failedretis no longer read as a delivered message.- A failed
notifystartnow throws and lets the outer loop back off, reacquire and retry startup instead of silently polling an unstarted session. - Byte-identical id-less messages in one batch get distinct fallback ids, so the inbox claim no longer collapses them — that was real data loss, not just a dedup quirk.
The three items I flagged as "not touched by this push" are also in better shape than my earlier note suggested: InMemoryWeixinStateStore now has completion retention plus removeAccount (:68, :206), and WeixinLoginClient handles the credential-bearing status URL carefully — exceptions carry only the HTTP status, the Jackson failure reports line/column instead of a body snippet, and safeMessage reduces to the exception class name. Good judgment there.
Approving. Three non-blocking notes for the follow-up in this stack:
WeixinChannel:255— the outer consumer retry sleeps at a fixedbackoffbecause the field is only escalated inside the polling loop and reset at:317, so a provider that keeps failing atnotifyStartretries at a steady 1 s cadence, which reads as log spam during a long outage. Waking the field-based backoff in the outer loop (or capping startup retries) would be cleaner; the observable behavior is acceptable as-is.WeixinOutboundClient:253— the provider-advertisedlongpolling_timeout_msis parsed and carried on the response but never read byWeixinChannel:299, so a provider asking for a longer window than our local setting still gets aborted at ours. Either honour it or drop the field so we are not carrying state with no effect.WeixinEndpointPolicy:82—isLoopbackmatches onlylocalhost/127.0.0.1/::1, so127.0.0.2,127.1and[::1](bracketed in a URI host) are rejected. That fails closed rather than open, but it will confuse anyone running the loopback fixture on a non-default 127/8 address.
CI is green (build on ubuntu and windows, license, module sync) and CLA is signed.
Automated review by github-manager-bot
|
Thanks for the approval — noted on all three follow-up notes, and I agree with each of them. Disposition, so they do not get lost:
Also accepted from the inline review: a dedicated exception type for the "no I have not pushed these onto this head — the PR is approved and green, and churning it would cost another full CI cycle for changes you have already marked non-blocking. They will land as the first commits of the follow-up work in this stack (#3186), or as a small hardening PR on the extension immediately after this merges, whichever the maintainers prefer. The module's remaining uncovered lines are the same list: hostile-store error injection and the unreachable digest branch. |
Follow-up to the review on agentscope-ai#3184. - `InMemoryWeixinStateStore` no longer creates accounts from read-only lookups (`loadCursor`, `loadContextToken`, `isLeaseCurrent`, `releaseLease`, claim validation), and every accepted batch now expires completed tombstones across all accounts and forgets accounts whose lease expired with an empty inbox. The javadoc states that this adapter is single-JVM state and not for production. - `WeixinChannel.fromProperties(...)` warns on construction that the standalone factory keeps credentials and runtime state inside that process. - The module README gains a "Compliance and scope" section (personal account, provider terms, host responsibility) and spells out what the in-memory store does and does not provide. The loopback claim in the review is not reproducible: loopback is matched on the literal host (`localhost`/`127.0.0.1`/`::1`), so `localhost.attacker.example` is rejected at `normalizeBaseUrl` and never reaches `validateProviderEndpoint`. Resolving the host instead would let such a name *into* the allowlist, so the literal match is kept and a regression test now pins the rejection. Also covers the remaining functional gaps: `WeixinLoginClient.verify(...)` (blank code, code in the status query, the verification challenge, untrusted redirect, provider failures) and the outbound `notifyStart`/`notifyStop` endpoints. Module line coverage is 93%.
Follow-up to the second review round on agentscope-ai#3184. **Correctness — the eviction rule dropped the cursor.** `prune` forgot any account whose lease had expired, cursor included, so an account idle for longer than the lease TTL would have resumed from `""` and replayed the provider backlog. The automatic sweep now only forgets accounts that hold nothing at all (no cursor, no peer context, no inbox, no live lease) — the ids materialised by validation paths that the previous commit stopped creating in the first place. Retiring real state is now an explicit host action: `WeixinStateStore.removeAccount(accountId)` (default no-op, implemented by the in-memory store), so a host can drop an account without the store guessing. `keepsTheCursorWhenAnAccountGoesIdle` replaces the assertion that used to demand the opposite. **Security — the login transport is query-string based.** The provider protocol puts `qrcode` and `verify_code` in the GET status query, so the status URL is a credential; that is now stated in the README with the operational consequence (no URI-level logging behind this client). The module was verified not to echo anything itself: provider failures report only the status code, and both JSON parse paths (login and outbound) now report line/column instead of Jackson's default message, which quotes the offending input — a partially parsed login response contains `bot_token`, and an inbox response contains message payloads and context tokens. Three tests assert that an HTTP failure, an unparseable login body and a transport failure carry neither the verify code, the QR id, nor body content. Not changed: `fromProperties` still logs WARN on every construction — that is the intent, and `create(...)` with an explicit store is the silent path.
…utcomes Addresses the CHANGES_REQUESTED review on agentscope-ai#3184. - **`longPollTimeoutMs` did nothing.** Every request used `requestTimeoutMs`, so a deadline shorter than the provider's long-poll window aborted every `getupdates` call. The client now derives the deadline per call: `longPollTimeoutMs` plus a short grace for `getupdates`, `requestTimeoutMs` for the control calls. `longPollSettingKeepsASlowGetUpdatesAlive` drives a 900 ms response with a 300 ms request timeout and asserts it still succeeds. - **A failed `notifystart` left the account polling.** The catch reported a transient failure and fell through into the poll loop with `providerStarted == false`, so with the lease still renewing the session was never started again and no `notifystop` was ever owed. The session now leaves the lease, so the outer loop backs off, reacquires and retries startup. `providerSessionStartIsRetriedAfterATransientFailure` fails `notifystart` twice and asserts the third attempt happens. This fix also exposed that `WeixinChannelLoopbackTest`'s fake provider had never implemented `notifystart` — it was returning 404 and the old code swallowed it — so the fixture now answers it. - **A missing provider outcome counted as success.** `asInt(0)` accepted `{}` or any 2xx body without `ret`/`errcode` as a delivered message, after which the inbox claim completed and the reply was dropped. Both `updates(...)` and `assertSuccess(...)` now require an explicit provider result. `responsesWithoutAProviderOutcomeAreRejected` covers poll and send. Provider outcome and credential failures also keep their exception type instead of being wrapped in a generic `RuntimeException` by `sendWithContext`. - **Byte-identical id-less messages collapsed into one.** The fallback inbox id was the payload digest alone, so two identical messages in one batch became one claim while the cursor advanced — a user typing the same thing twice lost one message. The fallback id now includes the position in the batch, which keeps a re-delivered batch deduplicated while separating genuine duplicates. `byteIdenticalMessagesWithoutIdsAreBothDispatched` covers it. Test-design fix along the way: the runtime fixture now emulates a long poll (25 ms) instead of answering instantly, which was spinning the consumer loop and starving the rest of the suite; the lease-lifecycle suite dropped from 17.8 s to 7.0 s.
Add `agentscope-extensions-channel-weixin`, a native Java Channel adapter for Tencent's official iLink personal Weixin API, at the same scope as the existing DingTalk, Feishu, WeCom, GitHub, and GitLab channel extensions. The module owns provider protocol and Channel runtime behavior only: - `WeixinLoginClient` — stateless QR login (`start`/`poll`/`verify`) returning a portable `WeixinLoginSession`, so an attempt can be resumed by another process without carrying the QR image along. - Long polling, `get_updates_buf` cursor persistence, `context_token` replies, and direct-text inbound mapping. - Account-scoped leases with at-least-once inbox processing: one active consumer per account, standalone instances keep trying to acquire a released lease, and a lost lease cancels local dispatch before it can send a late reply. - `WeixinEndpointPolicy` — the endpoint allowlist that runs before credentials are sent: https (or loopback http), official `*.weixin.qq.com` hosts on 443, and redirects only within the official provider, between loopback endpoints, or to the same host. - Neutral host seams (`WeixinCredentialProvider`, `WeixinStateStore`, `WeixinRuntimeListener`) plus `WeixinChannel.fromProperties(...)` for standalone use and `WeixinChannel.create(...)` for managed hosts. An in-memory state store is included for standalone development and tests. It carries no AgentScope product concepts: no owner or vault references, no persistence policy, no public routes, no console state. iLink `-14` is reported as a credential-rejection observation and the host decides what that means. Register the module in the channel parent POM, `agentscope-all`, and `agentscope-bom`.
Follow-up to the review on agentscope-ai#3184. - `InMemoryWeixinStateStore` no longer creates accounts from read-only lookups (`loadCursor`, `loadContextToken`, `isLeaseCurrent`, `releaseLease`, claim validation), and every accepted batch now expires completed tombstones across all accounts and forgets accounts whose lease expired with an empty inbox. The javadoc states that this adapter is single-JVM state and not for production. - `WeixinChannel.fromProperties(...)` warns on construction that the standalone factory keeps credentials and runtime state inside that process. - The module README gains a "Compliance and scope" section (personal account, provider terms, host responsibility) and spells out what the in-memory store does and does not provide. The loopback claim in the review is not reproducible: loopback is matched on the literal host (`localhost`/`127.0.0.1`/`::1`), so `localhost.attacker.example` is rejected at `normalizeBaseUrl` and never reaches `validateProviderEndpoint`. Resolving the host instead would let such a name *into* the allowlist, so the literal match is kept and a regression test now pins the rejection. Also covers the remaining functional gaps: `WeixinLoginClient.verify(...)` (blank code, code in the status query, the verification challenge, untrusted redirect, provider failures) and the outbound `notifyStart`/`notifyStop` endpoints. Module line coverage is 93%.
Follow-up to the second review round on agentscope-ai#3184. **Correctness — the eviction rule dropped the cursor.** `prune` forgot any account whose lease had expired, cursor included, so an account idle for longer than the lease TTL would have resumed from `""` and replayed the provider backlog. The automatic sweep now only forgets accounts that hold nothing at all (no cursor, no peer context, no inbox, no live lease) — the ids materialised by validation paths that the previous commit stopped creating in the first place. Retiring real state is now an explicit host action: `WeixinStateStore.removeAccount(accountId)` (default no-op, implemented by the in-memory store), so a host can drop an account without the store guessing. `keepsTheCursorWhenAnAccountGoesIdle` replaces the assertion that used to demand the opposite. **Security — the login transport is query-string based.** The provider protocol puts `qrcode` and `verify_code` in the GET status query, so the status URL is a credential; that is now stated in the README with the operational consequence (no URI-level logging behind this client). The module was verified not to echo anything itself: provider failures report only the status code, and both JSON parse paths (login and outbound) now report line/column instead of Jackson's default message, which quotes the offending input — a partially parsed login response contains `bot_token`, and an inbox response contains message payloads and context tokens. Three tests assert that an HTTP failure, an unparseable login body and a transport failure carry neither the verify code, the QR id, nor body content. Not changed: `fromProperties` still logs WARN on every construction — that is the intent, and `create(...)` with an explicit store is the silent path.
…utcomes Addresses the CHANGES_REQUESTED review on agentscope-ai#3184. - **`longPollTimeoutMs` did nothing.** Every request used `requestTimeoutMs`, so a deadline shorter than the provider's long-poll window aborted every `getupdates` call. The client now derives the deadline per call: `longPollTimeoutMs` plus a short grace for `getupdates`, `requestTimeoutMs` for the control calls. `longPollSettingKeepsASlowGetUpdatesAlive` drives a 900 ms response with a 300 ms request timeout and asserts it still succeeds. - **A failed `notifystart` left the account polling.** The catch reported a transient failure and fell through into the poll loop with `providerStarted == false`, so with the lease still renewing the session was never started again and no `notifystop` was ever owed. The session now leaves the lease, so the outer loop backs off, reacquires and retries startup. `providerSessionStartIsRetriedAfterATransientFailure` fails `notifystart` twice and asserts the third attempt happens. This fix also exposed that `WeixinChannelLoopbackTest`'s fake provider had never implemented `notifystart` — it was returning 404 and the old code swallowed it — so the fixture now answers it. - **A missing provider outcome counted as success.** `asInt(0)` accepted `{}` or any 2xx body without `ret`/`errcode` as a delivered message, after which the inbox claim completed and the reply was dropped. Both `updates(...)` and `assertSuccess(...)` now require an explicit provider result. `responsesWithoutAProviderOutcomeAreRejected` covers poll and send. Provider outcome and credential failures also keep their exception type instead of being wrapped in a generic `RuntimeException` by `sendWithContext`. - **Byte-identical id-less messages collapsed into one.** The fallback inbox id was the payload digest alone, so two identical messages in one batch became one claim while the cursor advanced — a user typing the same thing twice lost one message. The fallback id now includes the position in the batch, which keeps a re-delivered batch deduplicated while separating genuine duplicates. `byteIdenticalMessagesWithoutIdsAreBothDispatched` covers it. Test-design fix along the way: the runtime fixture now emulates a long poll (25 ms) instead of answering instantly, which was spinning the consumer loop and starving the rest of the suite; the lease-lifecycle suite dropped from 17.8 s to 7.0 s.
The Windows build failed in an untouched module: JdkHttpTransportTest .testStreamErrorResponseWithEmptyBody timed out waiting for the stubbed server, so agentscope-core failed first and every later module was skipped — this module never got built. Re-running to get a clean signal.
WeixinChannelRuntimeTest.peerThrottleStopsDispatchAndSurfacesATransientFailure asserted the transient-failure notification after calling channel.stop(), which races the poll loop: a stop that lands between failMessage() and the loop's catch makes it skip the notification (correct shutdown behaviour), leaving the list empty and the assertion red. It failed on the agentscope-ai#3186 CI run on both runners. Wait for the report before stopping instead. Same coverage, no race.
The channel sent each reply inline, inside the dispatch of the inbound message, and the client required an explicit result code from every provider call. Both were wrong for iLink: an idle long poll and an accepted send report success through their own fields (`get_updates_buf`, `msgs`, `message_id`) with no `ret`, so every reply looked failed, the message returned to PENDING, and the Agent ran again — one user message produced eight Agent executions and eight replies. - Accept the provider's own way of reporting success: batch fields for `getupdates`, the message id for `sendmessage`. A body that carries neither the status fields nor those outcomes is still rejected, so a proxy error page cannot pass as a delivered reply. - Poll and consume fail independently, so a provider outage cannot starve an accepted message and a message that cannot be dispatched cannot freeze the cursor. - Bound dispatch retries and tombstone an abandoned message; report it through the new `onDispatchFailed` (a refused reply stays `onDeliveryFailed`). The Agent is never re-run for a reply the provider refused. - Implement `deliverWithReceipt(...)` so a host that owns durable delivery can send one persisted reply and receive the provider's receipt. The inline path, used by standalone hosts and by the courtesy replies that answer an unbound peer, makes a single attempt. - Report provider outcomes as structured codes; the provider's own error text never reaches a host log or reporter.
44b5b0f to
82b31cb
Compare
… channel extension
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of the push since my last approval on this branch (80cfaf37 → 82b31cb9, "settle reply delivery semantics in the channel extension"). Splitting "who owns retry" is the right call: the transport should not be doing backoff inside the dispatch budget, and it is what was letting a slow provider time the whole dispatch out and replay the Agent. Reporting the root cause instead of RetryExhaustedException, requiring a real provider receipt before calling a send delivered, and dropping errmsg from exception text are all improvements in the same direction, and the listener split (onDeliveryFailed vs onDispatchFailed) keeps a message-level failure from flipping the channel's runtime state.
Four findings inline, all on the new commit, and none of them is about the happy path:
- [Warning]
WeixinChannel.java:231— on the inline path a failed send is swallowed and the claim still completes, so in a standalone deployment (no host queue behind it) the reply disappears with no second attempt. - [Warning]
WeixinChannel.java:508— a credential rejection is charged toMAX_DISPATCH_ATTEMPTS, so a 30-second token expiry permanently tombstones the three messages that were in flight instead of letting them recover after re-authorization. - [Info]
WeixinChannel.java:77—dispatchFailureshas no eviction for messages that are never claimed here again, against the "bound in-memory state" goal of this PR. - [Info]
WeixinChannel.java:617—safeMessagedecides what is safe to report by exception type, butIllegalStateException/IllegalArgumentExceptionare also what the surrounding stack throws, androotCausewalks straight to them.
CLA status is not reported for this head (license/cla context absent) and CI (ubuntu + windows builds, license, module sync, codecov patch) is green, so I am leaving this as a comment rather than repeating an approval. Happy to re-review after the next push.
Automated review by github-manager-bot
Propagate inline send failures so a standalone consumer cannot complete an inbox claim before the provider accepts its reply. Managed gateways still return no reply after persisting delivery, and do not trigger an inline send. Standalone retries retain the documented at-least-once processing semantics. Release credential-rejected claims without charging their dispatch budget, so reauthorization can recover a message even after two ordinary failures. Scope retry counters to the lease session so a handover releases stale state. Report text only for module-owned operational exceptions; generic state and argument exceptions from a host or transport can contain secrets. Keep structured provider codes while excluding arbitrary third-party messages. Correct successful send fixtures to return actual provider receipts. Five regression assertions failed before the fix, including credential recovery after two failures and a lease handover; all now pass. The full Weixin suite passes: 71 tests, 1 opt-in live test skipped. Spotless and module sync pass.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review after the new head (4229159, previously reviewed at 82b31cb). The incremental commit addresses the delivery-semantics feedback well: inline reply failures now propagate (doOnError instead of swallow) so the inbox claim stays retryable, the retry counters moved from a process-wide ConcurrentHashMap to a per-LeaseSession HashMap (fresh budget per tenure, no stale keys across takeover), and credential rejections no longer consume the poison-message budget. The WeixinOperationException refactor also tightens secret exposure — safeMessage now reports third-party IllegalStateException/IllegalArgumentException by type only, which is the right call since their messages can embed provider data (nice regression coverage via the 'token=TEST-SECRET' assertion).
Findings
- Only two informational notes below (inline). No blockers in the incremental change itself.
Cross-repo / Merge note
This PR accumulated many external reviews after my previous COMMENT. I could not verify the resolution state of every human review thread from this run, and CI is still queued on the new head — treating this as a readiness COMMENT rather than an approval. Recommend a follow-up pass once the new-head CI finishes and existing threads are resolved.
Automated review by github-manager-bot
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of head 03d5f2aa (previous pass at 42291591). The increment is documentation-only — three added comment lines in WeixinChannel.java, no behavioral change — and the readiness condition I set last time (CI on the new head) is now satisfied: License, Module Sync, build (ubuntu-latest), build (windows-latest), codecov patch and license/cla are all green. I checked both new claims against the code rather than trusting them, and both are accurate. Holding at a readiness COMMENT: the only thing standing between this and my approval is acknowledgement or resolution of the two threads below, both of which I raised and both of which are informational rather than defects.
What I verified in the new comments
drainInbox()— "Called only by the consumer thread, which owns the session's dispatch retry counters." Confirmed. The only call site isWeixinChannel.java:359insideconsume(), which runs on the single-threadweixin-<accountId>executor built at lines 101-107, andLeaseSession.dispatchFailures(a plainHashMap, line 537) is mutated only at 502 / 511 / 513 — all insidedrainInbox(). The other threads that can reach a session are theweixin-lease-renewalheartbeat (renew()→loseLease()) and caller threads entering viadispatch(in)→dispatch(in, active); both touch onlysession.valid(volatile) andsession.cancelled, never the map. So the confinement the comment asserts is real, not aspirational — the javadoc is worth keeping as-is.- The credential-rejection note at 506-507 — confirmed.
stateStore.failMessage()'s boolean is deliberately discarded at 508 and the error is rethrown intoconsume()'scatch (WeixinCredentialRejectedException)at 374, which setsrunning = false; thefinallyat 377-400 then cancels the renewal, callsloseLease()andreleaseLease(). "Propagate either way soconsume()stops this consumer and releases its lease" matches the control flow, including the fenced-claim case.
Open items from my earlier passes (unchanged by this commit)
WeixinChannel.java:336— anotifystartfailure exitsconsume()before the polling loop ever escalates, sopoll()'scatchat 299-304 sleeps a flatbackoff(1000 ms) while the polling path doubles it tomaxBackoffMs. A provider down for an hour costs ~3600notifystartattempts and ~3600 warn lines. Either grow the backoff on this path or lift the escalation intopoll()so startup and polling share one policy.WeixinOutboundClient.java:131—timeoutFor()still derives the deadline from locallongPollTimeoutMs+LONG_POLL_GRACE_MSand never consults the advertisedlongpolling_timeout_ms, whichJsonNodeResponseparses (line 296) and drops. If the provider advertises a longer window than the local setting, the client still aborts the poll early — the failure mode the deadline commit set out to remove. Takingmax(local, advertised)once known, or logging when advertised exceeds configured, closes it.
Verdict
No new findings on the delta itself, and nothing in this head argues against merge from my side. Resolve or explicitly dismiss the two threads above and this is good to go from the bot's perspective.
Automated review by github-manager-bot
… channel extension
3 participants
AgentScope-Java Version
2.0.3-SNAPSHOT (main @ ca52405)
Description
Fixes #3183
Adds
agentscope-extensions-channel-weixin, a native JavaChannelextension for Tencent'sofficial iLink personal Weixin API, at the same scope and layout as the existing DingTalk,
Feishu, WeCom, GitHub, and GitLab extensions.
This is part 1 of the two-part split described in the issue. The extension is self-contained: it
carries no
agentscope-servicedependency and can be consumed by any Java host. The AgentScopeservice and console integration (control-plane connection and login flows, Scheduler adapters, and
the QR connection page) follows in a separate PR that depends on this module being merged.
What was added
WeixinChannel— the standardChannelimplementation for direct text conversations:long polling,
get_updates_bufcursor persistence,context_tokenreplies, and direct-textinbound mapping.
WeixinChannel.fromProperties(...)covers standalone use andWeixinChannel.create(...)takes host-supplied seams.WeixinLoginClient— stateless QR login (start/poll/verify), including thescaned_but_redirecthost switch. Each call returns a portableWeixinLoginSession, so anattempt can be resumed by another process without carrying the QR image along.
standby instances keep trying to acquire a released lease; a lease lost mid-dispatch cancels the
local subscription and suppresses the late reply instead of sending it. Inbound messages without
a provider
message_idare keyed by a payload digest so a repeated batch is still deduplicated.WeixinCredentialProvider,WeixinStateStore,WeixinRuntimeListener— the neutral seams amanaged host implements. The state contract covers batch acceptance, message claims, context
tokens, and lease fencing in one place.
InMemoryWeixinStateStoreis the reference implementation of that contract for standaloneuse and tests, and the README says plainly what it is not: cursor, peer context tokens and
leases live in one JVM, so a restart replays or drops messages and a second instance cannot
see the first.
WeixinChannel.fromProperties(...)warns on construction for that reason. Adurable store is the host's to implement; the module ships no Redis or JDBC adapter.
WeixinEndpointPolicy— the allowlist applied before credentials are sent: https (or loopbackhttp), official
*.weixin.qq.comhosts on port 443, no userinfo/query/fragment/path, andredirects only within the official provider, between loopback endpoints, or to the same host.
HttpServer.iLink
-14is surfaced as a credential-rejection observation rather than a product decision: thehost decides whether that means reauthorization, a notification, or something else. The module
carries no owner or vault references, no persistence policy, no public routes, and no console
state.
Because this drives a personal account rather than an enterprise bot, the README opens with a
Compliance and scope section: personal-account automation is governed by the provider's terms
and the operator's own policy, the account can be restricted by the provider, and the host owns
the decision to deploy it.
How to test
mvn -pl agentscope-extensions/agentscope-extensions-channel/agentscope-extensions-channel-weixin -am test— 50 tests, 1 skipped (
WeixinLiveSmokeTestis gated behindWEIXIN_LIVE_TEST=true).WeixinChannelLoopbackTest— round trip through a fake Gateway and an embedded iLink server:cursor,
context_token, reply routing, and recovery when cursor persistence is interrupted.WeixinChannelRuntimeTest— agent-initiateddeliver(...), delivery and polling credentialrejection, polling failure retry, messages without a provider id, peer throttling, context
fencing, listener isolation, and the stopped-channel guard.
WeixinLeaseLifecycleTest— lease takeover, renewal while a turn is running, and lease losscancelling dispatch.
WeixinStateStoreTest— claim and lease fencing, automatic expiry of completed tombstones,and eviction of accounts that went idle, plus the guarantee that read-only lookups do not
retain an account.
WeixinLoginClientTest— protocol headers, the redirect host switch, the verify-code step(blank code rejected, code carried in the status query,
need_verifycodesurfaced) andrejection of an untrusted
redirect_host.WeixinOutboundClientTest,WeixinChannelTest,WeixinEndpointPolicyTest— outbound-14mapping, the notify start/stop endpoints, secret redaction, and the endpoint allowlist
(12 cases).
WeixinChannelis at 92%. What remains uncovered ishostile-store error injection (a store that throws from
renewLease/isLeaseCurrent/releaseLease) and the unreachableNoSuchAlgorithmExceptionbranch of the digest helper.WeixinLiveSmokeTestcovers a real QR scan and two-way text through Tencent iLink. Credentialsnever touch disk.
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)