Skip to content

fix(googlechat): enable config-only startup#1390

Merged
thepagent merged 1 commit into
openabdev:mainfrom
canyugs:fix/googlechat-config-only-activation
Jul 13, 2026
Merged

fix(googlechat): enable config-only startup#1390
thepagent merged 1 commit into
openabdev:mainfrom
canyugs:fix/googlechat-config-only-activation

Conversation

@canyugs

@canyugs canyugs commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

What problem does this solve?

[googlechat] enabled = true could not activate a config-only deployment. Both startup gates used an env-only helper, so OpenAB exited with no adapter configured unless operators duplicated the setting as GOOGLE_CHAT_ENABLED=true.

This made the resolved Google Chat config correct but unreachable, and contradicted the config-first contract from #1383.

How it works

  • Replace the env-only unified-platform predicate with a config-aware predicate.
  • Resolve Google Chat enablement through GoogleChatConfig::resolve().
  • Use the same result for initial adapter validation and unified webhook startup.
  • Preserve env-only activation.
  • Preserve config authority: enabled = false still overrides GOOGLE_CHAT_ENABLED=true.
  • Correct the Google Chat mode-scoping documentation.

Regression coverage

The startup predicate test now covers:

  • no config or env activation;
  • env-only Google Chat activation;
  • invalid Google Chat env values;
  • config-only [googlechat] enabled = true;
  • config enabled = false overriding env true;
  • the existing Telegram env path.

Local smoke test

Built the all-features binary and launched it with GOOGLE_CHAT_ENABLED unset and only:

[googlechat]
enabled = true
access_token = "local-test-token"
webhook_path = "/webhook/googlechat-pr-smoke"
allowed_users = ["users/local-test"]

The unified server started successfully; /health and the configured Google Chat webhook route both returned HTTP 200.

Validation

  • cargo clippy --all-features -- -D warnings
  • cargo test --all-features (root package: 15 passed)
  • cargo test --all-features has_unified_platform_checks_config_and_env
  • cargo test -p openab-core googlechat (4 passed)
  • cargo test -p openab-gateway --all-features googlechat (67 passed; media tests run with writable HOME=/tmp)
  • cargo test -p openab-gateway --test config_first_conformance --all-features (2 passed)
  • cargo diff --check

cargo fmt --check remains blocked by existing repository-wide rustfmt drift across unrelated core and gateway files; this PR does not reformat those files.

Discussion

https://discord.com/channels/1491295327620169908/1491365158868619404/1526200802824421486

@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
@chaodu-agent chaodu-agent force-pushed the fix/googlechat-config-only-activation branch from 56d0af1 to a467747 Compare July 13, 2026 16:42

@chaodu-agent chaodu-agent 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 ✅ (with a maintainer rebase pushed, a467747)

Same root-cause family as #1389, and honestly the better fix shape: instead of adding a per-platform side helper, this makes the shared predicate itself config-aware (has_unified_platform_env()has_unified_platform(cfg)) — exactly the generalization direction we want for the remaining platforms. The 6-case regression test is thorough, and case 6 (config enabled = false beating env true) pins the config-authority semantics nicely.

What the rebase did (this PR raced #1389, which merged first and touched the same three regions of main.rs):

  1. Absorbed the WeCom arm into your predicate: has_unified_wecom_config() (from #1389) is now called from has_unified_platform's wecom arm — WECOM_CORP_ID env or credential-complete [wecom] section. The separate unified_wecom_configured binding is gone; your single unified_platform_enabled flag covers both platforms.
  2. Restored the #[cfg(any(...))] gate on the binding — under default features (no gateway platforms compiled) the unified block that consumes the flag doesn't exist, so the ungated binding failed -D warnings (unused variable). #1389 had gated it for the same reason.
  3. Kept both regression tests (your 6-case predicate test + #1389's wecom-section test), added [googlechat] to the bail message, and updated the predicate doc comment to note which platforms are config-aware vs still env-signaled (line/feishu/teams — tracked on #1356).

Verified on the rebased head: default + unified clippy clean (-D warnings), 16/16 bin tests pass under both feature sets, changed file rustfmt-clean.

With this, config-only activation parity stands at 2/6 platforms (wecom, googlechat) — line/feishu/teams remain env-signaled and are tracked as the #1356 follow-up. Thanks for the quick follow-through from the #1389 review thread, @canyugs!

@chaodu-agent

Copy link
Copy Markdown
Collaborator

Self-review of the maintainer rebase (a467747) — adversarial pass over my own conflict resolution. All checks passed; no further changes needed:

# Checked Result
1 Commit authorship Author preserved (Can Yu) — contribution credit intact; committer = chaodu-agent (rebase mechanics only)
2 docs/google-chat.md hunk survived the rebase ✅ present, wording accurate post-merge
3 WeCom arm BC both directions WECOM_CORP_ID env alone still activates (pre-#1389 behavior); credential-complete [wecom] section alone activates (#1389 behavior); incomplete section + no env stays inactive
4 enabled = false semantics end-to-end Predicate returns false (test case 6) and if the unified server starts anyway via another platform, apply_googlechat_config(enabled: false) → adapter None → route not mounted. Consistent at both layers
5 Test env races The 6-case predicate test mutates process-global env, but the wecom-section test is all-literals (config wins over env fallback by construction) — parallel-safe. Only one test mutates env and it self-contains with clear_all()
6 Binding #[cfg] gate Matches the consuming unified block's gate exactly (same six features) — default-features build verified clean under -D warnings (this was the failure my rebase fixed)
7 Doc-comment hygiene has_unified_wecom_config's intra-doc reference updated for the rename (has_unified_platform_envhas_unified_platform); predicate doc states which arms are config-aware vs env-signaled, with #1356 tracking pointer

Evidence: default + unified clippy clean, 16/16 bin tests under both feature sets, changed-file rustfmt zero drift, CI 34 pass (the lone check-pending fail is the fork-PR label-automation permission issue, unrelated to code).

@thepagent thepagent merged commit f89e9fe into openabdev:main Jul 13, 2026
37 of 39 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.

3 participants