Repository navigation
config(tts): one spelling per side of the boundary, and the inventory port configured nothing (#17782) - #17799
Conversation
… port configured nothing (#17782) ## The port, established from the running unit 8083, with three independent confirmations that agree: the live service answers `{"service":"tts-worker","engine":"pocket-tts",...}` on 8083, so the port is attributed by IDENTITY rather than inferred from an open socket; the deployed worker defaults `TTS_PORT` to 8083; and the repo-side origin of that value carries the reason -- tts_port: 8083 # Issue #3431: 8082 is WSL2/Hyper-V reserved (Windows NPU worker) So 8082 is not a defensible alternative that lost a coin toss. It is a port the worker cannot bind on that host class, which is why #3431 moved it and why #3464 happened. ## Three corrections to the issue's own evidence **1. `autobot-tts-worker/main.py` is not the worker.** 725 bytes against the deployed worker's 26,199; the real source is `ansible/roles/tts-worker/templates/tts-worker.py.j2` and `tts-worker.py` exists nowhere in the repository as a `.py`. Its docstring said "port 8082" and named an internal address. Corrected rather than deleted -- the file is a deliberate signpost to the template and says so; it just pointed at the wrong number. `README.md` was stale the same way. **2. `tts_worker_port: 8082` in the inventory configured nothing.** The tts-worker role reads `tts_port`. Every consumer of `tts_worker_port` is on the Python side: `services/tts_client.py:39` and an `ssot_config.py` backward-compat property. Nothing under `ansible/` reads it. So no host was ever configured to 8082 by that line -- it is removable rather than a conflicting authority, and the comment that replaces it says where the port really lives. **3. There is a fourth spelling, and unifying it would be wrong.** The worker binds via unprefixed `TTS_HOST`/`TTS_PORT`, written by its own `tts-worker.env.j2`. That is its BIND interface; the `AUTOBOT_*` names are how clients FIND it. The issue's criterion ("one env-var naming for the worker address") would collapse two concerns and break the env contract the role generates. **A name on both sides of a boundary is not one knob.** Bind-side and find-side look identical in a grep, which is how three namings survived. ## What actually duplicated Exactly one pair. `service_orchestrator.py` declared `AUTOBOT_TTS_HOST`/`_PORT`, which NOTHING in the repository writes -- no template, env file, compose file or inventory -- so the entry always fell through to its own `default_port=8083` literal, making that literal a second source of truth. It now reads the canonical `AUTOBOT_TTS_WORKER_HOST`/`_PORT`, which deployment does write (`roles/backend/templates/backend.env.j2`) and which the client that talks to the worker reads. The note at that call site is ONE LINE, not the seven the reasoning needs, because `service_orchestrator.py` sits at a frozen 955-line ceiling and the comment pushed it to 962. The ceiling does not move, so the argument moved instead -- into the guard's docstring, which is not size-constrained and is where someone debugging the naming will already be looking. `AUTOBOT_TTS_WORKER_URL` is NOT a third spelling of the address: it is the URL projection in `_URL_REGISTRY`, one of six siblings, already canonically prefixed. Left alone. ## The guard records a SIDE, not just a name `repo_tests/tts_address_env_names_17782_test.py` freezes every TTS address env var with the side of the boundary it serves, and fails an entry that does not declare one -- because "which side is this on" is the question that stops the next duplicate, and a bare name list would not have stopped these three. Its first version matched any UPPER_SNAKE token and reported four false positives, every one instructive: `TTS_WORKER_PORT = config.tts_worker_port` is a Python constant DERIVED from the SSOT, not a competing spelling; `TTS_HEALTH_URL = os.getenv("SLM_AGENT_TTS_HEALTH_URL")` puts the name in the argument, not the constant; and the remaining two were this guard's own contrast fixture and its own explanatory comment -- the "a text detector counts prose about itself" trap, in the guard written to catch drift. It now matches only quoted references and `NAME=` assignments, and skips its own file. That scan also found `SLM_AGENT_TTS_HEALTH_URL`, which the issue does not list. It is legitimate -- a health-endpoint override whose fallback is `get_config().port.tts`, so it cannot drift from the SSOT -- and it is recorded rather than changed. ## Verification Mutation-checked, three ways: a new client-side spelling in production -> failed, naming it a recorded name absent from the tree -> failed, naming it a recorded entry with no side declared -> failed One mutation of mine was wrong before it was right: renaming the template's `AUTOBOT_TTS_WORKER_PORT` left `service_orchestrator.py`'s quoted reference in place, so the name was never retired and the guard was correct to pass. The mutant has to remove the property before a survival means anything. Read-only guards: glob-declared reads + repo-root spelling + python-filter coverage + comment citations + this guard, 61 passed. flake8 clean; the inventory still parses as YAML. ## Not established, stated rather than assumed The deployed worker's `.env` is mode 0600 owned by the service user and reading it returned `Permission denied`, so whether it sets `TTS_PORT` explicitly or relies on the template default is unknown. It does not change the answer -- the process is listening on 8083 and identifies as the worker -- but the gap is recorded rather than implied to have been checked. `ss -ltnp` also shows no `pid=` column without root, which is why the attribution comes from the health payload. Refs #17782
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 40 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe TTS service configuration now uses worker-specific host and port environment variables. The documentation and production inventory identify port 8083. A repository test checks TTS address variable references. ChangesTTS Addressing
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The README still exposes an internal address. Removing it and lowering the baseline to zero would complete the stated documentation change; this is a bounded issue rather than a runtime blocker. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new address names match the current configuration, but an unset worker host could direct service controls to the backend machine instead. The effective production address and rollback behavior remain unconfirmed. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
… in the addressing improvement (#17782) Two red checks, one cause each, and the first one caught me not using machinery that exists. ## The reach floor was hand-written, which is the thing the mechanism replaces `guard_reach_meta_test` failed with: tree-scanning guard(s) with no reach floor: repo_tests/tts_address_env_names_17782_test.py Bind one with repo_tests._reach.declare(...), so an empty enumeration fails instead of reporting the same green as a clean tree. I had written my own non-vacuity check -- assert the names we know are there are found. That proves the population is non-empty TODAY and cannot prove the check would fire if it ever became empty. `_reach` exists for exactly that difference: declaring reach as DATA lets `reach_declarations_test` run the discovery against an empty directory and REQUIRE the failure, which is the mutation half applied mechanically instead of promised per guard. So `_scanned_files(root)` is now the declared population and `REACH` binds it. ## And my first floor was the failure a floor exists to prevent I set `floor=6000` against a population of 10,267, reasoning that a low floor is a safe floor. `reach_declarations_test` refused it: floor 6000 sits 4267 below its live population of 10267 ... A floor this far below what the sweep finds passes while most of the tree stops being reached. That is exactly right, and it is the same shape as a ratchet with headroom: a margin that looks like caution is room for the thing you are guarding against to happen with nothing failing. The floor is now 10,000 with `growth=400`, so the band 10,000..10,400 needs no action and beyond it the floor is ratcheted deliberately. The population is 10,267 rather than 10,268 because the sweep excludes this file -- it names every spelling it forbids, so scanning itself would report its own record as drift. ## The other two failures were one cause `code-quality` and `python-suite shard 8/12` both reported: autobot-tts-worker/README.md: down to 1 from 2 -- lower the baseline so the improvement is locked in and cannot drift back. One drift, seen by the hook and by its pytest mirror. Removing the internal address from the architecture diagram lowered the count, so the baseline is lowered to match: this is the shrink-only ratchet working in the change's favour, and not recording it would leave the improvement spendable. Verified: fleet-addressing audit clean (4807 uncovered files reached, 552 baselined occurrences in 110 files); the guard + reach meta-test + reach declarations 291 passed; flake8 clean. Refs #17782
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Remove the remaining internal address from the README. · README.md:4
autobot-tts-worker/README.md:4
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the remaining internal address from the README.
The PR objective says these documents remove internal addresses, but Line 4 still names
172.16.168.22. The changed baseline intools/lint/check_fleet_addressing_uncovered.pyLine 410 still allows one occurrence, so it does not enforce removal of this address. Replace it with the NPU node label and set the baseline to zero.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: aa757fbd-d972-44a3-babe-61eb19cccd33
📒 Files selected for processing (6)
autobot-slm-backend/ansible/inventory/production.ymlautobot-slm-backend/services/service_orchestrator.pyautobot-tts-worker/README.mdautobot-tts-worker/main.pyrepo_tests/tts_address_env_names_17782_test.pytools/lint/check_fleet_addressing_uncovered.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
…the repository (#17782) Review called this trivial and it is the guard's population. `Path(".env").suffix` is `""`. `Path(".env.docker").suffix` is `".docker"`. So the `".env"` entry in `_SUFFIXES` matched **zero of this repository's nine env files** -- every one is `.env` or `.env.<something>` -- and there is no file anywhere in the tree whose literal suffix is `.env`: env files by shape: 9 files with literal suffix .env: 0 The entry was decoration, and the file type most likely to hold a deployed override was the one type never scanned. A TTS address variable living only in a generated `.env` would have evaded the guard written to catch address-name drift. That is worse than review's framing, which was that exact `.env` files evade it: **no env file was ever in the population at all.** ## Why nothing failed The predicate was inlined in the walk, so the only thing any test could observe was the walk's RESULT. A result computed over the wrong population looks exactly like a result computed over the right one. `_is_scannable(name)` is now a named function with its own contrast pair, which is what makes the population assertable rather than implied. Nine cases, including two negatives that matter: `notes.txt` (a text file is not an env file) and `envelope.pdf` (so `endswith(".env")` is not a substring test). Plus `test_the_walk_actually_reaches_an_env_file`, because the predicate being right is not the same as the walk using it. ## The README table `:36-38` was one table row spread over three lines -- Markdown renders that outside the table, so the `TTS_PORT` description fell out of it. Mine, from the 8082 correction. Collapsed to a single line; every row now has four delimiters. ## Verification revert to suffix-only matching -> 5 failed: the four env name cases and the walk test, nothing else 15 passed; reach declarations still hold with the widened population (it grew by nine files, inside the declared band); the addressing audit is unchanged at 552 baselined occurrences in 110 files, confirming the README reflow moved no address. Refs #17782
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Remove the remaining internal address. · README.md:4
autobot-tts-worker/README.md:4
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the remaining internal address.
The PR objective says to remove internal addresses from the README, but Line 4 still contains
172.16.168.22. Remove this address to meet that objective.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 925c299c-2812-40d7-af7b-109a479ebf38
📒 Files selected for processing (2)
autobot-tts-worker/README.mdrepo_tests/tts_address_env_names_17782_test.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
|
1 participant
Thinking Path
The issue reports three env-var namings for one worker address, and an inventory port (8082) disagreeing with the SSOT default (8083). Establishing it from the running unit — as the acceptance criterion asks — changed three of the four premises.
The port is 8083, and 8082 cannot work on that host class
{"status":"healthy","service":"tts-worker","engine":"pocket-tts",…}on 8083 — attributed by identity, not inferred from an open socketTTS_PORTto 8083ansible/roles/tts-worker/defaults/main.ymlAnd #3 carries the reason, which settles why rather than just which:
So 8082 is not a defensible alternative that lost a coin toss — it is a port the worker cannot bind on that host class. That is why #3431 moved it and why #3464 happened.
What Changed
services/service_orchestrator.pyreads the canonicalAUTOBOT_TTS_WORKER_HOST/_PORTinstead ofAUTOBOT_TTS_HOST/_PORT, which nothing in the repository writes — so the entry no longer falls through to a literal that duplicated the SSOT default.ansible/inventory/production.ymllosestts_worker_port: 8082, which no Ansible task read, replaced by a comment naming where the port actually lives.autobot-tts-worker/main.pyandREADME.mdcorrected: port 8083 with the bug(ansible): distributed_setup/defaults backend_port: 8443 conflicts with backend role's 8001 #3431 reason, and the internal addresses removed. The stub is kept — it is a deliberate signpost to the Ansible template and says so.repo_tests/tts_address_env_names_17782_test.pyfreezes every TTS address env var with the side of the boundary it serves (find:/bind:), and fails an entry declaring neither.tools/lint/check_fleet_addressing_uncovered.pybaseline lowered 2 → 1 for the README, locking in the address removal.Three corrections to the issue's evidence
1.
autobot-tts-worker/main.pyis not the worker. 725 bytes against the deployed worker's 26,199; the real source isansible/roles/tts-worker/templates/tts-worker.py.j2, andtts-worker.pyexists nowhere in the repo as a.py. So "its docstring matches 8082" cited a file that does not run. Corrected rather than deleted — it is a deliberate signpost to the template and says so; it just pointed at the wrong number.README.mdwas stale the same way, and both named internal addresses.2.
tts_worker_port: 8082in the inventory configured nothing. The tts-worker role readstts_port. Every consumer oftts_worker_portis on the Python side (services/tts_client.py:39and anssot_config.pybackward-compat property); nothing underansible/reads it. No host was ever configured to 8082 by that line. Removed, with a comment saying where the port really lives.3. There is a fourth spelling, and unifying it would be wrong. The worker binds via unprefixed
TTS_HOST/TTS_PORT, written by its owntts-worker.env.j2. That is its bind interface; theAUTOBOT_*names are how clients find it. The criterion as written ("one env-var naming for the worker address") would collapse two concerns and break the env contract the role generates.What actually duplicated: exactly one pair
service_orchestrator.pydeclaredAUTOBOT_TTS_HOST/_PORT, which nothing in the repository writes — no template, env file, compose file or inventory. So the entry always fell through to its owndefault_port=8083literal, making that literal a second source of truth. It now reads the canonicalAUTOBOT_TTS_WORKER_HOST/_PORT, which deployment does write (roles/backend/templates/backend.env.j2) and which the client that talks to the worker reads.AUTOBOT_TTS_WORKER_URLis not a third spelling of the address — it is the URL projection in_URL_REGISTRY, one of six siblings, already canonically prefixed. Left alone.The guard records a SIDE, not just a name
repo_tests/tts_address_env_names_17782_test.pyfreezes every TTS address env var with the side of the boundary it serves, and fails an entry that does not declare one — because "which side is this on" is the question that stops the next duplicate, and a bare name list would not have stopped these three.Its first version was wrong in an instructive way. Matching any UPPER_SNAKE token produced four false positives:
TTS_WORKER_PORT = config.tts_worker_portTTS_HEALTH_URL = os.getenv("SLM_AGENT_TTS_HEALTH_URL")It now matches only quoted references and
NAME=assignments, and skips its own file. That scan also foundSLM_AGENT_TTS_HEALTH_URL, which the issue does not list — legitimate, since its fallback isget_config().port.ttsand it therefore cannot drift from the SSOT. Recorded, not changed.Verification
One of my mutations was wrong before it was right, and it is worth stating: renaming the template's
AUTOBOT_TTS_WORKER_PORTleftservice_orchestrator.py's quoted reference in place, so the name was never actually retired and the guard was correct to pass. A surviving mutant means nothing until the mutant provably removes the property.Read-only guards: glob-declared reads + repo-root spelling + python-filter coverage + comment citations + this guard — 61 passed. flake8 clean; the inventory still parses as YAML;
--audit-ceilingsclean over 5983 files.Risks
service_orchestrator.pysits at its frozen 955-line ceiling. The seven-line explanation pushed it to 962, so the note at the call site is one line and the argument lives in the guard's docstring instead. The ceiling does not move.AUTOBOT_TTS_HOSTby hand outside the repo would lose that override; the canonical pair is the replacement and the guard names it.Not established, stated rather than assumed
The deployed worker's
.envis mode0600owned by the service user and returnedPermission denied, so whether it setsTTS_PORTexplicitly or relies on the template default is unknown. It does not change the answer — the process listens on 8083 and identifies as the worker — but the gap is recorded rather than implied checked.ss -ltnpshows nopid=column without root, which is why the attribution comes from the health payload instead, and that is the stronger evidence anyway.Model Used
Claude, via Claude Code. The model identifier is withheld under the session's policy on artifacts.
Issue Link
Refs #17782
Refsrather thanCloses: the amended criteria on the issue distinguish the client-side consolidation (done here) from leaving the worker's bind pair alone (done) and from the guard (done) — but the criteria as originally written ask for "one naming" across all three consumers, which this PR argues is the wrong target. That wording needs the issue owner's amendment before it can be ticked.Changelog fragment
Checklist
mainorigin/mainSummary by CodeRabbit
Bug Fixes
Tests