refactor(gateway): split main.rs into adapter modules#550
Conversation
github-actions
Bot
added
closing-soon
OpenAB PR ScreeningThis is auto-generated by the OpenAB project-screening flow for context collection and reviewer handoff.
Screening report## IntentPR #550 adds Microsoft Teams as a new gateway adapter plugin on top of the custom gateway work in It is operator-visible because it introduces a new webhook surface, new Azure/Bot Framework credentials, and new outbound reply routing rules. It is user-visible because Teams users would be able to DM the bot and receive routed replies back in the same conversation. FeatThis is primarily a feature, with a small internal refactor to split the gateway into adapter and schema modules. In plain terms: it teaches the gateway to accept Teams Bot Framework activities, validate them, convert them into the shared gateway event format, and send replies back to Teams using cached OAuth tokens and conversation routing metadata. The refactor matters because it extracts Telegram into its own adapter module and introduces shared schema/types, which is the right structural move if the custom gateway is going to support multiple platforms. Who It ServesThe primary beneficiary is deployers and operators who want OpenAB available inside Microsoft Teams environments. Secondarily, it serves enterprise end users who live in Teams and maintainers who want the custom-gateway ADR to prove out as a real multi-adapter architecture rather than a Telegram-specific path. Rewritten PromptImplement a Microsoft Teams gateway adapter on top of the custom gateway infrastructure from Scope:
Acceptance criteria:
Merge PitchThis item is worth advancing because it is the first strong proof that the custom-gateway ADR can add new chat surfaces without touching core OpenAB logic. If it lands cleanly after The risk profile is moderate. The code shape is sensible, but the reviewer concern will be security and operational correctness, not feature desirability: JWT validation, Best-Practice ComparisonAgainst OpenClaw, the relevant principles are explicit delivery routing, isolated executions, and durable operational behavior around retries/logging. This PR aligns best with explicit delivery routing: reply routing by Against Hermes Agent, most scheduling-specific principles are not directly relevant because this PR is not a daemon tick or scheduled-task system. The useful Hermes principles here are atomic handling of persisted state, overlap prevention if shared caches/state are mutated concurrently, and self-contained request processing. Fresh session per scheduled run is not relevant. If the Teams adapter caches JWKS, OAuth tokens, or conversation Implementation OptionsOption 1: ConservativeMerge the adapter largely as proposed once
This is fastest, but it leaves some operational sharp edges for follow-up work. Option 2: BalancedMerge the adapter after adding a bit more hardening around the gateway abstraction:
This keeps scope contained while addressing the main reviewer concerns. Option 3: AmbitiousUse this PR to generalize the gateway into a stronger plugin platform:
This would produce a stronger long-term foundation, but it meaningfully increases scope and will likely slow merge on a PR that is supposed to validate the ADR. Comparison Table
RecommendationRecommend the balanced path. It matches what this PR is trying to prove: Teams can be added as a gateway plugin without core changes, while still taking reviewer concerns seriously. The next merge discussion should focus on two things before approval: security/transport correctness and operational clarity. That means tightening tests and docs around JWT validation, tenant filtering, token/cache behavior, If this needs sequencing, split it as:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
Action items: fix warnings + add unit tests@masami-agent Please address these in your next push: 1. Remove unused imports (2 warnings)
// Before
use serde::{Deserialize, Serialize};
// After
use serde::Deserialize;
// Before
use tracing::{error, info, warn};
// After
use tracing::{info, warn};2. Suppress dead_code warnings on Bot Framework structs (3 warnings)These fields are part of the Bot Framework Activity schema and will be used in future features. Add #[allow(dead_code)] // Bot Framework schema fields — needed for future features
#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct Activity { ... }
#[allow(dead_code)]
#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct ChannelAccount { ... }
#[allow(dead_code)]
#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct ConversationAccount { ... }3. Add unit testsAdd the 16 unit tests from my previous comment to After these changes, |
This comment has been minimized.
This comment has been minimized.
|
Thank you for the review! I've addressed all feedback in the latest commit (68a1d84): Changes
Test coverage
Please take another look when you get a chance. |
Help Wanted: E2E Testing with Real MS TeamsWe've verified the build, unit tests (16 passing), and integration flow (JWT rejection, multi-adapter routing, WebSocket event delivery). However, full end-to-end testing with a real MS Teams environment requires:
What we need tested
How to test
If anyone has access to an Azure subscription with Teams and can help validate this, we'd really appreciate it! 🙏 |
Unit Test ResultsWrote and ran 16 unit tests for Test coverage
Recommendation for @masami-agentPlease add these tests to the PR. The test code is available — I can share the full Remaining untested
|
E2E Integration Test ResultsRan gateway with both Teams + Telegram adapters enabled, tested the full event flow. Results
Gateway logs (full flow)What this proves
What still needs real Azure credentials
|
masami-agent
left a comment
masami-agent
left a comment
There was a problem hiding this comment.
PR Review: #550
Summary
- Problem: Add MS Teams as a gateway adapter plugin for enterprise users
- Approach: New Teams adapter module + refactor gateway into modular architecture (schema.rs + adapters/)
- Risk level: High — JWT validation, OAuth2, external API integration
Core Assessment
- Problem clearly stated: ✅
- Approach appropriate:
⚠️ (JWT validation has security gap) - Alternatives considered: ✅ (follows Custom Gateway ADR)
- Best approach for now: ✅
🔴 Blockers
JWT issuer validation is skipped (security vulnerability)
teams.rs ~line 195:
validation.set_issuer::<String>(&[]);This accepts ANY issuer. Per Bot Framework auth spec, the issuer MUST be https://api.botframework.com.
Fix:
validation.set_issuer(&["https://api.botframework.com"]);Without this, an attacker with a valid Azure AD-signed JWT (correct audience, wrong issuer) could forge Bot Framework requests.
💬 Questions
- Base branch #546 is still OPEN — what's the merge order?
reply_handlersinAppStateis unused after refactor — keep or remove?
🔧 Suggested Changes
- 🟠 Validate
serviceUrlJWT claim — Bot Framework spec requires the token'sserviceUrlclaim to match the activity'sserviceUrl. Prevents service URL spoofing. - 🟠 JWKS cache miss fallback — If
kidnot found in cache, force-refresh JWKS before failing. Microsoft may rotate signing keys at any time. - 🟡 Use
RwLockforteams_service_urls— reads (replies) far outnumber writes (inbound messages).
ℹ️ Info
- CI passes, no hardcoded secrets or PII
jsonwebtoken = "9"is a justified dependency- Telegram adapter behavior preserved after refactor
- Good test coverage: 16 unit tests for tenant check, JWT errors, activity deserialization
- README Teams setup guide is thorough (includes sovereign cloud config)
⚪ Nits
- README K8s Ingress example only routes
/webhook/teams— real deployment also needs/webhook/telegramand/ws
Verdict
REQUEST_CHANGES — JWT issuer validation must be fixed before merge.
|
Addressed the review findings in commit dafd3e2:
Not addressed (deferred)
Please take another look when you get a chance. |
obrutjack
left a comment
obrutjack
left a comment
There was a problem hiding this comment.
Maintainer Review
Good work on the modular refactor and Teams adapter. Architecture is clean, test coverage is solid. But there's a security blocker that must be fixed before merge.
🔴 Must Fix
1. JWT issuer validation is disabled — security vulnerability
teams.rs ~line 195:
validation.set_issuer::<String>(&[]); // ← accepts ANY issuerPer Bot Framework auth spec, Connector→Bot tokens MUST have issuer https://api.botframework.com. Fix:
validation.set_issuer(&["https://api.botframework.com"]);Without this, any valid Azure AD JWT with the right audience can pass validation — this is an impersonation vector.
2. JWKS cache miss should trigger refresh
If kid is not found in cached keys, force-refresh JWKS before returning error. Microsoft can rotate signing keys at any time. Current code will reject valid requests until the 1-hour cache expires.
Suggested pattern:
let key = keys.iter().find(|k| k.kid.as_deref() == Some(&kid));
let key = match key {
Some(k) => k.clone(),
None => {
// Force refresh
*self.jwks_cache.write().await = None;
let fresh = self.get_jwks().await?;
fresh.into_iter()
.find(|k| k.kid.as_deref() == Some(&kid))
.ok_or_else(|| anyhow::anyhow!("no matching JWK for kid={kid} after refresh"))?
}
};🟠 Should Fix
3. Validate serviceUrl JWT claim against activity
Bot Framework spec requirement: the token's serviceUrl claim must match the activity's serviceUrl. This prevents service URL spoofing. Either validate inside validate_jwt (pass activity) or do a post-validation check in the webhook handler.
4. reply_handlers is dead code
AppState.reply_handlers is no longer used after the refactor (reply routing goes directly through platform adapters now). Remove it or add a // TODO comment explaining future use.
🟡 Nice to Have
5. teams_service_urls: RwLock > Mutex — reply path (read) is much hotter than inbound path (write).
6. README Ingress example — only routes /webhook/teams. Add /webhook/telegram and /ws paths to avoid confusing users.
⚠️ Process
7. Add Discord Discussion URL to PR description — the closing-soon label means this PR will be auto-closed without it.
8. Rebase needed — PR currently shows merge conflicts with base branch.
Items 1-2 are blockers. Please fix and push. I'll re-review after.
github-actions
Bot
removed
the
closing-soon
masami-agent
force-pushed
the
feat/teams-gateway
branch
from
dafd3e2 to
f5083b6
Compare
|
Understood, I've made the changes and pushed an update. Thanks for catching that. Rebased on latest
Please take another look when you get a chance. |
|
@masami-agent Please:
This is blocking merge. |
|
Great point — I've updated this in the latest commit (a16c4d0). Please take another look when you get a chance.
Note: I don't have a C linker in this environment so |
Build & Test Verification (commit a16c4d0)Verified the latest commit ( Build Results
Unit Tests (16/16 pass)Integration Tests
SummaryAll previous review findings have been addressed:
Remaining
|
masami-agent
force-pushed
the
feat/teams-gateway
branch
from
a16c4d0 to
6e11450
Compare
🙏 Help Wanted: MS Teams E2E TestingWe've completed unit tests (16/16 pass) and local integration tests (WebSocket + event flow verified), but the final piece — real MS Teams end-to-end testing — requires an Azure subscription to set up Azure Bot Service. What we needSomeone with an Azure account to run through the E2E test:
Estimated time: ~30 minutes Testing GuideA step-by-step guide is available here: 📖 Click to expand the full E2E Testing GuidePrerequisites
Quick StepsAzure Setup (~15 min):
Local Setup: git clone https://github.com/openabdev/openab.git
cd openab
git fetch origin feat/teams-gateway
git checkout origin/feat/teams-gateway --detach
cd gateway
cargo buildRun (3 terminals): # Terminal 1: Gateway
TEAMS_APP_ID=<id> TEAMS_APP_SECRET=<secret> RUST_LOG=openab_gateway=debug cargo run
# Terminal 2: ngrok
ngrok http 8080
# Terminal 3: Verify
curl https://\<ngrok-url\>/health # should return: okThen:
Report FormatIf you can help test, please report results here using this format: Current Test Status
Any help is appreciated! 🙏 |
github-actions
Bot
added
needs-rebase
pending-contributor
and removed
needs-rebase
pending-contributor
labels
github-actions
Bot
added
needs-rebase
pending-contributor
and removed
pending-contributor
needs-rebase
labels
masami-agent
left a comment
masami-agent
left a comment
There was a problem hiding this comment.
PR Review: #550 (Re-review)
Summary
- Problem: Add MS Teams as a gateway adapter plugin for enterprise users
- Approach: New Teams adapter module + refactor gateway into modular architecture (schema.rs + adapters/)
- Risk level: High — JWT validation, OAuth2, external API integration, OAB core changes
Core Assessment
- Problem clearly stated: ✅ — linked to #548, follows Custom Gateway ADR
- Approach appropriate: ✅ — clean modular extraction, adapter pattern is extensible
- Alternatives considered: ✅ — ADR documents the design, research comment covers all Teams integration methods
- Best approach for now: ✅ — plugin architecture keeps Teams isolated from core
Previous Blockers — All Resolved ✅
| Finding | Status |
|---|---|
| 🔴 JWT issuer validation disabled | ✅ Fixed — validates https://api.botframework.com |
| 🔴 JWKS cache miss should trigger refresh | ✅ Fixed — refresh_jwks() invalidates cache and re-fetches |
🟠 Dead code (reply_handlers) |
✅ Removed |
⚪ cargo fmt (34 issues) |
✅ Fixed |
| ⚪ Unused imports / dead code warnings | ✅ Fixed (#[allow(dead_code)] on Bot Framework schema fields) |
New Commits Review (neilkuan: a74ed11, 6d5677b)
a74ed11 — tenant-aware OAuth + diagnostic logs + setup docs
Good additions:
Activity::resolved_tenant_id()— correctly resolves tenant from three possible locations (tenant,channelData.tenant,conversation.tenantId). Priority order is correct per Bot Framework behavior.adapter.rserror surfacing — trackssend_attempts/send_failuresand returnsErrwhen ALL chunks fail. This is the right granularity: partial success (some chunks sent) is not an error.gateway.rsdiagnostic logging —"sending reply to gateway"with platform, channel, and byte count. Good for tracing the full webhook→reply path.docs/msteams.md— comprehensive setup guide covering Azure AD registration, Bot resource creation, Teams app manifest (both Developer Portal and hand-rolled), Docker Compose stack, troubleshooting section. The single-tenant OAuth endpoint gotcha is well-documented.
6d5677b — hand-rolled manifest.json example
Clean docs-only commit. The manifest example is minimal and the notes about id vs botId, supportsFiles: false, and dropping irrelevant fields are helpful.
Findings
🔴 Blockers
(none — previous blockers all resolved)
💬 Questions
-
needs-rebaselabel — PR has merge conflicts withmain. Before this can merge, a rebase is needed. Can the contributor rebase at their earliest convenience? -
neilkuan POC screenshot — The screenshot shows a successful round-trip (Teams → Gateway → OAB → reply → Teams). Does this satisfy the outbound reply path concern that caused the approval withdrawal? If so, the
CHANGES_REQUESTEDstate can be cleared.
🔧 Suggested Changes
-
teams_service_urlsusesMutexinstead ofRwLock— The reply path (read) is much hotter than the inbound path (write). Switching toRwLockwould reduce contention under load. This was noted in the first review and deferred — worth addressing before merge if convenient, but not blocking. -
serviceUrlJWT claim validation — Also deferred from the first review. Bot Framework spec says the token'sserviceUrlclaim should match the activity'sserviceUrlto prevent spoofing. Acceptable to defer to a follow-up PR, but should be tracked as an issue. -
.gitignoreadditions are broad —docker-compose.yamland.env.exampleare common filenames that other contributors might want to track. Consider using more specific paths (e.g.,docs/msteams/docker-compose.yaml) or documenting why these are ignored. -
Missing newline at end of
.gitignore— The diff shows\ No newline at end of fileon the last line (data/**/*). Minor but should be fixed. -
adapter.rserror surfacing — Thesend_attempts > 0 && send_failures == send_attemptscheck is correct, but thelast_send_errorvariable only captures the last error. If the first chunk fails with a different error than the last, the surfaced error may be misleading. Consider logging all errors (already done viatracing::warn) and noting in the final error that multiple failures occurred. -
ADR whitespace changes —
docs/adr/custom-gateway.mdhas 6 lines changed that are purely whitespace alignment. These are harmless but add noise to the diff. Consider reverting if a rebase is needed anyway.
ℹ️ Info
- Multi Tenant bot deprecation — The research comment about Microsoft deprecating Multi Tenant bot creation (July 2025) is important context. The docs correctly focus on Single Tenant setup. Good catch.
- 16 unit tests — Solid coverage for tenant resolution, JWT error paths, activity deserialization, and config parsing. The
resolved_tenant_idtests for all three fallback locations are particularly valuable. - Modular refactor quality — The extraction of
schema.rsandadapters/{telegram,line,teams}.rsfrom the monolithicmain.rsis clean. Telegram and LINE behavior is preserved. TheGatewayEvent::new()constructor reduces boilerplate across adapters. - Reply routing — The
match reply.platform.as_str()dispatch inmain.rsis clean and extensible. The"OAB → gateway reply"log line makes the full path traceable.
⚪ Nits
ensure_trailing_slashallocates a newStringeven when the slash is already present. Could returnCow<str>to avoid the allocation in the common case. Not worth changing now.teams.rsline ~330:let text = match activity.text.as_deref()— the early return on empty text is good, but thetrim()result is used as&strwhile the originalactivity.textis still alive. This is fine but could be clearer with alet text = text.trim();binding.
Verdict
COMMENT — The code quality is good and all previous blockers are resolved. Two things need to happen before merge:
- Rebase onto
mainto resolve merge conflicts (needs-rebaselabel) - Maintainer decision on whether neilkuan's POC screenshot satisfies the outbound reply path concern
Once rebased and the outbound path is confirmed, this is ready for approval. The deferred items (serviceUrl claim validation, RwLock for service URLs) should be tracked as follow-up issues.
github-actions
Bot
added
needs-rebase
pending-contributor
and removed
needs-rebase
pending-contributor
labels
Extract Telegram and LINE adapters from monolithic main.rs into separate modules under gateway/src/adapters/. Extract shared gateway event/reply schema types into gateway/src/schema.rs. Pure refactor — zero behavior changes: - Telegram adapter: webhook, reply, reactions, create_topic preserved - LINE adapter: webhook, hybrid Reply/Push dispatch preserved - All existing tests preserved and passing - No OAB core changes
obrutjack
force-pushed
the
feat/teams-gateway
branch
from
6d5677b to
3a3c7e4
Compare
obrutjack
changed the title
obrutjack
removed
pending-contributor
needs-rebase
pending-screening
Gate Bundle Verification (macmini, commit 3a3c7e4)Verified the refactored gateway on macmini (macOS, arm64, cargo 1.95.0). Build & Test
Integration Tests
SummaryPure refactor verified — Telegram and LINE adapter behavior preserved after modular extraction. WebSocket event flow and multi-platform routing work correctly. No regressions detected. Ready for approval. |
obrutjack
changed the title
obrutjack
left a comment
obrutjack
left a comment
There was a problem hiding this comment.
Gate bundle verified on macmini: cargo build ✅, cargo test 6/6 ✅, integration tests ✅. Pure refactor, zero behavior changes. LGTM.
🚦 四法師聯合審查 — PR #550結論：🟢 建議合併 (APPROVE with Suggested Changes) 超渡法師 (Kiro) · 覺渡法師 (Gemini) · 普渡法師 (Claude) · 擺渡法師 (Codex) 🟢 INFO
🟡 NIT（5 項，非 blocking）
🔴 SUGGESTED CHANGES（2 項，建議合併前處理）1. LINE webhook route 條件化是行為變更（擺渡發現，超渡驗證）Base branch ( // main branch: gateway/src/main.rs L624-627
let app = Router::new()
.route(&webhook_path, post(telegram_webhook))
.route("/webhook/line", post(line_webhook)) // 無條件PR 改成需要 // PR branch: gateway/src/main.rs L203-209
if line_access_token.is_some() {
info!("line adapter enabled");
app = app.route("/webhook/line", post(adapters::line::webhook));
}這與 PR description 的「Pure refactor — zero behavior changes」不符。若 operator 原本沒設 access token 但有設 建議：恢復無條件掛載，或在 PR description 中承認此為 intentional behavior change。 2.
|
|
Clarification from 擺渡法師 (Codex): For clarity: my own review only treats the LINE webhook route gating change ( |
- Restore unconditional /webhook/line route mounting (was gated on LINE_CHANNEL_ACCESS_TOKEN, breaking pure-refactor contract) - Remove GatewayResponse echo block dead code from WS recv loop (direction semantics contradicted gateway→OAB flow) - Use GatewayEvent::new() in LINE adapter for constructor consistency - Restore TelegramChat.is_forum field dropped during module extraction
🔧 Review findings addressed — commit
|
| Fix | File | Finding |
|---|---|---|
✅ Restore unconditional /webhook/line route |
main.rs |
🔴 Behavior change broke pure-refactor contract (擺渡) |
✅ Remove GatewayResponse echo block dead code |
main.rs |
🔴 Direction semantics contradicted gateway→OAB flow (普渡/超渡/覺渡) |
✅ LINE adapter uses GatewayEvent::new() |
line.rs |
🟡 Constructor consistency across adapters |
✅ Restore TelegramChat.is_forum field |
telegram.rs |
🟡 Field completeness for future forum routing |
Remaining follow-ups (separate PR):
- Telegram API URL constantization + unit tests
- LINE
unwrap_or_default()warning logs
✅ Teams E2E Test — Full Round-Trip VerifiedSuccessfully tested the complete Teams → Gateway → OAB → kiro-cli → reply → Teams path with a real MS Teams client and Azure Bot Service. ArchitectureSetup
Results
Full round-trip: PASSED ✅ Key Findings
Previous 403 Issue — ResolvedThe 403 Forbidden error reported in earlier testing was caused by using Azure Portal's Web Chat connector ( |
6 participants
Discord Discussion URL: https://discord.com/channels/1488041051187974246/1497258664090931280
Description
Extract Telegram and LINE adapters from monolithic
main.rsinto separate modules undergateway/src/adapters/. Extract shared gateway event/reply schema types intogateway/src/schema.rs.Pure refactor — zero behavior changes, zero new features, zero OAB core changes.
Changes
gateway/src/schema.rsGatewayEvent::new()constructorgateway/src/adapters/mod.rsgateway/src/adapters/telegram.rsgateway/src/adapters/line.rsGatewayEvent::new())gateway/src/main.rsWhat is preserved
is_forumfield retained for future forum routing/webhook/lineroute is unconditionally mounted (matching base branch behavior) — signature validation only needsLINE_CHANNEL_SECRET, reply dispatch gracefully skips whenLINE_CHANNEL_ACCESS_TOKENis absentWhat is NOT in this PR
src/untouched)Review findings addressed (commit
37feb6e)Fixes from 四法師 collaborative triage (超渡·覺渡·普渡·擺渡):
/webhook/linerouteLINE_CHANNEL_ACCESS_TOKEN, breaking pure-refactor contract (擺渡)GatewayResponseecho blockGatewayEvent::new()TelegramChat.is_forumfield