Soft-deprecate env vars superseded by users.yaml

[dcellison]

Fixes #199.

Summary

Soft-deprecate 10 env vars that are now superseded by per-user config in users.yaml. No env vars are removed; existing installs continue to work unchanged.

• config.py: Emit WARNING log lines at startup when deprecated env vars are set alongside users.yaml, each with a specific migration path
• install.py: Config wizard skips interactive prompts for deprecated vars when users.yaml exists, printing migration guidance instead. Env file output omits deprecated keys in multi-user mode.
• .env.example: Deprecated vars commented out with migration notes pointing to users.yaml fields and Telegram commands
• tests: 22 new tests covering deprecation warnings, minimal-env boot with users.yaml, resolution function precedence without env fallbacks, and legacy env-only backward compatibility

Test plan

• All 10 deprecated vars trigger warnings when set alongside users.yaml
• No warnings when users.yaml is absent (legacy mode)
• Empty env vars do not trigger false-positive warnings
• Config loads with only global env vars + users.yaml (zero deprecated vars)
• Per-user model, budget, workspace, and GitHub settings resolve correctly without env fallbacks
• resolve_github_settings and resolve_workspace_access work with default globals
• Legacy env-only installs (no users.yaml) work exactly as before
• 1424 tests passing, lint clean

[dcellison]

Review by Kai

PR Review: chore/deprecate-env-vars

Overall this is clean, well-structured work. A few issues worth flagging:

---

Warning

install.py: PR_REVIEW_COOLDOWN silently dropped when users.yaml exists

In the has_users_yaml branch (~line 658 of the diff), the cooldown is only written if pr_review_enabled is truthy from the global env var:

else:
    # PR_REVIEW_COOLDOWN is a global rate limit - always write it
    # when review is enabled for any user, even with users.yaml.
    if pr_review_enabled and pr_review_cooldown != "300":
        env["PR_REVIEW_COOLDOWN"] = pr_review_cooldown

pr_review_enabled is read from existing_env.get("PR_REVIEW_ENABLED", "false"), which is the deprecated global flag. If an admin sets pr_review: true in users.yaml but never set PR_REVIEW_ENABLED in the env, a non-default cooldown value in the existing env will never be written to the new .env. The comment says "always write it ... even with users.yaml" but the condition contradicts this.

Fix: when has_users_yaml, always write PR_REVIEW_COOLDOWN if it's non-default, since any user may have PR review enabled:

if pr_review_cooldown != "300":
    env["PR_REVIEW_COOLDOWN"] = pr_review_cooldown

---

Suggestion

config.py / .env.example: ALLOWED_USER_IDS deprecation is inconsistent

.env.example marks ALLOWED_USER_IDS as deprecated (commented out with migration note), but config.py's _deprecated_env_vars dict does not include it. The existing code emits log.info(...) for it rather than log.warning(...). All 10 other deprecated vars get WARNING; this one gets INFO. Either add it to the dict or update .env.example to reflect that it's treated differently.

install.py: has_users_yaml is redundant with users_yaml_exists

Both variables are initialized and updated in lockstep through the /etc/kai/ check. After that block they always hold the same value. Pick one name and drop the other.

tests/test_config.py: test_warns_when_users_yaml_exists doesn't set caplog level

The test relies on caplog.text to catch WARNING logs without with caplog.at_level(logging.WARNING, logger="kai.config"). If the logger's effective level changes in CI (e.g. root logger raised to ERROR), the assertion silently passes as a false positive. Adding the explicit level guard makes the test robust.

---

No security issues, no auth bypasses, no injection surface introduced. The backward-compatibility story is solid.

[dcellison]

Review by Kai

This is clean work. The prior review's main bug (PR_REVIEW_COOLDOWN gated on the deprecated flag) was fixed correctly. The caplog.at_level guard was added. A few small issues remain:

---

Suggestion

config.py: _deprecated_env_vars dict recreated on every load_config() call

The dict is defined inside the if user_configs is not None: block (~line 1133), so it's allocated and thrown away on every startup call. Move it to module level as a constant. No behavioral impact, just cleaner.

---

Suggestion

install.py: pr_review_cooldown written without re-validation in users_yaml_exists path

In the else branch (~line 681):

pr_review_cooldown = existing_env.get("PR_REVIEW_COOLDOWN", "300")
# ...
if pr_review_cooldown != "300":
    env["PR_REVIEW_COOLDOWN"] = pr_review_cooldown

The non-users_yaml path validates via _validate_positive_int before writing. Here the value is read from the existing env and written verbatim - a pre-existing PR_REVIEW_COOLDOWN=abc would be silently copied to the new .env. The impact is low (only affects pre-existing broken envs), but the asymmetry is worth noting.

---

Suggestion

tests/test_config.py: test_pool_uses_per_user_model tests inlined logic, not the pool

resolved = user.model if user.model else config.claude_model
assert resolved == "opus"

This duplicates the resolution logic inline rather than calling the actual SubprocessPool method. If the pool's resolution diverges from this pattern later, the test won't catch it. Consider calling the real resolution path, or at minimum adding a comment that this is intentionally testing the pattern rather than the integration.

---

No security issues, no auth bypasses. The backward-compat story is solid and the 22 new tests cover the meaningful cases well.

[dcellison]

Review by Kai

Looking at the diff with the prior review context in mind, I won't re-raise the three open suggestions (module-level dict, cooldown validation asymmetry, inlined pool resolution). Here are new findings:

---

Suggestion

tests/test_config.py: TestLegacyEnvOnlyMode.test_no_deprecation_warnings missing caplog.at_level

def test_no_deprecation_warnings(self, monkeypatch, caplog):
    _set_required(monkeypatch)
    monkeypatch.setenv("CLAUDE_MODEL", "opus")
    load_config()
    assert "deprecated" not in caplog.text.lower()

Same false-positive risk that was fixed in TestDeprecationWarnings.test_no_warning_without_users_yaml in the previous round - if the root logger's effective level is above WARNING in CI, caplog.text will be empty and the assertion passes vacuously. TestDeprecationWarnings.test_no_warning_without_users_yaml was correctly fixed with caplog.at_level; this test wasn't. Add the same guard.

---

Suggestion

install.py: CLAUDE_USER migration drops silently - no printed guidance

Every other deprecated section in _cmd_config prints migration guidance when users_yaml_exists:

print("  Model, timeout, budget, and context window are now per-user.")
print("  Set defaults in users.yaml or let users configure via /settings.")

But the CLAUDE_USER path just silently assigns claude_user = "" with a code comment. If an admin runs make config on an existing install with CLAUDE_USER set and users.yaml present, the value is silently dropped from the new .env with no terminal output explaining why or where to migrate it. Add a print(" OS user isolation is now per-user. Set 'os_user' in users.yaml.") to match the pattern.

---

Otherwise the PR is clean. The PR_REVIEW_COOLDOWN fix from the first review is correct, the caplog.at_level guard was added to the right test, and the backward-compat story holds up. The env dict construction correctly gates all 10 deprecated vars behind not users_yaml_exists.

[dcellison]

Review by Kai

This diff addresses both issues raised in the third review: caplog.at_level is now present in TestLegacyEnvOnlyMode.test_no_deprecation_warnings, and install.py now prints migration guidance for CLAUDE_USER when users_yaml_exists.

The three open suggestions from prior rounds (module-level _deprecated_env_vars dict, cooldown validation asymmetry, inlined pool resolution test) remain open, as expected.

No new issues found. The PR is clean.