Skip to content

feat(reborn): add telegram v2 default-off config guard - #3356

Merged
nickpismenkov merged 6 commits into
reborn-integrationfrom
split/pr3316-06-telegram-v2-default-off-config
May 13, 2026
Merged

nickpismenkov merged 6 commits into
reborn-integrationfrom
split/pr3316-06-telegram-v2-default-off-config

Conversation

@serrrfirat

@serrrfirat serrrfirat commented May 7, 2026 •

Copy link
Copy Markdown
Collaborator

Split from #3316 (original PR by @nickpismenkov). This is PR 6/7 in the ProductAdapter stack.

Summary

  • Add REBORN_TELEGRAM_V2_ENABLED=false default-off config marker.
  • Add ChannelsConfig::reborn_telegram_v2_enabled plumbing.
  • Add fail-closed v1/v2 Telegram exclusivity validator.
  • Does not register production v2 routes or change legacy v1 Telegram behavior.

Stack

Authorship

Split commits retain Co-authored-by: Nikolay Pismenkov <nickpismenkov@gmail.com>.

Verification

  • cargo test --test telegram_v2_default_off_integration --offline --quiet — 5 tests passed.
  • cargo test -p ironclaw config::channels::telegram_v2_tests --lib --offline --quiet — 4 tests passed.
  • Final stack also ran cargo fmt --all -- --check and targeted ProductAdapter/Telegram/config tests.

Split from PR #3316. Adds the default-off REBORN_TELEGRAM_V2_ENABLED marker, config plumbing, and fail-closed v1/v2 Telegram exclusivity validator without registering production v2 routes.

Co-authored-by: Nikolay Pismenkov <nickpismenkov@gmail.com>
@github-actions github-actions Bot added size: M 50-199 changed lines scope: dependencies Dependency updates risk: medium Business logic, config, or moderate-risk modules contributor: core 20+ merged PRs and removed size: M 50-199 changed lines labels May 7, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a feature flag, REBORN_TELEGRAM_V2_ENABLED, to manage the transition between legacy Telegram v1 and the new v2 ProductAdapter, including a validation utility to ensure mutual exclusivity. Feedback suggests integrating this validation directly into the configuration resolution process and refactoring the validator to accept a configuration reference, which would encapsulate the activation logic and reduce the risk of logic drift.

Comment thread src/config/channels.rs
}
ids
},
reborn_telegram_v2_enabled: parse_bool_env("REBORN_TELEGRAM_V2_ENABLED", false)?,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Consider calling validate_telegram_v1_v2_exclusivity directly within ChannelsConfig::resolve. Enforcing configuration invariants during resolution ensures that an invalid configuration state is caught as early as possible (e.g., during Config::from_env or Config::from_db). When implementing this, ensure you use a specific error variant like MalformedConfig for these configuration errors to follow the repository's preflight check standards.

References
  1. When adding preflight configuration checks, introduce specific error variants (e.g., MalformedConfig) for configuration errors rather than reusing existing, potentially misleading variants.

Comment thread src/config/channels.rs
Comment on lines +476 to +479
pub fn validate_telegram_v1_v2_exclusivity(
v1_active: bool,
v2_active: bool,
) -> Result<(), ConfigError> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The logic for determining if the legacy Telegram v1 channel is active (checking both wasm_channels_enabled and configured_wasm_channels) is currently described in the docstring but left to the caller to implement. To prevent logic drift and ensure consistency across potential call sites, consider changing this function to take a reference to ChannelsConfig and encapsulating that check within the function body. This would make the validator more robust and easier to use correctly.

…Config::resolve

Two related changes addressing gemini-code-assist review comments on
PR #3356:

1. Wire `validate_telegram_v1_v2_exclusivity` into `ChannelsConfig::resolve`.
   The validator was exported but had no production caller — only its own
   unit tests invoked it. The intent (per CLAUDE.md and the issue body)
   was that startup would call it before binding the telegram webhook
   route. Moving the call inside `resolve` enforces the invariant
   eagerly during `Config::from_env` / `Config::from_db` rather than
   relying on a yet-to-be-written startup gate, and gives every consumer
   of `ChannelsConfig` the guarantee for free.

2. Change the validator signature from `(v1_active: bool, v2_active: bool)`
   to `(channels: &ChannelsConfig)`. The "v1 active" rule (wasm_channels
   enabled AND telegram listed in `configured_wasm_channels`) was
   documented in the docstring and left to the caller — a classic
   logic-drift seam. Encapsulating both axes inside the validator
   collapses them to one source of truth.

Updates the 4 existing unit tests to construct minimal `ChannelsConfig`
fixtures, adds 3 new cases covering the previously-uncovered partial-v1
states (telegram listed but wasm disabled; wasm enabled but telegram not
listed) plus a `resolve`-end-to-end regression test that drives the
full env→ConfigError path. Rewrites
`tests/telegram_v2_default_off_integration.rs` against the new
`&ChannelsConfig` shape.

`ConfigError::InvalidValue { key, message }` is the right variant here
and is preserved unchanged — there is no `MalformedConfig` variant in
this codebase, despite the bot's suggestion.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size: M 50-199 changed lines label May 11, 2026
Followup to cfd4a3b — two `validate_telegram_v1_v2_exclusivity` calls
chained with `.expect`/`.expect_err` exceeded rustfmt's line width.
Wrap the call and the .expect onto separate lines to satisfy `cargo fmt
--all -- --check`. No behavior change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@henrypark133 henrypark133 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.

Finding: the Telegram v1/v2 exclusivity guard misses persisted and hot-activated legacy Telegram channels.

validate_telegram_v1_v2_exclusivity only treats v1 as active when wasm_channels_enabled is true and configured_wasm_channels contains "telegram". But configured_wasm_channels is documented as the setup-wizard startup fallback, separate from runtime activated_channels. Startup loads persisted active WASM channel names from the extension manager, passes them into setup_wasm_channels, and auto-activates persisted WASM channels independently of configured_wasm_channels.

That leaves an existing installation with legacy Telegram active via persisted activated_channels able to start with REBORN_TELEGRAM_V2_ENABLED=true and bypass the fail-closed guard, despite both paths being active for the same webhook installation. Please move/extend the guard to the runtime activation path as well, or feed persisted active channel state into the startup validation, and add coverage for the persisted-active Telegram case.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds default-off configuration plumbing for the Reborn Telegram v2 ProductAdapter and enforces a fail-closed startup invariant that prevents v1 (legacy WASM channel) and v2 from being enabled for the same Telegram installation at the same time.

Changes:

  • Introduce REBORN_TELEGRAM_V2_ENABLED (default false) and plumb it through ChannelsConfig::reborn_telegram_v2_enabled.
  • Add validate_telegram_v1_v2_exclusivity() and invoke it from ChannelsConfig::resolve() to enforce v1/v2 mutual exclusivity at config resolution time.
  • Add integration + unit tests to pin the default-off behavior and exclusivity matrix; update test helpers to include the new config field.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/telegram_v2_default_off_integration.rs Adds caller-level integration coverage for the v1/v2 exclusivity validator and default-off contract.
src/tunnel/mod.rs Updates channel-config test helpers to include the new reborn_telegram_v2_enabled field.
src/config/mod.rs Re-exports the validator and ensures Config::for_testing() defaults v2 to disabled.
src/config/channels.rs Adds reborn_telegram_v2_enabled, parses REBORN_TELEGRAM_V2_ENABLED, and enforces v1/v2 exclusivity during resolve; adds unit tests.
Cargo.toml Extends workspace exclude list (now includes a v2 Telegram path).
.env.example Documents the new default-off env flag and the fail-closed startup behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +56 to +61
let err = validate_telegram_v1_v2_exclusivity(&channels_cfg(true, true, true))
.expect_err("simultaneous v1+v2 must reject");
let rendered = err.to_string();
assert!(rendered.contains("REBORN_TELEGRAM_V2_ENABLED"));
assert!(rendered.contains("3285"));
}
Comment thread src/config/channels.rs
Comment on lines +568 to 582
fn resolve_rejects_v1_and_v2_telegram_together() {
let _guard = lock_env();
// SAFETY: under ENV_MUTEX
unsafe { std::env::set_var("REBORN_TELEGRAM_V2_ENABLED", "true") };
let mut settings = Settings::default();
settings.channels.wasm_channels_enabled = true;
settings.channels.wasm_channels = vec!["telegram".to_string()];
let err = ChannelsConfig::resolve(&settings, "owner").expect_err("must reject");
assert!(
matches!(err, ConfigError::InvalidValue { ref key, .. } if key == "REBORN_TELEGRAM_V2_ENABLED"),
"expected REBORN_TELEGRAM_V2_ENABLED InvalidValue, got: {err:?}"
);
// SAFETY: under ENV_MUTEX
unsafe { std::env::remove_var("REBORN_TELEGRAM_V2_ENABLED") };
}
Comment thread Cargo.toml Outdated
"channels-src/telegram",
"channels-src/slack",
"channels-src/whatsapp",
"channels-src-v2/telegram",
nickpismenkov added a commit that referenced this pull request May 12, 2026
Henry's review on PR #3356 flagged a startup-bypass: the exclusivity
guard ran only at `ChannelsConfig::resolve` time and only consulted the
env-var view of v1 (`configured_wasm_channels`). But persisted
`activated_channels` rows can carry `telegram` independently of
`WASM_CHANNELS`, and `setup_wasm_channels` auto-loads persisted-active
channels at startup. So a deployment with:

- `REBORN_TELEGRAM_V2_ENABLED=true`
- `WASM_CHANNELS` env var that does NOT list `telegram`
- persisted `activated_channels` row that DOES list `telegram`

…would pass the env-tier guard, then have both v1 (via persisted
auto-activate) and v2 (via env flag) stand up for the same Telegram
webhook installation.

Fix:

- `validate_telegram_v1_v2_exclusivity` gains an
  `Option<&HashSet<String>>` argument carrying the persisted-active
  WASM channel set. v1 is now considered active when
  `wasm_channels_enabled && (telegram listed in env OR telegram in
  persisted-active)`. The Option lets `ChannelsConfig::resolve` keep
  calling it (env-only) without inventing fake state.
- `src/main.rs` re-runs the validator with `Some(&startup_active_wasm_channels)`
  right after the set is computed and before `setup_wasm_channels`
  fires. Fail-closed: a hit returns the same `ConfigError::InvalidValue`
  via `?` and aborts startup.
- Three new unit tests in `config::channels::telegram_v2_tests` and
  three new caller-tier tests in
  `tests/telegram_v2_default_off_integration.rs` cover the new axis:
  persisted telegram + v2 blocks; persisted non-telegram + v2 allows;
  persisted telegram with `wasm_channels_enabled=false` allows (setup
  doesn't run, the set is dormant).

Other reviewer findings addressed:

- Copilot #1 (`tests/.../integration.rs:61`): replaced the brittle
  `Display`-substring assertion with a structured
  `match err { ConfigError::InvalidValue { key, .. } => ... }`. Wording
  changes to the message no longer break the test.
- Copilot #2 (`channels.rs:582`): the unsafe `set_var` / `remove_var`
  pair in `resolve_rejects_v1_and_v2_telegram_together` clobbered any
  pre-existing developer-shell value. Replaced with a local `ScopedEnv`
  RAII guard that saves the previous value on `set` and restores it on
  drop (still gated by `lock_env()` for cross-test serialization).
@nickpismenkov

Copy link
Copy Markdown
Contributor

Pushed af4dfa160 (merge) + 284eff06e (review fixes). Summary:

@henrypark133 — persisted-active v1 gap (your finding):

You're right — the env-only guard let an installation with persisted activated_channels.telegram = true start up alongside REBORN_TELEGRAM_V2_ENABLED=true even when WASM_CHANNELS did not list telegram. setup_wasm_channels auto-loads persisted-active channels independently of the env list, so both paths would have stood up for the same webhook installation.

Fix:

  • validate_telegram_v1_v2_exclusivity now takes Option<&HashSet<String>> carrying the persisted-active WASM channel set. v1 is active when wasm_channels_enabled && (telegram listed in env OR telegram in persisted-active). The Option lets ChannelsConfig::resolve continue invoking it env-only (no DB yet) without inventing fake state.
  • src/main.rs re-invokes the validator with Some(&startup_active_wasm_channels) right after that set is resolved and before setup_wasm_channels runs. Fail-closed via ? — same ConfigError::InvalidValue returned on either tier, so the existing error variant pins the contract.
  • Three new unit tests (config::channels::telegram_v2_tests) and three new caller-tier tests (tests/telegram_v2_default_off_integration.rs) cover the persisted axis: persisted telegram + v2 blocks; persisted non-telegram + v2 allows; persisted telegram with wasm_channels_enabled=false allows (setup is dormant, persisted set is moot).

@copilot — inline findings:

  • Move whatsapp channel source to channels-src/ for consistency #1 (tests/...:61 brittle Display assertion): replaced rendered.contains("REBORN_TELEGRAM_V2_ENABLED") + contains("3285") with structural match err { ConfigError::InvalidValue { key, .. } => assert_eq!(key, "REBORN_TELEGRAM_V2_ENABLED") }. Wording changes to the message no longer break the regression.
  • feat: adding Web UI #2 (channels.rs:582 env-var clobber): the unsafe set_var / remove_var pair in resolve_rejects_v1_and_v2_telegram_together clobbered any developer-shell value of REBORN_TELEGRAM_V2_ENABLED. Replaced with a local ScopedEnv RAII guard that captures the previous value on set and restores it on drop (still gated by lock_env() so concurrent tests stay serialized).
  • Onboarding: show Telegram in channel selection and auto-install bundled channel #3 (Cargo.toml:8 dead workspace exclude): confirmed channels-src-v2/telegram does not exist on this branch and dropped the entry while resolving the reborn-integration merge.

@gemini-code-assist: both findings — (1) fold the validator call into ChannelsConfig::resolve, (2) take &ChannelsConfig and encapsulate the v1-active check inside the function — were already landed in cfd4a3b2a before this round. Today's change preserves both: the function still takes &ChannelsConfig and still derives v1_active/v2_active internally; it just gains a second input axis for the runtime tier.

10 unit tests + 10 integration tests pass; cargo fmt --all --check and cargo clippy --workspace --all-features --tests clean.

@github-actions github-actions Bot added size: M 50-199 changed lines scope: agent Agent core (agent loop, router, scheduler) scope: channel Channel infrastructure scope: channel/cli TUI / CLI channel scope: channel/web Web gateway channel scope: channel/wasm WASM channel runtime scope: tool Tool infrastructure and removed size: M 50-199 changed lines labels May 12, 2026
Henry's review on PR #3356 flagged a startup-bypass: the exclusivity
guard ran only at `ChannelsConfig::resolve` time and only consulted the
env-var view of v1 (`configured_wasm_channels`). But persisted
`activated_channels` rows can carry `telegram` independently of
`WASM_CHANNELS`, and `setup_wasm_channels` auto-loads persisted-active
channels at startup. So a deployment with:

- `REBORN_TELEGRAM_V2_ENABLED=true`
- `WASM_CHANNELS` env var that does NOT list `telegram`
- persisted `activated_channels` row that DOES list `telegram`

…would pass the env-tier guard, then have both v1 (via persisted
auto-activate) and v2 (via env flag) stand up for the same Telegram
webhook installation.

Fix:

- `validate_telegram_v1_v2_exclusivity` gains an
  `Option<&HashSet<String>>` argument carrying the persisted-active
  WASM channel set. v1 is now considered active when
  `wasm_channels_enabled && (telegram listed in env OR telegram in
  persisted-active)`. The Option lets `ChannelsConfig::resolve` keep
  calling it (env-only) without inventing fake state.
- `src/main.rs` re-runs the validator with
  `settings_persistence_available.then_some(&persisted_active_wasm_channels)`
  right before `setup_wasm_channels` fires. Fail-closed: a hit returns
  the same `ConfigError::InvalidValue` via `?` and aborts startup.
- Three new unit tests in `config::channels::telegram_v2_tests` and
  three new caller-tier tests in
  `tests/telegram_v2_default_off_integration.rs` cover the new axis:
  persisted telegram + v2 blocks; persisted non-telegram + v2 allows;
  persisted telegram with `wasm_channels_enabled=false` allows (setup
  doesn't run, the set is dormant).

Other reviewer findings addressed:

- Copilot #1 (`tests/.../integration.rs:61`): replaced the brittle
  `Display`-substring assertion with a structured
  `match err { ConfigError::InvalidValue { key, .. } => ... }`. Wording
  changes to the message no longer break the test.
- Copilot #2 (`channels.rs:582`): the unsafe `set_var` / `remove_var`
  pair in `resolve_rejects_v1_and_v2_telegram_together` clobbered any
  pre-existing developer-shell value. Replaced with a local `ScopedEnv`
  RAII guard that saves the previous value on `set` and restores it on
  drop (still gated by `lock_env()` for cross-test serialization).
@nickpismenkov
nickpismenkov force-pushed the split/pr3316-06-telegram-v2-default-off-config branch from 284eff0 to 6f24585 Compare May 12, 2026 22:22
@github-actions github-actions Bot added size: L 200-499 changed lines risk: medium Business logic, config, or moderate-risk modules and removed size: M 50-199 changed lines size: XL 500+ changed lines risk: high Safety, secrets, auth, or critical infrastructure labels May 12, 2026
Resolves merge conflicts against the PR's base branch:

- `Cargo.toml`: drop dead `channels-src-v2/telegram` exclude (path does
  not exist on this branch); take `crates/ironclaw_silk_decoder` from
  the base (Copilot #3 finding).
- `src/main.rs`: the base refactored where the persisted-active WASM
  channel set is computed (now `startup_active_wasm_channels`,
  resolved inside the `wasm_channels_enabled` block via
  `load_startup_active_channels` + `startup_active_wasm_channel_names`).
  Keep that resolution, then re-run the v1/v2 exclusivity validator
  with `Some(&startup_active_wasm_channels)` on the new variable name
  (still ahead of `setup_wasm_channels`, still fail-closed).

No behavior change beyond reconciling the two histories.
@github-actions github-actions Bot added risk: low Changes to docs, tests, or low-risk modules and removed risk: medium Business logic, config, or moderate-risk modules labels May 12, 2026
@henrypark133
henrypark133 requested a review from Copilot May 12, 2026 22:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comment thread src/config/channels.rs Outdated
Comment on lines +499 to +503
.configured_wasm_channels
.iter()
.any(|c| c == "telegram");
let v1_telegram_persisted =
persisted_active_wasm_channels.is_some_and(|active| active.iter().any(|c| c == "telegram"));

@henrypark133 henrypark133 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.

Thanks for the follow-up fixes here. I rechecked the current head and the earlier persisted-startup bypass is fixed: startup now re-runs the Telegram v1/v2 exclusivity validator with the persisted activated_channels set before setup_wasm_channels, and the new tests cover the persisted-active Telegram case.

What looks good:

  • ChannelsConfig::resolve now rejects env-configured v1 Telegram plus REBORN_TELEGRAM_V2_ENABLED=true.
  • Startup now rejects persisted-active telegram plus v2 before the legacy WASM channel is restored.
  • The tests cover the persisted-active Telegram case and avoid brittle error-string assertions.
  • CI is meaningful and green, and the targeted local tests below passed.

Concerning: hot activation can still bypass the v1/v2 exclusivity guard

The exclusivity check only runs during config resolution and startup. ExtensionManager::activate_wasm_channel can still later activate the legacy telegram WASM channel and persist it without knowing whether reborn_telegram_v2_enabled is true.

That leaves a remaining sequence where the process starts cleanly with v2 enabled and no active v1 channel, then a user activates the legacy telegram extension through /api/extensions/telegram/activate or the equivalent extension activation path. At that point both paths can be enabled for the same Telegram installation despite the startup guard.

Please thread the v2 flag or a small activation policy into the extension/channel activation path and reject activation of the legacy telegram WASM channel when v2 is enabled. Add caller-level coverage for the activation path, not just the validator helper.

Checks I ran:

  • git diff --check origin/review-base-pr3356...HEAD
  • cargo test --test telegram_v2_default_off_integration -- --nocapture
  • cargo test -p ironclaw config::channels::telegram_v2_tests --lib -- --nocapture
  • cargo test -p ironclaw extensions::manager::tests::test_activate_wasm_channel_finds_legacy_hyphen_alias --lib -- --nocapture

Base automatically changed from split/pr3316-05-telegram-v2-product-adapter to reborn-integration May 13, 2026 00:23
Henry's CHANGES_REQUESTED review on PR #3356 flagged that the startup
exclusivity guard does not protect against runtime activation: a clean
v2-only start followed by a `/api/extensions/telegram/activate` (or
the equivalent `ToolDispatcher::dispatch` call) would activate the
legacy v1 `telegram` WASM channel alongside v2, despite the
config-resolve and startup-time validators.

Fix:

- `ExtensionManager` gains a `reborn_telegram_v2_enabled: AtomicBool`
  field plus `set_reborn_telegram_v2_enabled(bool)` /
  `reborn_telegram_v2_enabled()` accessors. The host wires the flag
  at startup from `config.channels.reborn_telegram_v2_enabled` in
  `app.rs::init_extensions`.
- `ExtensionManager::activate_wasm_channel` now consults the flag
  before any other work: when `true`, the legacy `telegram` channel
  is rejected with a typed `ExtensionError::ActivationFailed` whose
  message names `REBORN_TELEGRAM_V2_ENABLED` and issue #3285.
- Canonicalize the input via `ironclaw_common::ExtensionName::new`
  inside the guard so non-canonical aliases (` telegram `,
  `tele-gram`) fail closed at the same shape activation accepts them.
- Three new caller-level tests
  (`test_activate_wasm_channel_rejects_legacy_telegram_when_v2_enabled`,
  `..._rejects_legacy_telegram_with_whitespace_when_v2_enabled`,
  `..._allows_non_telegram_when_v2_enabled`) drive
  `activate_wasm_channel` directly and assert the v2 exclusivity
  message fires for canonical / non-canonical telegram aliases and
  does NOT fire for other channels.

Copilot's adjacent finding on `src/config/channels.rs:503`:

- The validator previously compared via raw `c == "telegram"`.
  Startup activation canonicalizes through `ExtensionName` (trims
  whitespace, folds hyphens), so non-canonical inputs would pass the
  validator but still bind as `telegram` at activation. Replaced the
  raw equality with `ExtensionName::new(c).map(|n| n.as_str() ==
  "telegram")` so both the env-tier and the persisted-tier checks use
  the same canonicalization that `ExtensionManager` does.
- Two new unit tests
  (`non_canonical_telegram_name_in_configured_list_still_blocks_v2`,
  `non_canonical_telegram_name_in_persisted_set_still_blocks_v2`)
  prove ` telegram ` is canonicalized and rejected on both axes.

Verification:

- 12 unit tests under `config::channels::telegram_v2_tests` pass
  (was 10 — +2 for canonicalization).
- 5 `test_activate_wasm_channel_*` tests pass (was 2 — +3 for the
  v2 exclusivity guard).
- `cargo test --test telegram_v2_default_off_integration` 10/10
  pass.
- `cargo fmt --all --check` clean.
- `cargo clippy --workspace --all-features --tests` zero warnings.
@nickpismenkov

Copy link
Copy Markdown
Contributor

Pushed 8a6531591 addressing both new review rounds:

@henrypark133 — hot-activation bypass (your CHANGES_REQUESTED finding)

You're right — the config-resolve and startup-time validators don't cover the runtime path. A clean v2-only boot followed by /api/extensions/telegram/activate (or the ToolDispatcher::dispatch equivalent) would activate the legacy v1 telegram WASM channel alongside v2.

Fix threads the flag through to the activation path itself:

  • ExtensionManager gains a reborn_telegram_v2_enabled: AtomicBool field (parallels the existing gateway_mode pattern) with set_reborn_telegram_v2_enabled(bool) / reborn_telegram_v2_enabled() accessors.
  • src/app.rs::init_extensions wires the flag right after manager construction, from self.config.channels.reborn_telegram_v2_enabled.
  • ExtensionManager::activate_wasm_channel consults the flag before any other work. When true and the canonicalized input equals "telegram", the activation returns ExtensionError::ActivationFailed with a message naming REBORN_TELEGRAM_V2_ENABLED and issue [Reborn] Migrate external channel adapters onto ProductAdapter contract #3285. No channel-runtime lookup, no loader call, no active_channel_names write.
  • The guard canonicalizes via ironclaw_common::ExtensionName::new(name) so non-canonical aliases (telegram, future hyphen variants) fail closed at the same shape activation accepts them. (Same canonicalization Copilot asked the validator to apply — applied consistently in both places now.)

Three new caller-level tests drive activate_wasm_channel directly (not just the helper):

  • test_activate_wasm_channel_rejects_legacy_telegram_when_v2_enabled — canonical name, v2 flag set, asserts error message names the flag and active_channel_names does not contain telegram.
  • test_activate_wasm_channel_rejects_legacy_telegram_with_whitespace_when_v2_enabled — non-canonical telegram, asserts the canonicalization in the guard catches it.
  • test_activate_wasm_channel_allows_non_telegram_when_v2_enabled — guard does NOT fire for other channels (slack, discord, …). The v2 exclusivity is targeted, not blanket.

@copilot — validator canonicalization (your inline on channels.rs:503)

Replaced the raw c == "telegram" equality with ExtensionName::new(c).map(|n| n.as_str() == "telegram") so both axes (env-tier configured_wasm_channels and runtime-tier persisted-active set) use the same canonicalization that ExtensionManager does. Pulled into a local is_telegram_after_canonicalize helper so it's applied identically on both branches.

Two new unit tests cover the canonicalization contract on each axis:

  • non_canonical_telegram_name_in_configured_list_still_blocks_v2
  • non_canonical_telegram_name_in_persisted_set_still_blocks_v2

Both feed telegram (whitespace-padded) and assert the validator rejects.

Verification

  • 12 unit tests under config::channels::telegram_v2_tests (was 10 — +2 for canonicalization axes).
  • 5 test_activate_wasm_channel_* tests (was 2 — +3 for v2 exclusivity guard).
  • 10 caller-level integration tests under tests/telegram_v2_default_off_integration.rs.
  • cargo fmt --all --check clean.
  • cargo clippy --workspace --all-features --tests zero warnings.

Checks you can re-run:

cargo test -p ironclaw config::channels::telegram_v2_tests --lib -- --nocapture
cargo test -p ironclaw test_activate_wasm_channel --lib -- --nocapture
cargo test --test telegram_v2_default_off_integration -- --nocapture

@github-actions github-actions Bot added risk: medium Business logic, config, or moderate-risk modules and removed risk: low Changes to docs, tests, or low-risk modules labels May 13, 2026

@henrypark133 henrypark133 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.

Review: approve Telegram v2 default-off guard

The latest follow-up in 8a6531591 resolves my previous blocker. Runtime activation of the legacy telegram WASM channel now fails closed when REBORN_TELEGRAM_V2_ENABLED=true, and the flag is wired into ExtensionManager from app.rs.

What looks good:

  • The config-time and startup-time validators now cover env-configured and persisted-active legacy Telegram channels.
  • The activation path itself now rejects hot activation of legacy Telegram while v2 owns the Telegram webhook installation.
  • The guard canonicalizes extension names before comparison, so non-canonical Telegram aliases do not bypass the check.
  • The targeted config, activation-path, and integration tests pass locally, and CI is green.

Merge-order note:

  • This branch still carries stacked Telegram v2 adapter commits from #3355, so #3355 should land first or this branch should be rebased after it lands.

@nickpismenkov
nickpismenkov merged commit b89b33e into reborn-integration May 13, 2026
15 checks passed
@nickpismenkov
nickpismenkov deleted the split/pr3316-06-telegram-v2-default-off-config branch May 13, 2026 03:18
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
…Config::resolve

Two related changes addressing gemini-code-assist review comments on
PR nearai#3356:

1. Wire `validate_telegram_v1_v2_exclusivity` into `ChannelsConfig::resolve`.
   The validator was exported but had no production caller — only its own
   unit tests invoked it. The intent (per CLAUDE.md and the issue body)
   was that startup would call it before binding the telegram webhook
   route. Moving the call inside `resolve` enforces the invariant
   eagerly during `Config::from_env` / `Config::from_db` rather than
   relying on a yet-to-be-written startup gate, and gives every consumer
   of `ChannelsConfig` the guarantee for free.

2. Change the validator signature from `(v1_active: bool, v2_active: bool)`
   to `(channels: &ChannelsConfig)`. The "v1 active" rule (wasm_channels
   enabled AND telegram listed in `configured_wasm_channels`) was
   documented in the docstring and left to the caller — a classic
   logic-drift seam. Encapsulating both axes inside the validator
   collapses them to one source of truth.

Updates the 4 existing unit tests to construct minimal `ChannelsConfig`
fixtures, adds 3 new cases covering the previously-uncovered partial-v1
states (telegram listed but wasm disabled; wasm enabled but telegram not
listed) plus a `resolve`-end-to-end regression test that drives the
full env→ConfigError path. Rewrites
`tests/telegram_v2_default_off_integration.rs` against the new
`&ChannelsConfig` shape.

`ConfigError::InvalidValue { key, message }` is the right variant here
and is preserved unchanged — there is no `MalformedConfig` variant in
this codebase, despite the bot's suggestion.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
Henry's review on PR nearai#3356 flagged a startup-bypass: the exclusivity
guard ran only at `ChannelsConfig::resolve` time and only consulted the
env-var view of v1 (`configured_wasm_channels`). But persisted
`activated_channels` rows can carry `telegram` independently of
`WASM_CHANNELS`, and `setup_wasm_channels` auto-loads persisted-active
channels at startup. So a deployment with:

- `REBORN_TELEGRAM_V2_ENABLED=true`
- `WASM_CHANNELS` env var that does NOT list `telegram`
- persisted `activated_channels` row that DOES list `telegram`

…would pass the env-tier guard, then have both v1 (via persisted
auto-activate) and v2 (via env flag) stand up for the same Telegram
webhook installation.

Fix:

- `validate_telegram_v1_v2_exclusivity` gains an
  `Option<&HashSet<String>>` argument carrying the persisted-active
  WASM channel set. v1 is now considered active when
  `wasm_channels_enabled && (telegram listed in env OR telegram in
  persisted-active)`. The Option lets `ChannelsConfig::resolve` keep
  calling it (env-only) without inventing fake state.
- `src/main.rs` re-runs the validator with
  `settings_persistence_available.then_some(&persisted_active_wasm_channels)`
  right before `setup_wasm_channels` fires. Fail-closed: a hit returns
  the same `ConfigError::InvalidValue` via `?` and aborts startup.
- Three new unit tests in `config::channels::telegram_v2_tests` and
  three new caller-tier tests in
  `tests/telegram_v2_default_off_integration.rs` cover the new axis:
  persisted telegram + v2 blocks; persisted non-telegram + v2 allows;
  persisted telegram with `wasm_channels_enabled=false` allows (setup
  doesn't run, the set is dormant).

Other reviewer findings addressed:

- Copilot #1 (`tests/.../integration.rs:61`): replaced the brittle
  `Display`-substring assertion with a structured
  `match err { ConfigError::InvalidValue { key, .. } => ... }`. Wording
  changes to the message no longer break the test.
- Copilot #2 (`channels.rs:582`): the unsafe `set_var` / `remove_var`
  pair in `resolve_rejects_v1_and_v2_telegram_together` clobbered any
  pre-existing developer-shell value. Replaced with a local `ScopedEnv`
  RAII guard that saves the previous value on `set` and restores it on
  drop (still gated by `lock_env()` for cross-test serialization).
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
Henry's CHANGES_REQUESTED review on PR nearai#3356 flagged that the startup
exclusivity guard does not protect against runtime activation: a clean
v2-only start followed by a `/api/extensions/telegram/activate` (or
the equivalent `ToolDispatcher::dispatch` call) would activate the
legacy v1 `telegram` WASM channel alongside v2, despite the
config-resolve and startup-time validators.

Fix:

- `ExtensionManager` gains a `reborn_telegram_v2_enabled: AtomicBool`
  field plus `set_reborn_telegram_v2_enabled(bool)` /
  `reborn_telegram_v2_enabled()` accessors. The host wires the flag
  at startup from `config.channels.reborn_telegram_v2_enabled` in
  `app.rs::init_extensions`.
- `ExtensionManager::activate_wasm_channel` now consults the flag
  before any other work: when `true`, the legacy `telegram` channel
  is rejected with a typed `ExtensionError::ActivationFailed` whose
  message names `REBORN_TELEGRAM_V2_ENABLED` and issue nearai#3285.
- Canonicalize the input via `ironclaw_common::ExtensionName::new`
  inside the guard so non-canonical aliases (` telegram `,
  `tele-gram`) fail closed at the same shape activation accepts them.
- Three new caller-level tests
  (`test_activate_wasm_channel_rejects_legacy_telegram_when_v2_enabled`,
  `..._rejects_legacy_telegram_with_whitespace_when_v2_enabled`,
  `..._allows_non_telegram_when_v2_enabled`) drive
  `activate_wasm_channel` directly and assert the v2 exclusivity
  message fires for canonical / non-canonical telegram aliases and
  does NOT fire for other channels.

Copilot's adjacent finding on `src/config/channels.rs:503`:

- The validator previously compared via raw `c == "telegram"`.
  Startup activation canonicalizes through `ExtensionName` (trims
  whitespace, folds hyphens), so non-canonical inputs would pass the
  validator but still bind as `telegram` at activation. Replaced the
  raw equality with `ExtensionName::new(c).map(|n| n.as_str() ==
  "telegram")` so both the env-tier and the persisted-tier checks use
  the same canonicalization that `ExtensionManager` does.
- Two new unit tests
  (`non_canonical_telegram_name_in_configured_list_still_blocks_v2`,
  `non_canonical_telegram_name_in_persisted_set_still_blocks_v2`)
  prove ` telegram ` is canonicalized and rejected on both axes.

Verification:

- 12 unit tests under `config::channels::telegram_v2_tests` pass
  (was 10 — +2 for canonicalization).
- 5 `test_activate_wasm_channel_*` tests pass (was 2 — +3 for the
  v2 exclusivity guard).
- `cargo test --test telegram_v2_default_off_integration` 10/10
  pass.
- `cargo fmt --all --check` clean.
- `cargo clippy --workspace --all-features --tests` zero warnings.
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
…2-default-off-config

feat(reborn): add telegram v2 default-off config guard
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: core 20+ merged PRs risk: medium Business logic, config, or moderate-risk modules scope: agent Agent core (agent loop, router, scheduler) scope: channel/cli TUI / CLI channel scope: channel/wasm WASM channel runtime scope: channel/web Web gateway channel scope: channel Channel infrastructure scope: ci CI/CD workflows scope: config Configuration scope: db/postgres PostgreSQL backend scope: db Database trait / abstraction scope: dependencies Dependency updates scope: docs Documentation scope: extensions Extension management scope: hooks Git/event hooks scope: orchestrator Container orchestrator scope: pairing Pairing mode scope: setup Onboarding / setup scope: tool/builder Dynamic tool builder scope: tool/builtin Built-in tools scope: tool/wasm WASM tool sandbox scope: tool Tool infrastructure scope: worker Container worker scope: workspace Persistent memory / workspace size: L 200-499 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants