fix(e2e): require active Telegram configuration - #9375
Conversation
Fixes NVIDIA#9361 Signed-off-by: Deepak Jain <deepujain@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe Telegram E2E baseline probe reports Telegram as configured only when both the channel and plugin entries are enabled. The test also documents sandbox-state and policy-preset checks. ChangesTelegram detection
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This localized test change distinguishes active Telegram configuration from disabled configuration, and no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Deepak Jain <deepujain@gmail.com>
|
Added the missing probe docstring. The live selector is manual-only, so the PR keeps the change limited to its baseline predicate. |
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Why this blocks
- Lines 285-339 add a two-mode probe, dynamically generated predicate, and two assertion wrappers for three phase checks.
- That abstraction hides the Telegram contract and already omits one required fact: the
activepredicate checks only channel and plugin flags, while the manifest renders an enabled default account and the production parser treats a channel as configured only when an account is enabled. - Extending the predicate again would leave another local definition of active configuration.
Refactor direction
- Replace the
active | presentmode and both wrappers with one sandbox probe that emits small structured, non-secret Telegram state: entry presence, channel enabled, plugin enabled, and whether any account is enabled. - Assert the expected fields directly at the baseline, add, and removal call sites.
- Follow the structured-config pattern in
test/e2e/live/messaging-providers-helpers.ts:492-525and the direct assertions intest/e2e/live/messaging-providers.test.ts:449-489.
Expected result
The account gap is fixed without growing a predicate mini-language, each phase states the contract it proves, and the mode dispatch plus wrapper layer disappears, reducing the change by about 10-15 lines.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Why this blocks
- The latest update extends the existing predicate string instead of removing the local state machine.
test/e2e/live/channels-add-remove.test.ts:285-294still defines anactive | presentmode, and lines 300-309 interpolate its two predicates into Python.- The account requirement was appended to the
activebranch while both wrappers remain at lines 325-340. - The three phase assertions still collapse distinct configuration facts into booleans. In particular, the baseline passes both for the expected bundled-but-disabled entry and for a missing or malformed entry.
Refactor direction
- Replace the mode and predicate interpolation with one sandbox probe that emits small, non-secret JSON state: channel entry present, channel enabled, plugin entry present, plugin enabled, and any account enabled.
- Parse that state once per phase and assert the exact baseline, added, and removed fields at the call sites.
- Delete
openClawTelegramMatches(),expectOpenClawTelegram(), andexpectOpenClawTelegramRemoved()rather than adding another clause to the local definition of active configuration.
Expected result
Each phase proves the state named by issue #9361, the account requirement remains covered, and the probe-mode dispatch plus wrapper layer disappears, reducing the change by roughly 10-15 lines with clearer failure evidence.
Signed-off-by: Deepak Jain <deepujain@gmail.com>
|
Reworked the probe in 5bfbe62/c1ce269f3. It now emits non-secret structured state for channel/plugin presence, enabled flags, and enabled accounts; each lifecycle phase asserts those fields directly. Aaron’s account/removal commits are preserved. |
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Re-reviewed exact commit c1ce269f366b7ad1796ab418da5a3c2080b9ec4b.
The prior predicate-state-machine blocker is resolved. readOpenClawTelegramState() now emits one non-secret structured state, and the baseline, active, and removed phases assert the exact channel, plugin, and account facts at their call sites. The active | present dispatch, interpolated predicate, and assertion wrappers are gone.
I found no new blocking LOC-reduction or codebase-simplicity finding at this commit. This is a scope-limited follow-up, not an approval or a correctness, security, or CI review.
Resolved by c1ce269: one structured Telegram-state probe now owns all phase assertions. See the current scope-limited simplicity follow-up.
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed exact commit c1ce269f366b7ad1796ab418da5a3c2080b9ec4b.
Security review:
- Secrets and credentials — PASS: the sandbox probe emits boolean activation state, not token values.
- Input validation — PASS: baseline, active, and removed phases assert exact channel, plugin, and account facts.
- Authentication and authorization — PASS: no access-control changes.
- Dependencies and supply chain — PASS: no dependency changes.
- Error handling and logging — PASS: malformed or unexpected config output fails the E2E assertion.
- Cryptography — PASS: no cryptographic changes.
- Policy and network isolation — PASS: the existing active-preset assertion remains in place.
- Testing and regression safety — PASS: the three lifecycle phases distinguish bundled-disabled, enabled, and removed state.
- System security — PASS: this tightens E2E evidence without changing production behavior.
Summary
Make the Telegram add/remove E2E baseline distinguish an active channel from a disabled channel bundled in the managed image. The predicate now requires both the Telegram channel and its plugin entry to be explicitly enabled.
Related Issue
Fixes #9361
Changes
channels.telegram.enabled: falseas inactive instead of relying on object presence.plugins.entries.telegram.enabledstate before reporting Telegram as configured.Type of Change
Quality Gates
DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablechannels-add-removetarget requires a reviewed E2E runner and was not dispatched locally.npm run build:cli;npm run typecheck:cli;npm run lintnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Deepak Jain deepujain@gmail.com
Summary by CodeRabbit
Bug Fixes
Documentation