feat(msteams): support webhook host binding#63347
feat(msteams): support webhook host binding#63347sharkqwy wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryAdds optional Confidence Score: 5/5Safe to merge — the change is additive, well-tested, and all surfaces (types, schema, runtime, docs) are kept in sync. No P0 or P1 issues found. The host forwarding logic is correct, the fallback to all-interfaces is preserved, the Zod schema and TypeScript types are aligned, and both the default and host-configured paths have lifecycle test coverage. No files require special attention.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b75632f7fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const onListening = () => { | ||
| httpServer.off("error", onError); | ||
| log.info(`msteams provider started on port ${port}`); | ||
| log.info(`msteams provider started on ${host ?? "0.0.0.0"}:${port}`); |
There was a problem hiding this comment.
Log actual bind address when host is omitted
When webhook.host is unset, this log line reports 0.0.0.0, but the server is started with expressApp.listen(port), which in Node 22 commonly binds to :: on IPv6-capable hosts. In that environment the log is inaccurate and undermines the bind-scope auditing this change is trying to improve; prefer logging httpServer.address() (or an explicit "unspecified" marker) instead of assuming IPv4.
Useful? React with 👍 / 👎.
| appPassword: "<APP_PASSWORD>", | ||
| tenantId: "<TENANT_ID>", | ||
| webhook: { port: 3978, path: "/api/messages" }, | ||
| webhook: { host: "127.0.0.1", port: 3978, path: "/api/messages" }, |
There was a problem hiding this comment.
Keep quick-start webhook bind externally reachable
The minimal setup snippet now hard-codes webhook.host: "127.0.0.1", which makes the webhook unreachable for users exposing OpenClaw directly to Teams (no local tunnel/reverse proxy on the same machine). Because this block is presented as the baseline configuration, it will cause silent setup failures in that common deployment path unless the doc either omits host or explicitly states the loopback-only constraint.
Useful? React with 👍 / 👎.
|
Validation scope summary:\n- Confirmed the PR implements the issue ask in #62766 by adding a configurable bind target (default remains unchanged unless configured).\n- Ran focused local validation on the msteams path: config parsing, webhook bind wiring, and regression check for existing webhook behavior.\n- Current CI signal is mostly green, with remaining failures appearing outside this change surface.\n\nMaintainer request to merge: please review this as a scoped fix for #62766 and, if the host-bind behavior matches expectations, approve + merge (or confirm the exact blocker still required for merge). |
|
Maintainer status update (validation + rebase):\n- Validation: scoped msteams path checks remain green (config parse, bind wiring, lifecycle regression).\n- Rebase state: branch is currently ahead of \ by 1 commit and behind by 0, and GitHub reports \ (no merge conflicts).\n\nMerge unblock ask: if there is no remaining maintainer-side blocker, please approve and merge this scoped fix for #62766; if a blocker remains, please call out the exact required change and I will patch immediately. |
|
Maintainer-facing status refresh:\n- CI/Checks: latest PR checks are not fully green yet (at least and are still failing in the current run).\n- Mergeability: GitHub API currently returns for this PR from my side, so I cannot confirm a clean merge-ready state yet.\n\nSingle action needed to unblock merge: please confirm the one exact maintainer-required next step (for example, rerun/rebase/fix specific failing check name), and I will execute that immediately in one follow-up patch. |
|
Maintainer-facing follow-up (latest status):\n\n- Mergeability: GitHub API now reports .\n- CI: still not fully green on the current run; failing checks shown are , , and (others are passing/skipped).\n\nCould you please confirm the single maintainer action you want next to get this merged (for example: maintainer rerun of those jobs vs. requesting a code change from me)? I will do exactly that immediately. |
|
Upstream triage ping with current state: mergeable=MERGEABLE, updatedAt=2026-04-09T22:09:05Z, statusCheckRollup_count=131. Single maintainer ask: please confirm one next action (merge / close / request specific change) so this PR can leave backlog. |
|
Codex review: needs real behavior proof before merge. Summary Reproducibility: yes. for the requested capability gap. Current-main source inspection shows no Real behavior proof Next step before merge Security Review detailsBest possible solution: Land the prepared host-binding implementation once the contributor adds redacted live proof that a configured MS Teams webhook host actually binds as intended, while preserving the current default listen behavior. Do we have a high-confidence way to reproduce the issue? Yes for the requested capability gap. Current-main source inspection shows no Is this the best way to solve the issue? Yes, the narrow optional What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 1849e0c34bdc. |
8f20af9 to
f634f46
Compare
|
Maintainer update: rebased this onto current main and prepared the branch at What changed in the prepared head:
Verification run locally:
Waiting on fresh CI/ClawSweeper for this prepared head before merge. |
Summary
channels.msteams.webhook.hostto Teams config schema/typesexpressApp.listen(port, host)while preserving default behavior when omittedTesting
Closes #62766