feat(integrations): share one setup flow; require and resolve the Telegram chat id by muddlebee · Pull Request #4170 · Tracer-Cloud/opensre

muddlebee · 2026-07-21T11:03:54Z

Stacked on #4167 — base is refactor/shared-integration-setup-flow, not main. Review #4167 first; this PR's own diff is the last commit.

Relates to #4071. Follow-ups filed as #4168 (remaining 44 integrations) and #4169 (agent-driven setup).

Describe the changes you have made in this PR -

#4167 moved the .env/keyring writer down to config/ so integrations/ could reach it. Nothing used it yet — that PR changed no behavior. This one connects it.

The problem. Setting up an integration happens on three surfaces, and they persisted credentials differently:

Surface	store	keyring	`.env`
`opensre onboard`	✅	✅	✅
`opensre integrations setup telegram`	✅	❌	❌
`configure_telegram` action tool (proposed in #4071)	✅	❌	❌

Runtime hides this — integrations/telegram/credentials.py resolves the store first — so the CLI path looks fine right up until deploy, where platform/deployment/ecr_deploy/prep.py:75 checks the env var:

if not _env_set("TELEGRAM_BOT_TOKEN"):
    missing.append(DeployEnvIssue("telegram_bot", env_vars=("TELEGRAM_BOT_TOKEN",)))

Configure Telegram with the CLI, verify it, send a message — then watch make deploy tell you TELEGRAM_BOT_TOKEN is missing. Configure the same integration with onboard and deploy succeeds. This is not Telegram-specific: 0 of the 45 setup handlers in integrations/cli.py write the keyring or .env.

The change. integrations/setup_flow.py owns the whole lifecycle once — validate required fields → verify → resolve → persist to every tier — driven by a declarative spec:

TELEGRAM_SETUP = IntegrationSetupSpec(
    service="telegram",
    fields=(
        SetupField(name="bot_token", label="Telegram bot token",
                   env_var="TELEGRAM_BOT_TOKEN", secret=True),
        SetupField(name="default_chat_id", label="Default chat ID",
                   env_var="TELEGRAM_DEFAULT_CHAT_ID"),
    ),
    verify=verify_telegram,
    resolve=_resolve_default_chat_id,
)

Callers keep only their own value collection. _setup_telegram and _configure_telegram both shrink to a prompt pair plus one apply_setup(...) call, and neither decides where anything is stored — is_sensitive_env_key() routes by variable name, and the split is enforced (sync_env_values refuses a secret, sync_env_secret refuses a non-secret).

Two deliberate behavior changes, both Telegram-specific:

The chat id is now required. A bot token alone passes getMe, so setup reported success — but load_credentials_from_env() raises without a chat id, so the watchdog, Hermes sinks, and the send-message tool all fail. The old wizard printed a warning and saved anyway; that just moved the failure to the first real alert, hours later, far from the setup that caused it.

The chat id is resolved, not trusted. getChat accepts a numeric id or an @username, so users can supply a public channel's @name — much easier to obtain than digging a numeric id out of getUpdates — and it is stored as the numeric id, which survives a channel rename. More importantly it answers the question getMe cannot: can this bot see that chat? The most common Telegram misconfiguration is forgetting to add the bot to the channel, and that now fails at setup.

One limit is documented rather than hidden: getChat proves reachability, not posting rights. A channel bot still needs the Post Messages admin permission, and only an actual send would prove it.

Deleted: validate_telegram_bot, the wizard's httpx copy of the getMe probe. The registered verifier in integrations/telegram/verifier.py is now the single prober; the deleted copy's test coverage — including its token-redaction case — moved to tests/integrations/telegram/test_verifier.py rather than being dropped.

Demo/Screenshot for feature changes and bug fixes -

Code Understanding and AI Usage

Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?

No, I wrote all the code myself
Yes, I used AI assistance (continue below)

If you used AI assistance:

I have reviewed every single line of the AI-generated code
I can explain the purpose and logic of each function/component I added
I have tested edge cases and understand how the code handles them
I have modified the AI output to follow this project's coding standards and conventions

Explain your implementation approach:

Both Telegram setup paths had zero test coverage before #4167, so that PR characterized them first — prompts, validate-before-persist ordering, the wizard's retry loop, the exact credential shape, and the tiers each path wrote. Those tests are what made this change safe to make, and they are updated here only where behavior changed on purpose (chat id required, CLI path now writing all three tiers). Each such edit is called out in the test module docstrings, so a reviewer can tell an intended change from a regression.

The design question was where the "which tier does this value go to" decision lives. Three options:

A writer callback injected into each caller. Avoids moving code, but hands the decision back to every caller — precisely the drift that caused the bug.
Make upsert_integration mirror to the keyring and .env itself. Tempting, because it would fix all 45 handlers in one change. It does not work: the store key is bot_token while the env var is TELEGRAM_BOT_TOKEN, so a per-integration name mapping is required regardless. That mapping is the spec, so there is no way to avoid writing one per integration.
A declarative spec consumed by one flow — chosen. Callers declare what the integration needs; the flow decides where it goes. It also makes agent-driven setup tractable later (Agent-driven integration setup: one spec-driven action tool instead of one per integration #4169), since a spec is exactly the metadata an agent needs to know what to ask for.

verify is wired onto the spec as a direct function reference rather than looked up in the verifier registry. The caller already knows which integration it is configuring, and an explicit reference keeps the flow independent of import-time registration ordering — which also means tests can swap it with dataclasses.replace and still exercise the real field definitions instead of a stand-in spec.

The resolve hook exists because verification and normalization are genuinely different steps: verify answers "do these credentials work" and returns a status, while Telegram additionally needs to rewrite what the user typed. Folding that into the verifier would have changed the VerifierFn contract for all 45 integrations to serve one.

Not in this PR: the other 44 integrations (#4168) and the agent-driven setup tool (#4169). #4071 rebases onto this and shrinks to a thin adapter plus the execution gate it is currently missing.

Checklist before requesting a review

I have added proper PR title and linked to the issue
I have performed a self-review of my code
I can explain the purpose of every function, class, and logic block I added
I understand why my changes work and have tested them thoroughly
I have considered potential edge cases and how my code handles them
If it is a core feature, I have added thorough tests
My code follows the project's style guidelines and conventions

github-actions · 2026-07-21T11:04:03Z

Greptile code review

This repo uses Greptile for automated review. Before merge, aim for Confidence Score: 5/5 with zero unresolved review threads — see CONTRIBUTING.md.

Run a review — add a PR comment with:

@greptile review

Give it ~5-10 minutes (sometimes longer) for results, then fix feedback and re-trigger until you reach Confidence Score: 5/5.

Optional: automate with the greploop skill.

greptile-apps · 2026-07-21T11:11:41Z

Greptile Summary

This PR introduces a declarative IntegrationSetupSpec / apply_setup flow that consolidates credential persistence across the three Telegram setup surfaces (wizard, integrations setup, future agent tool). It fixes a real deployment bug where the CLI and agent paths wrote credentials only to the integration store, silently skipping the keyring and .env that the deploy preflight reads. Two intentional behavior changes: the default chat ID is now required (token-only setups produce an undeliverable integration), and the chat reference is resolved through getChat to a stable numeric ID before storage, which also catches the common "bot not added to channel" misconfiguration at setup time.

integrations/setup_flow.py — new central flow: validate required fields → verify → resolve → persist env/keyring first, then store; the write-order invariant is tested explicitly.
integrations/telegram/chat_lookup.py — new getChat probe that accepts @username or a numeric ID and returns the canonical numeric ID, failing setup if the bot cannot reach the chat.
integrations/telegram/setup.py — declarative TELEGRAM_SETUP spec wiring the two fields, the existing verifier, and the new resolver; both Telegram setup paths are reduced to prompt collection + one apply_setup(...) call.

Confidence Score: 5/5

Safe to merge. The three-tier write ordering is correct, the exception handling in apply_setup is sound, and the new behaviour changes (chat ID required, chat reference resolved at setup) are explicitly tested and documented.

The central apply_setup flow is well-structured: env/keyring writes before the store, _persist_env wrapped in a try/except, and verify_telegram's dict[str, Any] signature means the extra default_chat_id key passed by _verify is harmlessly ignored. The token-redaction guarantee is carried over from the deleted duplicate. Test coverage across all three surfaces (CLI, wizard, flow unit tests) is thorough and the write-ordering invariant is directly verified.

No files require special attention. The only finding is a documentation style nit in docs/messaging/telegram.mdx where tier-implementation prose can be trimmed per the new AGENTS.md guideline added in this same PR.

Important Files Changed

Filename	Overview
integrations/setup_flow.py	New central setup flow; write-ordering (env/keyring before store), exception handling for `_persist_env`, and all tier-coverage invariants are well-structured and explicitly tested.
integrations/telegram/chat_lookup.py	New `getChat` resolver; token redaction, blank-reference guard, and the "bot not in channel" failure case are all covered by dedicated tests.
integrations/telegram/setup.py	Declarative `TELEGRAM_SETUP` spec correctly maps both fields to their env-var tiers; `is_sensitive_env_key` routing confirmed against `verify_telegram`'s `dict[str, Any]` signature (extra keys ignored).
integrations/cli.py	New `_run_spec_setup` validates required fields per-field inside the prompt loop, so a blank token exits before the chat-ID prompt is shown — confirmed by the renamed characterization test.
surfaces/cli/wizard/configurators/chat_notifications.py	Wizard configurator correctly delegates to `apply_setup`; the `assert outcome.env_path is not None` guard addresses the earlier review thread; retry loop is preserved.
docs/messaging/telegram.mdx	Docs correctly add the `@channelname` shortcut Tip and the `getChat`-vs-post-permission Note, but the modified bullet points retain credential-tier prose that the new AGENTS.md guideline (added in this same PR) says to cut.
tests/integrations/test_setup_flow.py	Comprehensive: covers all tier writes, required-field failure, optional-field clearing, resolve rewrite, env-error abort, and write-ordering — including the `on_env`/`on_store` ordering hook.
tests/integrations/telegram/test_verifier.py	Ports the token-redaction guarantee (CWE-209) from the deleted `validate_telegram_bot` suite to the canonical verifier; all cases including the URL-embedded-token scenario are covered.
surfaces/cli/wizard/integration_validators/http_probe_validators.py	Correctly removes the now-redundant `validate_telegram_bot` function and its unused `redact_token` import; coverage moved to `test_verifier.py`.

Sequence Diagram

sequenceDiagram
    participant Surface as CLI / Wizard / Agent
    participant Flow as integrations/setup_flow.py
    participant Verifier as telegram/verifier.py
    participant Lookup as telegram/chat_lookup.py
    participant Env as config/env_file (keyring + .env)
    participant Store as integrations/store.py

    Surface->>Flow: "apply_setup(spec, {bot_token, chat_ref})"
    Flow->>Flow: _collect_credentials() — required field check
    Flow->>Verifier: "verify_telegram("setup", {bot_token, chat_ref})"
    Verifier-->>Flow: "{status: "passed", detail: "Connected to @bot"}"
    Flow->>Lookup: "resolve_chat_id(bot_token, "@acme_alerts")"
    Lookup-->>Flow: ("-1001234567890", "Acme Alerts (channel)", "")
    Flow->>Env: sync_env_secret("TELEGRAM_BOT_TOKEN", token)
    Flow->>Env: "sync_env_values({"TELEGRAM_DEFAULT_CHAT_ID": "-100..."})"
    Env-->>Flow: .env path
    Flow->>Store: "upsert_integration("telegram", {credentials: ...})"
    Flow-->>Surface: "SetupOutcome(ok=True, detail=..., env_path=...)"

_{Reviews (4): Last reviewed commit: "docs(agents): write docs/ for users, not..." | Re-trigger Greptile}

…egram chat id Every setup surface persisted credentials its own way. `opensre onboard` wrote the integration store, the keyring, and `.env`; `opensre integrations setup` wrote the store alone. Runtime hides the difference by resolving the store first, so it only surfaces in the deploy preflight, which reads env vars — you could configure Telegram, verify it, send messages, and still have `make deploy` report TELEGRAM_BOT_TOKEN as missing. `integrations/setup_flow.py` now owns validate -> verify -> resolve -> persist for every tier, driven by a declarative `IntegrationSetupSpec`. Telegram is migrated onto it as the reference; the remaining 44 handlers are tracked in a follow-up issue. Two behavior changes come with the Telegram spec: * the chat id is required. A bot token alone passes `getMe` but every delivery path raises without a chat id, so accepting a token-only setup produced an integration that looked healthy and failed at the first alert. * the chat id is resolved via `getChat` rather than trusted. Users may supply a public channel's `@name` — far easier to obtain than a numeric id — and it is stored as the numeric id, which survives a channel rename. A chat the bot cannot reach now fails setup instead of failing silently at delivery time. The wizard's httpx copy of the `getMe` probe is deleted; the registered verifier is the single prober. Its test coverage, including the token-redaction case, moves to tests/integrations/telegram/test_verifier.py.

* Persist env/keyring before the store. Previously the store was written first, so a failing `.env` write (PermissionError from write_env_lines on an unwritable file) left exactly the store-only state this module exists to prevent. With the order reversed, an env failure persists nothing at all, and a store failure still leaves a working integration because credential resolution falls back to the environment. The env write is also caught and reported as a failed SetupOutcome instead of propagating to the callers. * Restore fail-fast on a blank required field. Collecting every prompt before validating meant a user who skipped the token was still asked for the chat id before seeing the error. `_run_spec_setup` now checks each field as it is answered. It is spec-driven, so the remaining integrations get the same behavior when they migrate. * Narrow `outcome.env_path` before `str()` in the wizard, so a future change that stopped setting it would fail loudly rather than returning "None". Also drops `SetupOutcome.saved`, which vulture flagged: it was exactly `ok`. `SetupField.prompt` now carries the question text ("Default chat ID or @channelname") separately from `label`, which stays terse for error messages, and both surfaces read the prompt from the spec instead of hardcoding it.

muddlebee · 2026-07-21T18:36:25Z

@greptile review

…low (draft — blocked on Tracer-Cloud#4170) Add integrations/slack/setup.py with a declarative SLACK_SETUP_SPEC adapted to the IntegrationSetupSpec API from PR Tracer-Cloud#4170 (muddlebee). Consolidate the Slack verifier docstring to document it as the single source of truth for credential validation. This PR is blocked on Tracer-Cloud#4170 which introduces integrations/setup_flow.py. Once Tracer-Cloud#4170 lands, follow-up changes will migrate _setup_slack() in integrations/cli.py and _configure_slack() in chat_notifications.py onto apply_setup(SLACK_SETUP_SPEC, ...). AI-usage disclosure: Code was written and reviewed with AI assistance (opencode).

``_persist_env`` skipped blank non-secret fields, so clearing one left the old value in ``.env`` while the store recorded ``None``. Credential resolution falls back to the environment when the store is empty, so the value the user had just cleared kept resolving. Blank secrets were already cleared from the keyring — the two halves disagreed. Blank values are now written through, making ``KEY=`` in ``.env``, which loads as an empty string and reads as unset everywhere. Not reachable for Telegram, where both fields are required, but it is the shape an integration with an optional non-secret field would hit during the migration tracked in #4168. Also tells the user, up front, that the wizard's Telegram prompts are both mandatory and that Ctrl+C skips the integration without losing the rest of onboarding. Making the chat id required removed the previous "press Enter to skip" escape, and a bare "Required." did not say what to do instead.

CodeQL (py/import-and-import-from) flagged six new test modules for importing the same module with both ``import x`` and ``from x import y``. Each keeps the module alias — the monkeypatch seam these tests rely on — and drops the from-import, referencing symbols through the alias instead. No behavior change; the alias was already how every patched attribute was reached.

muddlebee · 2026-07-21T18:42:09Z

@greptile review

@name

The setup notes described getMe/getChat, the three credential tiers, and the deploy preflight — none of which a user configuring a bot acts on. Kept only what changes what they do: both answers are required, a public channel's @name works instead of hunting a numeric id, and the bot still needs Post Messages to actually post.

Adds a Docs section under Code Style. The rule is the test 'does this sentence change what the reader does?' — with the concrete cuts (vendor API endpoints, internal function names, which credential tier a value lands in, the bug a change fixed) and the keeps (required vs optional, shortcuts, invisible gotchas, exact commands). Prompted by the Telegram setup page picking up getMe/getChat, the three-tier write, and the deploy preflight — all true, none of it actionable by someone setting up a bot.

One test — does this sentence change what the reader does? — plus what to cut and what to keep. Prompted by the Telegram setup page picking up getMe/getChat, the three-tier write, and the deploy preflight: all true, none of it actionable by someone setting up a bot.

Devesh36

quite a good approach for this ! Nice work 👍

Davidson3556 · 2026-07-22T09:13:04Z

Nice consolidation, this is great.
One thing you need to look at is the comment in apply_setup says "if the env write fails nothing is persisted at all," but _persist_env writes each keyring secret before the final sync_env_values, so a .env PermissionError leaves the token in the keyring. The guarantee that actually holds is that the store is never left holding credentials alone — worth wording it that way.

muddlebee · 2026-07-22T09:24:45Z

demo updated

github-actions · 2026-07-22T09:28:26Z

🌮 @muddlebee's PR: showed up unannounced, improved everything, left zero bugs. Just like a perfect taco. 🌮

👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome.

greptile-apps Bot reviewed Jul 21, 2026

View reviewed changes

Comment thread integrations/setup_flow.py Outdated

Comment thread integrations/cli.py Outdated

Comment thread surfaces/cli/wizard/configurators/chat_notifications.py

muddlebee marked this pull request as draft July 21, 2026 11:21

Base automatically changed from refactor/shared-integration-setup-flow to main July 21, 2026 16:22

muddlebee added 2 commits July 21, 2026 22:08

muddlebee force-pushed the feat/shared-integration-setup branch from d4bc3d8 to b847b45 Compare July 21, 2026 16:44

github-advanced-security AI found potential problems Jul 21, 2026

View reviewed changes

github-code-quality Bot found potential problems Jul 21, 2026

View reviewed changes

muddlebee added 2 commits July 22, 2026 00:07

Drv4ever mentioned this pull request Jul 21, 2026

refactor(integrations): add Slack setup spec for shared apply_setup flow Drv4ever/opensre#1

Draft

Tracer-Cloud deleted a comment from Drv4ever Jul 21, 2026

muddlebee added 3 commits July 22, 2026 02:54

Devesh36 reviewed Jul 22, 2026

View reviewed changes

muddlebee marked this pull request as ready for review July 22, 2026 09:26

muddlebee merged commit 876e7b4 into main Jul 22, 2026
28 checks passed

muddlebee deleted the feat/shared-integration-setup branch July 22, 2026 09:28

Uh oh!

Conversation

muddlebee commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the changes you have made in this PR -

Demo/Screenshot for feature changes and bug fixes -

Code Understanding and AI Usage

Checklist before requesting a review

Uh oh!

github-actions Bot commented Jul 21, 2026

Greptile code review

Uh oh!

greptile-apps Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

muddlebee commented Jul 21, 2026

Uh oh!

muddlebee commented Jul 21, 2026

Uh oh!

Devesh36 left a comment

Choose a reason for hiding this comment

Uh oh!

Davidson3556 commented Jul 22, 2026

Uh oh!

muddlebee commented Jul 22, 2026

Uh oh!

Uh oh!

github-actions Bot commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

muddlebee commented Jul 21, 2026 •

edited

Loading

greptile-apps Bot commented Jul 21, 2026 •

edited

Loading