feat(config): warn when env Discord token shadows config token (#274)#275
Conversation
When both an environment Discord token (DISCORD_TOKEN or CLAWHIP_DISCORD_BOT_TOKEN) and a config-file token are present, env still wins, but the override was previously silent: startup and `clawhip status` only reported `token_source: env`, leaving operators to discover the ignored config token via a Discord 401 smoke failure. Add a non-secret precedence diagnostic surfaced in both startup telemetry/logs and the daemon health payload (consumed by `clawhip status`): - AppConfig::discord_token_env_shadow detects env-over-config conflicts and reports only the offending env var name, never token values. - Daemon startup emits a `warning:` log line plus a `discord_token_env_shadow` telemetry record. - health_payload gains `token_precedence_warning` (null unless shadowed). Env-over-config compatibility is unchanged. Adds focused tests covering the shadow detection, the legacy config token path, the no-conflict cases, and that the warning text leaks no secret values.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 575c4b8fc2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "ok": true, | ||
| "version": VERSION, | ||
| "token_source": config.discord_token_source(), | ||
| "token_precedence_warning": config.discord_token_env_shadow().map(discord_token_shadow_warning), |
There was a problem hiding this comment.
AGENTS.md says “Test coverage: flag new logic paths that lack corresponding tests.” This new health/status branch is only asserted for the null case, so the intended clawhip status behavior when an env token shadows a config token can regress without a test (for example, returning null or leaking the wrong string would not be caught). Please add a positive health/status test with a config token plus DISCORD_TOKEN/CLAWHIP_DISCORD_BOT_TOKEN set.
Useful? React with 👍 / 👎.
|
GJC red-team review: Evidence checked:
Non-blocking nit: health positive-path wiring is indirectly covered through helper tests rather than a dedicated health-payload positive assertion. Low risk and not merge-blocking. — |
1 participant
Closes #274.
Problem
When a systemd user unit (or any environment) still exports
CLAWHIP_DISCORD_BOT_TOKEN/DISCORD_TOKEN, env precedence silently overrides a newly configured config-file token. Startup andclawhip statusonly reportedtoken_source: env, so the ignored config token was discovered only after a Discord 401 smoke failure.Change
Keep env-over-config compatibility (env still wins), but make the override explicit and non-secret:
AppConfig::discord_token_env_shadowdetects when an env token shadows a present config token and returns only the offending env var name — never a token value.warning:log line and adiscord_token_env_shadowtelemetry record.health_payloadgainstoken_precedence_warning(null unless shadowed), surfaced byclawhip status.Safety
Tests
discord_token_env_shadow_detected_when_env_overrides_configdiscord_token_env_shadow_uses_legacy_config_tokendiscord_token_env_shadow_none_without_conflictdiscord_token_shadow_warning_names_env_var_without_leaking_valuehealth_payload_includes_version_and_token_sourceextended to assert the warning field is null when no shadow.cargo build,cargo clippy, and the fullcargo testsuite (623 tests) pass.