Skip to content

feat(teams): full [teams] section — credentials + connection config-first#1384

Merged
thepagent merged 2 commits into
mainfrom
feat/1380-teams-config-first
Jul 13, 2026
Merged

feat(teams): full [teams] section — credentials + connection config-first#1384
thepagent merged 2 commits into
mainfrom
feat/1380-teams-config-first

Conversation

@chaodu-agent

Copy link
Copy Markdown
Collaborator

Note

Stacked on #1383 (→ #1382#1381 → main). Net-new diff = the last commit; will re-target as parents merge.

What problem does this solve?

Fourth platform slice of #1375: Teams credentials/connection were env-only. [teams] graduates from the shared trust-only struct to a dedicated TeamsConfig (6 fields + trust), resolving each field config → TEAMS_* env → default.

How It Works

Layer Change
core TeamsConfig/ResolvedTeams + resolve(); trust_config() view keeps the registry path unchanged
gateway TeamsConfig::from_env refactored onto from_reader, shared with apply_teams_config() — same mandatory-credential semantics both paths (app_id+app_secret required; incomplete ⇒ adapter disabled); new teams_webhook_path honored by both binaries
unified Applies [teams] before warn_unenforceable_l1
docs example, config-reference (full field table), msteams-selfhosted.md

Tests

  • teams_resolve_all_scenarios — defaults (Bot Framework endpoints), config-wins-over-env, empty-string fallthrough, trust_config()
  • apply_teams_config_requires_credentials — complete credentials build the adapter; missing secret disables it

Validation

  • clippy -D warnings clean (all-features + gateway no-features); unified + default builds pass
  • core 663 passed (1 known pre-existing macOS secrets failure); gateway 261/261
  • rustfmt isolated: 0 new drift after fixes (config.rs 0→0, lib.rs 0→0, teams.rs 3→3 pre-existing)

Closes #1380. Refs #1375.

@chaodu-agent chaodu-agent requested a review from thepagent as a code owner July 13, 2026 00:58
@chaodu-agent chaodu-agent force-pushed the feat/1379-googlechat-config-first branch from 6f8ce39 to bcf35d8 Compare July 13, 2026 01:01
@chaodu-agent chaodu-agent force-pushed the feat/1380-teams-config-first branch from 6305407 to caadf8f Compare July 13, 2026 01:01
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent chaodu-agent force-pushed the feat/1380-teams-config-first branch from caadf8f to e77928c Compare July 13, 2026 01:05
@chaodu-agent chaodu-agent force-pushed the feat/1379-googlechat-config-first branch from bcf35d8 to 21f5f8a Compare July 13, 2026 01:05
@chaodu-agent chaodu-agent force-pushed the feat/1380-teams-config-first branch from e77928c to 4428463 Compare July 13, 2026 01:05
@chaodu-agent

This comment has been minimized.

@chaodu-agent chaodu-agent force-pushed the feat/1380-teams-config-first branch from 4428463 to f514587 Compare July 13, 2026 01:46
@chaodu-agent chaodu-agent force-pushed the feat/1379-googlechat-config-first branch from c71528c to 3c6a924 Compare July 13, 2026 01:46
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

Note

LGTM ✅ — Clean config-first graduation for [teams]; follows the established pattern from prior platform slices, maintains backward-compatible env-only semantics, good test coverage.

What This PR Does

Graduates [teams] from the shared PlatformTrustConfig (trust-only, 2 fields) to a dedicated TeamsConfig with 6 credential/connection fields + trust. Resolution invariant: config → TEAMS_* env → default. Fourth platform slice of #1375, closing #1380.

How It Works

  • Core: TeamsConfig/ResolvedTeams with resolve() applying three-layer fallback; trust_config() projects back into PlatformTrustConfig to keep the registry override path unchanged.
  • Gateway: from_reader<F> abstraction deduplicates env-only and config-driven adapter construction; apply_teams_config() + GatewayTeamsConfig bridge struct; teams_webhook_path promoted to AppState field.
  • Unified binary: Calls resolve()apply_teams_config() before warn_unenforceable_l1 — correct ordering so L1 audit sees config-supplied adapter state.
  • Docs: config.toml.example, config-reference table, msteams-selfhosted guide updated. PlatformTrustConfig doc comment updated to reflect its new role as the shared trust-view type.

Findings

# Severity Finding Location
1 🟢 from_reader keeps env-only and config paths DRY with identical mandatory-credential semantics adapters/teams.rs:128
2 🟢 Comprehensive resolve test covers defaults, config-wins-over-env, empty-string fallthrough, and trust_config() view config.rs test
3 🟢 Backward-compatible: absent [teams]None, legacy env-only path unchanged main.rs:534
4 🟢 Doc update wraps old trust-only description in <details> — preserves historical context config-reference.md
5 🟢 Second commit cleanly updates PlatformTrustConfig doc comment to reflect its graduated role as the shared trust-view type config.rs
Finding Details

🟢 F1: from_reader abstraction

from_reader<F: Fn(&str) -> Option<String>> lets both from_env() and apply_teams_config() share the same mandatory-credential gate (app_id + app_secret required → None disables adapter). No behavioral divergence possible between env-only and config paths.

🟢 F2: Test coverage

teams_resolve_all_scenarios covers: defaults (Bot Framework URLs), config-wins-over-env, empty-string fallthrough to env, trust_config() view. apply_teams_config_requires_credentials pins the missing-secret → disabled path. Env-var isolation notes prevent parallel test races.

🟢 F3: Backward compatibility

The trust_config() view returns PlatformTrustConfig so platform_trust_override in main.rs continues working unchanged. Migration is invisible to the trust registry.

🟢 F4: Documentation approach

Old trust-only description wrapped in a <details> collapse rather than deleted — readers of the history can still find it.

🟢 F5: PlatformTrustConfig doc comment

Clarifies that all platform sections have now graduated to dedicated structs and this type remains as the common-denominator view consumed by platform_trust_override. Removes stale references to trust-only-by-design semantics.

Baseline Check
What's Good (🟢)
  • Resolution order mechanically identical to LINE/Telegram/Google Chat — easy to audit
  • trust_config() avoids a breaking change while the section graduates
  • apply_teams_config test covers both happy path (adapter built) and missing-credential path (adapter disabled)
  • Standalone gateway serve() keeps env-only semantics (no config.toml dependency); unified binary gets config-first treatment — clean separation
  • csv closure correctly handles Some(vec![]) without falling through to env
  • Fail-closed mandatory-credential semantics: missing credentials = adapter disabled

5️⃣ Three Reasons We Might Not Need This PR

  1. Env vars already work — Teams adapter functions today with TEAMS_* env vars; config-file adds an alternative path but not a new capability.
  2. Stacked PR complexity — Four-deep stack; if any parent needs rebasing, downstream conflicts follow. Could have been a single larger PR.
  3. Dual-struct patternTeamsConfig lives in both core and gateway; GatewayTeamsConfig bridge adds a third representation. Could consolidate if the crate boundary were relaxed.

All three are reasonable trade-offs addressed by the project's config-first invariant (#1375), the stacked-PR workflow convention, and deliberate crate isolation policy.


Reviewed: f514587

…irst

Fourth platform slice of the config-first parity umbrella (#1375):
[teams] graduates from the shared PlatformTrustConfig to a dedicated
TeamsConfig carrying app_id, app_secret, allowed_tenants,
oauth_endpoint, openid_metadata, webhook_path alongside the trust
fields. Each field resolves config → TEAMS_* env → default.

- core: TeamsConfig/ResolvedTeams + resolve(); trust_config() view
- gateway: TeamsConfig::from_env refactored onto a from_reader shared
  with the new AppState::apply_teams_config (app_id + app_secret
  mandatory — incomplete section disables the adapter, matching
  env-only semantics); new teams_webhook_path state honored by both
  binaries' route mounts
- unified: applies [teams] before warn_unenforceable_l1
- docs: config.toml.example, config-reference.md (full field table),
  msteams-selfhosted.md

Closes #1380. Refs #1375. Stacked on #1383.
@chaodu-agent chaodu-agent changed the base branch from feat/1379-googlechat-config-first to main July 13, 2026 02:22
@chaodu-agent chaodu-agent force-pushed the feat/1380-teams-config-first branch from f514587 to 323fec3 Compare July 13, 2026 02:22
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

Note

LGTM ✅ — Clean graduation of [teams] from trust-only to full config-first section, matching the established pattern from LINE/Telegram/Google Chat slices.

What This PR Does

Teams credentials and connection settings were env-only (TEAMS_*). This PR introduces a dedicated TeamsConfig struct (8 fields + trust view) following the config-first invariant: each field resolves [teams].fieldTEAMS_* env → default — exactly like the preceding LINE, Telegram, and Google Chat slices.

How It Works

Layer Change
core New TeamsConfig/ResolvedTeams + resolve() (config→env→default); trust_config() view returns PlatformTrustConfig to keep the binary's trust-override registry path unchanged
gateway TeamsConfig::from_env refactored onto from_reader<F>, sharing mandatory-credential semantics with apply_teams_config(); new teams_webhook_path field on AppState
unified binary [teams] resolved + applied before warn_unenforceable_l1; webhook path sourced from state rather than inline env read
docs Example, config-reference field table, msteams-selfhosted.md new section

Findings

# Severity Finding Location
1 🟢 Consistent graduation pattern — same resolve/apply/trust-view shape as LINE, Telegram, Google Chat config.rs
2 🟢 from_reader abstraction in gateway avoids duplicating credential logic while keeping env-only path unchanged adapters/teams.rs
3 🟢 Tests cover defaults, config-wins-over-env, empty-string fallthrough, and mandatory-credential semantics config.rs, lib.rs
4 🟢 Documentation updated in three places — example, reference table, and deployment guide docs/
What's Good (🟢)
  • Architecture consistencyTeamsConfig follows the exact same graduation pattern as the three preceding platform slices, making the codebase predictable.
  • Backward compatibilityfrom_env() still works identically (delegates to from_reader), so existing env-only deployments break nothing.
  • trust_config() bridge — the binary's platform_trust_override call works unchanged by receiving the same PlatformTrustConfig shape via a view method.
  • Empty-string fallthrough — correct handling of ${} expansions that produce empty strings (treated as absent, falls through to env). Important for real config files where unexpanded variables may appear.
  • Mandatory-credential gate — both construction paths (env-only and config-first) require app_id + app_secret; incomplete section disables the adapter instead of panicking.
  • Webhook path consolidation — moved from scattered std::env::var reads to AppState.teams_webhook_path, single source of truth in unified mode.
Baseline Check

5️⃣ Three Reasons We Might Not Need This PR

  1. ENV-only was working fine — Teams deployments with just TEAMS_APP_ID/TEAMS_APP_SECRET env vars work today; the additional config-file path adds surface area with no new functionality for existing users.
  2. Stacked dependency chain risk — Four-deep stack (feat(line): full [line] section — credentials + connection config-first #1381feat(wecom): full [wecom] section — credentials + connection config-first #1382feat(googlechat): full [googlechat] section — credentials + connection config-first #1383feat(teams): full [teams] section — credentials + connection config-first #1384) means merge conflicts compound; could ship as one combined PR for simpler history.
  3. Duplicate resolution logicresolve() in core and from_reader in gateway overlap in semantics (both implement config→env→default). Could be unified into one resolution point.

All three are addressable: (1) config-first is the project's declared direction per #1375, (2) stacking is deliberate for reviewability and each slice is independently useful, (3) the two layers serve different roles (core = typed resolution for the binary; gateway = adapter construction with mandatory-credential gate).

@thepagent thepagent merged commit a5a33b2 into main Jul 13, 2026
41 of 60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(teams): full [teams] section — credentials + connection config-first

2 participants