refactor(config): move .env/keyring writing to config/ so every setup surface shares one writer

[muddlebee]

Relates to #4071, #3933

Describe the changes you have made in this PR -

Groundwork so that setting up an integration is written once, not once per surface. No user-visible behavior change.

The problem. Telegram (and every other integration) has three setup paths that drifted apart:

Path	Saves to
opensre onboard → surfaces/cli/wizard/configurators/chat_notifications.py	store + keyring + .env
opensre integrations setup telegram → integrations/cli.py	store only
configure_telegram action tool (proposed in #4071)	store only

All three look fine locally, because integrations/telegram/credentials.py resolves the store first. The divergence shows up at deploy time — platform/deployment/ecr_deploy/prep.py:75 checks the env var, not the store:

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

So you can configure Telegram via the CLI, verify it, send messages — and then have make deploy tell you TELEGRAM_BOT_TOKEN is missing. Configure it via onboard instead and the same deploy succeeds.

Why it drifted. The generic .env/keyring writer lived in surfaces/cli/wizard/env_sync.py. integrations/ (tier 2) and tools/ (tier 2) cannot import surfaces/ (tier 1), so the other two paths physically could not reach it and each wrote what they could.

This PR moves the generic half down to config/ — the tier-4 floor everything may import — and leaves the wizard-specific half behind:

Stays in surfaces/cli/wizard/env_sync.py	Moves to config/env_file.py
sync_provider_env, sync_reasoning_model_env (need ProviderOption)	sync_env_values, sync_env_secret
provider key ownership, stripping the old provider's keys on a switch	sensitive-key classification, .env line rewriting, 0o600 chmod, keyring persist

config/local_env.py already owned reading the project env file (get_project_env_path, _load_env_file); writing now sits beside it. No import-linter exception was needed — integrations → config is an allowed edge, so the follow-up needs no layering debt entry.

Per AGENTS.md there is no forwarding shim: all 17 importers move to the canonical path in this change.

Storage routing is decided by existing code, not by callers. is_sensitive_env_key() classifies on the variable name, and the split is enforced rather than advised — sync_env_values refuses a sensitive key and sync_env_secret refuses a non-sensitive one. A mis-named credential fails loudly instead of landing in clear text.

Demo/Screenshot for feature changes and bug fixes -

Routing is name-driven, so a new integration gets correct storage without the author deciding anything:

$ uv run python -c "from config.env_file import is_sensitive_env_key; ..."
TELEGRAM_BOT_TOKEN           -> keyring
TELEGRAM_DEFAULT_CHAT_ID     -> .env
DISCORD_PUBLIC_KEY           -> .env      # allowlisted: public despite ending in "key"
OPENAI_TOKEN_LIMIT           -> .env      # terminal word is "limit", not "token"

Layering holds, which is the whole point of the move:

$ uv run lint-imports
Analyzed 1331 files, 3662 dependencies.
Config is independent KEPT
Contracts: 1 kept, 0 broken.

Full gate:

$ make lint && make format-check
All checks passed!
2612 files already formatted

$ make typecheck
Success: no issues found in 1317 source files

$ make test-scope
2180 passed, 3 skipped, 2 warnings in 87.55s

---

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:

This is a behavior-preserving refactor, so it is TDD-guarded per AGENTS.md. Both Telegram setup paths had zero test coverage, so the first commit characterizes them against the pre-refactor code — prompts asked, validate-before-persist ordering, the wizard's retry loop on a bad token, the exact credential shape written, the delivery advisory when no chat id is set, and the credential tiers each path touches. Those 35 tests passed before the move and stayed green through it.

I deliberately did not pin "the CLI path writes only the store". That is the gap being closed, and asserting it would lock the bug in.

The alternative I considered was injecting a writer callback into integrations/ instead of moving the module — it avoids touching 17 files, but it pushes the decision of where credentials go back out to each caller, which is exactly the drift that caused this. Moving the writer to the floor means there is one answer and callers cannot disagree with it.

One real regression surfaced during the change, and it is worth calling out because it is the interesting part of the diff. sync_reasoning_model_env passed env_path=None and let sync_env_values fall back to its own module-level default. After the move that default resolved inside config/env_file.py, while the sibling writer sync_provider_env still resolved against env_sync.PROJECT_ENV_PATH — the name the .env-isolating test fixtures patch. The two writers disagreed about the target, so six tests escaped tmp_path and wrote to the real project .env. Fixed by resolving the default explicitly at the call site so both writers agree, and hardened by pinning the config.env_file default in both isolating fixtures (17 patch sites) so no test can reach the real file again.

Follow-up (not in this PR): with the writer reachable from integrations/, a shared apply_setup(service, values) can own merge → verify → persist-across-all-three-tiers, and the wizard, integrations setup, and the action tool from #4071 each keep only their own value collection. #4071 rebases onto that and shrinks to a thin adapter plus its execution gate.

---

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]

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]

Greptile Summary

This PR moves the .env/keyring writer from surfaces/cli/wizard/env_sync.py to config/env_file.py (the shared floor layer) so every setup surface — the onboarding wizard, opensre integrations setup, and future action tools — can share one credential-routing implementation. No user-visible behavior change is intended; the refactor closes the layering gap that prevented integrations/ from reaching the old writer, causing deploy checks to fail when credentials were configured via the CLI path.

• New config/env_file.py owns all .env/keyring write logic: sync_env_values for public config, sync_env_secret for keyring-bound secrets, write_env_lines for safe file writes, and is_sensitive_env_key for name-based routing.
• surfaces/cli/wizard/env_sync.py is trimmed to wizard-specific LLM provider logic (sync_provider_env, sync_reasoning_model_env) and now re-exports utilities from config.env_file.
• All 17 call-site imports are updated to the canonical path; no forwarding shim is added per project convention. A regression where sync_reasoning_model_env could resolve against a different PROJECT_ENV_PATH than sync_provider_env is fixed, and 17 test fixtures are updated to patch both path constants.

Confidence Score: 5/5

Safe to merge — the change is a behavior-preserving refactor with a green full-gate build and test suite, and the one real regression found during development (sync_reasoning_model_env resolving against the wrong path) is fixed with 17 test-fixture patches that prevent recurrence.

The shared writer in config/env_file.py faithfully reproduces the logic from env_sync.py. The previously-silent keyring failure now surfaces as an explicit RuntimeError, backed by a new test. The path-disagreement regression is fixed and hardened by fixture patches across all affected tests. Both new test files provide solid coverage of the pre-refactor contracts. The 17-site PROJECT_ENV_PATH patch pattern is applied consistently, and the import-linter and full test suite confirm no layering violations were introduced.

No files require special attention. The _NON_SECRET_ENV_KEYS allowlist in config/env_file.py is worth keeping in mind when adding integrations whose public keys end in _KEY or another sensitive terminal — future authors will need to add entries there.

Important Files Changed

Filename	Overview
config/env_file.py	New shared writer module. Key design choices: name-driven routing via is_sensitive_env_key, double-layer secret guard (strip then validate), explicit RuntimeError on keyring failure in sync_env_secret. All logic matches the pre-move env_sync.py implementation. Previously flagged concerns (silent keyring failure, /tmp test sentinel) are resolved in this PR.
surfaces/cli/wizard/env_sync.py	Correctly trimmed to wizard-specific LLM provider logic. sync_reasoning_model_env now explicitly resolves env_path or PROJECT_ENV_PATH before passing to sync_env_values, fixing a path-disagreement regression caught during this refactor.
tests/cli/wizard/test_env_sync.py	Updates imports to config.env_file. Adds two new tests: set_env_value raises ValueError for sensitive keys, and sync_env_secret raises RuntimeError on keyring failure.
tests/cli/wizard/test_configurators_chat_notifications.py	New characterization test suite for _configure_telegram. Uses a platform-neutral sentinel path (Path("sentinel.env")), addressing the /tmp hardcoded path concern from the prior review thread.
tests/integrations/telegram/test_cli_setup_characterization.py	New characterization tests for integrations/cli._setup_telegram. Deliberately does not pin credential tiers so the gap being closed can be fixed without asserting the bug.
tests/interactive_shell/test_commands.py	Adds monkeypatch.setattr("config.env_file.PROJECT_ENV_PATH", ...) alongside every existing env_sync.PROJECT_ENV_PATH patch — the 17-site fix preventing sync_reasoning_model_env from resolving against the real project .env in tests.
tests/cli/test_model_persistence_scenarios.py	Adds config.env_file.PROJECT_ENV_PATH patch to the persistence_paths fixture, consistent with the pattern applied across all 17 affected test sites.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant W as Wizard
    participant I as integrations/cli
    participant T as Action Tool
    participant EF as config/env_file.py
    participant KR as System Keyring
    participant ENV as .env file

    Note over W,ENV: After this PR – all surfaces share config/env_file.py

    W->>EF: sync_env_secret(key, value)
    I->>EF: sync_env_secret(key, value)
    T->>EF: sync_env_secret(key, value)
    EF->>EF: is_sensitive_env_key() → True
    EF->>KR: save_keyring_secret / save_api_key
    EF-->>W: raises RuntimeError if unavailable

    W->>EF: "sync_env_values({key: val})"
    I->>EF: "sync_env_values({key: val})"
    T->>EF: "sync_env_values({key: val})"
    EF->>EF: is_sensitive_env_key() → False
    EF->>EF: strip_secret_env_lines (existing file)
    EF->>EF: set_env_value per key
    EF->>EF: write_env_lines (strip + validate + chmod 0o600)
    EF->>ENV: write public lines only
    EF-->>W: Path to .env

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant W as Wizard
    participant I as integrations/cli
    participant T as Action Tool
    participant EF as config/env_file.py
    participant KR as System Keyring
    participant ENV as .env file

    Note over W,ENV: After this PR – all surfaces share config/env_file.py

    W->>EF: sync_env_secret(key, value)
    I->>EF: sync_env_secret(key, value)
    T->>EF: sync_env_secret(key, value)
    EF->>EF: is_sensitive_env_key() → True
    EF->>KR: save_keyring_secret / save_api_key
    EF-->>W: raises RuntimeError if unavailable

    W->>EF: "sync_env_values({key: val})"
    I->>EF: "sync_env_values({key: val})"
    T->>EF: "sync_env_values({key: val})"
    EF->>EF: is_sensitive_env_key() → False
    EF->>EF: strip_secret_env_lines (existing file)
    EF->>EF: set_env_value per key
    EF->>EF: write_env_lines (strip + validate + chmod 0o600)
    EF->>ENV: write public lines only
    EF-->>W: Path to .env

Loading

_{Reviews (2): Last reviewed commit: "fix(config): address greptile feedback o..." | Re-trigger Greptile}

[muddlebee]

@greptile review

[github-actions]

😭 Clear commit message. Green tests. Kind review. @muddlebee, stop making the rest of us look bad.

---

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