fix(daemon,onboard): include webhook in supervised and launchable channel checks#5799
Conversation
6db79f9 to
1288a5d
Compare
… checks has_supervised_channels() and has_launchable_channels() both called channels_except_webhook(), causing webhook-only configs to never start the channel supervisor and never show the launch prompt in the wizard. Change both callsites to channels(), which includes webhook with the same is_some() check used by every other channel. Fixes zeroclaw-labs#5798
1288a5d to
1abcb39
Compare
|
Confirm, it resolves #5798 |
WareWolf-MoonWall
left a comment
There was a problem hiding this comment.
Status: Approving. The fix is correct and the test discipline is excellent. Two conditional items need to be resolved before merge.
✅ The code change is right
channels() is defined as channels_except_webhook() + webhook appended. Using channels_except_webhook() in both has_supervised_channels() and has_launchable_channels() was therefore a logic error with an explicit name: the function says what it excludes. The one-character swap to channels() is the correct and complete fix at both callsites.
Bundling the wizard and daemon fixes is justified — same root cause, same predicate function, same fix — and the PR description explains the scope boundary clearly. The log message update ("No real-time channels" → "No channels") is accurate: the old phrasing encoded the assumption that webhook isn't "real-time," which is no longer how the predicate works.
After this merge, channels_except_webhook() will have no external callers — it will only be called from within channels() itself. That's a clean outcome. A non-blocking follow-up: consider marking it pub(crate) in zeroclaw-config to prevent a future contributor from reaching for it and re-introducing this exact class of bug. It's Beta tier so an API surface change needs its own deliberate commit, but the debt is worth labeling with a // TODO(debt): comment if not addressed immediately.
✅ TDD evidence is exactly right
Red → green with cargo test -p zeroclaw-runtime webhook_only is precisely the test discipline FND-006 §4.3 describes: "when you fix a bug, write a test that would have caught it." Both tests assert behaviour through the public predicate against a minimal webhook-only config — they will continue to pass if the implementation changes and break if the behaviour regresses. This is the pattern to repeat.
✅ Attribution and external confirmation
Naming @mn13's root-cause analysis in the PR body and asking them to test against their live reproduction before merge is exactly the ownership and collaboration the contribution culture RFC describes. The confirmation is on the record.
🟡 Conditional — apply labels before merge
gh pr view --json labels returns []. The Label Snapshot in the description names the right values (risk: low, size: XS, runtime, onboard, daemon:core, onboard:wizard, channel:webhook), but none of them are actually applied to the PR. The label taxonomy in FND-003 §9 and the Definition of Done expect them to be on the PR, not only in prose. Please apply them.
🟡 Conditional — five required template sections are absent
The PR description omits Privacy and Data Hygiene, i18n Follow-Through, Human Verification, Side Effects / Blast Radius, and Risks and Mitigations — all marked (required) in the template. The answers for this change are all trivially N/A or None, but the template is the project's Definition of Done checklist and skipping sections makes it harder to use as a review surface. Please add the missing sections with their N/A answers before merge.
After the rebase onto master, two WebhookConfig { ... } struct literals
added in fe3ec58 (zeroclaw-labs#5799) no longer compile without the three retry
fields introduced in dd93422. Both are test-only call-sites, so `None`
is correct for each.
has_supervised_channels 和 has_launchable_channels 之前都调 channels_except_webhook(),导致 webhook-only 配置: - daemon 启动时不拉起 channel supervisor - onboard wizard 不显示 launch 提示 两处都改成 channels(),和其它 channel 共享同一个 is_some() 判定。 Ports upstream fe3ec58 (zeroclaw-labs#5799) to master_wecom's pre-workspace-split layout. Co-authored-by: Shane Engelman <contact@shane.gg>
Summary
has_supervised_channels()incrates/zeroclaw-runtime/src/daemon/mod.rscalledchannels_except_webhook(), so a webhook-only config never started the channel supervisor — the HTTP listener never bound to its port.has_launchable_channels()incrates/zeroclaw-runtime/src/onboard/wizard.rshad the same bug: webhook-only configs never showed the "Launch channels now?" prompt after onboarding.channels_except_webhook()tochannels(), which includes webhook using the sameis_some()check every other channel uses.Label Snapshot
risk: lowsize: XSruntime,onboarddaemon:core,onboard:wizard,channel:webhookChange Metadata
bugruntimeLinked Issue
Validation Evidence
TDD approach: tests were written first and confirmed failing before the fix was applied, then confirmed passing after.
Attribution
@mn13's root cause analysis in #5798 was accurate and independently arrived at the same fix. The diagnosis (
channels_except_webhook()as the culprit,channels()as the fix) matches exactly.Note to @mn13
We do not run the webhook channel in our own setup, so end-to-end verification on a live webhook config falls to you. Please test this branch against your reproduction case from #5798 and confirm the
Connection refusedbehavior is resolved before this merges.Security Impact
Compatibility
Rollback Plan
git revert 6db79f98— reverts both callsite changes and both tests in one commit.