security(redis): every Ansible-rendered client pairs its Redis password with a username; deploy scripts drop the hardcoded password (#16678, #16686)

[mrveiss]

Closes #16678, closes #16686
Refs #16657, #13568, #16627, #16628

One Redis-credentials PR, batched per a7: the remaining clients' username pairing (#16678), the deploy scripts' hardcoded password (#16686), and #16657 AC1's endpoint-calling test.

Thinking Path

• Redis 7 refuses the password-only AUTH <password> while the default user is nopass. It accepts AUTH default <password>, which also works against a server enforcing that password. security(redis): backend.env renders a password-only AUTOBOT_REDIS_URL when a password is set without a username #16668 fixed this for backend.env.j2. The same defect was in the other Ansible-rendered clients: they sent --user or REDIS_USERNAME only when a username was configured, but always sent the password. P1b's own adoption path is safe, because the SLM generates the username beside the password. An operator who sets only a password is not.
• deploy-native.sh and deploy-hybrid.sh wrote AUTOBOT_REDIS_PASSWORD=autobot123 into their generated env files, a literal that dates from 2026-02. They are still referenced: NATIVE_DEPLOYMENT_READY.md and two security-validation utilities point at them. So they're fixed, not retired, because retiring needs proof they're unused.
• security(npu): unauthenticated POST /api/npu/workers/bootstrap returns the Redis password #16657's only test read the source with ast. Its closure audit asked for a test that actually calls the endpoint.

What Changed

• roles/ai-stack/templates/ai-stack.env.j2: REDIS_USERNAME is always rendered alongside REDIS_PASSWORD, as ai_redis_username or else default. With no password, nothing is rendered.
• playbooks/deploy-native-services.yml and playbooks/deploy-hybrid-docker.yml:
  • the redis-cli checks and the container healthcheck always pass --user <configured or default> with -a;
  • the units' REDIS_USERNAME is default alongside a password, and empty when there is none, so the no-password output is unchanged.
  • The server-side requirepass lines are untouched.
• ansible/deploy-native.sh and ansible/deploy-hybrid.sh:
  • Each takes the credential from the environment first, then from the SLM's /etc/autobot/slm-secrets.env. That is the file _shared/tasks/read_redis_password.yml reads, and the path mirrors that task.
  • Each writes AUTOBOT_REDIS_USERNAME with it, default when a password exists.
  • The literal is gone.
• repo_tests/redis_password_literal_guard_test.py (new): fails if any tracked *.sh, heredoc bodies included, assigns a literal to AUTOBOT_REDIS_PASSWORD. The measured population is 2 on main (deploy-hybrid.sh:151, deploy-native.sh:175) and 0 after this PR. It has a known-positive test covering the security(redis): deploy-hybrid.sh and deploy-native.sh write a hardcoded Redis password into the generated env file #16686 shape, and contrast cases for expansions, empty values and comments. Its blind spots are declared in the docstring.
• tests/test_redis_username_plumbing_16626.py: the deploy-playbook test pinned the defect ("no --user without a username"). It now asserts default is sent, and that a configured name is honoured. New tests cover the units' REDIS_USERNAME (default with a password, empty without) and the ai-stack env (password alone, custom username, no password).
• autobot-backend/tests/test_npu_bootstrap_credential_16657.py: test_calling_the_bootstrap_endpoint_returns_no_redis_password POSTs to /npu/workers/bootstrap through the real router, with a sample password set on the config, and asserts the response has no password key and never contains the sample.
• changelog/unreleased/16678-redis-client-usernames.md.

Acceptance criteria

Issue	AC	Evidence
#16678	Every Ansible-rendered Redis client that sends a password sends a username, default unless configured	The ai-stack env, native redis-cli, the hybrid healthcheck and redis-cli, and 4 unit REDIS_USERNAME lines, as described above
#16678	Render tests: password without a username, with a username, and no password, per client	test_deploy_playbook_redis_cli_sends_the_user_first, test_deploy_playbook_units_get_default_with_a_password_and_nothing_without, test_ai_stack_env_sends_a_username_whenever_it_sends_a_password
#16686	No literal password in either script, which reads the canonical secret	Both scripts, as described above
#16686	A username is written with the password, defaulting to default	AUTOBOT_REDIS_USERNAME=${redis_username} in both scripts
#16686	A guard fails on a literal AUTOBOT_REDIS_PASSWORD in any tracked shell script	repo_tests/redis_password_literal_guard_test.py
#16657	AC1 only: a test calls the bootstrap endpoint without credentials and asserts no password comes back	test_calling_the_bootstrap_endpoint_returns_no_redis_password. AC2 (an authenticated channel that survives a worker restart) and AC3 (a guard for credential fields under exempt paths) stay open, so this PR only references #16657.

Verification

• Pre-push hook at 55fce7cef: all relevant tests pass. My output filter didn't capture the hook's file list, so I can't show which files ran. CI's python-suite is the first run I can cite for the new endpoint test. It is also the first test in the repo to import the api.npu_workers router.

• Lint: ruff and black are clean on the three Python test files. Both playbooks parse as YAML, and both shell scripts pass bash -n. That is parse-only; no repo code was run.

• CI on 3c002f8a3: red on python-suite shard 12/12 again, and the cause was mine. P1b's test_redis_password_provisioned_16627.py::test_the_ai_stack_env_renders_the_username_only_with_the_password sliced the ai-stack Redis block with a regex that expects the old nested {% endif %}. My security(redis): the ai-stack env and deploy playbooks send a Redis password without a username #16678 template change removed the inner if, so the slice found nothing, and the test also pinned the old username-only-if-configured rule. It is renamed test_the_ai_stack_env_renders_a_username_whenever_it_renders_the_password: it slices up to the block's first {% endif %} and adds the password-alone case, which renders default. The pre-push hook didn't catch it, because it selects tests by the files a change touches, and this test file wasn't among them.

• Code review of 55fce7cef: REQUEST CHANGES; ledgered as blocked. Resolved in the follow-up commits:

  Finding	Resolution
  HIGH: both scripts run under set -euo pipefail. A missing key in slm-secrets.env makes grep exit 1 and aborts the deploy.	|| true added to all four grep | head reads, the same guard read_redis_password.yml uses.
  LOW, already on main: -a {{ password }} ping with an empty password renders as -a ping, which swallows ping, so the check silently becomes a no-op	Fixed here, since this PR already edits those lines. --user and -a go through Ansible's quote filter, so an empty password stays an explicit '' argument and ping stays the command. The test's plain Jinja environment registers shlex.quote as quote to render them.
  Nit: the npu test's docstring overstated what it checks	Reworded. Today's builders never read config.redis.password, so the sample is a forward guard.

  The review also checked:

  • Jinja precedence in every changed expression;
  • that the healthcheck is still valid YAML and a valid flow sequence;
  • that AUTH default '' against a nopass server is safe;
  • that the guard regex finds no false positives or misses in the repo's real shell code;
  • the npu router's prefix, and that the pydantic config can be patched.

• CI on 55fce7cef: red on python-suite shard 12/12. The cause was mine: my guard's docstring cited file:line references, which repo_tests/comment_line_number_citations_test.py (tech-debt(guards): audit comments that assert a mechanism exists elsewhere — three were false today, one preceded an outage #15877) rejects. It now names the scripts' update_backend_config heredoc instead. My other open branches were checked for the same pattern and have none.

Model Used

Claude Opus 5 (claude-opus-5).

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

Next included review available in 12 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 7f7a039a-59e7-4a46-93d3-3a590eb8e19a

📥 Commits

Reviewing files that changed from the base of the PR and between a494d4c and 2073807.

📒 Files selected for processing (11)

• autobot-backend/tests/test_npu_bootstrap_credential_16657.py
• autobot-slm-backend/ansible/deploy-hybrid.sh
• autobot-slm-backend/ansible/deploy-native.sh
• autobot-slm-backend/ansible/playbooks/deploy-hybrid-docker.yml
• autobot-slm-backend/ansible/playbooks/deploy-native-services.yml
• autobot-slm-backend/ansible/roles/ai-stack/templates/ai-stack.env.j2
• autobot-slm-backend/tests/test_redis_password_provisioned_16627.py
• autobot-slm-backend/tests/test_redis_username_plumbing_16626.py
• changelog/unreleased/16678-redis-client-usernames.md
• repo_tests/glob_declared_reads_15900_test.py
• repo_tests/redis_password_literal_guard_test.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Notice: 29 open PRs — past the runaway threshold (25)

There is no PR queue limit, and this is not a request to defer this PR. Work proceeds one issue at a time without a cap on open PRs; review capacity is the constraint.

This notice only means the count is high enough to be worth a glance for a runaway — something opening PRs in a loop, or a merge pipeline that has stalled so nothing is draining.

Currently open:

• fix(knowledge-ui): move Watch Folders under Knowledge → Manage and fix its form styling (#16695) #16696 fix(knowledge-ui): move Watch Folders under Knowledge → Manage and fix its form styling (fix(knowledge-ui): watch folders belongs under Knowledge → Manage; the Add watch folder form styling is broken #16695)
• security(kb): the canonical search and multi-source routes filter results by the caller's access (#16665) [1/3] #16691 security(kb): the search and RAG routes filter results by the caller's access (security(kb): the knowledge search and RAG routes filter results by the caller's access #16665) [WIP: increment 1/many]
• security(kb): the raw ChromaDB explorer is admin-only (#16666, part 1) #16689 security(kb): the raw ChromaDB explorer is admin-only (security(kb): the raw ChromaDB explorer and the MCP knowledge tools bypass fact visibility #16666, part 1)
• security(kb): only admins make a fact platform-wide; visibility writes keep the store, ChromaDB and ownership indexes current (#16663, #16670) #16677 security(kb): only admins make a fact platform-wide; visibility writes keep the store, ChromaDB and ownership indexes current (security(kb): only admins may make a fact platform-wide, and visibility writes keep the indexes current #16663, bug(kb): five callers await the synchronous KnowledgeBase.get_fact, which raises TypeError #16670)
• chore(deps): bump the all-dependencies group across 1 directory with 6 updates #16645 chore(deps): bump the all-dependencies group across 1 directory with 6 updates
• feat(knowledge): import Claude Code auto-memory into knowledge_facts (#16642) #16643 feat(knowledge): import Claude Code auto-memory into knowledge_facts (feat(memory): import Claude Code auto-memory into AutoBot knowledge base #16642)
• chore(deps): bump the all-dependencies group across 1 directory with 12 updates #16611 chore(deps): bump the all-dependencies group across 1 directory with 12 updates
• chore(deps): bump the uv group across 1 directory with 2 updates #16597 chore(deps): bump the uv group across 1 directory with 2 updates
• chore(deps): dependabot train — 10 members onto current main [no-merge] #16591 chore(deps): dependabot train — 10 members onto current main [no-merge]
• feat(llm): wire extractive prompt compression and enforce per-provider concurrency cap (#16526, #16527) #16546 feat(llm): wire extractive prompt compression and enforce per-provider concurrency cap (Wire the existing extractive prompt compressor into the live LLM completion path #16526, Enforce the existing per-provider LLM concurrency cap #16527)
• fix(infra): consolidate the last 3 private TLS-cert-generation copies (#16020) #16522 fix(infra): consolidate the last 3 private TLS-cert-generation copies (nginx cannot start on a node with no TLS cert: five roles consume the shared keypair, four provision it #16020)
• fix(frontend): retire four dead settings API paths, stop a fifth calling admin-gated settings from non-admin surfaces (#16481) #16484 fix(frontend): retire four dead settings API paths, stop a fifth calling admin-gated settings from non-admin surfaces (retire useSettingsApi/SystemRepository/services-api.ts/CacheService.ts dead settings paths #16481)
• feat(llm): cross-worker quota headroom store, persist real 429 headroom #16483 feat(llm): cross-worker quota headroom store, persist real 429 headroom
• chore(activity-tracking): retire the terminal/file/browser tracking hooks as dead code (#16466) #16475 chore(activity-tracking): retire the terminal/file/browser tracking hooks as dead code (chore(activity-tracking): wire in or retire terminal/file/browser activity tracking + secret_usage audit (backend) #16466)
• chore(deps): bump the npm_and_yarn group across 4 directories with 2 updates #16474 chore(deps): bump the npm_and_yarn group across 4 directories with 2 updates
• feat(collaboration): surface pending invitations in the collaboration panel (#16470) #16472 feat(collaboration): surface pending invitations in the collaboration panel (impl gap: surface pending collaboration invitations in a UI #16470)
• feat(collaboration): persist activity/notification history, add invitation list+respond endpoints (#16460) #16471 feat(collaboration): persist activity/notification history, add invitation list+respond endpoints (feat(collaboration): persist activity/notification history and add invitation list+respond endpoints #16460)
• security(settings): record the real caller on /sync and /hardware-priority, and test the anonymous and cross-org refusals (#16278) #16469 security(settings): record the real caller on /sync and /hardware-priority, and test the anonymous and cross-org refusals (security(settings): the core /api/settings routes read and write system config with no authentication #16278)
• feat(collaboration): wire the collaboration UI onto the real backend (#16443) #16462 feat(collaboration): wire the collaboration UI onto the real backend (impl gap: wire in the real-time collaborative-session UI (#608 phases 5-7 / #874) — 6 components + useSessionCollaboration, zero callers #16443)
• feat(mcp): admin-configured external MCP server bridge #16458 feat(mcp): admin-configured external MCP server bridge
• fix(mcp): rename autobot-backend/mcp/ off the name it shadows (#16449) #16454 fix(mcp): rename autobot-backend/mcp/ off the name it shadows (chore(mcp): autobot-backend/mcp/ shadows the real mcp PyPI SDK, making it unreachable by name #16449)
• docs(adr): supersede ADR-001's fixed VM count with role-separation placement (#15194) #16452 docs(adr): supersede ADR-001's fixed VM count with role-separation placement (docs: a fixed 6-VM topology is documented as the architecture — AutoBot scales from one VM or Docker to whatever is needed, and ADR-001 enshrines the wrong invariant #15194)
• docs: describe role-based placement instead of a fixed VM count (#15194) #16445 docs: describe role-based placement instead of a fixed VM count (docs: a fixed 6-VM topology is documented as the architecture — AutoBot scales from one VM or Docker to whatever is needed, and ADR-001 enshrines the wrong invariant #15194)
• feat(secrets): bridge connector credential templates to ConnectorCredentialStore (#16428) #16444 feat(secrets): bridge connector credential templates to ConnectorCredentialStore (secrets: connector credential templates must write where knowledge connectors read #16428)
• security(secrets): admin-gate infrastructure hosts, fix vault-backed template names (#16426, #16427) #16442 security(secrets): admin-gate infrastructure hosts, fix vault-backed template names (security(secrets): any logged-in user can list and delete infrastructure hosts through /api/infrastructure/hosts #16426, bug(secrets): the OpenAI and Anthropic quick-add templates save names the vault mirror never recognizes, so the key never reaches the runtime #16427)
• feat(slm): Redis control and detailed health live in the SLM only; remove the unreachable user-GUI copies (#16245, #16252) #16352 feat(slm): Redis control and detailed health live in the SLM only; remove the unreachable user-GUI copies (tech-debt(frontend): move SettingsPanel's detailed system health and Redis service management into the SLM #16245, bug(frontend): RedisServiceControl calls /api/service-monitor/services/* and no mounted router serves those paths #16252)
• fix(code-sync): delete files removed from source through the updater, and check drift across every file (#16310, #16322) #16351 fix(code-sync): delete files removed from source through the updater, and check drift across every file (deploy: the builtin updater never removes files deleted from source, so deleted modules pile up on every host #16310, deploy: ansible synchronize never deletes files removed from source on remote fleet nodes #16322)
• ci(python-suite): queue the shards first in, first out across PRs, with a per-shard job-level queue: max group (#16320) #16328 ci(python-suite): queue the shards first in, first out across PRs, with a per-shard job-level queue: max group (ci: queue python-suite shards FIFO across PRs (queue: max), so one PR's suite finishes before the next starts #16320)
• docs(env): move the generated env-var table into ENV_VARS.md and drop its count line, so env-var PRs stop conflicting (#16317) #16318 docs(env): move the generated env-var table into ENV_VARS.md and drop its count line, so env-var PRs stop conflicting (docs: the generated env-var table and its count line live in CLAUDE_RULES.md, so every env-var PR conflicts with every other #16317)

If the queue is draining normally, ignore this. Otherwise:

1. Check whether CI is dispatching at all — see the ci-dispatch-watchdog status on these PRs
2. Merge the ones whose CI has finished and review has passed: gh pr merge <number> --squash --delete-branch
3. Look for a loop opening near-identical PRs

Warn-only runaway detector — .github/workflows/pr-queue-gate.yml. It never blocks a merge.

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

[mrveiss]

Delta review at 3c002f8 (from 55fce7c): approve.

Commit 15d0c69 (pipefail-survival + Ansible quoting):

• deploy-hybrid.sh / deploy-native.sh: both add || true to the AUTOBOT_REDIS_PASSWORD= / AUTOBOT_REDIS_USERNAME= grep -oP extractions, matching the existing guard pattern already used elsewhere (e.g. read_redis_password.yml) for set -euo pipefail survival when the var is absent. Confirmed set -euo pipefail is in effect in both scripts (deploy-hybrid.sh:9).
• deploy-hybrid-docker.yml / deploy-native-services.yml: redis-cli --user ... -a ... now pipes both the username and password through Jinja's quote filter before the ping — closes the shell-injection surface on values sourced from secrets/env.
• Test harness registers shlex.quote as the Jinja quote filter (test_redis_username_plumbing_16626.py:39), so the rendered template is exercised the same way Ansible would render it. test_npu_bootstrap_credential_16657.py docstring reworded to match.
• Diff verified byte-for-byte against the PR description; no discrepancies.

Commit 3c002f8 (guard-citation fix for #15877):

• repo_tests/redis_password_literal_guard_test.py docstring previously cited ansible/deploy-hybrid.sh:151 / deploy-native.sh:175 — a path:LINE citation, which comment_line_number_citations_test.py (tech-debt(guards): audit comments that assert a mechanism exists elsewhere — three were false today, one preceded an outage #15877) forbids since line numbers drift.
• Now cites the update_backend_config heredoc by name. Confirmed that function exists in both scripts (deploy-hybrid.sh:134, deploy-native.sh:158) and is where the two literal AUTOBOT_REDIS_PASSWORD= assignments removed by security(redis): deploy-hybrid.sh and deploy-native.sh write a hardcoded Redis password into the generated env file #16686 lived — the citation is accurate, not just guard-compliant.

CI at head 3c002f8a3: no FAILURE/ERROR/TIMED_OUT checks; remainder SUCCESS/SKIPPED/in-progress. Mergeable.

[mrveiss]

Delta review at 59a0f92 (from 3c002f8): approve.

Verified the regex fix and the new assertions directly against the current template (`autobot-slm-backend/ansible/roles/ai-stack/templates/ai-stack.env.j2`, lines 37-42):

```
{% if ai_redis_password | length > 0 %}
REDIS_PASSWORD={{ ai_redis_password }}
REDIS_USERNAME={{ ai_redis_username | default('', true) or 'default' }}
{% endif %}
```

Confirms #16678 left exactly one `{% endif %}` in this block (the old test's two-`endif` regex would indeed match `None`) — the new single-`{% endif %}` regex is correct.

The three new assertions match the template's actual Jinja logic exactly: `ai_redis_username="svc"` → `default('', true)` passes it through unchanged (non-empty, non-undefined) → `REDIS_USERNAME=svc`; `ai_redis_username=""` → `default('', true)` treats the empty string as needing the default (the `true` flag), yielding `''`, then `'' or 'default'` → `REDIS_USERNAME=default`; no password → the whole block doesn't render. Also an improvement over the old test: using `"svc"` instead of `"default"` for the with-username case actually distinguishes "the template passes a real username through" from "the template always falls back to default" — the old test's use of `"default"` for both configured and fallback cases couldn't tell those apart.

CI: no FAILURE/ERROR/TIMED_OUT at this head; rest still in progress (expected, this is CI's first run of the fixed test per your note).

[mrveiss]

Root cause of the python-suite shard 8 red at 59a0f928b, fixed in 207380718.

repo_tests/glob_declared_reads_15900_test.py::test_the_record_names_exactly_the_guards_that_declare_each_glob failed on *.sh. The record didn't list repo_tests/redis_password_literal_guard_test.py, the guard this PR adds, which sweeps *.sh files.

The fix registers it under the record's *.sh entry, beside the ten guards already there and under the same stated reason: the matching files live outside the python filter's trees. It's a one-line data change with no code change.

The pre-push hook didn't catch this because the record test isn't selected by this PR's changeset. That's the selection gap tracked in #16711.

[mrveiss]

Re-stamped at 2073807 (from 59a0f92): approve.

Verified the delta: one line, adding `repo_tests/redis_password_literal_guard_test.py` to the `".sh"` entry in `GLOB_DECLARED_UNCOVERED` — correctly alphabetically placed between `one_git_enumeration_15926_test.py` and `shell_lib_test.py`. Confirmed it belongs there, not just guard-silencing: the guard's own scope does `scripts = tracked_paths(REPO_ROOT, ".sh")`, a genuine root-relative `*.sh` sweep matching this key's stated reason exactly ("the matching files live outside the python filter's trees"). No code change, no other lines touched.

CI: no FAILURE/ERROR/TIMED_OUT checks. Mergeable.