feat(service): run Personal Weixin iLink channels end to end

[DennisWLX]

Stacked on #3184 (agentscope-extensions-channel-weixin). Please review #3184 first — this branch sits on top of it, so the diff here also contains that module until #3184 merges, after which it collapses to the 54 files below.

What this adds

The AgentScope service side of the Personal Weixin (iLink) channel, in nine commits:

1. Session authorization by verified channel owner — channel conversations are registered by the control plane on behalf of an account, but session access was still derived from Chat ownership. WorkAccess now carries the authenticated account id, ChannelSessionOwnerRef exposes the account verified at registration, and the memory/Postgres session readers authorize that owner independently of later Chat ownership.

2. Control plane (aistio) — registers the personal-weixin channel type, persists connections and login challenges with a migration, and exposes the internal APIs the Scheduler needs: start/poll/verify headless QR login, and fetch runtime credentials for a managed channel.

3. Scheduler — JdbcWeixinStateStore (account leases, get_updates_buf cursors, inbox dedup, shared across replicas), WeixinControlPlaneCredentialProvider, WeixinLoginController, and managed-session bridge updates.

4. Console — scan-to-connect QR panel with login polling, plus an identity section that binds the signed-in account to the WeChat bot, with unit tests and a Playwright spec.

5. Docs — ADR 0001-personal-weixin-ilink-native-channel, a production integration plan, and a headless login runbook.

The channel extension itself (protocol, login primitives, leases, endpoint policy, neutral host seams) is PR #3184 and is not duplicated here.

6. Retirement and review fixes — JdbcWeixinStateStore.removeAccount (one transaction, one
account, other accounts untouched); the lease bootstrap insert moved inside the FOR UPDATE
transaction so a retirement cannot leave a null lease row; a Weixin channel whose start failed is
reported with started=false plus its reason instead of being dropped, and an unreadable
credentialRevision no longer aborts the whole status report (with the matching control-plane
change, since a lease-less item used to be discarded).

Testing

• mvn -pl agentscope-service/service-scheduler -am test — BUILD SUCCESS (scheduler 27 tests, weixin extension 60 tests)
• go test ./internal/product/... ./internal/httpapi/... ./internal/store/... in agentscope-service/aistio — ok
• console: npm test 146/146, tsc --noEmit clean, eslint 0 errors
• spotless:check clean

Not included on purpose

DeepSeek model wiring and local Docker/Maven-mirror build tweaks, which are unrelated to this channel.

Migration note

This branch changes the channel externalKey from originType|originRef|requestedSessionID to
plain originRef, so an environment that ran an earlier head of this branch will not find its
earlier channel sessions by the new key and will create fresh ones alongside them. The old rows stay
readable but are no longer resolved by find-or-create. Sessions written before channelOwnerRef
existed are self-healed on first resolution instead of failing with a conflict.

Deploy order

The internal registration endpoint (POST /api/internal/managed-sessions/find-or-create) now rejects
a request without externalKey instead of minting channel:<uuid> per call. A scheduler replica
that still omits the key therefore fails against the new control plane, so the control plane must
not be upgraded before every scheduler replica is on a build that always sends one. Take the
control plane last; nothing else in this change depends on the order.

The runtime-status fence in the control plane treats a report older than two minutes as stale, which
assumes the scheduler's report interval stays well below that (builder.scheduler.channel-refresh-ms,
default 15s). SchedulerChannelRuntime logs a warning when the interval is configured above a minute.

[codecov]

Codecov Report

❌ Patch coverage is 76.88044% with 292 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...scope/extensions/channel/weixin/WeixinChannel.java	80.23%	31 Missing and 35 partials ⚠️
...pe/builder/web/config/SchedulerChannelRuntime.java	57.14%	47 Missing and 16 partials ⚠️
...scope/builder/web/config/JdbcWeixinStateStore.java	80.66%	12 Missing and 17 partials ⚠️
...sions/channel/weixin/InMemoryWeixinStateStore.java	78.26%	4 Missing and 21 partials ⚠️
...e/extensions/channel/weixin/WeixinLoginClient.java	78.66%	7 Missing and 9 partials ⚠️
...xtensions/channel/weixin/WeixinOutboundClient.java	87.90%	7 Missing and 8 partials ⚠️
...ntscope/builder/web/managed/ChannelWorkBridge.java	65.71%	10 Missing and 2 partials ⚠️
...nsions/channel/weixin/WeixinChannelProperties.java	63.33%	3 Missing and 8 partials ⚠️
...xtensions/channel/weixin/WeixinEndpointPolicy.java	74.28%	0 Missing and 9 partials ⚠️
...extensions/channel/weixin/WeixinInboundMapper.java	74.28%	1 Missing and 8 partials ⚠️
... and 13 more

📢 Thoughts on this report? Let us know!

[oss-maintainer]

Summary

Service side of Personal Weixin (iLink) channels, stacked on #3184: Go control plane (connection/login-flow store with generation fencing, internal managed-session find-or-create, owner-scoped channel sessions via WorkAccess/ChannelSessionOwnerRef), Java scheduler (JdbcWeixinStateStore lease/cursor CAS store, control-plane credential provider, headless login endpoints, runtime status reporting), console QR panel and docs.

Verdict: COMMENT — no blocking defect found in the reviewed scope. The four risk areas held up: session reads are owner-scoped across canAccessSession/memory reader/Postgres filter with an owner-checked internal registration; no credential material in logs or public errors (toString overridden to [redacted]); lease + inbox + cursor mutations are SELECT ... FOR UPDATE-serialized in one transaction with claim CAS, so no cross-replica lost update; login endpoints validate input and sit behind the internal token. Two non-blocking robustness notes are pinned inline.

Caveat: at +8981/-121 across 86 files this review was bounded to the service-side Java and the Go control-plane/authz paths — the extension module (covered in #3184), console/TSX, docs and SQL migrations were not reviewed line by line, and nothing was built or exercised against a live PostgreSQL. Please re-check the unreviewed areas on the next push, and note that this PR inherits the unresolved critical from #3184.

Strengths

• Consistent fencing: lease generation + operation_id, DB-clock expiry, claim/complete CAS, single-transaction cursor + inbox writes.
• Clean secret hygiene in logs and error mapping, with tests asserting secrets never reach public responses.
• Strict channel-session reuse check (binding, origin type/ref and owner must all match) closes the obvious cross-binding reuse hole.

---

Automated review by github-manager-bot

[oss-maintainer]

Unguarded credentialRevision parse can abort the whole runtime-status report. When a Weixin channel is built, the credentialRevision property is parsed inside a try/catch; here it is parsed unguarded:

item.put("credentialRevision", Long.parseLong(String.valueOf(entry.getValue().getProperties().get("credentialRevision"))));

If the property is missing or non-numeric (config drift, or a channel persisted before the revision was attached), String.valueOf(null) produces "null" and Long.parseLong throws NumberFormatException. That aborts reportRuntimeStatus for all channels, not just the offending one — and since stop() bumps observations and calls reportRuntimeStatus() synchronously, the failure can also propagate out of shutdown. Suggest reusing the guarded parse used at channel-build time (skip the field or the item on failure).

[oss-maintainer]

Weixin start failures are invisible to the control plane while no lease observation exists. This line skips Weixin entries lacking an observation with a lease (if (weixin && (observation == null || observation.lease() == null)) continue;), and started/error are taken from the observation when present. A Weixin channel that fails to build or start (credential fetch failure, bad revision, provider outage) therefore never produces any status item and its recorded error is never reported — the control plane keeps the previous state (e.g. stuck at STARTING) with no operator-visible failure to act on. Consider emitting a failed/stopped item (or the recorded build error) for skips caused by a failed start instead of continue.

[oss-maintainer]

Summary

Re-review after the push on top of the head I reviewed at 09:12Z. The new commit b7d56c3d5b implements the state-retirement contract: JdbcWeixinStateStore.removeAccount deletes cursor / per-peer context / inbox / lease rows for one account in a single transaction, InMemoryWeixinStateStore keeps accounts that hold real state, and the production plan documents why the Scheduler must never evict on its own (a reversible disable is indistinguishable from a disconnect in the configuration feed, so automatic eviction would duplicate deliveries). That reasoning is sound and the docs now record it, which is the part most likely to be lost later.

Verdict: COMMENT — CI is red on this head, so the PR is not mergeable regardless of review state.

• build (ubuntu-latest) and build (windows-latest) both fail in agentscope-extensions-channel-weixin: WeixinChannelRuntimeTest.peerThrottleStopsDispatchAndSurfacesATransientFailure:208 throttling must surface a transient failure ==> expected: <false> but was: <true> (Tests run: 56, Failures: 1). Check License, Check Module Sync, validate and source pass, and CLA is signed. That test lives in the #3184 extension module, so the break is most likely an interaction between this branch and the sibling PR rather than the Scheduler code added here.
• One new blocking-adjacent defect in the retirement path itself, pinned inline: deleting the builder_weixin_lease row makes a previously unreachable null dereference in acquireLease reachable.
• Both findings from my 09:12 review are still open on this head, and I have re-pinned them to the correct lines: the previous round's comments were anchored at 430/437 of SchedulerChannelRuntime.java, which is the managedWeixinRuntimeListener block, not the code the text describes. The findings were right; the anchors were wrong. My mistake, corrected here rather than left to point at unrelated lines.

Strengths

• Retirement is correctly modelled as an explicit, one-way host action, and the reason is written down with the mechanism rather than just asserted — the disable-vs-disconnect ambiguity is the kind of thing that gets "optimized" into an automatic eviction by the next person to read the code.
• The new removeAccount tests check the durable effect (a freshly constructed store sees no cursor, no context token, no current lease, and zero rows in all four tables) instead of only the in-memory view, and they pin that a second account is untouched — which is the isolation property that actually matters here.
• Blank/null account id rejection is asserted rather than assumed.

Scope

adc7ca8eb6 -> b7d56c3d5b is 7 commits / 61 files, but only JdbcWeixinStateStore (+17), its test (+41) and the production-plan doc (+9) carry new Scheduler-side logic; the rest is already-reviewed #3186/#3184 content plus main merges. I read the retirement delta closely and re-checked the two previously reported spots in SchedulerChannelRuntime.java; the console/TSX, SQL migrations and extension module were not reviewed line by line, and nothing was built or run against a live PostgreSQL here.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] removeAccount opens a null-LeaseRow window in acquireLease, which is the one lease read in this class that is not null-guarded.

acquireLease performs its bootstrap insert ... on conflict do nothing through jdbc.update(...) as a standalone auto-commit statement, and only then opens the transaction that takes the row lock:

jdbc.update("insert into builder_weixin_lease ... on conflict do nothing", accountId);  // committed, outside the tx
return transactions.execute(status -> {
    LeaseRow row = lock(accountId);              // rows.isEmpty() -> null
    long timestamp = now();
    if (row.expiresAt > timestamp && ...)        // NullPointerException

Until this commit no code path ever deleted a builder_weixin_lease row (retirement only touched expires_at = 0), so a null lock() result inside acquireLease was unreachable and the unguarded dereference was harmless. Deleting the lease row makes it reachable: a retirement that commits between the bootstrap insert and the SELECT ... FOR UPDATE leaves row == null and the caller gets an NPE out of lease acquisition instead of the Optional.empty() backoff the rest of the class deliberately returns.

Note that withLease already handles exactly this correctly (if (row == null || !row.matches(lease) || row.expiresAt <= timestamp) return rejected;), so a consumer that loses its lease to a retirement is fenced safely — acquireLease is the single gap.

Either arm is sufficient, and both are small:

• null-guard row in acquireLease and return Optional.empty() (the next poll re-bootstraps and converges), or
• move the bootstrap insert inside the same transaction as the FOR UPDATE, so the insert and the lock cannot be split by a concurrent retirement.

The two new tests cover the sequential contract well; a retirement racing a start would pin whichever choice you make.

[DennisWLX]

Fixed in 1a4c775 by taking your structural option: the bootstrap insert now runs inside the same transaction as the SELECT ... FOR UPDATE. A concurrent retirement cannot delete a row this transaction has not committed yet, so lock() always has a row and the window is closed by construction rather than by a check. ON CONFLICT still keeps a concurrent first acquisition from aborting the transaction.

I kept the null guard as well (returns Optional.empty(), so the next poll re-bootstraps and converges). It costs nothing and a future refactor that moves the insert back out would otherwise reintroduce the NPE.

On the race test you suggested: I did not add one, and I want to be explicit rather than quietly skip it. A genuine interleaving needs two connections with a real concurrent commit; the suite's H2 mode serializes that, so a test would pass with or without the fix while claiming to pin it. The commit message records that instead of the test. If you want it pinned, running WEIXIN_TEST_JDBC_URL against a real PostgreSQL with two connections would do it and I am happy to add it.

[oss-maintainer]

[Warning] Re-pin of a finding from my adc7ca8eb6 review, still open on this head — note the anchor below is the correction: my previous comment on this point was posted against lines 430/437 of this file, which is the managedWeixinRuntimeListener region, not this code. The finding text was right, the line anchor was not, so please read this as the authoritative location rather than a duplicate.

Long.parseLong(String.valueOf(properties.get("credentialRevision"))) is unguarded inside the per-channel loop of reportRuntimeStatus(). A missing or non-numeric property yields the literal string "null" from String.valueOf, so parseLong throws NumberFormatException and aborts the whole status report — every other channel's state stops reaching the control plane because of one bad config entry. The same unguarded parse also appears on the start path (line ~355), where it can fail channel construction.

Parsing per entry with a fallback (skip the entry, or report a sanitized error code) keeps single-config breakage local, which is the same isolation the weixin && observation == null -> continue guard a few lines above already provides.

[DennisWLX]

Fixed in b21e0a2. credentialRevision(Map) now reads the value leniently — Number directly, a trimmed numeric string via parseLong, otherwise null — the start path fails with credentialRevision is missing or not a number instead of a raw NumberFormatException, and a report-path entry with an unreadable revision is emitted as a failed entry rather than throwing out of the loop. One bad config entry can no longer stop every other channel's state from reaching the control plane.

Two things fell out of it. First, the reconcile loop was calling lastErrors.put(channelId, "failed to build channel") after buildChannel had already recorded the specific reason, so the report would have said only "failed to build channel"; it is putIfAbsent now and the specific text survives (asserted). Second, credentialRevisionIsReadLenientlyPerEntry covers the read directly, since after this fix the end-to-end path cannot produce an unreadable revision for a started channel.

[oss-maintainer]

[Warning] Re-pin of the second finding from adc7ca8eb6 (same anchor correction as the neighbouring comment).

A Weixin channel whose start failed never acquires a lease, so observation == null here and the entry is dropped from the report entirely. The control plane is then shown nothing for a channel that is actively failing, which is indistinguishable from a channel that was never configured — the one case where an operator most needs the signal.

The errors map this method already snapshots carries the code for exactly this case, and the non-Weixin branch below already uses it (observation != null ? observation.errorCode() : errors.get(id)). Emitting an entry with started=false plus that error for a Weixin channel with no observation would surface failed starts without weakening the guard's actual purpose, which is correctly preventing a standby from overwriting the active replica.

[DennisWLX]

Fixed in b21e0a2 — but it needed a control-plane change as well, and that is worth flagging because the Java-side report alone would have been invisible.

Java: a Weixin channel with no lease observation but a recorded error now reports {channelId, accountId, credentialRevision?, started: false, error}. A healthy standby has no recorded error, so it stays silent exactly as before and the guard keeps its purpose.

Go: applyChannelRuntimeObservation returned early for any Weixin item without a lease holder, so your suggested entry would have been discarded by the control plane. A lease-less item carrying an error now writes channels.runtime_started/runtime_error and returns, leaving weixin_connections' lease fields to the replica that holds the lease; a lease-less item without an error is still ignored.

Tests on both sides: unreadableCredentialRevisionIsReportedInsteadOfAbortingTheReport (end-to-end, asserts started=false and that the specific reason survives) and TestWeixinRuntimeSurfacesAStartFailureWithoutLeaseAuthority, which additionally asserts the active replica's holder/generation/sequence are unchanged and that a lease-less report without an error is still ignored.

[DennisWLX]

Follow-up on the 17:02Z review (head b21e0a24). Three commits: the red CI, the lease-bootstrap window, and the two report-path findings.

The red CI was my test, not the Scheduler code. WeixinChannelRuntimeTest.peerThrottleStopsDispatchAndSurfacesATransientFailure asserted the transient-failure notification after calling channel.stop(). That races the poll loop: a stop landing between failMessage() and the loop's catch makes it skip the notification — correct shutdown behaviour — leaving the list empty. It happened to win the race on #3184's runs and lost it on both of this branch's runners. The test now waits for the report before stopping (2e2d9d49), same coverage. If you would rather have that fix on #3184 before the extension lands, it is a three-line change and I can push it there instead — I kept it here because that is where CI is red.

Lease bootstrap window (1a4c775e) — your structural option: the insert shares the transaction with the FOR UPDATE, so a retirement cannot delete an uncommitted row. Null guard kept as a safety net. I did not add a race test and said why in the commit: H2 serializes the interleaving, so the test would pass either way.

Report path (b21e0a24) — a lease-less Weixin channel with a recorded error is now reported with started=false plus the reason, an unreadable credentialRevision no longer aborts the loop, and the specific build reason is no longer overwritten by the generic "failed to build channel". This needed a Go change too: applyChannelRuntimeObservation discarded any Weixin item without a lease holder, so the Java-side entry would have been invisible; a lease-less item carrying an error now updates the channel's runtime error and leaves the connection's lease fields alone.

This head also rebases onto #3184's current head (80cfaf37), so it carries the four fixes from that PR's last round and its four new tests.

Verified locally: weixin extension 60 tests (1 live smoke skipped), service-scheduler 27 tests, internal/product Go tests, spotless clean. Waiting on CI.

[oss-maintainer]

Summary

Delta re-review of the 11 commits pushed since my previous review on b7d56c3d — the Personal Weixin end-to-end slice: channel-origin session registration in the Go control plane, per-channel runtime credential/lease state in internalChannelsConfig / internalChannelRuntimeReport, and the scheduler-side JDBC weixin state store. Direction looks right and the test additions are substantial (channel_session_test +207, JdbcWeixinStateStoreTest +253, SchedulerWeixinRuntimeTest +246, WeixinControlPlaneAdaptersTest +216). The issues below are all in the new ownership / runtime-state paths, which is exactly where a channel integration tends to hurt in production.

Findings

• [Warning] httpapi/agent_invocation_handler.go:222 — store.ChannelSessionOwnerRef(current) != agent.OwnerRef hard-fails with ErrConflict for any channel row written before channelOwnerRef existed, and this PR does not backfill it.
• [Warning] product/handlers_internal.go:978-993 — one weixinRuntimeProperties query per channel on the scheduler's config poll, and a single non-ErrNoRows lookup failure turns the whole config payload into a 500.
• [Warning] product/handlers_internal.go:1063 — the lease-less error report writes runtime_started=false with no generation/sequence fencing, so a standby that merely failed to build a channel can flap the active lease holder's healthy status.
• [Info] httpapi/channel_session.go:47 — an empty externalKey mints a fresh UUID, which creates a new session per call behind a long-lived internal token.
• [Info] store/postgres/sessions.go:144 — the channel branch matches access.User only while the sibling branches match access.Refs; worth confirming every construction site sets User, and the same for store/memory/access.go.

Verdict

COMMENT, not REQUEST_CHANGES: CI on this head (build (ubuntu-latest) / build (windows-latest)) is still running, so this is deliberately not a merge-readiness verdict, and the first finding is a data-migration question rather than a code-shape objection. The previous items I raised on b7d56c3d are not re-listed. The auth surface itself looks correctly gated — managedSessions.Use(s.internalTokenMiddleware()) is present in httpapi/server.go:409-411, and the OwnerRef/ManagedDefinitionRef cross-checks plus the RowsAffected()==0 CAS on the lease update are the right instincts; I would just close the migration and flap holes before this is exercised with real history. CLA is signed and mergeable=true. Merging stays with the maintainers.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] This check makes task_context.channelOwnerRef mandatory for every channel-origin session row, but nothing in the PR can produce it for rows written before this commit, and migrate.go only appends weixinMigrationSQL (no UPDATE sessions … anywhere in the diff).

store.ChannelSessionOwnerRef returns "" both when the key is absent and when session.AgentTaskID != nil, so "" != agent.OwnerRef is always true and the handler answers store.ErrConflict — a permanently stuck conversation rather than a recoverable one. Anyone who ran an earlier head of this branch (b7d56c3d already wrote payload = {"originType","originRef"} for the channel path) hits it, and the same row shape can come back any time the metadata write and the read disagree.

Either backfill in the migration, e.g.

UPDATE sessions SET task_context = jsonb_set(task_context::jsonb,'{channelOwnerRef}',to_jsonb(a.owner_ref))
  FROM agents a WHERE sessions.agent_id = a.id AND sessions.origin_type='channel'
    AND NULLIF(sessions.task_context->>'channelOwnerRef','') IS NULL;

or self-heal on read: when the stored ref is empty, treat the row as unclaimed, write agent.OwnerRef through the existing upsert and continue instead of failing. Self-heal is the safer option under a rolling deploy where the writer and the reader are not the same replica. Note also that this PR changes the channel externalKey from originType|originRef|requestedSessionID to plain originRef (line 183), so rows keyed the old way will not be found and new ones get created next to them — worth a line in the PR description about what happens to those.

[DennisWLX]

Fixed by self-heal in f502800, which is the arm you preferred for a rolling deploy.

ChannelSessionOwnerRef(current) == "" now means unclaimed rather than someone else's: the binding, origin type and origin ref have already been verified at that point, so the row is adopted for this agent and written back through the existing upsert with channelOwnerRef set. The copy carries the stored phase, busy and timestamps, so the metadata-only change does not disturb a live session; a stored owner belonging to a different account is still a conflict.

I did not add the SQL backfill, and the reason is schema rather than effort: sessions (with task_context/origin_type) is created by store/postgres/migrations/0001_init.up.sql, while agents is created by the control plane's product/migrate.go — the join would have to reach across the two schemas from the product migration set, and self-heal also covers the rolling-deploy window a backfill cannot.

Your note about the key change is right and I have added it to the PR description: externalKey went from originType|originRef|requestedSessionID to plain originRef, so rows written by an earlier head are not found by the new key and a fresh session is created next to them. That affects anyone who ran an earlier head of this branch; the old rows stay orphaned (they are still readable if addressed directly) and I have not attempted a merge of the two.

[oss-maintainer]

[Warning] Two things in this loop on the scheduler's config poll:

1. N+1 — one weixinRuntimeProperties query per weixin channel, on top of the full channel scan above. internalChannelsConfig is polled by every scheduler replica, so the cost grows with channel count × replica count.
2. all-or-nothing — the if err != nil { writeErr(500); return } turns a single channel's lookup failure (or a slow row) into a failed config response for every channel, which is worse than the behaviour before this commit: the scheduler then has no configuration at all rather than one missing entry.

Suggest batching the lookup (WHERE channel_id = ANY($1) over the weixin ids collected in the first pass) and, on per-channel error, delete(out, id) + slog/log a warning instead of failing the whole payload — the ErrNoRows branch already does exactly that, so the shape is here.

[DennisWLX]

Both fixed in 68b5ae0.

N+1: the per-channel four-join query is now one WHERE w.channel_id = ANY($1) batch returning the channel id alongside the properties, called once per config poll instead of once per weixin channel.

All-or-nothing: a channel whose connection is missing or unusable is dropped from the payload with a log line (log.Printf("weixin channel %s has no usable connection; omitted from channel config", id)) rather than failing the response, which is what the ErrNoRows branch already did. A genuine query failure (database down) still fails the payload — with one batched query there is no longer a per-channel failure to isolate, and returning a partial config because the database is unreachable would be worse.

internal/product and internal/httpapi tests pass.

[oss-maintainer]

[Warning] The lease-less branch skips the ordering guard that the leased branch is careful to add, so this write is unguarded: any replica (or a replayed/stale report) can stamp runtime_started=false plus an error string onto a channel whose lease is currently held by a healthy leader.

The leased path below protects exactly this with the runtime_lease_generation / runtime_report_sequence CAS, and internalChannelsConfig treats a stale runtime_updated_at as an operator-visible signal — so a standby that once failed to build a channel (config unreadable, MCP probe timeout, startup race) can make a running channel look broken in the console until the next successful report.

Options that keep the intent: require runtime_lease_holder IS NULL (or a lease older than the report interval) in the WHERE clause; or write only when runtime_updated_at IS NULL OR runtime_updated_at < $now so it cannot overwrite a fresher leased report; or land the failure on weixin_connections.last_build_error and let the console read the lease state separately.

[DennisWLX]

You are right, and this was my change from the previous round — the fence is now there in 68b5ae0.

A lease-less report only writes when no lease holder is recorded, or when the last accepted report is already stale (2 minutes — several scheduler poll intervals wide, since builder.scheduler.channel-refresh-ms defaults to 15s). The first arm is what stops a standby that failed to build from flapping a healthy leader; the second keeps a holder that died without being cleared (the holder is only reset on re-authorization, weixin_connection.go:83) from hiding a failed start indefinitely.

TestWeixinRuntimeSurfacesAStartFailureWithoutLeaseAuthority now covers both directions: the failure lands while the lease is unclaimed, then the active replica reports healthy, then a lease-less failure report is asserted to leave started=true and the leased fields alone.

[oss-maintainer]

[Info] This branch matches only on access.User, while the two branches above match on access.Refs ("legacy actor aliases", per the comment you added in store/access.go). That is defensible — channelOwnerRef stores the stable account id — but it means a caller whose identity only appears in Refs loses visibility of channel conversations that they could see before, and WorkAccess is constructed in more than one place.

Two small hardening ideas: (NULLIF(task_context->>'channelOwnerRef','') = $n OR NULLIF(task_context->>'channelOwnerRef','') = ANY($refs)), or keep the strict match and add a test that builds WorkAccess the way each real caller does (console JWT, legacy actor, internal bridge) so a future WorkAccess{Refs: …} without User cannot silently drop rows. Same note applies to the memory implementation in store/memory/access.go, which reads access.User only.

[DennisWLX]

Confirmed and fixed in 11e87f7 — both implementations now accept the stable id or an alias.

Worth recording what I found while checking it, because it narrows the impact: namespaceAccess (namespace_access.go:241) seeds Refs with the stable user id and appends the legacy aliases, so on the HTTP path Refs is a superset of User and the strict match was equivalent. The gap was in the other construction sites — WorkAccess is assembled directly in tests and internal paths (for example chat_handler_test.go:175, which sets only Refs) — where a channel conversation would have been invisible. The Postgres filter now matches ...=$user OR ...=ANY($refs) and the memory reader checks both.

I did not add a dedicated WorkAccess-construction test for the three caller shapes; if you want one as a guard against a future WorkAccess{Refs: …} without User, say so and I will add it to internal/store/memory where it can run without Postgres.

[oss-maintainer]

[Info] Minting a fresh "channel:" + uuid when externalKey is empty makes this endpoint create a new session row on every call for a caller that forgot the key — an unbounded growth path behind a long-lived internal token, and it silently defeats the WithSessionLock dedup you set up just below (each call has a distinct lock name).

The comment says it preserves the bridge's one-off overload; if the bridge always supplies a key now (the product handler builds the stable address key before calling), would a 400 externalKey is required be the safer contract, or at least a check that the caller is the bridge? If one-off sessions are genuinely wanted, a note on retention (who deletes them) would help whoever operates this.

[DennisWLX]

Fixed in a1eb68a along the lines you suggested — the endpoint now answers 400 externalKey is required and no longer mints anything.

On whether the bridge always supplies one: the production caller does. ChannelWorkBridge calls the four-argument overload with result.externalKey(), and that value is built by ChannelExternalKeys.forInbound(...), which always returns a non-blank key (the channel id falls back to "default"). The only path that relied on minting was the deprecated three-argument dispatchAndAwaitReply, which forwarded a null key. It now fails fast with a message pointing at the four-argument overload, rather than leaning on server-side minting.

TestChannelSessionRequiresAnExternalKey asserts the 400.

[DennisWLX]

All five findings closed on head 11e87f78.

Finding	Fix
agent_invocation_handler.go:222 — pre-existing rows fail with ErrConflict forever	self-heal: an empty stored owner means unclaimed, the row is adopted through the upsert and the turn proceeds (f502800f)
handlers_internal.go:978-993 — N+1 and all-or-nothing config lookup	one ANY($1) batch; a channel with no usable connection is dropped with a log line instead of 500-ing the payload (68b5ae0b)
handlers_internal.go:1063 — lease-less report can flap a healthy leader	fenced on "no lease holder recorded" or "last accepted report stale" (68b5ae0b)
channel_session.go:47 — empty key mints a session per call	400 externalKey is required; the deprecated bridge overload fails fast instead of relying on minting (a1eb68a2)
store/postgres/sessions.go:144 — User only while siblings use Refs	both readers accept the stable id or an alias (11e87f78)

Two notes on what I did not do:

• No SQL backfill. sessions comes from store/postgres/migrations/0001_init.up.sql and agents from the control plane's product/migrate.go; the join crosses schemas, and self-heal (your preferred option) also covers the rolling-deploy window. Accepting that as a trade rather than a gap — say the word if you want the migration anyway.
• No dedicated WorkAccess-shape test. namespaceAccess seeds Refs with the stable id, so the production path was equivalent and the gap was in other construction sites; the widening closes it. Happy to add a guard test in internal/store/memory if you would like one that runs without Postgres.

Verified locally: go test ./internal/product/... ./internal/httpapi/... ./internal/store/... all ok (including the rewritten fencing test and the new TestChannelSessionRequiresAnExternalKey), ManagedSessionChannelBridgeTest + ChannelWorkBridgeTest pass, spotless:apply applied. The PR description now records what happens to rows keyed by the old externalKey, as you asked.

[oss-maintainer]

Summary

Delta re-review of the four commits since b21e0a24. All five findings from the previous round are addressed, and not cosmetically: the lease-less failure report is now fenced the same way the leased path is, the per-channel config lookup is a single = ANY($1) batch with per-channel degradation instead of an all-or-nothing 500, pre-channelOwnerRef rows self-heal rather than dead-ending in ErrConflict, the accidental channel:<uuid> minting is gone, and both session readers now accept stable ids and aliases identically. Three of the four carry a test for the new arm, which is the standard this PR has been holding throughout.

The three items below are about the same hazard the new commits are fixing — upgrade skew and constants that assume a default — rather than about the shape of the code.

Findings

• [Warning] httpapi/channel_session.go:50 — the new 400 for a missing externalKey breaks a lagging scheduler replica on the control plane, which is the inverse of the self-healing f502800f deliberately adds. Pick a transition window or state the deploy order.
• [Warning] product/handlers_internal.go:1090 — channelRuntimeStaleReportMs is a fixed 2 minutes measured against a configurable poll interval; past that ratio the fence reopens and standby build failures flap a healthy leader again.
• [Warning] httpapi/agent_invocation_handler.go:269 — the adoption path rebuilds task_context from three keys, and the upsert replaces that column, so a legacy row carrying extra keys loses them on the first turn after the upgrade.

What I checked and did not flag

• store/postgres/sessions.go and store/memory/access.go now match on access.User or access.Refs, and the fmt.Sprintf placeholder numbering for the new = ANY($n::text[]) arm lines up with the two appended args — correct as written.
• The fence's NOT EXISTS on weixin_connections.runtime_lease_holder plus the runtime_updated_at IS NULL arm is the right guard given both paths open with SELECT … FROM channels … FOR UPDATE; I did not find an interleaving where a claim that commits before the fencing statement is missed.
• weixinRuntimeProperties keeps rows.Err(), closes the rows, and short-circuits on an empty id list; dropping a channel with no usable connection matches what ErrNoRows did before.

Verdict

COMMENT. CI is green on this head and license/cla is signed, so the only thing between this and an approval is whether you want the three items above as code or as a recorded deploy precondition — the second one in particular looks like a one-line change. Per the repo rules I am not merging or approving on your behalf, and nothing here claims a local build or test run: I read the head blobs, the four commit patches and the schema/upsert SQL, and the CI statements above are GitHub's own results.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] Closing the accidental path is right, but this is a rolling-deploy break in the opposite direction from the one f502800f just fixed.

f502800f was careful to self-heal rows written by an older head or a replica still running old code, on the explicit reasoning that a control-plane upgrade must not strand a lagging scheduler. This endpoint now answers 400 to exactly that lagging replica: a service-scheduler still building the request without externalKey did work before (server-minted key) and fails permanently afterwards. ManagedSessionChannelBridge.java:101 no longer protects against it, because the failure mode that matters is an already-deployed older replica, not the deprecated overload in the tree.

Two ways to keep both halves consistent:

• accept a missing key and mint one for a transition window, with a WARN naming the caller (and a follow-up to remove it), so the flag becomes observable rather than fatal; or
• state the deploy order in the commit message and in the release note — control plane only after every scheduler replica is on a build that always sends externalKey.

The unbounded-growth reasoning itself is sound, and TestChannelSessionRequiresAnExternalKey is the right test to have added; this is about which side of the upgrade the 400 lands on.

[DennisWLX]

Fair point, and I took your second option rather than the transition window — with one addition your note prompted.

Why not mint-for-a-window: this endpoint is new in this change. channel_session.go and the bridge's find-or-create call both arrive in this PR, so the skew is limited to environments that ran an earlier build of this branch — there is no previously released control plane serving this route for a lagging replica to be talking to. Reopening the minted path for that audience would restore exactly the unbounded growth the previous round asked to close, and would then need a removal follow-up of its own.

So the deploy order is stated next to the 400 and in the PR description as a release note: the control plane must not take this build before every scheduler replica sends a key.

One thing your comment made me check and fix: findOrCreateSession still omitted the field when a caller passed a blank key, so an in-tree caller could still produce the 400 after a round trip. It now throws locally instead. That makes the skew strictly about builds that predate this change rather than about the code in the tree.

[oss-maintainer]

[Warning] The 2-minute staleness bound is hardcoded against a poll interval that is configurable.

The comment reads "several scheduler poll intervals wide, which keeps a holder that died without being cleared from hiding a failed start forever", and at the default 15s interval that is exactly right — eight intervals of grace, and the healthy leader refreshes runtime_updated_at continuously so a lease-less report cannot flap it.

The bound only holds while interval << channelRuntimeStaleReportMs. A deployment that lengthens the report interval past two minutes (which the config allows, and which a busy or throttled replica can effectively reach on its own) turns every healthy leader into a stale row from the fence's point of view, and standby build failures start overwriting runtime_started=true again — the failure this commit was written to close, returning silently.

Cheap options, in increasing order of effort: derive the bound from the configured interval (e.g. 8 * interval, floored at the current constant); clamp it at startup with a log line when interval exceeds a fraction of the bound; or add the interval to the existing comment as a hard precondition rather than a default assumption. Same for weixin_runtime_test.go — it exercises the two arms at the default interval only.

[DennisWLX]

Fixed in 3fb5c9e by taking your middle option, plus the documentation.

The precondition is now on the constant itself: the default 15s interval is one eighth of the two-minute bound, the control plane cannot observe the configured value, and a deployment that raises builder.scheduler.channel-refresh-ms into that range would make every healthy leader look stale. SchedulerChannelRuntime additionally warns once at startup when the interval is configured above a minute — a tenth of the bound — naming both numbers in the message, so the coupling is visible at deploy time instead of showing up as a flapping console.

I did not derive the bound from the interval: the control plane cannot see it without adding it to the report protocol, and a per-request interval on a lease-less report is exactly the field a failing replica is least likely to send correctly. Happy to add the protocol field if you would rather have the derivation.

The test still exercises the arms at the default interval; asserting the other end of the ratio would need a second scheduler configured with a long interval, which the fixture does not have.

[oss-maintainer]

[Warning] Adoption rewrites the whole task_context column, so it can drop keys this helper does not know about.

The commit message and the comment here both say the upsert "keeps phase, busy and timestamps as stored", which is true — but sessions.go:87 applies task_context = COALESCE(EXCLUDED.task_context, sessions.task_context), so the payload this line builds replaces the stored JSON wholesale rather than adding one field. For the three keys the row was created with that is a no-op; for a legacy row that also carried anything else under task_context, adoption on the first turn after the upgrade deletes it, and the adoption path is exactly the one that runs on rows written by older code.

Since this is a single-field fix-up on a row already loaded under a List(... Limit: 2) probe, the narrower write is both safer and cheaper to reason about — either merge into the decoded payload before re-marshalling, or a targeted UPDATE ... SET task_context = jsonb_set(task_context, '{channelOwnerRef}', …) on the row key. Worth five lines: the whole point of the commit is that these rows were written by code this branch no longer has.

[DennisWLX]

Fixed in 6b6f2e9, and you are right that my comment described the wrong mechanism: the upsert replaces the column, and the helper rebuilt it from the three keys this branch knows about — on exactly the rows an older writer produced.

The stored payload is now decoded, channelOwnerRef is set on the decoded map and the result is re-marshalled, so unknown and nested keys survive; an unreadable payload falls back to a payload carrying just the owner rather than failing the turn. TestWithChannelOwnerPreservesExistingMetadata pins that a custom key and a nested conversationTurn object both survive, and that the stored session object is not mutated by adoption.

I took the merge rather than a targeted jsonb_set update because the row is already loaded and the merge needs no new store method — five lines, as you said.

[DennisWLX]

All three closed on head 57f1fdec. All were about upgrade skew or a constant that assumes a default, which is the right thing to be suspicious of in this PR.

Finding	Disposition
agent_invocation_handler.go:269 — adoption rebuilds task_context and drops unknown keys	merge instead of rebuild: decode the stored payload, set channelOwnerRef, re-marshal; a jsonb_set update was the alternative, the merge needs no new store method (6b6f2e9d, with a test that pins a custom key and a nested object surviving)
handlers_internal.go:1090 — the fence's 2-minute bound is hardcoded against a configurable interval	precondition written on the constant, plus a startup warning in SchedulerChannelRuntime when the interval is set above a minute — a tenth of the bound — naming both numbers (3fb5c9ed)
channel_session.go:50 — the 400 strands a lagging replica	deploy order recorded next to the 400 and in the PR description, per your second option; the endpoint is new in this change, so the skew is limited to environments that ran an earlier build of this branch (57f1fdec)

The third one also turned up a real inconsistency while I was checking it: findOrCreateSession was still omitting the field for a blank key, so an in-tree caller could produce the 400 after a round trip. It now throws locally, which keeps the skew strictly about builds that predate this change.

Two deliberate choices worth recording, both explained inline:

1. No interval-derived bound. The control plane cannot observe the scheduler's interval without adding it to the report protocol, and a per-request interval on a lease-less report is the field a failing replica is least likely to get right. Documentation plus an operator-visible warning instead — happy to add the protocol field if you prefer the derivation.
2. No transition window for the missing key. The minted path is precisely the growth the previous round asked to close, and this route is new in the same change, so a window would buy nothing for released deployments and would need its own removal follow-up.

Verified locally: go test ./internal/product/... ./internal/httpapi/... ./internal/store/... ok, ManagedSessionChannelBridgeTest + ChannelWorkBridgeTest + SchedulerWeixinRuntimeTest pass, spotless:apply applied. CI is running on 57f1fdec.

[DennisWLX]

Note on head 4b9ab20f: it is an empty commit, pushed only to re-trigger CI.

The build (ubuntu-latest) job failed in service-dataplane, in a test this branch does not touch:

ToolConfirmationCoordinatorTest.replacementTurnLeaseCannotReleaseOldTicketAndMayReuseToolUseId:452
CannotStubVoidMethodWithReturnValue: 'appendSessionEvent' is a *void method* and it cannot be
stubbed with a *return value*!

Two reasons I read this as a flake rather than a defect:

1. The line the failure is attributed to stubs managedExecutionScope, not appendSessionEvent — the message names a different method than the code at the reported line. That is the signature of the shared controlPlaneClient mock being invoked from the coordinator's worker thread (the appendSessionEvent void calls happen inside resolveManaged/cancelSession paths) while the test thread configures the next stub: Mockito records the void call and blames that instead of the stub.
2. The test is upstream's, from fix(core): emit tool result events for user-denied HITL calls #3104, and it passes 10/10 locally (mvn -pl agentscope-service/service-dataplane -am test -Dtest=ToolConfirmationCoordinatorTest). CI merges this branch with main, where that test comes from; this branch does not modify service-dataplane at all.

The windows job was cancelled by the matrix fail-fast rather than by its own failure. If it reproduces, the fix belongs to that test (stub before any worker thread can call the mock, or make the mock thread-safe), not to this PR — happy to file it separately if you want the record.

[oss-maintainer]

Summary

Delta re-review of the 3 commits since 11e87f78 (6b6f2e9d, 57f1fdec, 3fb5c9ed + a CI retrigger). All three previous-round findings are addressed substantively: the adoption path now merges into the stored task_context instead of rebuilding it, the deploy order for the required externalKey is documented on both sides of the wire (control-plane comment + scheduler-side fail-fast before the round trip), and the staleness fence's interval precondition is now backed by a startup warning on the scheduler. The merge fix introduces one new edge case worth a follow-up commit — a null-valued task_context panics the adoption path. No blocker otherwise; re-request review once that's in.

CI: the current 4b9ab20f head is a retrigger after a service-dataplane mock race, not a code change.

---

Automated review by github-manager-bot

[oss-maintainer]

[Critical] json.Unmarshal into a *map[string]any follows Go's decoder rules for null: a literal null in task_context unmarshals without error and sets the destination map to nil. The very next line then does metadata["channelOwnerRef"] = ownerRef — assignment to a nil map panics. Adoption is exactly the path that touches legacy rows, and a pre-channelOwnerRef row written by an INSERT ... task_context = NULL-style path (or any writer that persisted SQL NULL into the column, which the read layer surfaces as null) lands right here, so the fallback the comment promises ("an unreadable payload is not worth failing the turn over") turns into a panic in the gin handler instead. One-line guard closes it:

if metadata == nil {
    metadata = map[string]any{}
}
metadata["channelOwnerRef"] = ownerRef

(or var metadata map[string]any + if metadata == nil { metadata = map[string]any{} } after the unmarshal). For what it's worth, every other malformed shape — 123, "str", [1,2] — does return an UnmarshalTypeError and leaves the initialized map untouched, so null is the single hole here; a json.RawMessage("null") case in TestWithChannelOwnerPreservesExistingMetadata would pin it down.

[DennisWLX]

Fixed in 3402344. You were exactly right, and I confirmed the decoder behaviour before touching it:

null         -> err=<nil> nil=true        <- succeeds, destination map becomes nil
{}           -> err=<nil> nil=false
123          -> err=json: cannot unmarshal number into Go value of type map[string]interface {}
"str"        -> err=json: cannot unmarshal string ...
{not json    -> err=invalid character 'n' looking for beginning of object key string

So null is the single shape that reaches the assignment with a nil map — and, as you say, adoption is the path that only ever runs on rows written by older code, which is where an unusual payload is most likely. The guard is in place and TestWithChannelOwnerSurvivesAnUnusablePayload covers null plus the four shapes that error out.

I also checked the test is worth having rather than just describing the fix: with the guard removed, the null subtest fails with panic: assignment to entry in nil map, and with it restored the suite passes. go test ./internal/httpapi/... ./internal/product/... is green locally; CI is running on the new head.

[oss-maintainer]

[Info] Deploy-order note accepted — it answers the previous round's constraint, and pairing it with the scheduler-side fail-fast in ManagedSessionChannelBridge.findOrCreateSession (reject before the round trip) closes the skew cleanly. No action needed.

[DennisWLX]

Thanks — recording the order in the code comment plus the scheduler-side fail-fast is what I kept, so nothing further here.

[DennisWLX]

The null panic is fixed in 34023440, so this should be the last of it.

json.Unmarshal is documented to treat a JSON null as "set the destination to its zero value", and for a map that means success plus a nil map — I verified it directly rather than reasoning about it:

null      -> err=<nil> nil=true
{}        -> err=<nil> nil=false
123       -> err=json: cannot unmarshal number into Go value of type map[string]interface {}
"str"     -> err=json: cannot unmarshal string ...
{not json -> err=invalid character 'n' looking for beginning of object key string

So null was the only shape that reached the owner assignment with a nil map. The guard is in and TestWithChannelOwnerSurvivesAnUnusablePayload covers null plus the four shapes that fail to decode; with the guard removed the null subtest fails with panic: assignment to entry in nil map, so the test pins the bug rather than the fix.

CI is green on 34023440 (ubuntu, windows, validate, source, license, module sync, codecov, CLA). Re-review requested.

[oss-maintainer]

Summary

Delta re-review of 34023440 (the only commit since my 4b9ab20f round). It closes the last open finding: withChannelOwner now re-initialises the metadata map when task_context is a literal null, which json.Unmarshal decodes without an error and previously turned the metadata["channelOwnerRef"] assignment into a nil-map panic on the adoption path. The added table test covers the right set of shapes (null, number, string, array, truncated object) and asserts the owner key survives each one, so the guard is pinned rather than incidental. That was the only blocker; CI is green on this head and CLA is signed → approving.

Findings

None new in this delta.

• [Info] The fix is narrower than the general "unwriter payload" case, which is fine: the other malformed shapes error out and leave the initialised map alone, so they are already safe by construction. Worth keeping that reasoning in the comment (it is there) so a future edit doesn't move the nil check back inside the if err != nil branch.

Verification

• withChannelOwner at head 34023440: nil-map guard placed after json.Unmarshal, before the channelOwnerRef write.
• New test TestWithChannelOwnerSurvivesAnUnusablePayload exercises 5 payload shapes, including the adopted-session round trip.
• CI on 34023440: Check License, build (ubuntu + windows), source, validate, codecov/patch all passing.
• CLA: signed (license/cla=success).

---

Automated review by github-manager-bot

[oss-maintainer]

Summary

Re-review of the new push since my last approval (34023440 → 25a02852): reply-delivery semantics in the Weixin extension, replies moved onto the host's durable queue, DeepSeek packaged in the data plane, and build/registry hardening. The ADR argument holds — a provider outage should not fail the inbound message and re-run the Agent that produced the reply — and putting the retry/backoff/attempt-cap policy in the queue instead of the transport is the right place for it, with the extension left owning only the provider protocol. abandonMessage giving the claim back on a fenced update, and the idempotent-enqueue test, both show the crash paths were thought through.

Six findings inline, all on the new commits. Two are worth resolving before merge, and both are about who is trusted to decide:

• [Warning] channel_work_delivery.go:93 — whether a row skips the cfg.Enabled / route-gate checks is decided by a prefix of event_key, which the enqueue endpoint takes from the request body unchanged. Derive the key server-side or branch on a stored kind instead.
• [Warning] channel_work_delivery.go:201 — deferSeconds gives the attempt back with no count or age bound, so any error reported with a defer keeps a notification pending forever and never reaches the terminal failed state.
• [Warning] ChannelWorkBridge.java:138 — a failed enqueue still propagates, so a control-plane blip re-runs the Agent turn (the replay class this commit removes for provider failures), and after 3 tries the reply is abandoned.
• [Info] reply latency (one delivery per poll tick), the random-UUID event-key fallback, and the lockfile sed in Dockerfile.control.

CLA is signed; CI (ubuntu + windows builds, license, module sync, codecov patch) is green. Leaving this as a comment rather than an approval because of the two gate/queue findings above — they are local changes, and I am happy to re-review on the next push.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] The reply bypass is decided from a caller-supplied string. event_key comes from the POST /api/internal/channels/replies body verbatim (channel_work_api.go only checks it is non-blank), and enqueueChannelDelivery stores it unchanged. So the caller — not the queue — decides whether cfg.Enabled, the route/window checks and the transient-settings-error handling apply to a row, and a loadChannelWorkSettings failure is deliberately swallowed for anything whose key starts with channel-reply:.

Today's only caller is the scheduler, so nothing is exploitable in-process, but the invariant is held by convention. Deriving the key server-side ("channel-reply:" + ch.ChannelID + ":" + accountID + ":" + messageID, ignoring the client's value) or persisting an explicit kind column and branching on that would make the gate un-forgeable.

[oss-maintainer]

[Warning] A failed enqueue still replays the Agent, which is the failure class this commit set out to remove. enqueueReply is one POST with a 10s timeout and no retry: if the control plane is briefly unreachable or returns 503 (delivery queue unavailable), the error propagates out of receive(...), the provider message is failed, and the next claim re-runs chat.dispatchAndAwaitReply(...) — the model turn and any tool side effects happen again. After 3 such attempts WeixinChannel abandons the inbox row, so the reply is lost anyway.

The queue is idempotent per event key, so a couple of bounded retries on the enqueue itself (or deferring the claim instead of failing it) would keep the "a provider outage cannot re-run the Agent" guarantee true for control-plane outages too.

[oss-maintainer]

[Info] Replies now share the queue's one-per-poll throughput. deliverPending claims a single delivery per tick (LIMIT 1 on the Go side) with fixedDelay defaulting to 1000ms, and the inline reply path used to send immediately after the Agent finished. A chat that produces several replies in a burst will therefore drain at roughly one message per second regardless of how fast the provider responds, which is a visible latency change for interactive conversations.

If that is acceptable for now, worth a note in the ADR; otherwise claiming a small batch per tick would keep the durable semantics without the per-message serialization.

[oss-maintainer]

[Info] The blank-message-id fallback defeats the dedupe it is guarding. key + ":" + UUID.randomUUID() yields a fresh key on every attempt, so a redelivery of the same inbound message queues a second copy of the reply — the comment above describes the risk of a non-unique key as "silently deduplicate a genuine reply", but the chosen fallback trades that for duplicating it.

A deterministic stand-in (hash of channel + account + peer + text, or the inbound claim id if one is reachable here) keeps at-most-one-reply in the abnormal path as well. Separately, :-joining raw ids means an id that contains : can collide across accounts; an encoded or length-prefixed join would remove that class of surprise.

[oss-maintainer]

[Warning] Deferral has no bound, so a stuck notification never becomes terminal. The deferred branch resets attempts (GREATEST(attempts-1,0)) and pushes next_attempt out by up to 3600s, and nothing counts how many times that has happened. The scheduler asks for 300s on a credential rejection, which is the intended case, but deferSeconds is accepted for any error string, so one buggy or over-eager caller keeps a row pending forever — it never reaches failed, never surfaces in the "delivery failed" view, and the only signal is a debug log on the Java side.

Consider a deferred_count (or a total age cap) after which the row goes failed like everything else, and/or restricting deferral to a typed reason the queue recognizes.

[oss-maintainer]

[Info] Patching the lockfile inside the image layer hides the real problem. After the sed, npm ci verifies integrity hashes that were recorded for the internal mirror against tarballs fetched from npmjs.org, so the build breaks in a confusing way the moment the two registries disagree on a byte. It also means the console that ships is built from a dependency graph that no longer matches the checked-in lockfile — which is exactly what a lockfile is for.

Regenerating package-lock.json once against the public registry (or committing a second, public-resolved lockfile and selecting it by build arg) keeps external builds working and the artifact reproducible.