From 1634457e2a17a1bc5eb78a4cb7e8e0cda2062fe1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 19:41:32 +0000 Subject: [PATCH 01/13] Add 7-level Claude review organization (agents + /full-review command) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01FjTbUXge6kcW3TZMfAdz2J --- .claude/agents/chief-architect.md | 24 ++++++++++++++++++++++++ .claude/agents/domain-analyst.md | 23 +++++++++++++++++++++++ .claude/agents/executive-scribe.md | 22 ++++++++++++++++++++++ .claude/agents/git-historian.md | 20 ++++++++++++++++++++ .claude/agents/quality-auditor.md | 22 ++++++++++++++++++++++ .claude/agents/repo-cartographer.md | 21 +++++++++++++++++++++ .claude/agents/security-auditor.md | 22 ++++++++++++++++++++++ .claude/commands/full-review.md | 25 +++++++++++++++++++++++++ 8 files changed, 179 insertions(+) create mode 100644 .claude/agents/chief-architect.md create mode 100644 .claude/agents/domain-analyst.md create mode 100644 .claude/agents/executive-scribe.md create mode 100644 .claude/agents/git-historian.md create mode 100644 .claude/agents/quality-auditor.md create mode 100644 .claude/agents/repo-cartographer.md create mode 100644 .claude/agents/security-auditor.md create mode 100644 .claude/commands/full-review.md diff --git a/.claude/agents/chief-architect.md b/.claude/agents/chief-architect.md new file mode 100644 index 0000000..8ed6702 --- /dev/null +++ b/.claude/agents/chief-architect.md @@ -0,0 +1,24 @@ +--- +name: chief-architect +description: L5-L6 Synthesis. Reads every review report, spot-checks the code, and produces the architecture document and prioritized risk register. Use after all analysts and auditors complete. +tools: Read, Glob, Grep, Bash, Write +model: inherit +memory: project +color: purple +--- + +You are the chief architect (L5-L6) of a repository review organization. Everything below you has reported; your job is synthesis and judgment. You never modify source code. You may write exactly two report files - docs/review/40-architecture.md and docs/review/50-risk-register.md - plus files in your own agent memory directory. + +MANDATORY inputs: every file in docs/review/ (00, 10, all 20-domain-*, 30, 31). Spot-check the actual code wherever reports conflict or a claim carries major weight - you are the fact-checker of last resort. Where two reports disagree, resolve the disagreement in the code and record which report was wrong. + +docs/review/40-architecture.md: +1. System overview: what this software is and how it is shaped, one page, no fluff +2. Module map: domains, their boundaries, and dependency direction (ASCII or Mermaid diagram) +3. Data flow: the 2-3 most important end-to-end paths through the system +4. Design decisions inferred from the code, each with evidence, and whether it still serves the project +5. Coupling and boundary violations worth naming + +docs/review/50-risk-register.md: +Top 10 risks max, ranked by impact x likelihood. Each entry: risk, evidence (file paths, report references), blast radius, smallest credible mitigation, suggested owner-level (quick fix / project / strategic). Draw from ALL reports - security, quality, history (bus factor and abandonment are risks too). + +Check your agent memory for prior architectural understanding of this repo; update it afterward with the distilled system model so future reviews start smarter. diff --git a/.claude/agents/domain-analyst.md b/.claude/agents/domain-analyst.md new file mode 100644 index 0000000..99455a6 --- /dev/null +++ b/.claude/agents/domain-analyst.md @@ -0,0 +1,23 @@ +--- +name: domain-analyst +description: L3 Deep dive. Analyzes ONE assigned domain of the codebase in depth - modules, data flow, invariants, external dependencies. Spawn one instance per domain, in parallel, during a full-repo review. The task prompt must name the assigned domain and its directories. +tools: Read, Glob, Grep, Bash, Write +model: sonnet +memory: project +color: green +--- + +You are a senior domain analyst (L3) in a repository review organization. Each invocation assigns you exactly ONE domain (named in your task prompt, with its directories). Stay inside it; note cross-domain touchpoints without wandering into them. You never modify source code. You may write exactly one report file: docs/review/20-domain-.md (slug = your assigned domain, lowercased and hyphenated), plus files in your own agent memory directory. + +Read docs/review/00-inventory.md and 10-history.md first. Then produce your report covering: + +1. Responsibility: what this domain does, in two sentences a new engineer would understand +2. Key modules: each important file/class/function with path and one-line role +3. Data flow: how data enters, transforms, and leaves this domain (trace a representative request/operation end to end) +4. External dependencies: libraries, services, other domains it calls, and the contracts assumed +5. Invariants and conventions: implicit rules the code depends on (ordering, locking, schema shape, error contracts) +6. MATRIX FLAGS - two mandatory subsections the cross-cutting auditors will consume: + - "Security observations": anything touching auth, input parsing, secrets, network, filesystem, or deserialization + - "Quality observations": test coverage impressions, error-handling gaps, dead code suspicions, complexity hotspots + +Rules: every claim cites file:line where useful. Check your agent memory for patterns seen in prior reviews of this repo, and update it afterward with durable learnings (architecture facts, gotchas, invariants). Do not report speculation as fact. diff --git a/.claude/agents/executive-scribe.md b/.claude/agents/executive-scribe.md new file mode 100644 index 0000000..6739903 --- /dev/null +++ b/.claude/agents/executive-scribe.md @@ -0,0 +1,22 @@ +--- +name: executive-scribe +description: L7 Board report. Distills the entire review into an executive summary and updates CLAUDE.md so every future session inherits the understanding. Use as the final phase of a full-repo review. +tools: Read, Glob, Grep, Write, Edit +model: inherit +color: orange +--- + +You are the executive scribe (L7) of a repository review organization - the last mile between a pile of excellent reports and durable institutional understanding. You never modify source code. You may write docs/review/60-executive-summary.md and create or edit CLAUDE.md at the repository root. Nothing else. + +MANDATORY inputs: every file in docs/review/. Do not introduce new findings; you distill. + +docs/review/60-executive-summary.md (one page, board-level): +1. What this system is, in three sentences +2. Overall health assessment with a one-line verdict +3. Top 5 risks (from the risk register, in the architect's priority order) +4. Top 5 recommendations with rough effort sizing +5. Pointers: table of contents of docs/review/ with one line per report + +CLAUDE.md update - add or refresh a clearly delimited section: + ... +containing: the distilled system map (domains + one-liners), verified build/run/test/lint commands, conventions and invariants future agents must respect, danger zones (files where extra care is required and why), and the review date. Preserve all existing human-written CLAUDE.md content outside your markers exactly as-is. Keep your section under ~120 lines - it loads into every future session, so every line must earn its context cost. diff --git a/.claude/agents/git-historian.md b/.claude/agents/git-historian.md new file mode 100644 index 0000000..965168f --- /dev/null +++ b/.claude/agents/git-historian.md @@ -0,0 +1,20 @@ +--- +name: git-historian +description: L2 Forensics. Analyzes git history for churn hotspots, bus factor, abandoned areas, commit conventions, and recent activity. Use after repo-cartographer in a full-repo review. +tools: Bash, Read, Grep, Write +model: haiku +color: blue +--- + +You are the forensic historian (L2) of a repository review organization. You work exclusively through read-only git commands (git log, git shortlog, git blame, git branch -r, git diff --stat). You never modify source code. You may write exactly one file: docs/review/10-history.md. + +Read docs/review/00-inventory.md first for orientation. Then produce docs/review/10-history.md covering: + +1. Repository age, total commits, default branch, active branches and how stale each is +2. Churn hotspots: the 15 most-modified files/directories (these predict where bugs and knowledge live) +3. Bus factor: authorship concentration per major area +4. Abandoned zones: directories with no commits in 6+ months +5. Commit conventions actually in use (message format, PR patterns, tags/releases) +6. Recent trajectory: what the last 30-90 days of commits say the project is currently focused on + +Rules: show the actual git commands used and summarize their output rather than dumping it raw. Cite paths. Flag any anomaly (force-push scars, giant binary commits, orphaned branches) for the architect. diff --git a/.claude/agents/quality-auditor.md b/.claude/agents/quality-auditor.md new file mode 100644 index 0000000..059ad41 --- /dev/null +++ b/.claude/agents/quality-auditor.md @@ -0,0 +1,22 @@ +--- +name: quality-auditor +description: L4 Cross-cutting quality audit. Assesses tests, CI, lint, error handling, and maintainability across all domains after the analysts finish. Runs the test suite when feasible. +tools: Read, Glob, Grep, Bash, Write +model: sonnet +color: yellow +--- + +You are the quality auditor (L4) of a repository review organization - the second "column" of the review matrix. You never modify source code. Bash may run tests, linters, and type checkers in read-only fashion. You may write exactly one file: docs/review/31-quality.md. + +MANDATORY inputs first: docs/review/00-inventory.md, 10-history.md, and every 20-domain-*.md - especially each "Quality observations" subsection. Confirm or refute each analyst flag explicitly. + +Then assess: + +1. Test reality: does the suite exist, does it run, does it pass? (Run it if it completes in reasonable time; otherwise run a representative subset and say so.) Rough coverage impression per domain +2. CI/CD: what pipelines exist, what they actually gate, what they silently skip +3. Error handling: consistent strategy or ad hoc? Swallowed exceptions, bare catches, missing timeouts/retries +4. Type safety and lint posture: configs present vs. actually enforced; suppression density +5. Maintainability: duplication, god-files (cross-reference the historian's churn hotspots - churn x complexity = danger), dead code candidates +6. Developer experience: can a newcomer build and test from the documented commands alone? Try it literally + +Report format: same severity ranking as the security report (Critical -> Info), every finding with file:line evidence and the smallest credible fix. End with a "Health scorecard": one-line grade per domain with justification. diff --git a/.claude/agents/repo-cartographer.md b/.claude/agents/repo-cartographer.md new file mode 100644 index 0000000..173c9df --- /dev/null +++ b/.claude/agents/repo-cartographer.md @@ -0,0 +1,21 @@ +--- +name: repo-cartographer +description: L1 Recon. Maps the repository territory - file tree, languages, LOC, dependencies, build/test commands, entry points, config surface. Use as the first phase of any full-repo review. +tools: Read, Glob, Grep, Bash, Write +model: haiku +color: cyan +--- + +You are the reconnaissance scout (L1) of a repository review organization. You never modify source code. Bash is for read-only inspection only (ls, tree, wc, cloc, git ls-files, cat of manifests). You may write exactly one file: docs/review/00-inventory.md. + +Produce docs/review/00-inventory.md covering: + +1. Directory tree (top 3 levels) with a one-line purpose annotation per directory +2. Languages and approximate LOC per language +3. Every dependency manifest found (package.json, Package.swift, requirements.txt, go.mod, Cargo.toml, etc.) and its key dependencies with versions +4. Build, run, test, and lint commands as actually configured (scripts, Makefiles, CI files) +5. Entry points: main files, servers, CLIs, exported public APIs +6. Configuration and environment surface: env vars, config files, secrets PATTERNS (names only - never print values) +7. Oddities: generated code, vendored deps, git submodules, monorepo boundaries, unusually large files + +Rules: every claim cites a file path. If something cannot be determined, say so explicitly rather than guessing. End the report with a "Suggested domain decomposition" section: the 3-6 major domains a deep-dive team should split along, with the directories belonging to each. diff --git a/.claude/agents/security-auditor.md b/.claude/agents/security-auditor.md new file mode 100644 index 0000000..eb1adb5 --- /dev/null +++ b/.claude/agents/security-auditor.md @@ -0,0 +1,22 @@ +--- +name: security-auditor +description: L4 Cross-cutting security audit. Sweeps the entire codebase for vulnerabilities after domain analysts finish, cross-checking their flagged concerns. Produces a severity-ranked findings report. +tools: Read, Glob, Grep, Bash, Write +model: inherit +color: red +--- + +You are the security auditor (L4) of a repository review organization - the "column" of the review matrix that cuts across every domain "row". You never modify source code. Bash is for read-only scanning and dependency audit tools only (grep sweeps, npm audit, pip-audit, cargo audit, osv-scanner if available). You may write exactly one file: docs/review/30-security.md. + +MANDATORY inputs before any scanning: read docs/review/00-inventory.md, 10-history.md, and every 20-domain-*.md - especially each domain's "Security observations" subsection. Cross-check every concern the analysts flagged: confirm, refute, or escalate each one explicitly. + +Then run your own independent sweep: + +1. Secrets: hardcoded credentials, keys, tokens (report locations and patterns, never the values) +2. Injection surfaces: SQL/command/template injection, unsafe deserialization, eval-like constructs +3. AuthN/AuthZ: how identity is established, where checks live, endpoints or paths missing them +4. Input handling at trust boundaries: network, file uploads, IPC, env vars +5. Dependency risk: known-vulnerable versions from audit tooling; unpinned or abandoned deps +6. Filesystem and network hygiene: path traversal, SSRF, permissive CORS, TLS handling + +Report format: findings ranked Critical / High / Medium / Low / Info. Each finding = title, file:line, evidence snippet, why it matters, smallest credible fix. You are a skeptic: re-verify every finding against the actual code before it enters the report - false positives destroy this report's credibility. Include a final "Cleared" section listing analyst flags you investigated and dismissed, with reasons. diff --git a/.claude/commands/full-review.md b/.claude/commands/full-review.md new file mode 100644 index 0000000..7652fb6 --- /dev/null +++ b/.claude/commands/full-review.md @@ -0,0 +1,25 @@ +--- +description: Run the 7-level full repository review organization (agents in .claude/agents/) +argument-hint: [optional subtree path to scope a pilot run] +--- + +Run a complete review of this repository to build durable, full understanding. + +You are the executive layer of a 7-level review organization; your standing staff is defined in .claude/agents/ — repo-cartographer (L1), git-historian (L2), domain-analyst (L3), security-auditor (L4), quality-auditor (L4), chief-architect (L5-6), executive-scribe (L7). Do not perform any analysis yourself: delegate every phase to the named agent and hold each to the output contract in its definition. On surfaces that support dynamic workflows you may run this as a workflow; otherwise orchestrate it directly with subagents. + +If arguments were provided ($ARGUMENTS), treat this as a scoped pilot: restrict the entire organization to that subtree and cap Phase 3 at two domain-analysts. + +Pipeline — strict ordering between phases, maximum parallelism within a phase. A phase may not begin until the prior phase's report file(s) exist on disk: + +Phase 1 — Recon: repo-cartographer → docs/review/00-inventory.md +Phase 2 — Forensics: git-historian → docs/review/10-history.md +Phase 3 — Deep dives (matrix rows): take the "Suggested domain decomposition" from 00-inventory.md, spawn one domain-analyst PER DOMAIN in parallel, each assigned its domain name and directories → docs/review/20-domain-.md each +Phase 4 — Cross-cutting audits (matrix columns, in parallel): security-auditor → docs/review/30-security.md, quality-auditor → docs/review/31-quality.md. Both must consume every Phase 3 report and explicitly confirm or refute each analyst's flagged observations. +Phase 5 — Synthesis: chief-architect reads everything, resolves conflicts against the code → docs/review/40-architecture.md and docs/review/50-risk-register.md +Phase 6 — Board report: executive-scribe → docs/review/60-executive-summary.md, then updates CLAUDE.md inside its generated markers. + +Rules of engagement: +- The entire organization is read-only toward source code. Writes are permitted ONLY under docs/review/, .claude/agent-memory/, and to CLAUDE.md. +- Every claim in every report cites file paths (file:line where useful). Surprising findings are independently re-verified before they appear in any report. +- If a phase's output is missing or malformed, re-run that agent before advancing; do not paper over gaps yourself. +- When complete: commit all review outputs on the current working branch, then reply with the executive summary verbatim and a table of contents of docs/review/. From 66f2f9431fc59613fe02ceb623caa0d49b525fc7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 22:09:20 +0000 Subject: [PATCH 02/13] =?UTF-8?q?docs(review):=20Phase=201=20recon=20?= =?UTF-8?q?=E2=80=94=20repository=20inventory=20(00-inventory.md)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01FjTbUXge6kcW3TZMfAdz2J --- docs/review/00-inventory.md | 528 ++++++++++++++++++++++++++++++++++++ 1 file changed, 528 insertions(+) create mode 100644 docs/review/00-inventory.md diff --git a/docs/review/00-inventory.md b/docs/review/00-inventory.md new file mode 100644 index 0000000..c3118c9 --- /dev/null +++ b/docs/review/00-inventory.md @@ -0,0 +1,528 @@ +# NIGHTWATCH Repository Inventory (L1 Reconnaissance) + +**Generated:** 2026-07-12 +**Repository:** /home/user/NIGHTWATCH +**Git Status:** Clean repository (main branch) + +--- + +## 1. Directory Tree (Top 3 Levels) + +``` +. +├── bin/ # CLI entry points (bash launcher scripts) +├── deploy/ # Deployment and systemd service files +│ ├── scripts/ # Installation and upgrade shell scripts +│ └── systemd/ # systemd service definitions for NIGHTWATCH +├── docker/ # Container configurations +│ └── simulators/ # Docker images for testing (mount, camera, weather, PHD2) +├── docs/ # Project documentation +│ ├── assets/ # Images and diagrams +│ ├── decisions/ # Architecture Decision Records (ADRs) +│ ├── pos/ # Points of Study (research/analysis documents) +│ └── research/ # Research and sourcing guides +├── examples/ # Example usage scripts +├── firmware/ # Hardware firmware configs (OnStepX telescope controller) +│ └── onstepx_config/ # OnStepX board configuration headers +├── nightwatch/ # Core Python package (orchestrator, LLM client, safety) +├── pos/ # POS (Points Of Study) agent documents +│ └── agents/ # Named analysis agents (Howard Dutton, Damian Peach, etc.) +├── services/ # 22 independent service modules +│ ├── alerts/ # Alert manager and escalation +│ ├── alpaca/ # ASCOM Alpaca device client (network telescope control) +│ ├── astrometry/ # Plate solver integration (astrometry.net) +│ ├── camera/ # ZWO ASI camera driver wrapper +│ ├── catalog/ # Messier and object catalog with scoring +│ ├── enclosure/ # Roof/dome controller +│ ├── encoder/ # Encoder bridge for mount position tracking +│ ├── ephemeris/ # Skyfield-based celestial calculations +│ ├── focus/ # Autofocus service (V-curve analysis) +│ ├── guiding/ # PHD2 guide camera integration +│ ├── indi/ # INDI device client (Linux astronomy devices) +│ ├── meteor_tracking/ # Fireball network and shower tracking +│ ├── mount_control/ # LX200 and OnStepX mount commands +│ ├── nlp/ # Natural language processing (conversation, preferences) +│ ├── power/ # Power management and reboot scheduling +│ ├── safety_monitor/ # Safety interlocks (weather, limit sensors) +│ ├── scheduling/ # Task scheduling and conditions +│ ├── simulators/ # Mock devices for testing +│ ├── voice/ # Voice vocabulary and wake-word training +│ └── weather/ # Weather station integration (Ecowitt, AAG) +├── tests/ # Automated tests (110 Python files) +│ ├── e2e/ # End-to-end voice flow tests +│ ├── fixtures/ # Mock services and test utilities +│ ├── hardware/ # Hardware-dependent tests (skipped in CI) +│ ├── integration/ # Integration tests with Docker simulators +│ ├── mocks/ # Mock device implementations +│ └── unit/ # Unit tests per service +├── voice/ # Voice pipeline package +│ ├── stt/ # Speech-to-text (faster-whisper) +│ ├── tts/ # Text-to-speech (piper-tts) +│ ├── tools/ # Telescope and meteor command tools +│ └── wyoming/ # Wyoming voice integration protocol servers +└── .claude/ # Claude Code agent configuration + ├── agents/ # Domain-expert agent role definitions + └── commands/ # Custom command workflows +``` + +--- + +## 2. Languages and Approximate LOC + +| Language | Files | LOC | Purpose | +|----------|-------|-----|---------| +| Python | 213 | ~122,342 | Core application, services, voice pipeline, tests | +| YAML | 30+ | ~2,500 | Config files, CI/CD workflows, docker-compose | +| Markdown | 40+ | ~5,000+ | Documentation, architecture decisions, ADRs | +| Bash | 2 | ~100 | CLI launcher scripts (bin/nightwatch) | +| Batch | 1 | ~150 | Windows launcher (bin/nightwatch.bat) | +| C/Header | 1 | ~200 | OnStepX firmware config (firmware/onstepx_config/Config.h) | + +**Total Source LOC (excluding docs/tests):** ~25,000 (Python core + services) +**Total Project LOC (including tests/docs):** ~135,000+ + +--- + +## 3. Dependency Manifests and Key Dependencies + +### 3.1 Main Project Manifest +**File:** `/home/user/NIGHTWATCH/pyproject.toml` + +- **Build System:** `setuptools>=61.0`, `wheel` +- **Python Requirement:** `>=3.11` +- **Package Name:** `nightwatch` (v0.1.0-dev) +- **Entry Point:** CLI via `nightwatch = nightwatch.main:main` + +**Core Dependencies (nightwatch package):** +- `pydantic>=2.0` — Configuration validation and type checking +- `PyYAML>=6.0` — YAML configuration parsing + +**Optional Dependency Groups:** +- `services`: `skyfield>=1.48`, `aiohttp>=3.9`, `pyserial>=3.5`, `astropy>=6.0` +- `voice`: `faster-whisper>=1.0`, `piper-tts>=1.2`, `sounddevice>=0.5`, `numpy>=1.26`, `pymicro-vad>=1.0` +- `dev`: `pytest>=8.0`, `pytest-asyncio>=0.24`, `pytest-cov>=5.0`, `mypy>=1.14`, `ruff>=0.8`, `pre-commit>=4.0` + +### 3.2 Services Dependencies +**File:** `/home/user/NIGHTWATCH/services/requirements.txt` + +- `skyfield~=1.48` — JPL ephemeris (DE440) for celestial calculations +- `aiohttp~=3.9` — Async HTTP client for weather and external APIs +- `pyserial~=3.5` — Serial communication (telescope mounts) +- `pyindi-client~=2.0.8` — INDI device control (Linux astronomy) +- `alpyca~=3.0.0` — ASCOM Alpaca network device protocol + +### 3.3 Voice Pipeline Dependencies +**File:** `/home/user/NIGHTWATCH/voice/requirements.txt` + +- `faster-whisper~=1.0.3` — Speech-to-text (CTranslate2 optimized) +- `piper-tts~=1.2.0` — Text-to-speech synthesis +- `sounddevice~=0.5.1` — Audio I/O +- `numpy~=1.26` — Numerical computing for audio processing +- `pymicro-vad~=1.0.0` — Neural voice activity detection +- `webrtcvad~=2.0.10` — Google WebRTC VAD (fallback) + +### 3.4 Development Dependencies +**File:** `/home/user/NIGHTWATCH/requirements-dev.txt` + +**Testing:** `pytest~=8.3`, `pytest-asyncio~=0.24`, `pytest-cov~=5.0`, `pytest-xdist~=3.5`, `pytest-timeout~=2.3`, `pytest-mock~=3.14`, `responses~=0.25`, `aioresponses~=0.7` + +**Linting & Type Checking:** `ruff~=0.8`, `mypy~=1.14`, `types-PyYAML~=6.0`, `types-requests~=2.32` + +**Development Tools:** `pre-commit~=4.0`, `docker~=7.1`, `ipython~=8.30`, `ipdb~=0.13`, `build~=1.2`, `twine~=5.1` + +### 3.5 Dependency Lock File +**File:** `/home/user/NIGHTWATCH/uv.lock` (~460 KB) + +Pinned transitive dependencies for reproducible builds via `uv` package manager. + +--- + +## 4. Build, Run, Test, and Lint Commands + +### 4.1 Installation and Running + +**Primary Entry Point:** +```bash +# Via installed CLI command +nightwatch [--config /path/to/config.yaml] [--log-level DEBUG] [--dry-run] + +# Via Python module +python -m nightwatch.main [options] + +# Via bash launcher script +./bin/nightwatch [options] + +# Windows batch launcher +.\bin\nightwatch.bat [options] +``` +**Sources:** `/home/user/NIGHTWATCH/bin/nightwatch`, `/home/user/NIGHTWATCH/nightwatch/main.py` (lines 1–17) + +**Installation:** +```bash +pip install -e ".[all]" # Full development install +pip install -e ".[services]" # Services only +pip install -e ".[voice]" # Voice only +pip install -r requirements-dev.txt # Dev dependencies +``` + +### 4.2 Testing Commands (from CI) +**File:** `/home/user/NIGHTWATCH/.github/workflows/ci.yml` + +```bash +# Unit tests with coverage +pytest tests/unit/ -v \ + --cov=services --cov=nightwatch --cov=voice \ + --cov-report=term-missing --cov-report=xml:coverage.xml \ + --cov-report=html:coverage_html \ + -x --tb=short + +# Integration tests (Alpaca simulators) +pytest tests/integration/test_device_layer.py -v --tb=short -m "alpaca" --timeout=120 + +# Full integration suite +pytest tests/integration/ -v --tb=short --timeout=120 + +# E2E tests +pytest tests/e2e/ -v --tb=short --timeout=180 -m "e2e" + +# Mock service integration tests +pytest tests/integration/test_mount_catalog.py tests/integration/test_safety_mount.py -v +``` + +### 4.3 Linting and Type Checking +**File:** `/home/user/NIGHTWATCH/.github/workflows/ci.yml` + +```bash +# Ruff linting +ruff check services/ voice/ nightwatch/ --ignore=E501,F401,F841 --output-format=github + +# Ruff formatting check +ruff format --check services/ voice/ nightwatch/ + +# MyPy type checking +mypy nightwatch/ --ignore-missing-imports --no-error-summary --show-error-codes --pretty +mypy services/ --ignore-missing-imports --no-error-summary --show-error-codes --pretty +mypy voice/ --ignore-missing-imports --no-error-summary --show-error-codes --pretty + +# Security scanning +bandit -r services/ nightwatch/ voice/ -ll -f txt --exclude "**/test*,**/*_test.py" +pip-audit --requirement services/requirements.txt --format columns + +# Pre-commit hooks (local) +pre-commit install +pre-commit run --all-files +``` + +### 4.4 Docker and Deployment + +**Docker Compose:** +```bash +docker compose -f docker/docker-compose.dev.yml up -d # Development +docker compose -f docker/docker-compose.prod.yml up -d # Production +docker compose -f docker/docker-compose.test.yml up -d # Testing +``` + +**Deployment Scripts:** +- `/home/user/NIGHTWATCH/deploy/scripts/install.sh` — Installation script +- `/home/user/NIGHTWATCH/deploy/scripts/upgrade.sh` — Upgrade script + +**Systemd Services:** +```bash +# Install service +sudo systemctl enable /home/user/NIGHTWATCH/deploy/systemd/nightwatch.service +sudo systemctl start nightwatch +sudo systemctl status nightwatch +``` + +--- + +## 5. Entry Points + +### 5.1 CLI Entry Point +**Main Module:** `/home/user/NIGHTWATCH/nightwatch/main.py` (lines 1–50) + +Exports: +- `main()` — CLI entry point with argparse +- `async_main()` — Async orchestration logic +- `create_parser()` — Argument parser builder + +**Arguments:** +- `--config PATH` — Configuration file path +- `--log-level LEVEL` — Logging level (DEBUG, INFO, WARNING, ERROR) +- `--dry-run` — Validate config without starting +- `--help` — Show help + +### 5.2 Voice and Tool Execution +**Modules:** +- `/home/user/NIGHTWATCH/nightwatch/voice_pipeline.py` (83.5 KB) — Voice command parsing and LLM integration +- `/home/user/NIGHTWATCH/nightwatch/tool_executor.py` (57 KB) — Executes telescope/weather/mount commands +- `/home/user/NIGHTWATCH/voice/tools/telescope_tools.py` (225 KB) — Exported telescope control functions + +### 5.3 Orchestrator +**Module:** `/home/user/NIGHTWATCH/nightwatch/orchestrator.py` (121.7 KB) + +Main orchestration class that coordinates: +- Service initialization +- Health monitoring +- LLM client communication +- Emergency response handling +- Safety interlocks + +### 5.4 Services Initialization +**Module:** `/home/user/NIGHTWATCH/services/__init__.py` (3.3 KB) + +Exports each service module for dynamic loading: +- Alert manager, Alpaca client, Plate solver, Camera, Catalog, Enclosure, Encoder, Ephemeris, Focuser, Guider, INDI, Meteor tracking, Mount control, NLP, Power, Safety monitor, Scheduler, Simulators, Voice trainer, Weather + +--- + +## 6. Configuration and Environment Surface + +### 6.1 Configuration File +**Path:** `./nightwatch.yaml` (current dir) → `~/.nightwatch/config.yaml` (user) → `/etc/nightwatch/config.yaml` (system) +**Example:** `/home/user/NIGHTWATCH/nightwatch.yaml.example` (10.5 KB) + +**Configuration Sections:** +- `site` — Observatory location (latitude, longitude, elevation, timezone) +- `mount` — Telescope mount (type, host, port, serial, timeout, retry) +- `weather` — Weather station (type, host, poll interval) +- `voice` — Speech pipeline (model, device, language) +- `tts` — Text-to-speech (model, device) +- `llm` — LLM backend (model, endpoint, api_key) +- `safety` — Safety thresholds (wind, humidity, temperature, rain) +- `camera` — Camera setup +- `guider` — Guide camera (PHD2) +- `encoder` — Mount encoders +- `alert` — Alert escalation +- `power` — Power management (reboot scheduling) +- `enclosure` — Roof/dome control + +### 6.2 Environment Variables (NIGHTWATCH_ Prefix) + +**Supported Override Pattern:** `NIGHTWATCH_
_=value` + +Example: +```bash +NIGHTWATCH_MOUNT_HOST=192.168.1.100 +NIGHTWATCH_SITE_LATITUDE=38.9 +NIGHTWATCH_VOICE_MODEL=large-v3 +``` + +**Safety Override Allowlist:** Empty by default (line 90 in `/home/user/NIGHTWATCH/nightwatch/config.py`) +Only explicitly allowlisted safety thresholds can be overridden via env vars to prevent accidental disabling of safety interlocks. + +**Source:** `/home/user/NIGHTWATCH/nightwatch/config.py` (lines 8–90) + +### 6.3 Logging Configuration +**Module:** `/home/user/NIGHTWATCH/nightwatch/logging_config.py` (12.2 KB) + +Configurable log levels and output handlers (console, file, structured). + +### 6.4 Launcher Environment +**Script:** `/home/user/NIGHTWATCH/bin/nightwatch` (lines 14–17) + +Recognized env vars: +- `NIGHTWATCH_CONFIG` — Configuration file path +- `NIGHTWATCH_LOG_LEVEL` — Logging verbosity +- `VIRTUAL_ENV` — Python venv activation path + +--- + +## 7. Oddities and Special Features + +### 7.1 No Generated Code, Vendored Dependencies, or Monorepo Boundaries +- No `generated/` directories detected +- No vendored third-party code; all dependencies managed via pyproject.toml and requirements files +- No git submodules (checked via `.gitmodules`) +- Single-package monorepo structure: core (`nightwatch`) + services layer (`services/`) + voice pipeline (`voice/`) + +### 7.2 Large Files +**Lock File:** `/home/user/NIGHTWATCH/uv.lock` (~460 KB) +Contains pinned transitive dependencies for `uv` package manager. + +**Large Modules:** +- `nightwatch/orchestrator.py` (121.7 KB) — Main orchestration engine +- `voice/tools/telescope_tools.py` (225 KB) — Exported telescope command signatures for voice control +- `nightwatch/voice_pipeline.py` (83.5 KB) — Voice command parsing and LLM orchestration +- `nightwatch/llm_client.py` (42.8 KB) — LLM API client abstraction +- `nightwatch/tool_executor.py` (57 KB) — Tool execution and parameter validation +- `services/safety_monitor/monitor.py` (71 KB) — Safety interlock engine + +### 7.3 Testing Infrastructure +**110 Python test files** across: +- `tests/unit/` — Unit tests (default, fast) +- `tests/integration/` — Tests requiring Docker simulators or mock services +- `tests/e2e/` — End-to-end voice flow tests +- `tests/hardware/` — Hardware tests (skipped in CI, require real telescope hardware) +- `tests/fixtures/` — Mock implementations of all services (weather, mount, camera, guider, LLM, etc.) + +**Docker Simulators:** +- Mount simulator (`docker/simulators/Dockerfile.mount`) +- Camera simulator (`docker/simulators/Dockerfile.mount`) +- Weather simulator (`docker/simulators/Dockerfile.weather`) +- PHD2 guide simulator (`docker/simulators/Dockerfile.phd2`) +- Cloud watcher simulator (`docker/simulators/Dockerfile.cloud`) + +### 7.4 Safety-Critical Features +**Module:** `/home/user/NIGHTWATCH/nightwatch/safety_interlock.py` (18.7 KB) + +Implements: +- Dual-redundant rain sensor voting (SAFE-002) +- Hardware watchdog fail-safe (SAFE-004) +- Cancellation token propagation (ARCH-003) +- Safety threshold allowlisting (SAFE-003, line 90 of config.py) + +### 7.5 Firmware Configuration +**Included:** `/home/user/NIGHTWATCH/firmware/onstepx_config/Config.h` (~200 lines) + +OnStepX telescope controller configuration header (not built as part of Python app; provided for reference). + +### 7.6 Structured Documentation +**Architecture Decisions:** `/home/user/NIGHTWATCH/docs/decisions/` (ADR format) +**Research/Analysis:** `/home/user/NIGHTWATCH/docs/pos/` and `/home/user/NIGHTWATCH/pos/agents/` (Points of Study) +**Agent Definitions:** `/home/user/NIGHTWATCH/.claude/agents/` (Claude Code agent roles) + +### 7.7 Pre-commit Hooks (Mandatory) +**File:** `/home/user/NIGHTWATCH/.pre-commit-config.yaml` (155 lines) + +Enforces before every commit: +- File format checks (trailing newlines, merge markers, case conflicts) +- YAML/TOML/JSON validation +- Private key detection +- Ruff linting and formatting (auto-fix) +- MyPy type checking (nightwatch only) +- Bandit security checks (excluding tests) +- No direct commits to main/master + +--- + +## 8. Suggested Domain Decomposition + +Based on L1 reconnaissance, this system decomposes into **5 primary domains** with clear architectural boundaries: + +### 8.1 **Core Orchestration & Safety** (Decision Authority) +**Directories:** +- `nightwatch/` — Main orchestrator, config, logging, safety interlocks, health checks, emergency response +- `nightwatch/safety_interlock.py`, `nightwatch/emergency_response.py`, `nightwatch/watchdog.py` + +**Responsibilities:** +- System state machine and lifecycle management +- Safety threshold enforcement and veto logic +- Configuration loading and validation +- Signal handling and graceful shutdown +- Health monitoring and watchdog timers + +**Coupling:** Highest coupling; depends on all other domains +**Test Coverage:** Unit and e2e tests in `tests/unit/`, `tests/e2e/` + +--- + +### 8.2 **Voice & Natural Language Processing** (Input Interface) +**Directories:** +- `voice/` — Speech-to-text, text-to-speech, audio I/O, Wyoming protocol servers + - `voice/stt/` — faster-whisper speech recognition + - `voice/tts/` — piper-tts synthesis + - `voice/wyoming/` — Wyoming voice protocol implementation +- `services/nlp/` — Clarification, conversation context, user preferences, sky description + +**Responsibilities:** +- Audio capture and voice activity detection +- Speech-to-text inference (faster-whisper) +- Natural language understanding (conversation tracking, preferences) +- Text-to-speech synthesis (piper-tts) +- Wyoming protocol bridge for third-party integrations + +**Coupling:** Upstream of tool execution; depends on LLM client for intent routing +**Test Coverage:** Unit tests in `tests/unit/test_voice*`, fixtures in `tests/fixtures/mock_stt.py`, `tests/fixtures/mock_tts.py` + +--- + +### 8.3 **Command Execution & Tool Integration** (Orchestration Agents) +**Directories:** +- `nightwatch/tool_executor.py` — Command parsing, parameter validation, tool invocation +- `nightwatch/response_formatter.py` — Format tool outputs for TTS/display +- `voice/tools/telescope_tools.py` — Exported telescope command signatures (225 KB) +- `voice/tools/meteor_tools.py` — Meteor and shower tracking commands +- `services/voice/vocabulary_trainer.py`, `services/voice/wake_word_trainer.py` + +**Responsibilities:** +- Parse and validate voice commands into structured parameters +- Route commands to appropriate service modules +- Format structured responses for human-readable TTS +- Maintain vocabulary and command recognition models +- Tool parameter type validation and bounds checking + +**Coupling:** Central hub between voice and hardware services; delegates to astronomy/device services +**Test Coverage:** Unit tests, mock LLM in `tests/fixtures/mock_llm.py` + +--- + +### 8.4 **Astronomy & Hardware Services** (Capability Modules) +**22 Service Modules in `services/`:** + +**Astronomy/Observation:** +- `ephemeris/` — Skyfield celestial calculations (sun, moon, planets, custom objects) +- `catalog/` — Messier and object catalog with scoring and identification +- `astrometry/` — Plate solver integration (astrometry.net, ASTAP) +- `meteor_tracking/` — Fireball network, shower calendar, trajectory +- `guiding/` — PHD2 guide star auto-calibration and guiding loop + +**Hardware Control:** +- `mount_control/` — LX200 and OnStepX commands (goto, park, unpark, sync) +- `camera/` — ZWO ASI camera capture and frame analysis +- `focus/` — Autofocus V-curve analysis and stepping +- `enclosure/` — Roof/dome open/close control +- `encoder/` — Mount encoder position tracking + +**Infrastructure:** +- `alpaca/`, `indi/` — Device protocol clients (network and local) +- `weather/` — Ecowitt, AAG, WS90 weather station integration +- `power/` — Power management and scheduled reboots +- `safety_monitor/` — Real-time safety veto (wind, humidity, rain, temperature) +- `alerts/` — Alert escalation and notification + +**Utility:** +- `scheduling/` — Cron-like task scheduling and condition evaluation +- `simulators/` — Mock mount, camera, guider, weather for testing + +**Responsibilities:** +- Encapsulated hardware and external service communication +- State caching (position, temperature, etc.) +- Retry logic and connection resilience +- Async device operations + +**Coupling:** Loosely coupled to each other; tightly coupled to orchestrator for dispatch +**Test Coverage:** Unit per module, integration with Docker simulators, mocks in `tests/fixtures/` + +--- + +### 8.5 **LLM Client & Tool Binding** (Decision Engine) +**Directories:** +- `nightwatch/llm_client.py` (42.8 KB) — LLM API client, token management, model abstraction +- `nightwatch/tool_params.py` — Tool parameter schema definitions +- `nightwatch/cancellation.py` — Async cancellation token propagation + +**Responsibilities:** +- Abstract LLM backend (OpenAI-compatible API, local inference, etc.) +- Tool schema registration and parameter binding +- Token usage tracking and cost monitoring +- Request/response streaming and error handling +- Cancellation propagation for safe interruption + +**Coupling:** Used by orchestrator and voice pipeline; knows about all tool signatures from command services +**Test Coverage:** Unit tests with mock LLM in `tests/fixtures/mock_llm.py` + +--- + +## Summary Table: Domain Allocation + +| Domain | Directories | Purpose | Key Files | +|--------|-----------|---------|-----------| +| **Core Orchestration & Safety** | `nightwatch/` | System state, safety enforcement, lifecycle | `orchestrator.py`, `safety_interlock.py`, `config.py` | +| **Voice & NLP** | `voice/`, `services/nlp/` | Audio I/O, speech recognition, understanding | `voice_pipeline.py`, `whisper_service.py`, `clarification.py` | +| **Command Execution & Tools** | `nightwatch/tool_executor.py`, `voice/tools/` | Command routing, parameter validation | `tool_executor.py`, `telescope_tools.py` (225 KB) | +| **Astronomy & Hardware Services** | `services/` (22 modules) | Observatory hardware and external service integration | `services/*/`, simulators, device clients | +| **LLM Client & Tool Binding** | `nightwatch/llm_client.py`, `nightwatch/tool_params.py` | LLM abstraction, tool schema, token management | `llm_client.py`, `tool_params.py` | + +Each domain can be reviewed, tested, and evolved independently by a focused team, with clear interfaces defined by the orchestrator's tool invocation API and configuration schemas. + From 92e7c4d5f765e3e0e46aefdfa449cc4e1e4ad03a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 22:13:01 +0000 Subject: [PATCH 03/13] =?UTF-8?q?docs(review):=20Phase=202=20forensics=20?= =?UTF-8?q?=E2=80=94=20git=20history=20analysis=20(10-history.md)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01FjTbUXge6kcW3TZMfAdz2J --- docs/review/10-history.md | 432 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 432 insertions(+) create mode 100644 docs/review/10-history.md diff --git a/docs/review/10-history.md b/docs/review/10-history.md new file mode 100644 index 0000000..d2bdbba --- /dev/null +++ b/docs/review/10-history.md @@ -0,0 +1,432 @@ +# NIGHTWATCH Repository Forensics (L2 History) + +**Generated:** 2026-07-12 +**Repository:** /home/user/NIGHTWATCH +**Analysis Date Range:** 2026-01-20 (first commit) through 2026-07-12 (current) +**Analyst:** Forensic Historian L2 + +--- + +## 1. Repository Age, Commits, and Branch Status + +### 1.1 Repository Chronology + +**Command:** `git log --all --format=%ai --reverse | head -1` and `git log --oneline | head -1` + +**Findings:** +- **First commit:** 2026-01-20 05:29:45 UTC +- **Repository age:** ~5.5 months +- **Total commits:** 61 +- **Most recent commit:** `66f2f94` (2026-07-12) — "docs(review): Phase 1 recon — repository inventory (00-inventory.md)" + +The repository is young, still in active development phase (v0.1.0-dev per `/home/user/NIGHTWATCH/pyproject.toml`). + +### 1.2 Default and Active Branches + +**Command:** `git branch -r --sort=-committerdate --format='%(refname:short)%09%(committerdate:short)'` + +**Active remote branches:** +| Branch | Last Commit | +|--------|-------------| +| `origin/claude/install-review-org-37y4ck` | 2026-07-12 (current, 0 days old) | +| `origin/main` | 2026-06-15 (27 days stale) | + +**Key observation:** Main branch is 27 days behind the current working branch. The primary development is happening on the `claude/install-review-org-37y4ck` review/organization branch, not on main. Only 2 remote branches visible; minimal feature branching detected in git log history. + +--- + +## 2. Churn Hotspots: 15 Most-Modified Files/Directories + +**Command:** `git log --name-only --pretty=format: | sort | uniq -c | sort -rn | head -20` + +These files are the prediction points for bugs, knowledge concentration, and integration risk: + +| Rank | File/Directory | Commits | Type | Notes | +|------|---|---|---|---| +| 1 | `nightwatch/orchestrator.py` | 10 | Core | Central command orchestration and service coordination | +| 2 | `tests/unit/test_camera_service.py` | 7 | Test | Camera service test suite (heavy iteration) | +| 3 | `services/camera/asi_camera.py` | 7 | Service | ZWO ASI camera driver (frequent fixes/features) | +| 4 | `tests/unit/test_llm_client.py` | 6 | Test | LLM client test coverage | +| 5 | `services/nlp/__init__.py` | 6 | Service | NLP service module (conversation, preferences) | +| 6 | `services/safety_monitor/monitor.py` | 5 | Service | Safety interlocking engine (critical, high churn) | +| 7 | `nightwatch/llm_client.py` | 5 | Core | LLM backend abstraction (token management, model routing) | +| 8 | `voice/tools/telescope_tools.py` | 4 | Voice | Exported tool signatures for voice command binding | +| 9 | `tests/unit/test_tool_executor.py` | 4 | Test | Tool execution and parameter validation tests | +| 10 | `services/weather/ecowitt.py` | 4 | Service | Weather station integration (Ecowitt protocol) | +| 11 | `tests/unit/test_orchestrator.py` | 3 | Test | Orchestrator integration tests | +| 12 | `tests/unit/test_config.py` | 3 | Test | Configuration validation and override tests | +| 13 | `services/catalog/__init__.py` | 3 | Service | Messier catalog scoring and object selection | +| 14 | `services/alerts/alert_manager.py` | 3 | Service | Alert escalation logic | +| 15 | `nightwatch/tool_executor.py` | 3 | Core | Command routing and parameter binding | + +**Key pattern:** Orchestrator + core LLM/tool infrastructure (rows 1, 5, 7, 15) account for most churn; camera service (rows 2–3) shows early hardware integration focus; test files (rows 2, 4, 9, 11, 12) indicate reactive rather than proactive test-driven development. + +**Directory-level churn:** (from `git log --name-only` across all 61 commits) +``` +tests/ 31 file-touches (test suite growth) +services/ 24 file-touches (22 service modules) +nightwatch/ 23 file-touches (core orchestration) +.claude/ 8 file-touches (agent configuration) +voice/ 6 file-touches (speech pipeline) +docs/ 4 file-touches (documentation) +``` + +--- + +## 3. Bus Factor: Authorship Concentration Per Major Area + +**Commands:** +- `git log --all --pretty=format:%an | sort | uniq -c | sort -rn` +- `git shortlog --all -sn --email` +- `git log --all --pretty=format:%h%x09%an -- ` + +### 3.1 Overall Authorship + +| Author | Commits | Percentage | Email | +|--------|---------|-----------|-------| +| THOClabs | 31 | 51% | `timothyehennessey@gmail.com` | +| Tim Hennessey | 28 | 46% | `timothyehennessey@gmail.com` | +| Claude | 2 | 3% | `noreply@anthropic.com` | + +**CRITICAL FINDING:** Two email identities for the same person (Timothy Hennessey) account for **97% of all commits**. This is an extreme single point of failure. + +### 3.2 Bus Factor by Domain + +**Command:** `git log --all --pretty=format:%h%x09%an -- "services/camera/*"`, etc. + +| Area | Authors | Commit Distribution | Risk Level | +|------|---------|---------------------|-----------| +| **Orchestrator** (`nightwatch/orchestrator.py`) | Tim Hennessey (100%) | 10 commits all from one author | CRITICAL | +| **LLM/Tool Execution** (`nightwatch/llm_client.py`, `nightwatch/tool_executor.py`) | Tim Hennessey (100%) | 8 commits all from one author | CRITICAL | +| **Camera Service** (`services/camera/`) | THOClabs (50%), Tim Hennessey (50%) | Mixed but only 2 identities | HIGH | +| **Safety Monitor** (`services/safety_monitor/monitor.py`) | Tim Hennessey (60%), THOClabs (40%) | 5 commits, slight Tim skew | HIGH | +| **NLP Service** (`services/nlp/`) | THOClabs (100%) | 6 commits all from one person | CRITICAL | +| **Guiding/Focus** (`services/guiding/`, `services/focus/`) | Tim Hennessey (100%) | Recent work all one author | CRITICAL | + +**Conclusion:** Every major subsystem is owned by a single person (either Tim or THOClabs, same individual). No code review distribution. Zero knowledge redundancy. + +--- + +## 4. Abandoned Zones: Directories Without Commits 6+ Months + +**Command:** `find services -type d -maxdepth 1 | while read dir; do git log --all --max-count=1 --format=%ai -- $dir; done` + +Since the repository is only 5.5 months old, "6+ months" is not applicable, but we can identify **stale subsystems** (not touched since initial implementation in January): + +### 4.1 Truly Abandoned (Last commit: 2026-01-20, ~171 days ago) + +These modules were implemented at inception and never changed: +- `services/alpaca/` — ASCOM Alpaca device client (network telescope control) +- `services/enclosure/` — Roof/dome controller +- `services/encoder/` — Encoder bridge for mount position +- `services/ephemeris/` — Skyfield-based celestial calculations +- `services/indi/` — INDI device client (Linux astronomy devices) +- `services/simulators/` — Mock devices for testing + +**Risk:** No maintenance, no bug fixes, no refactoring. May contain outdated patterns or undetected issues. High refactoring debt. + +### 4.2 Stale (Last commit: 2026-01-28, ~165 days ago) + +- `services/alerts/` — Alert manager and escalation +- `services/meteor_tracking/` — Fireball network and shower tracking + +**Risk:** Set-and-forget implementations. No recent validation against evolving requirements. + +### 4.3 Recently Active (Last commit: 2026-05-25 onwards) + +Hardware-critical modules that received heavy attention in the May "worktree-modernization" branch: +- `services/camera/` — Last: 2026-05-25 04:14:01 (47 days of work) +- `services/weather/` — Last: 2026-05-25 12:11:30 +- `services/safety_monitor/` — Last: 2026-05-25 13:36:35 (CRITICAL, ongoing) +- `services/guiding/` — Last: 2026-05-25 14:31:31 +- `services/focus/` — Last: 2026-05-25 15:39:22 (most recent service work, May 25) + +**Insight:** Planned modernization push in May targeted observation/guiding pipeline. Earlier infrastructure (device clients, ephemeris) deemed stable. + +--- + +## 5. Commit Conventions Actually in Use + +**Commands:** +- `git log --all --oneline | head -30` +- `git log --all --pretty=format:%s | grep -E "^(feat|fix|docs|test|chore|refactor)"` (convention audit) +- `git log --all --pretty=format:%s | head -50` + +### 5.1 Conventional Commit Format + +The repository enforces **strict conventional commits** with area prefixes: + +``` +(): [ADR/spec reference] [Risk notes] +``` + +**Observed types (in order of frequency):** +- `feat()` — 16 commits in last 60 days (53% of recent work) +- `refactor()` — 4 commits (13%) +- `fix()` — 3 commits (10%) +- `docs()` — 3 commits (10%) +- `chore()` — 1 commit (2%) +- `test()` — 1 commit (2%) + +**Sample recent commits:** +``` +feat(safety): SAFE-001 cancel-before-close ordering + EMERGENCY_CLOSE actually closes roof (Risk #2) +feat(safety): SAFE-004 hardware-level watchdog fail-safe (roof close on safety_monitor timeout) +feat(cancellation): ARCH-003 propagate CancelToken through orchestrator + camera +feat(llm): VOX-003 validate LLM tool-call args against ARCH-001 Pydantic models +fix(tools): ARCH-001 reject bool coercion in RA/Dec + ClassVar annotation +refactor(camera): HWS-001 extract _do_exposure, fix _capturing race + per-frame stats +``` + +### 5.2 Architecture Decision Record Integration + +**Observed pattern:** Commits heavily reference ADRs, hardware specs, and feature specs: + +| Reference Prefix | Examples | Purpose | +|------------------|----------|---------| +| `ARCH-` | ARCH-001, ARCH-002, ARCH-003 | Architectural decisions (tool params, health gating, cancellation) | +| `SAFE-` | SAFE-001, SAFE-002, SAFE-003, SAFE-004 | Safety requirements (rain voting, watchdog, allowlisting, close ordering) | +| `HWS-` | HWS-001 through HWS-005 | Hardware/workflow specs (camera capture, TEC cooling, autofocus, etc.) | +| `VOX-` | VOX-002, VOX-003 | Voice/LLM features (refusal handling, tool validation) | +| `DEP-` | DEP-001 | Deployment specs (Dockerfile, production readiness) | +| `Risk #X` | Risk #2, Risk #9 | Linked risk register items | + +**Quality observation:** Disciplined traceability from commits to requirements. Every feature tied to a spec. Suggests mature engineering practices despite young repo. + +### 5.3 Merge/PR Patterns + +**Command:** `git log --all --grep="Merge" --oneline` + +**Findings:** +- **1 merge commit** in entire history: `fdad491` (2026-05-24) "Merge branch 'worktree-modernization-2026-05-24-continued'" +- **No GitHub PR merge commits** detected in log +- **Mostly direct commits** to working branches (rebase/squash workflow, or direct push) + +**Interpretation:** +- Minimal branch-per-feature workflow (only 1 feature branch merged in 61 commits) +- Likely direct commit to branches or squash-rebase before merge +- No GitHub PR template enforcement visible +- Pre-commit hooks enforce no direct commits to main (per `.pre-commit-config.yaml`) + +### 5.4 Releases/Tags + +**Command:** `git tag -l` + +**Finding:** **No tags.** No semantic versioning (v0.1.0, v0.2.0, etc.). + +**Risk:** No release history, no rollback points, no versioned artifacts. Development-only state. + +--- + +## 6. Recent Trajectory: Last 30–90 Days of Commits + +**Analysis period:** May 13, 2026 (60 days ago) through July 12, 2026 (today) + +**Command:** `git log --all --since="2026-05-13" --oneline | wc -l` and `git log --all --since="2026-05-13" --pretty=format:%s` + +### 6.1 Commit Velocity Trend + +| Period | Commits | Commits/Month | Interpretation | +|--------|---------|---------------|---| +| Jan 20 – Apr 20 (first 3 months) | 9 | 3/month | **Slow bootstrap** | +| Apr 20 – Jul 12 (last 3 months) | 30 | 10/month (projected) | **Acceleration x3** | +| May 13 – Jul 12 (last 60 days) | 30 | 15/month (projected) | **Current sustained velocity** | + +**Interpretation:** Project entered **active development phase** in late May. 3x acceleration in commit rate suggests: +- Shift from planning/architecture to implementation +- May 24 "worktree-modernization" branch as inflection point (merged May 24) +- Current team bandwidth at ~15 commits/month + +### 6.2 Feature Breakdown (Last 60 Days) + +**Command:** `git log --all --since="2026-05-12" --pretty=format:%s | grep -o "^[a-z]*(" | sort | uniq -c | sort -rn` + +| Commit Type | Count | % | Focus | +|---|---|---|---| +| feat | 16 | 53% | New capabilities (hardware, safety, tools) | +| refactor | 4 | 13% | Code quality (camera race conditions, wording) | +| fix | 3 | 10% | Bug fixes (tool coercion, orchestrator bypass) | +| docs | 3 | 10% | Documentation updates | +| test | 1 | 3% | Test suite growth | +| chore | 1 | 2% | Dependency management | + +**ANOMALY ALERT:** 16 feature commits vs. only 1 test commit. Feature velocity (53%) far outpaces test coverage growth (3%). High technical debt risk. + +### 6.3 What the Team is Focused On (Last 60 Days) + +**Thematic analysis of recent `feat` commits:** + +1. **Safety-critical hardening** (3 commits) + - `SAFE-001`: Cancel-before-close ordering (roof control risk) + - `SAFE-002`: Dual-redundant rain sensor voting + - `SAFE-004`: Hardware-level watchdog (fail-safe close) + +2. **Hardware integration modernization** (5 commits) + - `HWS-001`: ZWO ASI camera capture (real SDK integration, TEC cooling) + - `HWS-002`: TEC closed-loop controller (autofocus pre-work) + - `HWS-003`: PHD2 guiding orchestration + - `HWS-004`: Astrometry plate solver + mount sync + - `HWS-005`: V-curve autofocus confidence metrics + +3. **Tool/LLM validation** (4 commits) + - `ARCH-001`: Pydantic model validation for tool parameters (RA/Dec bool coercion rejection) + - `VOX-003`: LLM tool-call argument validation + - Stricter type checking to prevent voice command misinterpretation + +4. **Orchestration resilience** (2 commits) + - `ARCH-002`: Health-gating bypass logic (allow graceful shutdown during health-monitor outage) + - `ARCH-003`: Cancellation token propagation (safe async task interruption) + +5. **Deployment readiness** (1 commit) + - `DEP-001`: Production Dockerfile + .dockerignore (containerization) + +**Dominant theme:** Hardware reliability + safety + LLM tool correctness. The team is validating and hardening a complex voice-controlled telescope automation system against real-world failure modes. + +### 6.4 Who is Driving Recent Work + +**Command:** `git log --all --since="2026-05-12" --pretty=format:%an | sort | uniq -c | sort -rn` + +| Author | Commits (last 60 days) | Note | +|--------|---|---| +| Tim Hennessey | 28 | Dominant; all core infrastructure, safety, hardware | +| THOClabs | — | Minimal recent activity | +| Claude | 2 | Late additions (review org setup, inventory) | + +**Observation:** Tim Hennessey is the sole active developer in the recent acceleration phase. THOClabs went dormant after Jan/early Feb; Claude joined very recently for documentation/review infrastructure. + +--- + +## 7. Anomalies and Flags for the Architect + +### 7.1 Severity: CRITICAL — Single Point of Failure + +**Issue:** One person (Timothy Hennessey) has authored 97% of commits. All decision-critical subsystems (orchestrator, LLM client, tool executor, safety monitor, guiding/focus) have zero code review and zero secondary ownership. + +**Implication:** Knowledge bus factor = 1. Project at risk of: +- Continuity loss (illness, departure, burnout) +- Architectural fragility (no cross-review to catch design flaws) +- Onboarding wall for new contributors + +**Recommendation:** Immediate code review pairing; accelerate secondary ownership of safety-critical modules. + +### 7.2 Severity: HIGH — Feature Velocity >> Test Coverage Velocity + +**Issue:** 16 feature commits vs. 1 test commit in last 60 days. Test-to-feature ratio **16:1** (should be closer to 1:1 or 1:2). + +**Implication:** Features deployed with unvalidated coverage. Example: 5 camera/hardware commits but only 7 camera test file touches since Jan 20 (reactive tests, not TDD). + +**Data point:** `git log --all --pretty=format:%s | grep "test("` yields only 1 commit in the entire 61-commit history, vs. 26 feature commits total. + +**Recommendation:** Implement test-before-commit gate in CI; track test/feature coverage ratio per sprint. + +### 7.3 Severity: MEDIUM — Stale Subsystem Modules + +**Issue:** 6 service modules (alpaca, enclosure, encoder, ephemeris, indi, simulators) haven't been touched since Jan 20 initial commit. Total of 8 modules not modified in 165+ days. + +**Implication:** +- Dead code or unrealistic first-pass implementation +- Unknown bugs in non-critical paths (will surface in integration) +- Refactoring debt (older code patterns vs. newer ARCH-001 Pydantic models) +- Inconsistent error handling across 22 service modules + +**Example risk:** `services/alpaca/` (network device protocol) and `services/indi/` (Linux device control) are untested in recent refactoring; both are integration points that often fail. + +**Recommendation:** Audit "abandoned" modules for compliance with current ARCH decisions (ARCH-001 Pydantic models, ARCH-003 cancellation tokens). Refresh or deprecate. + +### 7.4 Severity: MEDIUM — No Releases / No Rollback Points + +**Issue:** Repository is at v0.1.0-dev with zero git tags. 61 commits, zero semantic releases. + +**Implication:** +- Impossible to bisect bug introductions across versions +- No external consumption (PyPI, container registry) possible +- Rollback in production limited to raw git reset (dangerous) + +**Recommendation:** Tag v0.1.0-alpha at next stable point; establish release cadence (weekly/sprint-end). + +### 7.5 Severity: LOW — Limited Feature Branching + +**Issue:** Only 1 merge commit in 61 commits. Minimal feature branch history (likely direct commits or squash-rebases with no merge commit). + +**Implication:** +- Possible, no merge-conflict resolution history visible (good for small team) +- But also suggests weak branch discipline (pre-commit hooks block main, but branches may not be consistent) + +**Observation:** `.pre-commit-config.yaml` (line ~48) blocks commits to main/master; enforced via hooks. This explains direct commits to `claude/install-review-org-37y4ck` and quick merges. + +**Recommendation:** Formalize feature branch naming (`feat/*`, `fix/*`, `safety/*`) and require PR for all non-main branches. + +### 7.6 Severity: LOW — Architecture Reference Discipline Inconsistent + +**Issue:** Some commits reference ARCH/SAFE/HWS specs; others don't. E.g., "Add meteor tracking to Configuration Guide" (Jan 28) has no spec reference. + +**Implication:** Traceability not 100% enforced. Older commits (Jan 20–28) less disciplined than recent work (May+). + +**Recommendation:** Strengthen commit message template to require spec reference for feat/fix; enforce via hook or pre-commit check. + +### 7.7 Status: OK — No Forced Push Scars + +**Finding:** Reflog analysis shows clean history. No `git reset --hard`, no `git rebase --force`, no commit rewrites. + +**Implication:** History is reliable; no hidden work loss. + +--- + +## 8. Commit Message Patterns and Discipline + +**Sample of message structures observed:** + +``` +feat(safety): SAFE-001 cancel-before-close ordering + EMERGENCY_CLOSE actually closes roof (Risk #2) +↑type ↑area ↑spec ↑feature ↑risk register + +refactor(camera): HWS-001 extract _do_exposure, fix _capturing race + per-frame stats +↑type ↑area ↑spec ↑refactoring rationale + +test(llm): VOX-003 add partial-pass + bool-RA coverage + empty-name cosmetic +↑type ↑area ↑spec ↑test scenarios + +docs(review): Phase 1 recon — repository inventory (00-inventory.md) +↑type ↑area ↑summary ↑file link +``` + +**Discipline score:** 8/10 +- Consistent type/area structure +- Strong spec/ADR traceability +- But: type/area not enforced by commit hook (older commits less consistent) +- Suggestion: Add `commitlint` hook to enforce ARCH-/SAFE-/HWS-/VOX-/DEP- references for feat/fix + +--- + +## 9. Summary: Repository Health Scorecard + +| Dimension | Score | Notes | +|-----------|-------|-------| +| **Commit Hygiene** | 8/10 | Conventional commits, good messaging, no forced pushes | +| **Test Coverage Growth** | 3/10 | Features 16x ahead of test commits; reactive testing | +| **Code Review Coverage** | 1/10 | Single author 97% of commits; no distributed ownership | +| **Release Hygiene** | 2/10 | No tags, no versions, no rollback points | +| **Documentation** | 7/10 | ADR discipline good; commit messages link to specs | +| **Architecture Compliance** | 7/10 | ARCH/SAFE specs enforced in recent work; older modules inconsistent | +| **Module Maintenance** | 5/10 | 40% of services untouched since Jan 20; widening gap | +| **Deployment Readiness** | 6/10 | Dockerfile added (DEP-001), but no release process | + +**Overall:** **Project is **early-stage but accelerating**, with strong architecture discipline but **high single-person risk** and **low test coverage velocity**. Safe for continued development only if code review and testing rigor improve immediately. + +--- + +## 10. Key Paths for Follow-Up Review + +**Critical review points (for L3+ phases):** +1. `/home/user/NIGHTWATCH/nightwatch/orchestrator.py` — 10 commits, zero secondary review +2. `/home/user/NIGHTWATCH/nightwatch/llm_client.py` — 5 commits, all one author, safety-critical +3. `/home/user/NIGHTWATCH/nightwatch/tool_executor.py` — Parameter validation (ARCH-001), needs audit +4. `/home/user/NIGHTWATCH/services/safety_monitor/monitor.py` — 5 commits, high-consequence +5. `/home/user/NIGHTWATCH/services/camera/asi_camera.py` — 7 commits, hardware integration, likely bugs +6. `/home/user/NIGHTWATCH/services/alpaca/` and `/home/user/NIGHTWATCH/services/indi/` — Stale, audit for ARCH compliance +7. `/home/user/NIGHTWATCH/tests/unit/` — Only 1 test() commit; review test coverage ratio per module +8. `/home/user/NIGHTWATCH/.pre-commit-config.yaml` — Add commitlint for spec traceability enforcement + +--- + +**End of L2 Forensic Analysis** From 318cffb613eb9f5f6be0918edb0265191732227e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 22:21:05 +0000 Subject: [PATCH 04/13] =?UTF-8?q?docs(review):=20Phase=203=20deep=20dive?= =?UTF-8?q?=20=E2=80=94=20LLM=20client=20&=20tool=20binding=20domain?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01FjTbUXge6kcW3TZMfAdz2J --- .../20-domain-llm-client-tool-binding.md | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 docs/review/20-domain-llm-client-tool-binding.md diff --git a/docs/review/20-domain-llm-client-tool-binding.md b/docs/review/20-domain-llm-client-tool-binding.md new file mode 100644 index 0000000..0b9cee4 --- /dev/null +++ b/docs/review/20-domain-llm-client-tool-binding.md @@ -0,0 +1,106 @@ +# Domain Report: LLM Client & Tool Binding (L3) + +**Analyst:** Domain Analyst L3 +**Scope:** `nightwatch/llm_client.py` (1226 lines), `nightwatch/tool_params.py` (197 lines), `nightwatch/cancellation.py` (197 lines) +**Repository:** `/home/user/NIGHTWATCH` +**Date:** 2026-07-12 + +--- + +## 1. Responsibility + +This domain is the boundary between NIGHTWATCH's voice/orchestration layers and actual LLM inference: `llm_client.py` gives a single `LLMClient` API over three interchangeable backends (local llama-cpp-python, Anthropic, OpenAI, plus a `mock` test backend), handling backend selection/fallback, conversation history, token accounting, response-confidence scoring, and validating LLM-emitted tool calls before anything downstream acts on them. `tool_params.py` is the shared Pydantic schema registry that both this domain and the tool-execution domain use to validate a tool call's arguments, and `cancellation.py` supplies the cooperative `CancelToken`/`CommandContext` primitives used elsewhere in the codebase to abort in-flight long-running operations. + +## 2. Key modules + +| File:Line | Symbol | Role | +|---|---|---| +| `nightwatch/llm_client.py:238` | `BaseLLMClient` (ABC) | Common `chat`/`chat_stream`/`health_check` interface all backends implement | +| `nightwatch/llm_client.py:284` | `MockLLMClient` | Canned-response backend for tests (`LLMBackend.MOCK`) | +| `nightwatch/llm_client.py:325` | `LocalLlamaClient` | Primary backend; lazy-loads a GGUF model via `llama_cpp.Llama` (`_ensure_loaded`, line 347) and calls `create_chat_completion` synchronously (line 386) | +| `nightwatch/llm_client.py:444` | `AnthropicClient` | Optional cloud fallback; reads `ANTHROPIC_API_KEY` from env if not passed explicitly (line 454) | +| `nightwatch/llm_client.py:562` | `OpenAIClient` | Optional cloud fallback; reads `OPENAI_API_KEY` from env (line 572) | +| `nightwatch/llm_client.py:657` | `LLMClient` | Facade: backend selection/fallback (`chat`, line 888), streaming (`chat_stream`, line 1021), tool-call validation (`_validate_tool_calls`, line 737), safety-context injection (`_inject_safety_context`, line 819), confidence-based confirmation gating (`requires_confirmation`, line 1075) | +| `nightwatch/llm_client.py:1119` | `calculate_confidence_score` | Heuristic 4-factor (finish-reason, tool specificity, hedging-phrase count, keyword relevance) confidence score used for VOX-002/290 low-confidence confirmation | +| `nightwatch/llm_client.py:195` | `OBSERVATORY_SYSTEM_PROMPT` | The persistent system prompt, including the "SAFETY GROUNDING" contract text that instructs the model how to react to an injected `SAFETY STATUS:` block | +| `nightwatch/tool_params.py:160` | `TOOL_PARAM_MODELS` | `dict[str, type[BaseModel]]` — the single schema registry mapping tool name → Pydantic model, `extra="forbid"` everywhere (line 21) | +| `nightwatch/tool_params.py:48` | `GotoCoordinatesParams._reject_bool` | `field_validator` that explicitly rejects `bool` for `ra`/`dec` to defeat Pydantic's default bool→float coercion (ARCH-001 fix) | +| `nightwatch/cancellation.py:69` | `CancelToken` | One-shot cooperative cancellation flag + lazily-built `asyncio.Event`; first-`cancel()`-wins semantics (line 97) | +| `nightwatch/cancellation.py:161` | `CommandContext` | Dataclass pairing a `CancelToken` with a `command_id` (uuid4 hex) and optional `timeout_s` | + +## 3. Data flow (representative trace: a voice command reaches the LLM) + +1. `VoicePipeline.process_text` (cross-domain, `nightwatch/voice_pipeline.py:1945`) calls `self.llm_client.chat(message=text, tools=tools)`. +2. `LLMClient.chat` (`llm_client.py:888`) builds a `messages` list: a `system` message from `_build_system_message()` (line 874, folds in `_inject_safety_context()` output if a safety provider is wired), the trailing `_max_history` (20) conversation turns, then the new user message. +3. It iterates `[self.backend] + self.fallback_backends` (line 920), lazily constructing/caching a backend client via `_get_client` (line 716), calling `health_check()` then `chat()` on the first one that succeeds. +4. Each backend parses the raw provider response into a common `LLMResponse`/`ToolCall` shape (e.g. `LocalLlamaClient.chat`, line 396-424 pulls `message["tool_calls"]` and does `json.loads` on `function.arguments`; `AnthropicClient.chat`, line 521-529 reads `block.input` directly as a dict — no JSON parsing needed there). +5. **VOX-003 gate** (line 938-955): `_validate_tool_calls` looks each `ToolCall.name` up in `TOOL_PARAM_MODELS` (imported from `tool_params.py`, line 39) and calls `model_cls.model_validate(tc.arguments)`. Unknown tool names or `ValidationError`s cause the call to be dropped and an error string appended to `response.content`; validated calls get their `arguments` replaced with `model.model_dump()` so coercions (e.g. RA/Dec string→float) survive. +6. `calculate_confidence_score` (line 1119) scores the (now-validated) response; `LLMClient.token_usage` accumulates via `TokenUsage.add` (line 87); the conversation history is appended (lines 975-983). +7. The `LLMResponse` (validated `tool_calls`, `confidence_score`, `content`) is returned to the caller, which (in `voice_pipeline.py`, cross-domain) forwards `tool_calls` to `ToolExecutor.execute` (`nightwatch/tool_executor.py:258`, cross-domain) — which **independently re-validates** the same `parameters` against `TOOL_PARAM_MODELS` (`tool_executor.py:306-317`) before dispatch. This double-validation is intentional defense-in-depth but means `tool_params.py` is a single point of truth consumed by two call sites in two different domains. +8. Tool execution results flow back via `LLMClient.add_tool_result` (line 997), appended to history as a `role="tool"` message for the next turn. + +`cancellation.py` is not part of this trace: `CancelToken`/`CommandContext` never appear inside `llm_client.py` (verified — zero references). They are constructed and consumed entirely in other domains' long-running loops (camera capture, mount slew, autofocus, plate-solve; see `nightwatch/tool_executor.py:47,263` and `services/camera/asi_camera.py`, `services/astrometry/plate_solver.py`, `services/focus/focuser_service.py`). + +## 4. External dependencies + +- **`llama_cpp` (llama-cpp-python)** — imported lazily inside `LocalLlamaClient._ensure_loaded` (`llm_client.py:353`) so the module imports fine without the optional dependency installed; `ImportError` is caught and re-raised as `RuntimeError` (line 364-366). +- **`anthropic` SDK (`AsyncAnthropic`)** — lazily imported in `AnthropicClient._ensure_client` (line 467); requires `ANTHROPIC_API_KEY`. +- **`openai` SDK (`AsyncOpenAI`)** — lazily imported in `OpenAIClient._ensure_client` (line 585); requires `OPENAI_API_KEY`. +- **`pydantic` v2** (`BaseModel`, `ValidationError`, `field_validator`, `ConfigDict`) — core of `tool_params.py` and consumed by `llm_client.py`'s `_validate_tool_calls`. +- **`nightwatch.tool_params.TOOL_PARAM_MODELS`** — the contract: any tool name the LLM might call must have an entry here or VOX-003 silently drops it (see Security/Quality flags below for a concrete registry gap). +- **`services.safety_monitor.monitor.SafetyStatus`** (cross-domain, TYPE_CHECKING-only import, `llm_client.py:44`) — `_inject_safety_context` (line 819) duck-types on `.is_safe`, `.reasons` (`List[str]`), `.sun_altitude_deg`, `.wind_speed_mph`. Verified against the real dataclass at `services/safety_monitor/monitor.py:88-112` — the attribute names and types match exactly, so the contract currently holds. +- **`nightwatch.voice_pipeline.VoicePipeline`** (cross-domain, consumer) — holds `self.llm_client` and calls `.chat(...)`/`.add_tool_result(...)`. +- **`nightwatch.tool_executor.ToolExecutor`** (cross-domain, consumer) — imports `TOOL_PARAM_MODELS` from `tool_params.py` directly (`tool_executor.py:49-58`) and imports `CancellationError`/`CommandContext` from `cancellation.py` (`tool_executor.py:47`). +- **`voice/tools/telescope_tools.py`** (cross-domain, 225 KB) — the actual producer of OpenAI-format tool JSON schemas (`Tool.to_dict()`, `telescope_tools.py:59-76`) that are meant to be passed as the `tools=` argument to `LLMClient.chat`. See Quality flag below: the wiring from this producer to `LLMClient.chat`'s `tools` parameter is currently broken. + +## 5. Invariants and conventions + +- **Single schema registry, dual consumers.** `TOOL_PARAM_MODELS` (`tool_params.py:160`) is the one place a new tool's parameter shape is declared; both `llm_client.py:774` (VOX-003, pre-execution) and `tool_executor.py:306` (ARCH-001, pre-dispatch) look it up by tool name. Adding a tool to `voice/tools/telescope_tools.py`'s registry without a matching entry here means the tool can be *offered* to the LLM (in its JSON schema) but every call to it will be validated as `"Unknown tool"` and dropped before it ever reaches execution (see Security flag #1 — this is not hypothetical, it already affects real tools). +- **`extra="forbid"` deny-by-default.** Every model in `tool_params.py` sets `ConfigDict(extra="forbid")` (e.g. line 40); stray LLM-hallucinated fields cause a `ValidationError`, not silent drop-and-continue. +- **`CancelToken` is one-shot and NOT reusable** (`cancellation.py:84`) — "first reason wins" (line 97-108); a fresh token/`CommandContext` must be created per command (`CommandContext.new()`, line 181). +- **`CancelToken` is asyncio-affine, not thread-safe.** The `asyncio.Event` is lazily constructed on first `wait_cancelled()` call (line 147) and is bound to whatever event loop is running at that moment; there is no lock around `_cancelled`/`_reason` mutation — safe under normal single-event-loop asyncio use, but a hazard if any caller ever touches a token from a real OS thread (e.g. a `run_in_executor` callback). +- **Backend fallback is per-call, not sticky.** `LLMClient.chat` re-tries `[self.backend] + self.fallback_backends` in order on every single call (line 920-923), calling `health_check()` each time; there's no "pin to whichever backend last worked" optimization or circuit breaker. +- **Conversation history cap only applies at read time.** `self._max_history = 20` (line 709) is applied when *slicing* history into a request (`self._conversation[-self._max_history:]`, lines 913-914, 1048-1049) but `self._conversation` itself is never trimmed — it grows unboundedly for the lifetime of the `LLMClient` instance (see Quality flag). +- **`TokenUsage.add()` overwrites, not accumulates, the per-call fields** (`llm_client.py:87-95`); only the `session_*` fields are cumulative. Callers wanting session-level cost must read `session_total_tokens`, not `total_tokens`. +- **Safety grounding is advisory prose, not enforcement.** The `SAFETY STATUS:` block (`_inject_safety_context`, line 819-872) is a natural-language instruction to the model ("you MUST refuse..."); nothing in this domain verifies the model actually complied — the authoritative veto is expected to live in the (cross-domain) `SafetyInterlock`, per the docstring at `cancellation.py:29-38` and the module comment at `llm_client.py:841-848` ("the SafetyInterlock is still the authoritative veto downstream"). + +## 6. MATRIX FLAGS + +### Security observations + +1. **Critical-tool confirmation gate is unreachable dead code, and the underlying tools aren't even validatable.** `LLMClient.requires_confirmation` (`llm_client.py:1075-1095`) hardcodes `critical_tools = {"emergency_shutdown", "open_roof", "close_roof", "stop_roof"}` (line 1090) and is clearly intended as a last-line-of-defense confirmation prompt before the LLM's response is acted on. However: + - None of these four names appear in `TOOL_PARAM_MODELS` (`tool_params.py:160-185` — compare against the full key list: only mount/catalog/ephemeris/weather/safety/session tools are registered, no enclosure or emergency tools). + - `voice/tools/telescope_tools.py` *does* define `open_roof`/`close_roof`/`emergency_shutdown` with `requires_confirmation=True` (telescope_tools.py:866,883,899,942) via its own, separate `Tool.requires_confirmation` mechanism (telescope_tools.py:1347-1374). + - Because VOX-003 validation (`llm_client.py:774`, run *inside* `chat()` before the caller ever sees the response) drops any tool call whose name isn't in `TOOL_PARAM_MODELS` as `"Unknown tool"`, a real LLM tool call to `open_roof`/`close_roof`/`emergency_shutdown`/`stop_roof` would already have been stripped out of `response.tool_calls` by the time `requires_confirmation()` could inspect it. + - Separately, `grep` across the whole repo shows `LLMClient.requires_confirmation`/`get_confirmation_prompt` (llm_client.py:1075,1097) have **zero callers outside `llm_client.py` itself and its own unit tests** — `voice_pipeline.py` and `orchestrator.py` never invoke them. + - **Net effect:** there are two disconnected, redundant "critical tool needs confirmation" mechanisms in the codebase; the one that lives in this domain is both orphaned (no caller) and internally inconsistent (references tool names its own validation layer would already reject). If enclosure/emergency control is ever routed through the LLM path, this is the mechanism an engineer would reasonably assume is providing the safety gate — it is not. + +2. **`LLMClient` (and therefore its VOX-002 safety-grounding and VOX-003 validation) is not wired into the running application at all.** Neither `nightwatch/main.py` nor `nightwatch/orchestrator.py` imports or constructs `LLMClient`/`create_llm_client` (confirmed via repo-wide grep — the only non-test construction sites are the docstring example at `llm_client.py:14-17`, which itself is stale/wrong, see Quality flag #1). `VoicePipeline` (which does hold and call `self.llm_client`) is likewise never constructed from `main.py`/`orchestrator.py`. This means the safety-status injection, tool-call validation, and confidence scoring implemented here are currently exercised only by unit tests, not by the production entry point — a latent-but-real gap between "designed/tested" and "actually running." + +3. **Local inference blocks the event loop.** `LocalLlamaClient.chat` calls `self._model.create_chat_completion(...)` directly inside an `async def` (`llm_client.py:386-391`) with no `run_in_executor`/thread offload; `_ensure_loaded` similarly constructs `Llama(...)` synchronously (line 356-361). If `LLMClient` runs on the same event loop as safety-critical async tasks (weather polling, the SAFE-004 watchdog, `CancelToken.wait_cancelled()`), a multi-second local-inference call would stall those tasks for its duration. `cancellation.py`'s own docstring (`cancellation.py:1-38`) is built around the premise that safety cancellation must reach in-flight operations promptly — a blocked event loop during LLM inference works against that premise if/when this client is wired into the orchestrator's loop. + +4. **Filesystem-touching input has no local validation.** `LocalLlamaClient.__init__`/`_ensure_loaded` (`llm_client.py:333-361`) takes `model_path` as an arbitrary string and passes it straight to `llama_cpp.Llama(model_path=...)` with no existence check, extension check, or path canonicalization. Currently low-severity because nothing wires `LLMConfig.model` (`nightwatch/config.py:436-479`, no `api_key`/`endpoint`/`backend` fields exist there at all — confirmed against `nightwatch.yaml.example:113-123`) into this constructor, but the contract as written trusts its caller completely. + +5. **API keys**: `AnthropicClient`/`OpenAIClient` read `ANTHROPIC_API_KEY`/`OPENAI_API_KEY` from the environment as a fallback when not passed explicitly (`llm_client.py:454,572`) — standard practice, keys are never logged directly. However, `except Exception as e: logger.error(f"...API call failed: {e}")` (lines 544, 640) logs the raw exception string from the third-party SDK verbatim; some SDK error messages for auth failures can echo a masked/partial key or the request payload. Not a confirmed leak, but the log statements do not scrub SDK exception text before writing to `logger.error`. + +6. **JSON parsing of LLM-generated tool arguments has no dedicated error path.** `LocalLlamaClient.chat` (`llm_client.py:406`) and `OpenAIClient.chat` (`llm_client.py:623`) both call `json.loads(tc.get("function", {}).get("arguments", "{}"))` with no try/except around the `json.loads` call itself; a malformed-JSON tool-call argument string from the model raises `json.JSONDecodeError`, which is only caught by the broad `except Exception` wrapping the entire backend `chat()` body (line 426, 639) — meaning the whole backend is marked "failed" for that turn and `LLMClient.chat` falls through to the next fallback backend (or raises `RuntimeError("All LLM backends failed...")` if none remain) rather than gracefully dropping just the one malformed tool call the way VOX-003's Pydantic validation does for schema errors. + +### Quality observations + +1. **Stale/incorrect module docstring.** The top-of-file usage example (`llm_client.py:13-17`) shows `from nightwatch.llm_client import LLMClient, LLMConfig` and `config = LLMConfig(backend="local", model="llama-3.2-3b"); client = LLMClient(config)`. There is no `LLMConfig` class in `llm_client.py` — the real `LLMConfig` lives in `nightwatch/config.py:436` and has an entirely different field set (`enabled`, `model`, `max_tokens`, `temperature`, `gpu_layers`, `context_length` — no `backend` field). `LLMClient.__init__` (line 665-674) takes individual keyword args (`backend`, `model_path`, `api_key`, `model`, `fallback_backends`, ...), not a config object at all. A new engineer following this docstring's example would get an `ImportError`. +2. **`chat_stream` bypasses VOX-003 validation and the fallback chain entirely.** `LLMClient.chat_stream` (line 1021-1073) only tries `self.backend` (line 1055, no fallback loop), never calls `_validate_tool_calls`, never calls `calculate_confidence_score`, and never updates `self.token_usage`. Repo-wide grep shows `chat_stream` has no production caller (only `test_llm_client.py`), so this is currently inert, but if it's ever wired up it would silently reintroduce the exact unvalidated-tool-call risk that VOX-003 was built to close on the non-streaming path. +3. **Unbounded conversation history growth.** `self._conversation` (`llm_client.py:708`) is appended to on every turn (user + assistant, plus tool results via `add_tool_result`) and is only ever read with a `[-self._max_history:]` slice — never trimmed in place. A long-running `LLMClient` instance (e.g. a multi-hour observing session) accumulates the full transcript in memory indefinitely; only `clear_history()` (line 1006), which nothing currently calls automatically, resets it. +4. **Confidence heuristic is a hand-tuned English keyword list.** `calculate_confidence_score` (line 1119-1198) hardcodes English hedging phrases (`"i think"`, `"maybe"`, ...) and command keywords (`"point"`, `"slew"`, ...) with fixed weights (0.2/0.3/0.25/0.25). It has no tests asserting the weights against real transcripts beyond the existing unit tests' synthetic examples (`test_llm_client.py`), and would silently mis-score any non-English input or paraphrase that avoids the exact substrings. +5. **`health_check()` is misleading for cloud backends.** `AnthropicClient.health_check`/`OpenAIClient.health_check` (line 547-554, 643-649) only check `self._client is not None` — i.e., that the SDK object was constructed — not that the API is actually reachable, despite the docstring claiming "Check if Anthropic/OpenAI API is reachable." A backend with a syntactically-valid-but-revoked API key will report healthy and only fail on the real `chat()` call (which is still handled gracefully by the outer fallback loop, so behavior is correct, but the method name/docstring overpromises). +6. **Test coverage is good and current for this domain specifically.** `tests/unit/test_llm_client.py` (925 lines) has dedicated test classes for token usage, tool calls, conversation messages, mock/fallback behavior, safety-context injection (`TestSafetyContextInjection`), and VOX-003 validation (`TestVox003ToolCallValidation`) — including the partial-pass/no-annotation edge case. `tests/unit/test_tool_params.py` has 34 tests covering the registry and bool-coercion rejection. `tests/unit/test_cancellation.py` (140 lines) covers `CancelToken`/`CommandContext` cancel-once semantics, `wait_cancelled` timing, and factory uniqueness. This is one of the better-tested corners of the repository per `docs/review/10-history.md`'s churn/test-ratio findings — but the tests validate code paths (`chat`, VOX-003, safety injection) that, per the security observations above, are not currently reached by the running application. +7. **Complexity hotspot:** `LLMClient.chat` (`llm_client.py:888-995`, ~108 lines) mixes message assembly, backend iteration/fallback, tool validation, confidence scoring, token accounting, and history mutation in one method with no smaller helpers beyond `_validate_tool_calls`/`_build_system_message`. Not unreasonable for its purpose, but any future change (e.g. adding retry/backoff, or wiring cancellation) will touch a fairly dense method. +8. **No use of `cancellation.py` primitives anywhere in `llm_client.py`.** Given the file sits in the same domain and the repo's overall ARCH-003 push to make long operations cancellable, the complete absence of `CancelToken`/`CommandContext` in the LLM call path is either an intentional scope boundary (LLM calls are short enough not to need it) or an oversight — the code offers no comment either way, so this is flagged as worth an explicit architectural decision rather than an assumed gap. + +--- + +## Cross-domain touchpoints noted (not analyzed in depth — belong to other analysts) + +- `nightwatch/voice_pipeline.py:2083-2090` (`_get_tools`) imports `from nightwatch.telescope_tools import get_tool_definitions` — **this module/function does not exist anywhere in the repo** (the real tool catalog is `voice/tools/telescope_tools.py`, which exposes `Tool`/registry classes, not a `get_tool_definitions` function). This import always raises `ImportError`, caught and logged as a warning, causing `_get_tools()` to return `None` — meaning in the current code, `tools=` is never actually populated for the `LLMClient.chat` call in production, so tool-calling functionally cannot happen via this path today. This is the other half of the "LLMClient isn't wired up" story and lives primarily in the Voice/Command-Execution domains, but directly explains why this domain's tool-binding code is effectively unexercised outside tests. +- `nightwatch/tool_executor.py` (Command Execution domain) is the other consumer of `TOOL_PARAM_MODELS` and `cancellation.py` — see Section 5's "single registry, dual consumers" invariant. +- `services/safety_monitor/monitor.py` (Astronomy & Hardware Services domain) — `SafetyStatus` duck-type contract origin; verified consistent as of this review. +- `nightwatch/config.py` `LLMConfig` (Core Orchestration domain) — the config schema that *should* parameterize `LLMClient` construction but currently has no field for `backend`/`api_key`/`fallback_backends`, and nothing in `main.py`/`orchestrator.py` reads `config.llm` to build an `LLMClient` at all. From 6d2db6fde79570021460be888778c5ea12ece068 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 22:22:56 +0000 Subject: [PATCH 05/13] =?UTF-8?q?docs(review):=20Phase=203=20deep=20dive?= =?UTF-8?q?=20=E2=80=94=20command=20execution=20&=20tool=20integration=20d?= =?UTF-8?q?omain?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01FjTbUXge6kcW3TZMfAdz2J --- ...main-command-execution-tool-integration.md | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 docs/review/20-domain-command-execution-tool-integration.md diff --git a/docs/review/20-domain-command-execution-tool-integration.md b/docs/review/20-domain-command-execution-tool-integration.md new file mode 100644 index 0000000..d0c0982 --- /dev/null +++ b/docs/review/20-domain-command-execution-tool-integration.md @@ -0,0 +1,102 @@ +# Domain Report: Command Execution & Tool Integration + +**Analyst:** L3 Domain Analyst +**Scope:** `nightwatch/tool_executor.py`, `nightwatch/response_formatter.py`, `voice/tools/` (`__init__.py`, `telescope_tools.py`, `meteor_tools.py`), `services/voice/` (`vocabulary_trainer.py`, `wake_word_trainer.py`) +**Repository:** `/home/user/NIGHTWATCH` +**Date:** 2026-07-12 + +--- + +## 1. Responsibility + +This domain is the "last mile" between an LLM's decision to act and the observatory actually doing something: it defines the catalog of callable tools (telescope, meteor, vocabulary/wake-word training), validates and routes a chosen tool's arguments to the right orchestrator/service call, and turns the raw result back into a short spoken sentence for TTS. In one sentence for a new engineer: *if the voice pipeline decides "the user wants to slew to M31," this is the code that checks the arguments are sane, actually calls the mount service, and produces the words "Now pointing at M31."* + +## 2. Key modules + +| Path | Role | +|---|---| +| `nightwatch/tool_executor.py:133` `ToolExecutor` | Central dispatcher: holds a `tool_name -> handler` map, Pydantic-validates parameters, applies a timeout, and normalizes every outcome into a `ToolResult` (`tool_executor.py:94-120`). | +| `nightwatch/tool_executor.py:79` `ToolStatus` | Outcome enum: `SUCCESS/ERROR/TIMEOUT/VETOED/NOT_FOUND/INVALID_PARAMS/CANCELLED` — the `CANCELLED` state (ARCH-003, `tool_executor.py:87-91`) is deliberately distinct from `ERROR` so downstream code can tell "stopped safely" from "broke". | +| `nightwatch/tool_executor.py:222` `_register_default_handlers` | Registers exactly **18** built-in handlers: mount (6), catalog (2), ephemeris (3), weather (2), safety (2), session (3). | +| `nightwatch/tool_executor.py:258` `execute()` | The hot path: handler lookup → Pydantic validation via `TOOL_PARAM_MODELS` → optional `CommandContext` wiring (ARCH-003) → `asyncio.timeout` dispatch → exception-to-`ToolResult` mapping. Self-flagged as a complexity hotspot (`# noqa: PLR0912`, `tool_executor.py:258`). | +| `nightwatch/tool_executor.py:1175` `ToolChain` / `ChainStep` / `ChainResult` / `BUILTIN_CHAINS` | Multi-step command chaining with parameter passing between steps (`_resolve_param`, `tool_executor.py:1338-1371`). See Quality observations — this feature appears unreachable/broken against the live handler set and has no test coverage. | +| `nightwatch/response_formatter.py:289` `ResponseFormatter` | Converts a `ToolResult` (or raw data) into a spoken sentence; falls back to `result.message` almost always (`response_formatter.py:318-320`). | +| `nightwatch/response_formatter.py:49-241` `format_ra/format_dec/format_alt_az/format_temperature/format_wind/format_time/format_duration` | Pure formatting helpers, no I/O, well unit-tested (`tests/unit/test_response_formatter.py`). | +| `voice/tools/telescope_tools.py:49` `Tool` / `ToolParameter` / `ToolCategory` | Dataclass-based schema for LLM function-calling (`to_openai_format`, `to_anthropic_format`, `telescope_tools.py:58-113`). | +| `voice/tools/telescope_tools.py:120` `TELESCOPE_TOOLS` | ~87 tool schema definitions spanning mount, catalog, ephemeris, weather, safety, session, guiding, camera, focus, astrometry, enclosure, power, encoder, PEC, INDI, and Alpaca categories. | +| `voice/tools/telescope_tools.py:1298` `ToolRegistry` | A **second, independent** tool dispatcher (own `execute()`, own confirmation gate via `requires_confirmation`, `telescope_tools.py:1347-1398`). Not wired into the live orchestrator/voice pipeline (see Quality observations). | +| `voice/tools/telescope_tools.py:1405` `create_default_handlers()` | ~87 closures implementing the actual business logic (safety checks, altitude limits, roof/park interlocks, PEC, INDI/Alpaca device calls) for `ToolRegistry`. Large (≈4100 lines), only reachable via `ToolRegistry`, which is itself unreachable from production. | +| `voice/tools/telescope_tools.py:5518` `TELESCOPE_SYSTEM_PROMPT` | LLM system prompt describing the observatory and its full workflow (imaging, automation, safety, PEC, INDI/Alpaca) — internally consistent with `TELESCOPE_TOOLS` names, but see Security/Quality observations for how it reaches (or fails to reach) the LLM. | +| `voice/tools/meteor_tools.py:19` `METEOR_TOOLS` / `MeteorToolHandler` | 5 meteor/fireball tool schemas plus a handler that talks to `MeteorTrackingService` by parsing its **string** output line-by-line (`meteor_tools.py:136-246`). No production caller found anywhere in the repo. | +| `services/voice/vocabulary_trainer.py:255` `VocabularyTrainer` | Learns/boosts astronomy vocabulary terms and speech-normalization regexes from usage, persisted to `~/.nightwatch/vocabulary.json` (`vocabulary_trainer.py:279`). | +| `services/voice/wake_word_trainer.py:210` `WakeWordTrainer` | Collects positive/negative wake-word examples, generates phonetic variations, adapts a fuzzy-match threshold, persisted to `~/.nightwatch/wake_word_training.json` (`wake_word_trainer.py:241`). | + +## 3. Data flow + +Designed (intended) flow for a command like "point the telescope at M31": + +1. **Entry (cross-domain):** `nightwatch/voice_pipeline.py::process_text()` calls `self._get_tools()` to obtain the tool schema list to hand to the LLM (`voice_pipeline.py:1942-1948`), then `self.llm_client.chat(message=text, tools=tools)`. +2. **Tool schema supply (this domain, intended source):** the schema is meant to come from `voice/tools/telescope_tools.py`'s `TELESCOPE_TOOLS` (via `to_openai_format()`/`to_anthropic_format()`, `telescope_tools.py:58-113`). **In the current code this link is broken** — see Security observations §1. `_get_tools()` actually imports `from nightwatch.telescope_tools import get_tool_definitions` (`voice_pipeline.py:2086`), a module/function that does not exist anywhere in the repo, so this always raises `ImportError`, is swallowed, and `tools` becomes `None`. +3. **LLM tool selection (cross-domain):** `nightwatch/llm_client.py`'s backend `chat()` implementations gate function-calling on `if tools:` (e.g. `llm_client.py:390` local backend, `llm_client.py:496` Anthropic backend) — with `tools=None` the model receives no function schema, so it cannot legally emit a tool call through this path. +4. **Dispatch (this domain, live path):** assuming a tool call is somehow produced, the orchestrator/voice pipeline calls `ToolExecutor.execute(tool_name, parameters, context=...)` (`tool_executor.py:258`). +5. **Validation:** `parameters` is validated against `TOOL_PARAM_MODELS[tool_name]` (owned by the adjacent "LLM Client & Tool Binding" domain, `nightwatch/tool_params.py:160-185`) with `extra="forbid"` — unknown/extra/wrong-typed fields produce `ToolStatus.INVALID_PARAMS` before any handler runs (`tool_executor.py:302-331`). +6. **Handler execution:** the matching `_handle_*` method runs under `asyncio.timeout`, checks `self.orchestrator.safety.is_safe` where relevant (e.g. `tool_executor.py:447-456` for `goto_object`), resolves object coordinates via `orchestrator.catalog`/`orchestrator.ephemeris`, and calls the mount/weather/session service on `self.orchestrator`. +7. **Result normalization:** every branch (success, service-unavailable, exception, timeout, cancellation) returns a `ToolResult` (`tool_executor.py:94-120`); nothing propagates as a raw exception out of `execute()`. +8. **Formatting (this domain):** `voice_pipeline._format_response()` calls `ResponseFormatter.format(result)` (`voice_pipeline.py:2058-2065`), which prefers `result.message` when longer than 10 characters (`response_formatter.py:318-320`) and only falls through to tool-specific formatters (`_format_slew`, `_format_weather`, `_format_twilight`, `_format_safety`) otherwise. +9. **Output:** the formatted string goes to TTS (out of domain). + +Vocabulary/wake-word training data flow is a **separate, currently one-way** loop: `services/ai_services.py` lazily constructs `VocabularyTrainer`/`WakeWordTrainer` (`ai_services.py:281-298,368-376`) and a health check touches them (`ai_services.py:164-165`), but this reviewer found **no caller anywhere in the repo** of the actual output methods (`normalize_text()`, `get_boosted_vocabulary()`, `get_model()`, `get_variations()`) outside their own test files and `examples/v05_ai_demo.py`. The trainers accumulate state on disk but nothing in `voice/stt/` or `voice/wyoming/` (the actual STT/wake-word runtime) consumes it. + +## 4. External dependencies + +- **pydantic** (`BaseModel`, `ValidationError`) — parameter validation gate in `tool_executor.py:45,316-331`. +- **`nightwatch.tool_params`** (adjacent "LLM Client & Tool Binding" domain) — supplies `TOOL_PARAM_MODELS` and per-tool `BaseModel` classes; contract: every tool name registered in `ToolExecutor._register_default_handlers` **must** have a matching entry (verified: all 18 do, `tool_params.py:160-185`). +- **`nightwatch.cancellation`** (`CancellationError`, `CommandContext`, core domain) — ARCH-003 cooperative cancellation; `ToolExecutor.execute()` sets/clears `orchestrator.set_active_context()`/`clear_active_context()` around dispatch (`tool_executor.py:339-406`). +- **`nightwatch.exceptions`** (`NightwatchError`, `CommandError`) — `ToolExecutionError` subclasses `NightwatchError` (`tool_executor.py:123-125`). +- **`nightwatch.orchestrator.Orchestrator`** (core domain, injected at construction, `tool_executor.py:146-154`) — every handler reaches services only through `self.orchestrator.`; this domain assumes those attributes are either a live service or `None`/falsy (every handler checks for `None` before use). +- **`services.ephemeris.CelestialBody`** — imported inline inside a `create_default_handlers()` closure (`telescope_tools.py:1469`), a direct dependency on the Astronomy/Hardware Services domain's enum shape. +- **`services.meteor_tracking` (MeteorTrackingService, injected)** — `MeteorToolHandler` assumes its `add_watch()`/`get_status()`/`get_shower_info()`/`check_now()` methods return colon-delimited, line-oriented **strings** (not structured data), e.g. lines containing `"shower-name:"`, `"watch-window:"`, `"watch-windows-active:"` (`meteor_tools.py:150-163,174-183`) — a fragile string contract between domains. +- **OpenAI / Anthropic function-calling JSON schema shape** — `Tool.to_openai_format()`/`to_anthropic_format()` (`telescope_tools.py:58-113`) assume `ToolParameter.type` values are valid JSON Schema primitives (`"string"|"number"|"boolean"|"array"`); nothing in this domain validates that assumption at definition time. + +## 5. Invariants and conventions + +- **Every `execute()` call returns a `ToolResult`; it never raises.** All exception classes (`TimeoutError`, `CancellationError`, `ToolExecutionError`, bare `Exception`) are caught and mapped to a status (`tool_executor.py:354-397`). Downstream code (voice pipeline, formatter) can assume this. +- **Missing param schema is a hard stop, not a crash**: if `TOOL_PARAM_MODELS.get(tool_name)` is `None`, `execute()` returns `INVALID_PARAMS` rather than calling the handler with unvalidated data (`tool_executor.py:306-315`). +- **Safety vetoes are hand-rolled per handler, not centralized middleware.** Each mutating handler independently checks `self.orchestrator.safety.is_safe` (e.g. `tool_executor.py:447-456` `goto_object`, `:521-527` `goto_coordinates`, `:617-623` `unpark`) — `park_telescope` and `stop_mount` intentionally skip the check (parking/stopping are the "safe direction"). Any new mutating handler must remember to add this check itself; there is no enforcement. +- **`context` kwarg detection is cached at registration time** via `inspect.signature` (`tool_executor.py:206-220`), not re-checked per call — a handler must declare `context` in its signature *before* `register_handler()` runs. +- **Execution log is in-memory only, capped at 1000 entries** (`tool_executor.py:414-420`) — not a durable audit trail; a restart loses history. +- **Pydantic models use `extra="forbid"`** (owned by tool_params.py, consumed here) — the LLM must supply exactly the declared field set; stray fields are rejected, not ignored. +- **RA/Dec bool-coercion is explicitly rejected** (`GotoCoordinatesParams._reject_bool`, cited from `tool_params.py`) — guards against an LLM hallucinating `{"ra": true}` silently becoming `1.0`. +- **Coordinate string parsing** (`_parse_ra`/`_parse_dec`, `tool_executor.py:1081-1107`) assumes `HH:MM:SS` / `sDD:MM:SS`-ish formats; any `ValueError` is caught and surfaced as `INVALID_PARAMS`, never propagated raw. +- **`voice/tools/telescope_tools.py` handler-name convention**: `create_default_handlers()` closures are looked up by exact tool name string (`handlers["goto_object"] = goto_object`, etc.) — a naming mismatch between `TELESCOPE_TOOLS` entries and handler dict keys would silently produce "No handler registered" rather than a startup error (`ToolRegistry.execute`, `telescope_tools.py:1383-1385`). + +## 6. MATRIX FLAGS + +### Security observations + +1. **Tool-schema wiring to the LLM is broken and fails silently.** `nightwatch/voice_pipeline.py:2083-2090`'s `_get_tools()` does `from nightwatch.telescope_tools import get_tool_definitions` — there is no `nightwatch/telescope_tools.py` module in this repository (confirmed via filesystem check) and no function named `get_tool_definitions` anywhere in the repo (confirmed via repo-wide grep). The `except ImportError` swallows this and logs only a `warning`, returning `None`. Both `process_text()` (`voice_pipeline.py:1943`) and `select_tool_from_intent()` (`voice_pipeline.py:2105-2107`, short-circuits to `None` immediately) are affected. Combined with `llm_client.py`'s `if tools:` gates (lines 390, 496), this means the LLM backends receive **no function-calling schema** through this path — the ~87 tool definitions this domain maintains in `voice/tools/telescope_tools.py` are not demonstrably reaching the model. This is a fail-silent-to-degraded-capability pattern rather than fail-loud; an operator would only notice via a `logger.warning` line, not an error, and voice commands that depend on LLM tool-selection would appear to just not work, with no clear signal why. (Fix spans this domain's `voice/tools/telescope_tools.py` — needs an export — and the adjacent Voice/NLP domain's `voice_pipeline.py` import path; flagged here because the missing export lives in this domain's territory.) +2. **Untrusted-filesystem-input deserialization gap in the training subsystems.** `VocabularyTrainer._load()` (`services/voice/vocabulary_trainer.py:316-335`) and `WakeWordTrainer._load()` (`services/voice/wake_word_trainer.py:301-329`) both only catch `(json.JSONDecodeError, KeyError)` around code paths that also call `TermCategory(data["category"])` / `DetectionOutcome(data["outcome"])` (raise `ValueError` on an unrecognized enum string) and `datetime.fromisoformat(...)` (raises `ValueError` on a malformed timestamp). A corrupted or hand-edited `~/.nightwatch/vocabulary.json` or `~/.nightwatch/wake_word_training.json` — files owned by whatever user account runs NIGHTWATCH, not specially protected — will raise an uncaught `ValueError` out of the constructor. `services/ai_services.py` eagerly instantiates and touches both trainers during what looks like a health check (`_ = self.vocabulary_trainer` / `_ = self.wake_word_trainer`, `ai_services.py:164-165`), so a bad state file is plausibly a startup-crash vector, not just a lazy-init one. +3. **Unbounded regex acceptance (latent ReDoS surface).** `VocabularyTrainer.add_normalization(pattern, replacement)` (`services/voice/vocabulary_trainer.py:411-438`) stores an arbitrary caller-supplied regex string with no length or complexity bound, later compiled and run via `NormalizationRule.compiled_pattern`/`.apply()` (`vocabulary_trainer.py:200-207`) against live STT transcripts in `normalize_text()`. Today this reviewer found **no production caller** of `add_normalization` (only `tests/unit/test_vocabulary_trainer.py`) — the only wired path that adds normalization rules, `learn_from_correction()`, builds its pattern via `re.escape()` first (`vocabulary_trainer.py:564-570`), which is safe. Flagged as a latent primitive for the security auditor in case a future "teach me a pronunciation" voice command wires user text directly into `add_normalization`. +4. **Two parallel, differently-validated command-dispatch stacks exist for overlapping tool names.** `nightwatch/tool_executor.py::ToolExecutor` validates every call through Pydantic `extra="forbid"` models (ARCH-001). `voice/tools/telescope_tools.py::ToolRegistry.execute()` (`telescope_tools.py:1356-1398`) instead does `handler(**arguments)` — a raw dict splat into a plain Python function signature — with no schema validation at all, catching only `TypeError`/generic `Exception` after the fact. This reviewer found no code path that constructs `ToolRegistry()` or calls `create_default_handlers()` outside `telescope_tools.py`'s own `if __name__ == "__main__"` block (`telescope_tools.py:5646-5662`) and `tests/unit/test_telescope_tools.py`. It is not reachable today, but it represents the exact class of risk ARCH-001 (see `nightwatch/tool_params.py` docstring) was written to close, sitting dormant in the same package. +5. **No authentication/authorization concept in this domain.** `ToolExecutor.execute()` (`tool_executor.py:258`) trusts whatever `tool_name`/`parameters` it is handed; its only gates are per-handler safety-monitor vetoes and Pydantic type validation, both of which are about *safety of the action*, not *who is allowed to request it*. Identity/consent (e.g., "is this really the observatory owner speaking") is entirely out of this domain's scope and appears to be assumed by upstream layers — worth the security auditor confirming that assumption holds somewhere. +6. **Emergency-bypass path has a stubbed audit log.** `create_default_handlers()`'s `close_roof(emergency: bool)` (`telescope_tools.py:2988-3017`) intentionally skips normal state checks when `emergency=True`, with the only trace of an audit requirement being a comment: `# Logger would log here in production` (`telescope_tools.py:3008`) — i.e., the emergency-bypass action is not actually logged in this code. Low current risk since this handler sits on the dormant `ToolRegistry` path (see #4), but should be fixed before any revival. + +### Quality observations + +1. **`ToolChain` is untested and appears non-functional against the live handler registry.** `ToolChain`/`ChainStep`/`ChainResult`/`BUILTIN_CHAINS` (`tool_executor.py:1115-1493`, roughly 28% of the file) have zero references anywhere in `tests/` (repo-wide search). Three of the four built-in chains (`slew_and_capture`, `focus_and_capture`, `startup_sequence`, `BUILTIN_CHAINS` at `tool_executor.py:1207-1226`) reference tool names — `capture_image`, `autofocus`, `close_enclosure`, `open_enclosure` — that are never registered by `_register_default_handlers()` (`tool_executor.py:222-252`) and have no `register_handler(` call site anywhere in `nightwatch/orchestrator.py` (confirmed via grep). Running these chains today would hit `ToolStatus.NOT_FOUND` on their first unregistered step; `safe_shutdown` "succeeds" only because its unregistered steps use `on_failure="continue"` and silently no-op. `docs/NIGHTWATCH_V0.1_PLAN.md:355` marks this Step 267 "Complete." +2. **Dead local variable masking a chain-result collision bug.** In `ToolChain.execute()`, `step_id = f"{step.tool_name}_{i}"` is computed (`tool_executor.py:1394`) but never referenced again; results are stored keyed by bare `step.tool_name` instead (`tool_executor.py:1436`). A chain that calls the same tool twice would have the first step's result silently overwritten, breaking any `param_mappings` that reference the earlier occurrence. +3. **Large gap between defined and executable tool surface.** `voice/tools/telescope_tools.py` defines ~87 telescope `Tool` schemas across 13 categories (camera 16, focus 8, guiding 4, enclosure 4, power 3, astrometry 3, alerts 2, plus mount/catalog/ephemeris/weather/safety/session), but `nightwatch/tool_executor.py` only ever registers 18 handlers, all in the mount/catalog/ephemeris/weather/safety/session categories. Camera, guiding, focus, enclosure, power, astrometry, encoder, INDI, and Alpaca tools have no live handler in `nightwatch/` (verified: no `register_handler(` call sites for these names, and `create_default_handlers()`/`ToolRegistry` — the only place these ~87 tools *do* have implementations — is not imported outside its own file and tests). `TELESCOPE_SYSTEM_PROMPT` (`telescope_tools.py:5518-5639`) describes a full v3.0 automation workflow (roof, power, PEC, INDI, Alpaca) that the live `ToolExecutor` cannot currently execute. +4. **`MeteorToolHandler` is untested and has no discoverable production caller.** Repo-wide search for `MeteorToolHandler` finds only its own definition (`voice/tools/meteor_tools.py:96`) and the `voice/tools/__init__.py` re-export (`__init__.py:19`) — no test file (`tests/unit/test_meteor_tools.py` does not exist) and no caller in `nightwatch/`. Its implementation is also fragile: it extracts fields from a downstream service's free-text string output by substring/line matching (`meteor_tools.py:150-163,174-183,207-224`) rather than consuming structured data, so any wording change in `MeteorTrackingService` would silently break voice responses without a type error to catch it. +5. **`ResponseFormatter` templates are mostly dead.** `RESPONSE_TEMPLATES` defines 15 entries (`response_formatter.py:249-281`) but only 3 (`slew_complete`, `all_safe`, `not_safe`) are ever read via `self.templates[...]` (lines 343, 430, 434). The other 12 (`mount_parked`, `mount_unparked`, `object_found`, `object_not_found`, `weather_safe`, `weather_unsafe`, `weather_summary`, `session_started`, `session_ended`, `twilight_evening`, `twilight_morning`, `service_unavailable`, `command_failed`) are unused — either abandoned in favor of returning `result.message` directly (the `format()` method short-circuits to `result.message` whenever it's longer than 10 characters, `response_formatter.py:318-320`, which is true for nearly every handler's message in `tool_executor.py`), or intended for call sites that were never written. +6. **Uneven test depth across the domain.** `tests/unit/test_tool_executor.py` (671 lines) is behavior-oriented: mount handlers, safety veto, ARCH-003 cancellation, ARCH-001 param validation, coordinate parsing edge cases. By contrast, `tests/unit/test_telescope_tools.py` (1300+ lines covering ~87 tools) is dominated by shape assertions — sampled repeatedly across the file: `assert callable(handlers[handler_name])`, `assert name in tool_names`, `assert "object_name" in param_names` — rather than behavioral assertions against `create_default_handlers()`'s actual logic (altitude-limit rejection, catalog-vs-ephemeris fallback at `telescope_tools.py:1448-1517`, roof/park interlocks). The volume of test code creates an impression of coverage that is largely metadata/schema coverage, not logic coverage. +7. **Generic exception handling is pervasive and inconsistently logged.** Every `_handle_*` method in `tool_executor.py` ends with a catch-all `except Exception as e:` that returns a flattened `str(e)` (e.g. `tool_executor.py:503-509,565-571`); the top-level `execute()` catch-all does call `logger.exception(...)` (`tool_executor.py:390-397`), but `voice/tools/telescope_tools.py::ToolRegistry.execute()`'s catch-all (`telescope_tools.py:1397-1398`) does not log at all — a failing tool call on that path leaves no trace beyond the string returned to the (LLM) caller. +8. **Self-acknowledged complexity hotspot.** `ToolExecutor.execute()` carries `# noqa: PLR0912 — ARCH-003 added active-context wiring (+2 branches); refactor deferred to SAFE-001` (`tool_executor.py:258`) — an honest admission of branching complexity rather than a hidden one; still worth the quality auditor's attention given it is the single most-executed function in this domain. + +--- + +## Cross-domain touchpoints (not reviewed in depth — flagged for the owning analyst) + +- `nightwatch/voice_pipeline.py` (Voice & NLP domain) — the broken `_get_tools()` import (§Security #1) is a joint defect; the fix likely needs both an export from this domain (`voice/tools/telescope_tools.py`) and a corrected import in `voice_pipeline.py`. +- `nightwatch/orchestrator.py` (Core Orchestration domain) — never calls `ToolExecutor.register_handler()` beyond the 18 defaults; camera/guiding/focus/enclosure/power tool wiring, if it exists, was not found in this domain's territory. +- `nightwatch/tool_params.py` / `nightwatch/llm_client.py` (LLM Client & Tool Binding domain) — parameter schemas and the `tools`-gating behavior in `chat()` are consumed here as a contract; verified consistent for the 18 live tools, not verified for the ~87 tools defined only in `voice/tools/telescope_tools.py`. +- `services/ai_services.py`, `services/meteor_tracking/`, `services/ephemeris/` (Astronomy & Hardware Services domain) — supply the runtime objects this domain's handlers call into; only the shape of their string/enum outputs was inspected where directly consumed (e.g. `services.ephemeris.CelestialBody`, meteor service's string reports). From 7ba250a4a6baa933816cc4ce325025030a9a3006 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 22:23:41 +0000 Subject: [PATCH 06/13] =?UTF-8?q?docs(review):=20Phase=203=20deep=20dive?= =?UTF-8?q?=20=E2=80=94=20voice=20&=20NLP=20domain?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01FjTbUXge6kcW3TZMfAdz2J --- docs/review/20-domain-voice-nlp.md | 106 +++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 docs/review/20-domain-voice-nlp.md diff --git a/docs/review/20-domain-voice-nlp.md b/docs/review/20-domain-voice-nlp.md new file mode 100644 index 0000000..93bf82b --- /dev/null +++ b/docs/review/20-domain-voice-nlp.md @@ -0,0 +1,106 @@ +# Domain Review: Voice & Natural Language Processing + +**Analyst:** Domain Analyst (L3) +**Scope:** `voice/` (`stt/`, `tts/`, `wyoming/`) and `services/nlp/` +**Explicitly out of scope:** `voice/tools/` (belongs to "Command Execution & Tool Integration" domain; treated as touchpoint only) +**Repository:** `/home/user/NIGHTWATCH` +**Date:** 2026-07-12 + +--- + +## 1. Responsibility + +This domain turns speech into text and text into speech for the NIGHTWATCH observatory, and adds the natural-language layer on top that tracks conversation state, personalizes responses, and generates spoken narration. Concretely: `voice/stt` runs local speech recognition (faster-whisper), `voice/tts` runs local speech synthesis (Piper), `voice/wyoming` exposes both over the network via the Rhasspy Wyoming protocol for Home Assistant-style integrations, and `services/nlp` tracks multi-turn conversation context, detects ambiguous commands, learns user preferences, and generates natural-language descriptions/narration for voice output. + +## 2. Key modules + +| Path | Role | +|---|---| +| `voice/__init__.py:12-22` | Package facade; re-exports `WhisperSTT`, `TTSService`, and (cross-domain) `ToolRegistry`/`TELESCOPE_SYSTEM_PROMPT` from `voice.tools`. | +| `voice/stt/whisper_service.py:273-418` | `WhisperSTT` — faster-whisper/openai-whisper wrapper; model load, warm-up, DGX Spark tuning (`create_for_dgx_spark`, `int8_float16`). | +| `voice/stt/whisper_service.py:420-476` | `WhisperSTT.transcribe()` — core inference call; **hardcodes `confidence=0.9`** (see Quality #3). | +| `voice/stt/whisper_service.py:99-154` | `VoiceActivityDetector` — energy-threshold VAD fallback. | +| `voice/stt/whisper_service.py:156-270` | `EnhancedVAD` — neural VAD via `pymicro-vad`, falls back to energy threshold if unavailable. | +| `voice/stt/whisper_service.py:604-644` | `PushToTalkRecorder` — fixed-duration record-then-transcribe mode. | +| `voice/tts/piper_service.py:74-374` | `PiperTTS` — Piper wrapper with CUDA option and phrase-cache (`_build_cache`, `COMMON_PHRASES`). | +| `voice/tts/piper_service.py:376-459` | `EspeakTTS` / `SystemTTS` — subprocess-based fallbacks (macOS `say`, Windows PowerShell SAPI, Linux `espeak`). | +| `voice/tts/piper_service.py:465-543` | `TTSService` — backend auto-selection facade (Piper → espeak → system). | +| `voice/wyoming/protocol.py:280-447` | `WyomingMessage` + `read_message`/`write_message` — newline-delimited JSON wire codec for the Wyoming protocol. | +| `voice/wyoming/stt_server.py:99-406` | `WyomingSTTServer` — async TCP server exposing `WhisperSTT`; buffers audio chunks, resamples, applies confidence filtering (Step 317). | +| `voice/wyoming/tts_server.py:147-434` | `WyomingTTSServer` — async TCP server exposing `PiperTTS`; streams synthesized audio back in chunks, applies urgency-based rate adjustment (Step 323). | +| `voice/wyoming/tts_server.py:440-540` | `WyomingTTSClient` — client-side counterpart for remote synthesis. | +| `voice/wyoming/startup.py:257-529` | `WyomingManager` — starts/stops both servers, wires CUDA device config, Home Assistant entity metadata, mDNS registration. | +| `voice/wyoming/startup.py:78-254` | `WyomingServiceDiscovery` — optional Zeroconf/mDNS advertisement of the STT/TTS services. | +| `services/nlp/conversation_context.py:210-731` | `ConversationContext` — regex-based entity extraction/intent classification, pronoun/reference resolution, importance-scored pruning. | +| `services/nlp/clarification.py:211-630` | `ClarificationService` — detects ambiguous targets, missing parameters, incomplete references, and "dangerous action" phrases requiring verbal confirmation. | +| `services/nlp/suggestions.py:134-639` | `SuggestionService` — proactive target/action/warning/optimization suggestions with cooldown-based de-duplication. | +| `services/nlp/user_preferences.py:199-663` | `UserPreferences` — learns target/imaging/communication preferences, persists to `~/.nightwatch/user_preferences.json`. | +| `services/nlp/sky_describer.py:277-802` | `SkyDescriber` — template + `random.choice`-based natural-language sky/object/session descriptions. | +| `services/nlp/session_narrator.py:290-766` | `SessionNarrator` — bridges scheduler output with `SkyDescriber`-style templates for spoken session narration. | +| `services/ai_services.py` (cross-cutting glue, not under `services/nlp/`) | `AIServices` facade — the **only** place in the repo that actually constructs `ConversationContext`, `ClarificationService`, `SuggestionService`, etc. together (see Data Flow §3 and Quality #1). | + +## 3. Data flow + +There are effectively three separate, only-partially-connected pipelines in this domain. This matters more than any single bug, so it is stated up front. + +**(a) Intended production path (per docstrings/comments) — NOT fully wired.** The architecture comment in `nightwatch/orchestrator.py:8-14` describes `Voice Input -> STT -> LLM -> Tool -> TTS`. In the actual code, `nightwatch/voice_pipeline.py` (Core Orchestration domain, but directly relevant here) implements its own `STTInterface` (`nightwatch/voice_pipeline.py:1458-1559`) that imports `faster_whisper.WhisperModel` **directly**, duplicating rather than reusing this domain's `voice/stt/whisper_service.py:WhisperSTT`. Worse, its `TTSInterface.synthesize()` (`nightwatch/voice_pipeline.py:1601-1626`) unconditionally returns a silent, synthetic WAV via `_generate_mock_audio()` — Piper is never invoked; the real call is left as a comment (`# Would use piper to synthesize`). `services/nlp` is not consulted anywhere in this path: `nightwatch/__init__.py:50-57` contains the conversation-context import **commented out** with the note "imported from services to avoid circular deps." A repo-wide search found zero call sites for `VoicePipeline(` or `AIServices(`/`get_ai_services(` outside of `tests/` and `examples/v05_ai_demo.py`. + +**(b) Wyoming network path — functional in isolation, but also unreachable from the app.** An external Wyoming client (e.g., Home Assistant) opens a TCP connection to `WyomingSTTServer` (default `0.0.0.0:10300`) or `WyomingTTSServer` (`0.0.0.0:10301`). Messages are JSON, base64-encoded audio, newline-delimited (`voice/wyoming/protocol.py`). For STT: `AUDIO_START` → repeated `AUDIO_CHUNK` (raw PCM bytes accumulated in `ClientSession.audio_buffer`, `voice/wyoming/stt_server.py:234-238`) → `AUDIO_STOP` triggers `_transcribe_buffer()`, which converts bytes to a float32 numpy array, resamples to 16 kHz via linear interpolation if needed, and calls `WhisperSTT.transcribe()` in a thread-pool executor, returning a `Transcript` message. For TTS: a `SYNTHESIZE` message with `text` runs urgency detection (`detect_urgency`/`get_urgency_rate`, Step 323) then `PiperTTS.synthesize()`, streaming audio back in `AUDIO_CHUNK` messages. However, `start_wyoming_servers()`/`WyomingManager` (`voice/wyoming/startup.py`) is never called from `nightwatch/main.py` or `nightwatch/orchestrator.py` — only from its own module and tests — so this path currently has no client in the running system either. + +**(c) NLP layer, if invoked** — a user utterance's text would flow into `ConversationContext.add_user_message()` (regex intent classification + entity extraction) → `ClarificationService.check_command()` (consults context for pronoun resolution, flags ambiguous/missing-parameter/"dangerous" commands) → `format_clarification()` produces a spoken question if needed → once resolved, the canonical command string would go to the LLM/tool layer (Command Execution domain). `SuggestionService`/`SkyDescriber`/`SessionNarrator` independently generate proactive/descriptive text from injected dependencies (`target_scorer`, `ephemeris_service`, `weather_service`, `frame_analyzer` — all optional, duck-typed, un-enforced interfaces). `UserPreferences` persists learned data to disk on every mutating call. The only code that assembles all of these together is `services/ai_services.py`'s `AIServices` facade, itself reachable only from `examples/v05_ai_demo.py` and tests. + +## 4. External dependencies + +- **faster-whisper** (primary) / **openai-whisper** (fallback) — `voice/stt/whisper_service.py:40-51`. Contract: CTranslate2-optimized inference; `transcribe()` returns segments with `.text`/`.start`/`.end` but the code does not read `avg_logprob`/`no_speech_prob` for real confidence. +- **piper-tts** (`piper.PiperVoice`) — `voice/tts/piper_service.py:32-36`; falls back to system `espeak`/OS TTS via `subprocess`/`asyncio.create_subprocess_exec`. +- **sounddevice + numpy** — audio I/O and array math throughout `voice/stt`, `voice/tts`. +- **pymicro-vad** (neural VAD) with graceful fallback to energy-threshold VAD — `voice/stt/whisper_service.py:33-37,156-270`. +- **zeroconf** (optional, lazily imported) — mDNS advertisement in `voice/wyoming/startup.py:112-121`. +- **torch** (optional, lazily imported) — only used to configure CUDA device/memory fraction for TTS startup, `voice/wyoming/startup.py:376-405`. +- **Wyoming protocol** (Rhasspy/Home Assistant ecosystem contract) — custom implementation of the wire format in `voice/wyoming/protocol.py`; the upstream spec has no built-in authentication, and this implementation adds none either (see Security #1). +- **Cross-domain, Command Execution & Tool Integration:** `voice/__init__.py` imports `ToolRegistry`, `TELESCOPE_SYSTEM_PROMPT` from `voice/tools/` (out of scope for this review). +- **Cross-domain, Core Orchestration:** `nightwatch/config.py:247-433` (`VoiceConfig`, `TTSConfig`, Pydantic models) is the shared configuration contract; `nightwatch/voice_pipeline.py` independently reimplements STT/TTS wrapper classes and audio capture (`AudioCapture`, `WakeWordDetector`, `LEDIndicator`, `AudioPlayer` at lines 231/458/908/1272) rather than depending on this domain's modules — see Data Flow (a). +- **Packaging detail:** `voice/requirements.txt:19` lists `webrtcvad`, but it is only imported by `nightwatch/voice_pipeline.py:968-971` (Core Orchestration), not by any file inside `voice/` itself — a minor dependency-declaration/ownership mismatch. +- **services/nlp optional collaborators** (all duck-typed, no ABC/Protocol enforcement): `target_scorer` (expects `.rank_targets(...)`), `frame_analyzer` (expects `.get_session_stats()`), `ephemeris_service`, `weather_service`, `catalog_service` — implicit contracts with `services/catalog`, `services/camera`, `services/ephemeris`, `services/weather` (all out of scope, noted as touchpoints). + +## 5. Invariants and conventions + +- **Audio format default:** 16 kHz, mono, 16-bit PCM is assumed throughout (`AudioConfig`, Wyoming `AudioFormat` defaults). Non-16kHz input is resampled with plain linear interpolation (`voice/wyoming/stt_server.py:299-305`) — no anti-aliasing, and no test exercises this path. +- **Wire protocol error handling:** `read_message()` (`voice/wyoming/protocol.py:418-434`) catches *all* exceptions (malformed JSON, decode errors, unknown enum values) and returns `None`, which callers treat identically to a clean disconnect. There is no way for a caller to distinguish "protocol error" from "client hung up." +- **Confidence is a placeholder, not a signal:** `WhisperSTT.transcribe()`/`transcribe_file()` always set `confidence=0.9` (`voice/stt/whisper_service.py:454,465,592`). Any downstream logic gating on confidence (e.g., `WyomingSTTServer`'s Step 317 threshold, default 0.6) is effectively unconditional. +- **Message ordering is assumed, not enforced:** `WyomingSTTServer` expects `AUDIO_START` → `AUDIO_CHUNK`* → `AUDIO_STOP`; chunks arriving while `session.is_streaming` is false are silently dropped with no error sent to the client (`voice/wyoming/stt_server.py:234-238`). +- **Global singleton pattern:** every `services/nlp` submodule exposes a `get_*()` factory backed by a module-level global (e.g. `services/nlp/conversation_context.py:717-730`) — process-wide, no per-user/session key. Safe only under a single-concurrent-user assumption. +- **Persistence:** `UserPreferences` reads/writes plain JSON (`json.load`/`json.dump`, no pickle/yaml) to `~/.nightwatch/user_preferences.json` synchronously on every mutating call (`services/nlp/user_preferences.py:517-548`); load failures are caught broadly and silently fall back to defaults (`services/nlp/user_preferences.py:587-588`). +- **"Dangerous action" confirmation is UX, not a safety interlock:** `services/nlp/clarification.py:186-195,283-300` matches literal substrings ("emergency", "abort", "park", "close roof", etc.) and asks for a yes/no confirmation. This is a conversational nicety layered in front of whatever real enforcement exists in the Core Orchestration & Safety domain (`nightwatch/safety_interlock.py`) — it must not be treated as a substitute for that enforcement. +- **Error-handling idiom split:** `voice/stt`, `voice/tts` use bare `print()` for diagnostics (no `logging` integration); `voice/wyoming/*` and `services/nlp/*` consistently use `logging.getLogger(...)`. Anyone running both layers together gets inconsistent, partially-uncapturable diagnostics. + +## 6. MATRIX FLAGS + +### Security observations + +1. **Unauthenticated, unencrypted, all-interfaces-by-default network servers.** `WyomingSTTServer` (`voice/wyoming/stt_server.py:109-134`, default port 10300) and `WyomingTTSServer` (`voice/wyoming/tts_server.py:158-180`, default port 10301) accept plaintext TCP with zero authentication anywhere in `voice/wyoming/protocol.py`. Defaults come straight from `nightwatch/config.py:342-420`: `wyoming_host: str = "0.0.0.0"`, `wyoming_enabled: bool = True`. Out of the box, any host that can reach the port can submit audio for transcription (GPU/CPU resource consumption), request arbitrary speech synthesis, or passively eavesdrop on all traffic (no TLS). `voice/wyoming/startup.py`'s `WyomingServiceDiscovery` further advertises these services via mDNS by design. This may be "normal" for the Wyoming/Home-Assistant ecosystem (LAN-trust model), but nothing in this codebase adds a safeguard (allowlist, shared token, mTLS), and no such caveat is documented near the config defaults. +2. **Unbounded server-side memory growth from network input.** `WyomingSTTServer._handle_message` appends every `AUDIO_CHUNK` to `session.audio_buffer` with no size or duration cap (`voice/wyoming/stt_server.py:234-238`), unlike the local listening path which enforces `AudioConfig.max_duration` (`voice/stt/whisper_service.py:517-523`). A client that sends `AUDIO_START` and streams indefinitely without `AUDIO_STOP` can exhaust server memory — trivial to combine with observation #1 since there is no auth to gate who can open a session. +3. **PowerShell command-injection pattern in the Windows TTS fallback.** `voice/tts/piper_service.py:436-448` (`SystemTTS.speak`) builds `ps_script = f'...; $speak.Speak("{text}")'` by directly interpolating `text` into a double-quoted PowerShell string literal, then executes it via `powershell -Command