Skip to content

fix(wecom): enable config-only unified startup#1389

Merged
thepagent merged 1 commit into
openabdev:mainfrom
canyugs:fix/1388-wecom-config-only-startup
Jul 13, 2026
Merged

fix(wecom): enable config-only unified startup#1389
thepagent merged 1 commit into
openabdev:mainfrom
canyugs:fix/1388-wecom-config-only-startup

Conversation

@canyugs

@canyugs canyugs commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • recognize a complete first-class [wecom] section during no-adapter startup validation
  • start the embedded unified gateway when WeCom credentials resolve from config without WECOM_CORP_ID being exported
  • evaluate activation after secret substitution and before config fields are moved into runtime components
  • add a regression test for config-only WeCom activation

Root cause

#1382 made every WeCom field resolve config -> WECOM_* env -> default, but the outer startup gates still relied on has_unified_platform_env(). A literal or secret-substituted [wecom] section was therefore rejected before apply_wecom_config() could construct the adapter.

The new helper requires all five credentials needed by the WeCom adapter after config/env resolution, so incomplete sections do not become an activation signal.

Validation

  • cargo clippy -- -D warnings
  • cargo clippy --features unified -- -D warnings
  • cargo test
  • cargo test --features unified
  • config-only startup smoke with all WECOM_* variables unset: adapter configured, /webhook/wecom mounted, server listening, graceful shutdown successful
  • changed hunks are rustfmt-clean; repository-wide cargo fmt --all -- --check remains blocked by pre-existing formatting drift across unrelated files

Closes #1388.

Context

@canyugs canyugs requested a review from thepagent as a code owner July 13, 2026 15:53
@openab-app openab-app Bot added closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. and removed closing-soon PR missing Discord Discussion URL — will auto-close in 24 hours. labels Jul 13, 2026

@thepagent thepagent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer review — Approving

Root cause analysis is exactly right: #1382 made every [wecom] field resolve config-first, but both activation gates (no adapter configured bail + unified-server spawn) still keyed solely on has_unified_platform_env() — so a config-only section was rejected before apply_wecom_config() ever ran. Thanks for the precise diagnosis and the tight scope.

What I verified locally (macmini, PR head):

  • cargo clippy -- -D warnings and --features unified — both clean
  • complete_wecom_section_enables_unified_startup_without_env passes under default and unified feature sets — nice touch asserting == cfg!(feature = "wecom") so the test is correct in both matrices
  • Traced the config-only path end-to-end: bail gate → unified_wecom_configured (computed post-substitution, pre-move, as your comment documents) → server spawn → apply_wecom_config (if let Some(ref w) = cfg.wecom, no env needed) → route mount keyed on gw_state.wecom — no remaining env dependency anywhere on the path
  • The all-five-credentials requirement is a good anti-footgun: an incomplete section can't become an activation signal

Non-blocking findings, all fine to land as-is:

  1. Bail-gate runs pre-substitution (line ~288 vs substitution at ~318): for a secrets-driven section, the first has_unified_wecom_config call sees unresolved placeholder strings — non-empty, so the gate passes and the authoritative post-substitution recompute takes over. Correct today, but subtly order-dependent; a one-line comment at the bail-gate call noting "placeholder values are fine here — only reachability matters, the post-substitution recompute is authoritative" would protect it from future refactors.
  2. Sibling platforms have the same gap: [line]/[feishu]/[googlechat]/[teams] config-only startup still requires the env activity signal. Scoping this PR to WeCom (matching #1388) is right; I'll track the generalization (a has_unified_platform_config() covering all six, likely replacing the wecom-specific helper) as a follow-up on #1356.
  3. Trust-override activity signal (line ~549) still keys on WECOM_CORP_ID env — in a config-only deployment the Phase-1 deprecation warning for uniform GATEWAY_* trust won't fire. Warn-only impact, and it folds naturally into follow-up (2).

CI matrix still finishing at review time; assuming green, this is ready to merge from my side.

@thepagent thepagent merged commit e1aaabf into openabdev:main Jul 13, 2026
36 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(wecom): allow config-only unified startup without WECOM_CORP_ID env

2 participants