diff --git a/.claude/agent-memory/chief-architect.md b/.claude/agent-memory/chief-architect.md new file mode 100644 index 0000000..ebe5a26 --- /dev/null +++ b/.claude/agent-memory/chief-architect.md @@ -0,0 +1,93 @@ +# Agent Memory: Chief Architect (L5/L6) — NIGHTWATCH + +Distilled system model after the first full review cycle (2026-07-12, branch +claude/install-review-org-37y4ck, 61 commits, repo age 5.5 months). Update — don't replace — +on future passes. Deliverables written: docs/review/40-architecture.md, 50-risk-register.md. + +## System model (one paragraph) + +Voice-controlled autonomous observatory. Hub-and-spoke around `nightwatch/orchestrator.py` +(3,446-line god-file). Three layers: `nightwatch/` core, `services/` (22 capability modules), +`voice/` (STT/TTS/Wyoming + 87-tool LLM schema catalog). Dependency direction is clean +(services never import nightwatch). The defining property as of 2026-07: **breadth-first +scaffolding, near-zero integration** — components are individually well unit-tested but the +running system does not exist. Single author (Tim Hennessey = THOClabs, 97% of commits). + +## Verified load-bearing facts (re-check these first on any future review) + +- **Entry point crashes:** `main.py:308,325` `setup_logging(level=)` vs param `log_level`. + One-line fix; if fixed, most "dormant" findings go live (adopt security report's Tier A/B rule). +- **Emergency roof close broken twice:** (1) `RoofController.__init__` (roof_controller.py:484-531) + never sets `self._gpio` (only unreferenced `setup_rain_sensor_interrupt()` does, :1061); + `_run_motor:848` AttributeErrors, swallowed. (2) `monitor.py:1535` calls async `close()` without + await from sync `_close_enclosure_safely`. Both masked by tests that patch `_run_motor`. +- **Unwired in production (zero construction sites in main.py/orchestrator.py, all verified):** + LLMClient, VoicePipeline, Wyoming servers, AIServices/services-nlp, SafetyInterlock, + EmergencyResponse, SafeStateHandler, EventBus, CommandQueue, ToolChain, ToolRegistry + (~4,100 dead lines in telescope_tools.py, repo's largest file), execute_cancellable/_active_commands. +- **Live safety boundary is exactly:** services/safety_monitor loop → orchestrator + `_on_safety_change`/`_on_safety_veto` cancel of the single `_active_context` (set only by + tool_executor.py:351,406) + inline park/close in `_safe_shutdown`/`end_session`/ + `emergency_shutdown` + SAFE-004 watchdog heartbeat path (safety_monitor only; other services' + watchdog heartbeats never called → UNKNOWN forever). +- **Tool surface:** TOOL_PARAM_MODELS has exactly 18 keys (mount/catalog/ephemeris/weather/ + safety/session). 87 schemas defined in voice/tools. voice_pipeline.py:2086 imports nonexistent + `nightwatch.telescope_tools` → tools=None → LLM never gets schemas → VOX-003 inert on real traffic. + LLMClient.requires_confirmation's 4 critical tool names aren't in the registry (double-dead). +- **Protocol mismatches (M3):** LX200Client park/stop/unpark are sync; orchestrator awaits them. + SafetyMonitor lacks `is_safe` property; emergency_response/watchdog expect roof.get_state() + (real API: `state` property). MagicMock tests hide all of it. +- **CI cannot fail:** 12× continue-on-error + `|| true`/`|| echo` on every real check in ci.yml. + Test suite polluted by module-level `sys.modules['numpy']=MagicMock()` in test_piper_service.py:27 + and test_whisper_service.py:35 (order-dependent failures). Coverage 48.25% vs unenforced 60/80. + pytest.ini wins; pyproject.toml pytest block dead. Real bugs found by suite: power_restore 300s + hang; double `_save_session_log` (orchestrator.py:2059+2391). +- **Config truths:** LLMConfig (config.py:436-479) has NO api_key/endpoint/backend fields — + 00-inventory.md:291 was wrong. Keys read from env in llm_client.py:454,572. SafetyConfig is the + threshold source of truth; constants.py has drifted flat copies (unreferenced). SAFETY env + allowlist (config.py:90) is real, empty, well-tested — the repo's best control. +- **Ecowitt parser fails open** (defaults 70°F/dry on garbled JSON); SAFE-002 secondary rain + sensor unimplemented but `require_secondary_rain_sensor=True` default with no config surface. +- **Network defaults:** Wyoming 0.0.0.0:10300/10301, enabled=True, no auth, unbounded audio buffer; + PDU admin/admin + SNMP "private". docker prod exposes 10300; systemd wyoming unit ExecStart's + `voice.wyoming_server` module does not exist. README's `nightwatch.cli` does not exist. + `pyindi-client~=2.0.8` has never existed on PyPI (install fails atomically). +- **aiohttp 3.13.5 in uv.lock: 11 CVEs, fix 3.14.1** (as of 2026-07). + +## Git/history facts + +- Two identities, one person; Claude has 2 docs commits. Main branch stale vs review branch. +- Stale-since-2026-01-20 (verified): alpaca, enclosure, encoder, ephemeris, indi, simulators, + voice/stt, voice/tts, voice/wyoming, services/nlp (historian's §4.1 missed the voice/nlp set; + voice/ recent activity is only voice/tools/telescope_tools.py). alerts/meteor stale since 01-28. +- No tags/releases. 1 merge commit. Commit discipline high (ARCH-/SAFE-/HWS-/VOX- refs) but specs + are marked "Complete" at code-exists, not wired (ToolChain Step 267; SAFE-001 claim vs C1). + +## Judgment calls I made (keep consistent next time) + +- Adopted security auditor's rule: "unwired" is NOT a mitigation for anything the deploy artifacts + intend to run (entry fix is one line). +- Ranked R1 (roof) and R2 (CI/no-feedback-loop) co-equal at 20; R2 is the enabling risk. +- Treated the review corpus itself as partial bus-factor mitigation. +- 437 broad-except sites framed as the root-cause pattern (with power_manager.py:309 bare except + returning success) rather than as individual findings. +- meteor_tracking hopi_circles/lexicon_prayers: editorial/cultural-review item only, no code defect. + +## Where reports were wrong (recorded in 40-architecture.md §6) + +- 00-inventory: LLMConfig fields (wrong); SAFE-002/004 attributed to safety_interlock.py (wrong — + they live in monitor.py:553 / watchdog.py:505); tests/hardware "skipped in CI" (wrong — collect + 0 items, they're manual scripts). +- 10-history: abandoned-zones list under-counted (missed voice/* subdirs + services/nlp). +- No domain analyst's concrete code claim was found wrong; 31-quality confirmed all flags. + +## Next-review checklist + +1. Is main.py fixed and does `--dry-run` run? If yes, re-tier all Tier-B findings. +2. `grep -n "_gpio" services/enclosure/roof_controller.py` — still uninitialized in __init__? +3. Does monitor.py `_close_enclosure_safely` await? Is `_action_callback` assigned? +4. ci.yml: count continue-on-error; is unit-tests job gated? Is mypy nightwatch/ gated? +5. Does voice_pipeline._get_tools resolve? TOOL_PARAM_MODELS key count vs live handlers? +6. Any wire-or-delete triage done on ToolRegistry/EventBus/CommandQueue/EmergencyResponse? +7. Tags created? Second contributor? PRs to main? +8. aiohttp bumped? pyindi-client pin fixed? pip-audit gating? diff --git a/.claude/agent-memory/domain-analyst-voice-nlp.md b/.claude/agent-memory/domain-analyst-voice-nlp.md new file mode 100644 index 0000000..32a2ff1 --- /dev/null +++ b/.claude/agent-memory/domain-analyst-voice-nlp.md @@ -0,0 +1,104 @@ +# Agent Memory: Domain Analyst — Voice & NLP (voice/, services/nlp/) + +Durable notes for future reviews of NIGHTWATCH. Written 2026-07-12 after first full +domain review. Update this file (don't replace wholesale) as new reviews confirm, +refute, or extend these findings. + +## Architecture facts (high confidence, file:line verified) + +- **The voice/NLP subsystem is built but not wired into the running app.** + `nightwatch/__init__.py:50-57` has the `from services.nlp import (...)` line + literally commented out ("to avoid circular deps"). `nightwatch/voice_pipeline.py` + reimplements its own `STTInterface`/`TTSInterface` (lines ~1458-1659) instead of + reusing `voice/stt/whisper_service.py:WhisperSTT` or + `voice/tts/piper_service.py:PiperTTS`, and its `TTSInterface.synthesize()` is a + hardcoded mock (`_generate_mock_audio`, silent WAV) — Piper is never actually + called from the main pipeline. `voice/wyoming/startup.py:start_wyoming_servers()` + and `services/ai_services.py:AIServices` (the only code that assembles + `ConversationContext`/`ClarificationService`/`SuggestionService`/etc.) are each + only called from their own module, `examples/v05_ai_demo.py`, or `tests/` — never + from `nightwatch/main.py` or `nightwatch/orchestrator.py`. + **Before trusting any "data flow" narrative about voice control in this repo, + grep for actual call sites — the docstrings/comments describe an architecture + that isn't fully assembled yet.** + +- **`services/ai_services.py`** (top-level file directly under `services/`, not in + any subdirectory) is the de facto integration facade for `services/nlp/*` but was + not explicitly assigned to any domain in the 00-inventory decomposition. Whoever + reviews "Astronomy & Hardware Services" or does synthesis should know it exists + and is currently a dead end (only used by `examples/v05_ai_demo.py` + tests). + +- **Every file in `voice/stt`, `voice/tts`, `voice/wyoming`, and all six + `services/nlp/*.py` files has exactly ONE commit**, all dated 2026-01-20 between + 05:29-06:29 UTC. Zero commits since (as of 2026-07-12, ~171 days). The L2 + git-historian's "abandoned zones" list (`docs/review/10-history.md` §4.1) did + NOT include voice/ or services/nlp — but by the same "no commits in 165+ days" + criterion it uses for alpaca/enclosure/encoder/ephemeris/indi/simulators, this + entire domain qualifies too. Worth checking if future historian passes catch this. + +- **Wyoming protocol servers (`voice/wyoming/stt_server.py`, + `voice/wyoming/tts_server.py`) have zero authentication anywhere**, bind + `0.0.0.0` by default (`nightwatch/config.py:346-349,411-414`), and are enabled by + default (`wyoming_enabled: bool = True`). No TLS, no token, no allowlist in + `voice/wyoming/protocol.py` at all. This is consistent with the general + Wyoming/Home-Assistant LAN-trust ecosystem model but there's no code-level + mitigation or documented caveat in this repo. + +- **`WhisperSTT.transcribe()` hardcodes `confidence=0.9`** in all code paths + (`voice/stt/whisper_service.py:454,465,592`), regardless of backend. This makes + the Wyoming STT server's "Step 317" confidence-threshold filtering + (`voice/wyoming/stt_server.py`, default threshold 0.6) permanently inert. Tests + (`tests/unit/test_whisper_service.py:800`) assert `confidence == 0.9`, so this + is an accepted stub, not an oversight anyone will "just fix" without prompting. + +- **`services/nlp/clarification.py`'s "SAFETY_CONFIRMATION" ambiguity type is a + conversational nicety, not a safety interlock.** It matches literal substrings + ("emergency", "abort", "park", "close roof", etc.) in `DANGEROUS_ACTIONS` and + asks a yes/no question. The real safety enforcement lives in + `nightwatch/safety_interlock.py` (Core Orchestration & Safety domain). Any + synthesis-level report should NOT conflate these two — they are unconnected. + +- **Naming convention across `services/nlp/*.py`:** every submodule follows the + same shape — dataclasses + Enum types, a `logging.getLogger("NIGHTWATCH.")` + logger, a service class with public methods, and a `get_()` factory backed + by a module-level singleton (`_default_*: Optional[...] = None`). All six + singletons are process-wide with no session/user key — a real risk if NIGHTWATCH + ever needs concurrent sessions. + +- **Two different code-style zones inside this domain:** `voice/stt/*.py` and + `voice/tts/*.py` use bare `print()` for all diagnostics (no `logging` at all). + `voice/wyoming/*.py` and `services/nlp/*.py` use `logging.getLogger(...)` + consistently. If reviewing again, check whether this has been unified. + +## Test coverage facts + +- Strong, well-mocked unit tests exist for: `test_whisper_service.py` (1162 lines), + `test_piper_service.py` (1017 lines), `test_wyoming_protocol.py` (867 lines — + protocol/dataclass serialization ONLY), and all six `services/nlp` submodules + (`test_clarification.py`, `test_conversation_context.py`, + `test_session_narrator.py`, `test_sky_describer.py`, `test_suggestions.py`, + `test_user_preferences.py`, ~500-700 lines each). This contradicts a naive + "young repo = low test coverage" assumption for this specific domain — the NLP + side is actually well tested for pure logic. +- **Zero test coverage found for the actual network server classes** + (`WyomingSTTServer`, `WyomingTTSServer`, `WyomingManager`, + `start_wyoming_servers`) — confirmed via repo-wide grep, no hits under `tests/`. + This is exactly where the unbounded-buffer-growth and silent-exception-swallowing + issues live. High-value gap to flag again if it persists in future reviews. + +## Gotchas for future analysts of this repo + +- Don't assume `voice/requirements.txt` dependencies are only used inside `voice/` + — `webrtcvad` is declared there but actually imported by + `nightwatch/voice_pipeline.py` (Core Orchestration domain), not by anything + under `voice/`. +- `services/ai_services.py` sits at `services/` top level, outside any + subdirectory-based domain in the 00-inventory decomposition — easy to miss when + scoping a domain review strictly by directory list. +- When checking "is X wired up," grep for the actual constructor call + (`ClassName(`) or function call site across the whole repo excluding `tests/` + and `examples/` — docstrings and `__init__.py` comments in this repo are + sometimes aspirational (see `nightwatch/__init__.py:50-57`). +- Git blame/log per-file is fast and decisive for "is this maintained" questions; + `git log --all -- | wc -l` plus `git log -1 --format=%ai --all -- ` + gave crisp, citable evidence for the "built in one hour, day one" finding. 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/. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..54c8a94 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,88 @@ + +# Repository Review Summary (generated 2026-07-12) + +Full review corpus: `docs/review/` (00-inventory through 60-executive-summary). Do not trust +the green CI badge or the README quickstart — see "Verified commands" below. + +## What this is +NIGHTWATCH: voice-controlled autonomous observatory controller (Python 3.11, v0.1.0-dev). +Intended loop: voice -> Whisper STT -> LLM tool call -> validated dispatch -> mount/camera/roof, +with a continuous safety monitor that parks the mount and closes the roof on unsafe weather. +Key fact: components are well built and unit-tested but NOT wired into a running system. + +## System map +- **Core orchestration & safety** (`nightwatch/`): `orchestrator.py` (3,446-line hub), `config.py` + (pydantic + safety env allowlist), `watchdog.py` (SAFE-004). `safety_interlock.py`, + `emergency_response.py`, `EventBus`, `CommandQueue` are DORMANT (zero production call sites). +- **Command execution** (`nightwatch/tool_executor.py` + `voice/tools/`): 18 live + Pydantic-validated handlers; ~87 tool schemas defined, most with no live handler; + `ToolRegistry` (~4,100 lines of `telescope_tools.py`) is dead code with NO validation. +- **LLM client** (`nightwatch/llm_client.py`, `tool_params.py`, `cancellation.py`): local llama + + Anthropic/OpenAI fallback, VOX-003 tool-call validation — never constructed in production. +- **Voice & NLP** (`voice/`, `services/nlp/`): real Whisper/Piper/Wyoming engines, all unwired; + `nightwatch/voice_pipeline.py` duplicates STT and returns mock silent TTS audio. +- **Astronomy & hardware** (`services/`, ~21 modules): mount, camera, roof, weather, power, + ephemeris, catalog, guiding, etc. `services/safety_monitor/monitor.py` is the LIVE safety brain. + +## Verified commands (state as of 2026-07-12, per docs/review/31-quality.md) +- Install: `pip install -r services/requirements.txt` FAILS on a clean machine — + `pyindi-client~=2.0.8` never existed on PyPI. README/QUICKSTART cannot be completed as written. +- Run: `python -m nightwatch.main --dry-run` CRASHES — `setup_logging(level=...)` vs. parameter + `log_level` (`main.py:308,325`). `python -m nightwatch.cli` (README) does not exist. +- Test: `pytest tests/unit/` — ~2618 tests; expect ~48 failures, most caused by unscoped + `sys.modules['numpy'] = MagicMock()` in `tests/unit/test_piper_service.py:27` and + `test_whisper_service.py:35` (collection-order pollution; they pass in isolation). + `pytest.ini` is the live config; `pyproject.toml`'s `[tool.pytest.ini_options]` block is DEAD. + `tests/hardware/` collects 0 pytest items (manual CLI scripts, not tests). +- Lint/type: `ruff check services/ voice/ nightwatch/ --ignore=E501,F401,F841` (2585 errors); + `mypy nightwatch/ --ignore-missing-imports` (233 errors — it flags the main.py crash). +- CI (`.github/workflows/ci.yml`) CANNOT FAIL: every real check is muted via + `continue-on-error: true` and/or `|| true`/`|| echo`. A green badge certifies nothing. +- Coverage: 48.25% measured vs. unenforced 60% (pyproject) and 80% (CI script) thresholds. + +## Conventions and invariants to respect +- Commits: conventional `type(area): SPEC-### subject` referencing ARCH-/SAFE-/HWS-/VOX-/DEP- + specs and `Risk #N`; pre-commit blocks direct commits to main. +- `SAFETY_ENV_OVERRIDE_ALLOWLIST` (`nightwatch/config.py:90`) is deny-by-default and EMPTY: + `NIGHTWATCH_SAFETY_*` env overrides are rejected. Never widen it casually; it is well tested. +- `TOOL_PARAM_MODELS` (`nightwatch/tool_params.py`) is the single tool-schema source of truth, + `extra="forbid"`, validated in both `llm_client.py` (VOX-003) and `tool_executor.py` + (ARCH-001). A tool offered to the LLM without a registry entry is silently dropped. +- Cancellation is cooperative (`CancelToken`/`CommandContext`, ARCH-003) — never + `Task.cancel()`. SAFE-001 requires safety callbacks (cancel) to run BEFORE the roof moves. + Only one active context is supported; a second `set_active_context` displaces the first. +- Shutdown/safety code must use `registry.get_for_shutdown()` (not `get_running()`) so an + ERRORed mount still gets parked (ARCH-002 bypass invariant). +- Every new mutating tool handler must add its own `orchestrator.safety.is_safe` check — + there is no centralized safety middleware. +- NLP "dangerous action" clarification and the LLM `SAFETY STATUS:` prompt are advisory UX + only; the authoritative veto is `services/safety_monitor`. Never treat them as enforcement. +- Do NOT add `except Exception`-and-continue on safety/hardware paths without a structured + alert plus one test through the real (unmocked) call path — this idiom (437 sites) has + already hidden three safety-critical defects. + +## Danger zones (extra care required) +- `services/enclosure/roof_controller.py` — `self._gpio` never initialized in `__init__`; + emergency close raises AttributeError, swallowed → roof never moves (Risk R1, security C1). +- `services/safety_monitor/monitor.py` — `_close_enclosure_safely()` calls async `close()` + without `await` (M2); `handle_power_failure_response` references never-assigned + `_action_callback` (M1). Live safety brain; `config.py:81` intends edits here restricted. +- `nightwatch/orchestrator.py` — 3,446 lines, highest churn, single author; contains dormant + subsystems and a confirmed double `_save_session_log()` (lines 2059 + 2391). +- `nightwatch/main.py` — crashes on every invocation; `main()` has zero test coverage. +- `voice/tools/telescope_tools.py` — 5,662 lines, ~4,100 dead (`ToolRegistry` does raw + `handler(**arguments)` with no validation; its emergency `close_roof` audit log is a stub). + Do not revive without routing through `TOOL_PARAM_MODELS`. +- `nightwatch/constants.py` — safety thresholds duplicated from `SafetyConfig` and already + drifted; always use `config.py`'s `SafetyConfig`, never `constants.py`. +- Protocol mismatch: orchestrator does `await mount.park()` but `LX200Client.park()` is + synchronous — TypeError against real hardware; MagicMock tests hide all such contract breaks. +- Network defaults: Wyoming STT/TTS bind `0.0.0.0`, no auth, unbounded audio buffer + (`config.py:346-420`); PDU defaults `admin`/`admin` + SNMP `private` + (`services/power/power_manager.py:50-55`). +- Dormant-but-plausible code (`SafetyInterlock`, `EmergencyResponse`, `SafeStateHandler`, + `EventBus`, `CommandQueue`, `ToolChain`, `LLMClient`, `VoicePipeline`, Wyoming servers, + `services/nlp`): zero production call sites — never assume they enforce anything at runtime. + +Review date: 2026-07-12. Regenerate this section when the review corpus is refreshed. + 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. + 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** diff --git a/docs/review/20-domain-astronomy-hardware-services.md b/docs/review/20-domain-astronomy-hardware-services.md new file mode 100644 index 0000000..deb1784 --- /dev/null +++ b/docs/review/20-domain-astronomy-hardware-services.md @@ -0,0 +1,118 @@ +# Domain Report: Astronomy & Hardware Services + +**Domain:** `services/` (all subdirectories **except** `services/nlp/`, which belongs to the Voice & NLP analyst) +**Analyst:** L3 Domain Analyst (Astronomy & Hardware Services) +**Scope:** ~21 service modules covering hardware device integration, astronomy calculations, external service APIs, equipment drivers, and safety veto logic. + +**Depth note (per assignment):** Deep-dived `safety_monitor`, `enclosure`, `mount_control`, `weather`, and `power` (the safety-critical set), plus `astrometry`, `ephemeris`, `catalog`, `alpaca` at moderate depth. **Only skimmed:** `camera` (2494 lines — read init/SDK-wrapper/capture-loop comments only), `focus` (2414 lines — header/config only), `guiding` (protocol header only), `encoder` (header only), `indi` (structure only), `meteor_tracking` (structure + two oddity files), `scheduling` (`__init__` only), `simulators` (not read directly, only referenced by grep), `services/voice/` (vocabulary/wake-word trainers — grepped for I/O patterns only). Findings below on skimmed modules are narrower and flagged as such. + +--- + +## 1. Responsibility + +This domain is the hardware-and-astronomy capability layer of NIGHTWATCH: it talks to physical telescope equipment (mount, camera, focuser, roof, encoders), external astronomy services (plate solving, ephemeris, meteor/fireball networks, object catalogs), and environmental sensors (weather, power), and it owns the safety-veto logic that decides whether it is safe to observe. Each module is a mostly-independent driver/client wrapped in dataclasses and async methods that the Core Orchestration domain (`nightwatch/orchestrator.py`) is *designed* to register and drive, though (see Section 5/6) that wiring is largely absent from the current production entry point. + +## 2. Key modules + +**Safety-critical (deep-dived):** +- `services/safety_monitor/monitor.py` — `SafetyMonitor` (continuous evaluation loop), `SafetyThresholds`, `SafetyStatus`, `SafetyAction` enum. Central veto engine: `evaluate()` (monitor.py:1234) fuses weather/cloud/daylight/altitude/power/enclosure/meridian/battery/network checks into one `SafetyStatus` and a `SafetyAction`; `run()` (monitor.py:1580) polls this every `poll_interval` seconds and calls `execute_action()` (monitor.py:1438). +- `services/enclosure/roof_controller.py` — `RoofController` (roof state machine + safety interlocks), `GPIOInterface` (mock/RPi.GPIO/gpiozero backend abstraction). `open()`/`close()` (lines 635, 690), rain-sensor hardware interrupt path (`_on_rain_interrupt`, line 1085), power-loss brake logic (`_on_power_loss`, line 1379). +- `services/mount_control/lx200.py` — `LX200Client`: Meade LX200 serial/TCP protocol client for the OnStepX controller. `connect()`/`_send_command()` (lines 98, 128), `goto_ra_dec`/`park`/`unpark`/`stop` (lines 374, 580, 585, 530), encoder-corrected position via `get_corrected_position()` (line 233). +- `services/mount_control/onstepx_extended.py` — `OnStepXExtended(LX200Client)`: PEC recording/playback, TMC stepper-driver diagnostics. +- `services/weather/ecowitt.py` — `EcowittClient`: polls Ecowitt WS90 local HTTP API (`fetch_data`, line 136), parses into `WeatherData`. +- `services/weather/secondary_rain.py` — `SecondaryRainReading` data contract for the (not-yet-implemented) dual-redundant rain sensor described in SAFE-002. +- `services/weather/cloudwatcher.py`, `services/weather/unified.py` — AAG CloudWatcher client and a combining `UnifiedWeatherService` facade (skimmed at header level only beyond grep). +- `services/power/power_manager.py` — `PDUClient` (HTTP/SNMP smart-PDU outlet control, lines 70-450), `PowerManager` (UPS monitoring + staged shutdown, class starts line 853). + +**Astronomy / observation (moderate depth):** +- `services/ephemeris/skyfield_service.py` — `EphemerisService`: Skyfield/JPL DE440 wrapper for planet/sun/moon positions, rise/set/twilight (`initialize()` line 211). +- `services/astrometry/plate_solver.py` — plate-solve orchestration over `solve-field` (astrometry.net) and ASTAP via `asyncio.create_subprocess_exec` (lines 368, 520); retry-with-growing-radius logic (`_run_astrometry_net_once`, line 349). +- `services/catalog/catalog.py` — `CatalogDatabase`/`CatalogService`: SQLite-backed Messier/NGC catalog with parameterized queries (`search_by_type`/`search_by_magnitude`/`search_by_constellation`, lines 403-507). +- `services/catalog/target_scorer.py`, `success_tracker.py`, `object_identifier.py` — scoring/learning/pattern-match layers (skimmed). +- `services/guiding/phd2_client.py` — `PHD2Client`: JSON-RPC over TCP to PHD2's socket server (port 4400), `readline()`-based framing (lines 214, 226). +- `services/alpaca/alpaca_client.py` — `AlpacaDiscovery` (UDP broadcast + `alpyca`/fallback discovery, lines 83-189), `AlpacaDevice`/`CameraState`/`ImageData` wrappers. Flagged stale in `10-history.md` (no commits since 2026-01-20). +- `services/indi/indi_client.py` — `NightwatchINDIClient`/`AsyncINDIClient`, thin wrapper over `pyindi-client`'s `PyIndi.BaseClient`; no manual protocol parsing in this file (protocol XML handling lives in the underlying C++ library, out of scope). +- `services/indi/device_adapters.py` — higher-level per-device adapters (this file, not `indi_client.py`, is what `tests/unit/test_indi_adapters.py` actually exercises — see Section 6). +- `services/meteor_tracking/*` — `fireball_client.py` (NASA CNEOS/AMS API client, `api_key` handling), `shower_calendar.py`, `trajectory.py`, `watch_manager.py`, plus two thematically unusual files: `hopi_circles.py` (concentric-circle ground-search-pattern geometry) and `lexicon_prayers.py` (flavor-text generator in an invented "Lexicon" language for meteor alerts). Functionally these are ordinary haversine-geometry and string-formatting code (skimmed, no bugs found in the portions read); the naming is an oddity worth flagging for editorial/cultural-sensitivity review rather than a code-quality issue. +- `services/alerts/alert_manager.py` — SMTP (`_send_smtp_email_sync`, line 816), SMS, and `ntfy.sh` push notification dispatch with per-channel rate limiting. +- `services/scheduling/scheduler.py`, `condition_provider.py` — target scheduling and multi-source condition aggregation (header-level review only; no `eval`/dynamic-condition-parsing found). +- `services/simulators/*` — mock mount/camera/guider/weather/star-field implementations used by integration tests (not read directly; referenced only via grep for this report). +- `services/voice/vocabulary_trainer.py`, `wake_word_trainer.py` — astronomy-vocabulary and wake-word personalization trainers; both load JSON training data (`json.load`, vocabulary_trainer.py:323, wake_word_trainer.py:308), no pickle/unsafe deserialization found. +- `services/encoder/encoder_bridge.py` — `EncoderBridge`: serial protocol client for absolute-position encoder hardware (header-level review). +- `services/__init__.py` / `services/ai_services.py` — top-level `AIServices` facade aggregating scheduling/catalog/NLP/voice services for a "v0.5" unified access point. + +## 3. Data flow (representative trace: weather → safety veto → hardware action) + +1. **Ingress:** `EcowittClient.fetch_data()` (ecowitt.py:136) issues an `aiohttp` GET to the local Ecowitt gateway's `/get_livedata_info` endpoint (plaintext HTTP, no auth) every `poll_interval` seconds, and `_parse_response()` (line 159) extracts fields from a `common_list` array by scanning for hex-string IDs (`"0x02"` for temp, etc.). +2. **Transform:** Missing/unexpected keys silently fall back to hardcoded defaults (e.g., `temp_f = get_common("0x02", 70.0)`, `is_raining = rain_rate > 0` where `rain_rate` defaults to `0` — ecowitt.py:174-200) rather than marking the reading invalid. The resulting `WeatherData` is handed to `SafetyMonitor.update_weather(data)` (monitor.py:394), which wraps it in a `SensorInput(is_valid=True, ...)` — the safety layer has no signal that a partial/garbled parse occurred. +3. **Decision:** `SafetyMonitor.evaluate()` (monitor.py:1234) calls `_evaluate_weather()`, `_vote_rain_status()` (the SAFE-002 dual-sensor voting, monitor.py:553), `_evaluate_clouds/_daylight/_altitude_limit/_power/_enclosure/_meridian/_staged_battery_shutdown/_network/_power_failure`, ANDs all the `*_ok` booleans into `is_safe`, and picks a `SafetyAction` (EMERGENCY_CLOSE > battery-shutdown > power-failure > network-failure > daylight > weather/rain-holdoff > power > altitude > enclosure > SAFE_TO_OBSERVE), matching the priority order documented in the module docstring (monitor.py:9-14). +4. **Egress:** `run()` (monitor.py:1580) notifies registered callbacks first (SAFE-001 ordering, monitor.py:1607-1629), waits a bounded `cancel_settle_timeout_s` window for in-flight cancellation on the destructive path (`_wait_for_cancellations_to_drain`, monitor.py:1550), then calls `execute_action()` which for `EMERGENCY_CLOSE`/`LOW_BATTERY_SHUTDOWN` calls `self.mount.stop()/.park()` followed by `self._close_enclosure_safely()` (monitor.py:1515), which calls `self.enclosure.close()` (i.e., `RoofController.close(emergency=True)`). +5. **Hardware:** `RoofController.close()` (roof_controller.py:690) transitions state to `CLOSING`, calls `_run_motor(direction="close")` (line 822), which reads `self._gpio.read_motor_current()`/`is_motor_overcurrent()` for over-current protection. + +**Verified defect in this exact path (see Section 6, Security/Quality):** step 5's `_run_motor` unconditionally references `self._gpio`, which is never initialized by `RoofController.__init__`; the emergency-close call raises `AttributeError` and is swallowed, so the roof never actually closes despite the safety monitor correctly detecting the emergency. Reproduced live (see Section 6). + +A second representative flow — **goto command**: a target from `catalog.py`'s SQLite lookup or `ephemeris/skyfield_service.py`'s planet-position calculation produces RA/Dec in decimal degrees → `LX200Client.sync_to_coordinates()`/`goto_ra_dec()` (lx200.py:515, 374) formats LX200 wire strings and sends over the serial/TCP link → `plate_solver.py` can subsequently correct pointing via `solve-field`/ASTAP subprocess invocation and call back into `sync_to_coordinates` (HWS-004 in commit history) → `guiding/phd2_client.py` then takes over via JSON-RPC to PHD2 for closed-loop guiding. + +## 4. External dependencies + +- **Skyfield + JPL DE440** (`skyfield~=1.48`, `services/ephemeris/skyfield_service.py:224`) — downloads `de440s.bsp` (~32 MB) from NASA JPL on first run via Skyfield's default `Loader` singleton. See dead-code note in Section 6 (the module's own `DATA_DIR` is created but not actually used to scope this download). +- **aiohttp** (`~=3.9`) — outbound HTTP client for Ecowitt weather gateway, PDU HTTP control, ntfy.sh push, ASI/PDU simulation fallbacks. +- **pyserial** (`~=3.5`) — mount (LX200 serial), roof controller (implied `/dev/ttyUSB0` default, though `connect()` in `roof_controller.py:588` is currently simulated, not real serial I/O), encoder bridge. +- **pyindi-client** (`~=2.0.8`) and **alpyca** (`~=3.0.0`) — INDI and ASCOM Alpaca device protocol bindings; both optional/`ImportError`-guarded. +- **zwoasi** — ZWO camera SDK, optional/guarded (`asi_camera.py:71`). +- **pysnmp** — SNMP PDU control, optional/guarded (`power_manager.py:199,240`). +- **External processes:** `solve-field` (astrometry.net) and `astap` binaries, invoked via `asyncio.create_subprocess_exec` with an argv list (not shell) — no shell-injection surface, but implies a PATH/binary-installation trust assumption. +- **External network services:** PHD2 guide-camera socket server (localhost:4400 JSON-RPC), NASA CNEOS/AMS fireball APIs (`meteor_tracking/fireball_client.py`), SMTP server, ntfy.sh. +- **Cross-domain contract:** the orchestrator (`nightwatch/orchestrator.py`) defines `Protocol` classes (`SafetyServiceProtocol`, `MountServiceProtocol`, `WeatherServiceProtocol`, `CameraServiceProtocol`, `PowerServiceProtocol`, `EnclosureServiceProtocol`, lines 871-1135) that this domain's concrete classes are expected to satisfy so they can be passed to `register_safety`/`register_mount`/etc. (orchestrator.py:1687-1755). **This contract is largely unmet by the concrete implementations — see Section 6, this is the single most important cross-domain finding of this review.** +- **GPIO libraries** (`RPi.GPIO`, `gpiozero`) — optional, `ImportError`-guarded, roof controller only. + +## 5. Invariants and conventions + +- **Fail-safe-by-default intent:** missing/stale sensors are meant to be treated as unsafe (weather timeout 120s, cloud 180s, ephemeris 600s, secondary-rain 60s — `monitor.py:212-221`). Rain voting requires *both* primary and secondary sensors fresh-and-dry to permit operation (`_vote_rain_status`, monitor.py:553-639); either sensor reporting rain, or either missing, is unsafe. +- **SAFE-001 cancel-before-close ordering:** `run()` notifies safety callbacks (which cancel in-flight operations via `nightwatch.cancellation.CancelToken`) *before* calling `execute_action()` for the destructive `EMERGENCY_CLOSE` path, with a bounded settle window (`monitor.py:1607-1653`). This ordering is explicitly documented as fixing a prior water-damage risk (Risk #2). +- **Hysteresis on all threshold checks** (wind, humidity, cloud cover, twilight) to prevent oscillation near boundary values (`monitor.py:710-833`). +- **`require_secondary_rain_sensor=True` by default** (monitor.py:244) — but no Hydreon/secondary-rain driver exists anywhere in the codebase (`secondary_rain.py` is data-shape-only, per its own docstring lines 13-23) and no config surface exposes this flag (`nightwatch.yaml.example` has no `secondary_rain` keys). If this module were wired up as-is with defaults, the observatory could never reach `SAFE_TO_OBSERVE` for the rain check. +- **Broad exception swallowing as a deliberate pattern:** almost every `execute_action`/hardware-call site wraps in `except Exception: logger.error(...)` and continues, on the theory that "the run() loop must keep running even if the enclosure driver throws" (monitor.py:1526-1530). This is a defensible design choice for a monitoring loop, but (see Section 6) it has already hidden at least two real `AttributeError` bugs. +- **`asyncio.to_thread` wrapping for blocking mount I/O is inconsistent:** `LX200Client.sync_to_coordinates()` explicitly wraps its blocking 3-command exchange in `asyncio.to_thread` with a documented rationale (lx200.py:515-528, "would stall the safety watchdog, weather monitor, and TTS pipeline"), but sibling methods (`goto_ra_dec`, `park`, `unpark`, `stop`, all of `onstepx_extended.py`'s PEC/driver methods) remain plain synchronous blocking-socket calls with no such wrapping. +- **Parameterized SQL throughout `catalog/catalog.py`** — all dynamic queries build `sql`/`params` separately and pass both to `cursor.execute(sql, params)`; no string-interpolated SQL found. + +## 6. MATRIX FLAGS + +### Security observations + +1. **Reproducible fail-to-close bug in the emergency roof-close path.** `RoofController.__init__` (roof_controller.py:484-531) never initializes `self._gpio`; it is only ever set as a side effect of `setup_rain_sensor_interrupt()` (line 1051, sets at line 1061), which is called **nowhere** in the codebase (verified via repo-wide grep — zero call sites outside its own definition). `_run_motor()` (line 822) unconditionally evaluates `if self._gpio:` (line 848). Reproduced live: + ``` + RoofController().close(emergency=True) → "Roof close failed: 'RoofController' object has no attribute '_gpio'" + → state=ERROR, returns False (roof never moves) + ``` + The exception is swallowed by `open()`/`close()`'s own `except Exception` (lines 685-688, 737-740), so no caller sees an exception — `SafetyMonitor._close_enclosure_safely()` (monitor.py:1515-1537) also swallows the resulting error and only logs it. **This directly undermines the SAFE-001 guarantee ("EMERGENCY_CLOSE actually closes the roof") that a recent commit specifically added.** The bug is masked from the test suite because `tests/unit/test_roof_controller.py`'s `controller` fixture (lines 89-110) monkey-patches `ctrl._run_motor` with a fake that never touches `self._gpio`. +2. **Reproducible dead-on-arrival power-failure response.** `SafetyMonitor.handle_power_failure_response()` (monitor.py:1116-1161) references `self._action_callback` (line 1141), which is never assigned anywhere in the class (no `__init__` field, no setter). Reproduced live: calling `execute_action(SafetyAction.POWER_FAILURE)` with a mock mount raises `AttributeError: 'SafetyMonitor' object has no attribute '_action_callback'` every time, caught by the inner `except` (re-raised, line 1161) and then the outer `except Exception` in `execute_action` (lines 1512-1513), which only logs it. The detected power-failure condition never actually parks the mount or closes the roof through this path. +3. **Systemic Protocol/implementation mismatch — the orchestrator cannot currently drive real hardware.** `nightwatch/orchestrator.py` defines `Protocol` classes the concrete services must satisfy to be registered (`SafetyServiceProtocol` orchestrator.py:948-976 requires `is_safe` as a property + `get_unsafe_reasons()`; `MountServiceProtocol` orchestrator.py:871-892 requires `async def park()`/`unpark()`; `CameraServiceProtocol` orchestrator.py:978-993 requires `capture()`/`is_exposing`; `PowerServiceProtocol` orchestrator.py:1106-1117 requires `on_battery`/`battery_percent`). None of the corresponding concrete classes in this domain satisfy their Protocol: `SafetyMonitor` has neither `is_safe` (only `SafetyStatus.is_safe`, a data field) nor `get_unsafe_reasons()`; `LX200Client.park()`/`unpark()`/`stop()` (lx200.py:530,580,585) are plain **synchronous** methods, not `async` — `await mount.park()` (called at orchestrator.py:2035, 2366, 2920) would raise `TypeError: object bool can't be used in 'await' expression` against a real `LX200Client`; `ASICamera` exposes `capturing`/`capture_single`/`capture_frame`, not `capture()`/`is_exposing`; `PowerManager` has no `on_battery`/`battery_percent`. Corroborating this, repo-wide grep shows `register_safety`, and constructors (`RoofController(`, `LX200Client(`, `EcowittClient(`, `PDUClient(`) are **only ever invoked from test files** — never from `nightwatch/main.py` or anywhere in `nightwatch/`. (`RoofController`/`EnclosureServiceProtocol` is the one exception that does conform.) This means the entire hardware layer, while richly unit-tested in isolation, has no verified production wiring path today — a fact the Core Orchestration domain reviewer should independently confirm since `main.py` is outside this domain's boundary. +4. **Hardcoded default credentials for power-control hardware.** `services/power/power_manager.py`: `PDUConfig.http_password = "admin"` (line 51) and `snmp_community = "private"` (line 55), duplicated in a second config class (`pdu_password = "admin"`, `pdu_snmp_community = "private"`, lines 807-808). These control real outlet power (mount/camera/computer, per `port_names` default mapping, line 59-64) via plaintext HTTP Basic Auth (line 150) or unauthenticated-in-practice SNMPv2c (community-string "auth"). If left at defaults on a shared network, any host could cut power to the mount/camera/computer. +5. **Unauthenticated device discovery accepts attacker-shaped network input.** `services/alpaca/alpaca_client.py._fallback_discover()` (lines 154-188) sends a UDP broadcast and does `json.loads(data.decode())` on any reply (line 175), then stores the sender's advertised `AlpacaPort` with no bounds/type validation before later using it to build HTTP endpoint URLs. Any host on the broadcast domain can register itself as a "discovered" Alpaca device. This mirrors the real ASCOM Alpaca discovery protocol (not a novel design flaw), but there is no allowlisting/validation layer visible in this codebase before such a discovered endpoint would be used for device control. +6. **Weather sensor parsing fails open, not closed.** `EcowittClient._parse_response()` (ecowitt.py:159-250) defaults missing/malformed fields to benign values (`temp_f` default `70.0`, `is_raining = rain_rate > 0` where `rain_rate` defaults to `0`) rather than marking the reading invalid, and the comment at line 161 acknowledges "structure varies by firmware version." A parse failure on this safety-relevant, network-sourced JSON payload silently reports "dry, comfortable" conditions instead of failing unsafe. This is the same primary sensor SAFE-002's redundancy design (`secondary_rain.py`) was meant to hedge against — but the redundancy is itself unimplemented (see invariant note above). +7. **Plain HTTP, no integrity/auth, for safety-relevant sensor feeds:** `EcowittClient` (gateway HTTP, no TLS/auth), `CloudWatcherClient` (skimmed only, same pattern expected). LAN-local risk profile, but no defense in depth if the LAN segment is compromised. +8. **Subprocess use is safe (positive finding):** `astrometry/plate_solver.py` uses `asyncio.create_subprocess_exec(*cmd, ...)` with an argv list (lines 368, 520), not `shell=True`/string interpolation — no shell-injection surface found. +9. **No eval/exec/pickle/marshal/yaml.load(unsafe) found anywhere in this domain** (repo-wide grep across all non-nlp `services/*.py`). +10. **Inconsistent logging on connection/command failures in `mount_control/lx200.py`:** `connect()` (line 116) and `_send_command()` (line 147) use bare `print()` instead of the module `logger`, so mount connection/command failures may not appear in structured logs at all, weakening observability for a safety-adjacent subsystem. + +### Quality observations + +1. **Two safety-critical `AttributeError` bugs (detailed above) were both masked by test doubles**, not caught by the test suite: `test_roof_controller.py`'s fixture replaces `_run_motor` entirely, and no test appears to exercise `handle_power_failure_response()` end-to-end through `execute_action(POWER_FAILURE)` with a configured mount. This is concrete evidence — not speculation — that broad `except Exception` swallowing (53 occurrences in `alpaca_client.py`, 40 in `asi_camera.py`, 22 in `roof_controller.py`, 21 each in `power_manager.py` and `phd2_client.py`, 17 in `focuser_service.py`) is hiding real defects, consistent with the L2 history report's "feature velocity 16x test velocity" finding. +2. **Dead/unused directory creation in `ephemeris/skyfield_service.py`:** `DATA_DIR = Path(__file__).parent / "data"` (line 181) is created via `mkdir()` (line 217) but the actual ephemeris download at line 224 uses Skyfield's global `load()` singleton, which caches to the process's current working directory, not `DATA_DIR`. The created directory is never used; the ephemeris file location is not actually controlled by this class. +3. **Test coverage gap on the raw INDI protocol client:** `tests/unit/test_indi_adapters.py` tests only `services/indi/device_adapters.py` using its own locally-defined mock types; `services/indi/indi_client.py`'s `AsyncINDIClient`/`NightwatchINDIClient` (656 lines) has no dedicated test file exercising it directly. +4. **`services/simulators/` (mock mount/camera/guider/weather/star-field, ~2700 combined lines) has no standalone unit tests** — it's exercised only indirectly as a fixture dependency of a few integration tests (`test_astrometry_camera.py`, `test_focus_camera.py`, `test_camera_simulator.py`). Given it's test infrastructure, this is lower severity, but any bug in the simulator itself would not be independently caught. +5. **Stale-since-inception modules** (confirmed via `10-history.md`, not re-verified via git here): `alpaca`, `enclosure`, `encoder`, `ephemeris`, `indi`, `simulators` — no commits since 2026-01-20. This review found concrete bugs in one of them (`enclosure`), raising the prior of similar undiscovered issues in the others given the shared "richly implemented, never re-validated against ARCH-00x conventions" pattern. +6. **Inconsistent async/sync API surface within a single class hierarchy:** `LX200Client`/`OnStepXExtended` mix synchronous blocking-socket methods with one explicitly `to_thread`-wrapped async method (`sync_to_coordinates`), with no consistent policy documented for which methods callers must wrap themselves — a latent event-loop-stall risk for the safety watchdog if `park()`/`goto_ra_dec()`/PEC commands are ever called directly from async code without wrapping (this domain cannot verify actual call sites in `nightwatch/`, which is out of scope, but flags it for the Core Orchestration reviewer). +7. **Unusual/undocumented thematic naming** in `meteor_tracking/hopi_circles.py` and `meteor_tracking/lexicon_prayers.py` (an invented "Lexicon" language with "prayers of finding/watching"). Not a functional defect in the code read, but worth an editorial/naming-convention pass — the docstrings reference sacred geometry and prayer generation in production astronomy code with no explanation of intent or ownership. +8. **`services/power/power_manager.py` and `services/alerts/alert_manager.py` define credential fields as plain dataclass attributes** (`http_password`, `snmp_community`, `email_smtp_password`) with no indication of how they're meant to be sourced from a secrets store versus plaintext YAML config — cross-references the Core Orchestration `config.py`, out of this domain's direct scope, but the field shapes originate here. +9. **Broad `except Exception` counts by file** (for the reviewing organization's cross-cutting quality tracking): `alpaca/alpaca_client.py` 53, `camera/asi_camera.py` 40, `enclosure/roof_controller.py` 22, `power/power_manager.py` 21, `guiding/phd2_client.py` 21, `focus/focuser_service.py` 17, `alerts/alert_manager.py` 7, `safety_monitor/monitor.py` 6, `astrometry/plate_solver.py` 6. One bare `except:` (no exception class) found repo-wide (location not pinpointed further given time budget — repo-wide grep count was 1). +10. **Camera capture race-condition history:** commit history shows a dedicated fix (`refactor(camera): HWS-001 extract _do_exposure, fix _capturing race`); current code has extensive docstrings explaining `_capturing` flag ownership (asi_camera.py:823-861) — this appears to be a genuinely well-reasoned fix on inspection of the comments, though full verification of the async race would require a deeper read than the time budget allowed (camera was only skimmed). + +--- + +**Cross-domain touchpoints (not investigated further, noted for the other analysts):** +- `nightwatch/orchestrator.py` — owns `register_safety`/`register_mount`/etc. and the `Protocol` definitions discussed in Security Observation #3; recommend the Core Orchestration analyst confirm whether any bootstrap code (perhaps outside `nightwatch/main.py`) actually wires concrete services, since none was found from this domain's side. +- `nightwatch/safety_interlock.py` — a **separate**, pre-command veto gatekeeper (different from `services/safety_monitor/monitor.py`'s continuous-evaluation loop); the two modules have overlapping vocabulary (both mention SAFE-002/SAFE-004 in comments) but distinct responsibilities. Worth the Core Orchestration analyst confirming there is no unintended logic duplication/divergence between them. +- `voice/tools/telescope_tools.py`, `nightwatch/tool_executor.py` — call into this domain's services; not reviewed here (Command Execution & Tool Integration domain). +- `services/nlp/` — explicitly excluded from this review per assignment; owned by the Voice & NLP analyst. 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). diff --git a/docs/review/20-domain-core-orchestration-safety.md b/docs/review/20-domain-core-orchestration-safety.md new file mode 100644 index 0000000..d709e76 --- /dev/null +++ b/docs/review/20-domain-core-orchestration-safety.md @@ -0,0 +1,363 @@ +# Domain Report: Core Orchestration & Safety + +**Domain:** `nightwatch/` core package (excluding `tool_executor.py`, `response_formatter.py`, +`llm_client.py`, `tool_params.py`, `cancellation.py`, which belong to other analysts' domains) +**Analyst:** L3 Domain Analyst +**Date:** 2026-07-12 + +Files reviewed in depth: `__init__.py`, `constants.py`, `types.py`, `exceptions.py`, `config.py`, +`logging_config.py`, `safety_interlock.py`, `emergency_response.py`, `watchdog.py`, `health.py`, +`main.py`, `orchestrator.py` (3,446 lines — the largest file, read in full via targeted sections). +`voice_pipeline.py` (2,517 lines) lives in this directory but is thematically STT/TTS/audio +plumbing for the Voice & NLP domain; it is covered only briefly here as a boundary note. +`cancellation.py` is owned by another analyst but is read here just enough to describe the +`CommandContext`/`CancelToken` contract that `orchestrator.py` consumes. + +All findings below were verified by reading source and, where noted, by executing code directly +(`python3 -m nightwatch.main --dry-run`, targeted `grep` across the whole repo to confirm +call-site counts). No claim below is speculative extrapolation from a single read. + +--- + +## 1. Responsibility + +This domain is the system's central nervous system and safety brain: it loads and validates +configuration, starts/stops all hardware/software services in a defined order, tracks the +current observing session, and is supposed to guarantee that whenever conditions turn unsafe +(weather, a hung safety monitor, an operator Ctrl-C, an unhandled exception) the telescope mount +gets parked and the roof gets closed before anything else happens. A new engineer should think of +it as "the thing every other service reports to, and the last line of defense that closes the +roof no matter what else is going wrong." + +## 2. Key modules + +| File | Role | +|---|---| +| `nightwatch/main.py` | CLI entry point (`main()`, `async_main()`, `create_parser()`), signal handling (`GracefulShutdown`). | +| `nightwatch/orchestrator.py` | `Orchestrator` class — service registry, session state, event bus, command timeouts/cancellation, shutdown sequences. 3,446 lines, by far the largest and highest-churn file in the domain (10 commits per `10-history.md:46`). | +| `nightwatch/config.py` | Pydantic-based `NightwatchConfig` and sub-configs (`SafetyConfig`, `MountConfig`, etc.); `load_config()`; `SAFETY_ENV_OVERRIDE_ALLOWLIST` (config.py:90) — deny-by-default guard against disabling safety thresholds via env var. | +| `nightwatch/safety_interlock.py` | `SafetyInterlock` — pre-command gatekeeper (`check_command`) meant to veto unsafe commands before dispatch. **Not wired into production** (see §6). | +| `nightwatch/emergency_response.py` | `EmergencyResponse` — retrying emergency park/close sequences with alert escalation. **Not wired into production** (see §6). | +| `nightwatch/watchdog.py` | `WatchdogManager` — per-service heartbeat/timeout tracking with SAFE-004 hardware fail-safe (`_execute_safety_veto`, watchdog.py:505). `SafeStateHandler` (watchdog.py:733) is a second park/close implementation, also **not wired into production**. | +| `nightwatch/health.py` | `HealthChecker`, service-specific checks (mount/weather/voice/guider/power), `StartupSequence` for ordered startup. Contains a confirmed dependency-check bug (§6). | +| `nightwatch/logging_config.py` | `setup_logging()`, correlation-ID context vars, `log_exception`/`log_timing` helpers. | +| `nightwatch/exceptions.py` | `NightwatchError` hierarchy. Only `NightwatchError` and `ConfigurationError` are actually used elsewhere; the rest is unreferenced (§6). | +| `nightwatch/constants.py` | Centralized magic numbers, including a full parallel copy of safety thresholds that duplicates (and has drifted from) `SafetyConfig` — unused elsewhere (§6). | +| `nightwatch/types.py` | Shared type aliases/TypedDicts/Protocols; imported by nothing outside `__init__.py`'s convenience re-export (§6). | + +## 3. Data flow — startup and safety-shutdown, traced end to end + +**Startup:** `bin/nightwatch` → `python -m nightwatch.main` → `main()` (main.py:296) parses args, +calls `load_config()` (config.py:963, YAML via `yaml.safe_load` + `NIGHTWATCH_*` env overrides), +then `asyncio.run(async_main(...))` → `Orchestrator(config)` is constructed (orchestrator.py:1564) +→ `orchestrator.start()` (orchestrator.py:1912) iterates `ServiceRegistry` entries in registration +order, calling `service.start()` for each and marking `ServiceStatus.RUNNING`/`ERROR`; a required +service failing to start aborts startup. A background `_health_loop` task (orchestrator.py:2096) +is then spawned (30s poll of `service.is_running` + restart-policy dispatch), and +`async_main` blocks on `shutdown_event.wait()` until SIGINT/SIGTERM. + +**Safety-critical path (the one that matters most):** the actual continuous environmental +monitoring lives in `services/safety_monitor/monitor.py` (cross-domain). When it detects an +unsafe condition it (a) drives the enclosure closed itself and (b) invokes the callback +registered via `register_safety()` (orchestrator.py:1703-1729), which is `_on_safety_change` +(orchestrator.py:1823). That callback cancels `self._active_context` — a single +`CommandContext` from `nightwatch/cancellation.py` set by the tool-dispatch layer +(`tool_executor.py:351,406`, confirmed by grep) around whichever long-running tool call +(slew/capture/focus) is currently in flight. Separately, `WatchdogManager` (owned by +`Orchestrator.watchdog`, orchestrator.py:1602) tracks heartbeats from `safety_monitor` itself; if +`safety_monitor` goes silent for >90s, `_execute_safety_veto` (watchdog.py:505) closes the +enclosure directly (bypassing the orchestrator) and fires `_on_safety_veto` +(orchestrator.py:1865), which also cancels `_active_context` and emits `EventType.SAFETY_VETO`. + +**Shutdown (signal/exception path):** `main.py`'s `GracefulShutdown._handle_signal` sets an +`asyncio.Event`; `async_main` wakes up and calls `orchestrator.shutdown(safe=True)` +(orchestrator.py:1965) → `_safe_shutdown()` (orchestrator.py:2015) → parks mount / closes +enclosure via `registry.get_for_shutdown()` (a deliberate bypass of the RUNNING-only gate, +documented at orchestrator.py:1282-1311, so park/close still fire on an ERRORed mount) → saves a +JSON session log → stops all services in reverse registration order. On an unhandled exception in +`async_main`, `orchestrator.shutdown(safe=False)` is attempted as a last resort (main.py:289-292). + +## 4. External dependencies + +- **pydantic ≥2.0** — all configuration validation (`config.py`). **PyYAML** — `yaml.safe_load` + only (config.py:991), no unsafe YAML loading. +- **Cross-domain, this domain calls into:** `services/safety_monitor` (safety callback contract, + duck-typed `SafetyStatus` with `.is_safe`/`.action`/`.reasons`, documented at + orchestrator.py:1833-1837), `services/enclosure.RoofController` (`close()`/`open()`/`stop()`, + all `async def`), mount services (`park()`/`stop()`/`get_status()` — **contract is + inconsistent across implementations**, see §6), `services/mount_control/lx200.py` + (`LX200Client`, confirmed synchronous `stop()`/`park()`, lx200.py:530,580) vs. + `services/simulators/mount_simulator.py` (confirmed `async def stop()/park()`, + mount_simulator.py:131,143). +- **Cross-domain, calls in:** `nightwatch/tool_executor.py` (owned by another analyst) is the + only real caller of `Orchestrator.set_active_context`/`clear_active_context` + (tool_executor.py:351,406) — i.e. the actual command-dispatch entry point into this domain's + cancellation machinery. +- **`nightwatch/cancellation.py`** (another analyst's domain) supplies `CommandContext`/ + `CancelToken` — a deliberately *cooperative* cancellation primitive, explicitly designed to + replace hard `asyncio.Task.cancel()` (cancellation.py:11-22) because mid-write cancellation of + FITS files / mount serial transactions can corrupt state. `Orchestrator` also has an + **independent, older, hard-cancel command-tracking system** (`execute_cancellable`, + `cancel_command`, `_active_commands: Dict[str, asyncio.Task]`) that predates ARCH-003 and is + not the live path — see §6. + +## 5. Invariants and conventions + +- **Safety env-override allowlist is deny-by-default** (config.py:90, + `SAFETY_ENV_OVERRIDE_ALLOWLIST: Final[frozenset[str]] = frozenset()`): any `NIGHTWATCH_SAFETY_*` + env var not in the (currently empty) allowlist is rejected with a `logger.critical` and the + YAML/default value is kept. Well tested (`tests/unit/test_config.py:395-505`). +- **`get_running` vs. `get_for_shutdown`** (orchestrator.py:1254 vs. 1282): the documented, + deliberate rule is that the command-dispatch path only ever sees `RUNNING` services (ARCH-002), + while the three safety-shutdown call sites (`_safe_shutdown`, `end_session`, + `emergency_shutdown`) bypass that gate so park/close is attempted even on an `ERROR`ed service. + This is a real, well-documented, well-reasoned invariant — but it is only as good as callers + remembering to use the right accessor; a new safety-shutdown path that reaches for + `self.mount`/`self.registry.get_running(...)` instead of `get_for_shutdown(...)` would silently + skip parking an errored mount. +- **Cooperative cancellation, not task cancellation** (cancellation.py:1-38): long-running ops + are expected to poll `CommandContext`/`CancelToken` at safe iteration boundaries rather than be + killed via `Task.cancel()`. SAFE-001 depends on ordering: the safety monitor's + `_notify_callbacks` (which reaches `_on_safety_change`) must run **before** + `execute_action`/EMERGENCY_CLOSE (cancellation.py:29-38) so the cancel signal reaches in-flight + ops before the enclosure physically starts moving. +- **Single active context** (orchestrator.py:1772-1811): `set_active_context` is explicitly + documented as *not* supporting concurrent commands — a second call silently displaces the first + (logged at ERROR, not raised) and the displaced (older, possibly still-running) operation is no + longer reachable by a subsequent safety cancel. This is a known, TODO-tagged gap, not a bug I'm + reporting as new, but it is a real invariant callers must respect. +- **Config loading precedence**: CLI `--config` > `./nightwatch.yaml` > `~/.nightwatch/config.yaml` + > `/etc/nightwatch/config.yaml` > built-in pydantic defaults (config.py:963-996), env overrides + applied last except for the safety allowlist gate. + +## 6. MATRIX FLAGS + +### Security observations + +1. **CLI entry point cannot start (confirmed by execution).** `nightwatch/main.py:308` and `:325` + call `setup_logging(level=log_level)` / `setup_logging(level=config.log_level)`, but + `setup_logging()`'s only parameter is named `log_level` (`nightwatch/logging_config.py:185`). + I verified this empirically: + ``` + $ python3 -m nightwatch.main --dry-run + TypeError: setup_logging() got an unexpected keyword argument 'level' + ``` + Every invocation of the documented entry point (`nightwatch` CLI, `bin/nightwatch`, + `python -m nightwatch.main`) crashes before configuration is even validated. This is a + reliability finding first, but it is also security-relevant: **it means no deployment can + currently be running the code in this branch as its production entry point**, so any security + posture claims about "the system enforces X at startup" are unverifiable/moot until this is + fixed. `tests/integration/test_startup.py` imports `async_main` directly and never calls + `main()`, which is why this shipped undetected (`nightwatch/main.py:296-368` has zero test + coverage; confirmed via repo-wide grep for `test_main`). + +2. **Multiple safety subsystems are fully built, unit-tested in isolation, and never wired into + the running orchestrator.** This is the domain's most important structural risk: a reviewer + reading `safety_interlock.py`, `emergency_response.py`, or `watchdog.py`'s `SafeStateHandler` + would reasonably conclude the system has defense-in-depth. Grep across the entire repository + shows otherwise: + - `SafetyInterlock` (safety_interlock.py:154, the documented "gatekeeper for all telescope + commands") is only ever constructed in `tests/unit/test_safety_interlock.py` — zero + production call sites in `orchestrator.py`, `tool_executor.py`, or `main.py`. + - `EmergencyResponse` (emergency_response.py:100) is likewise only constructed in + `tests/unit/test_emergency_response.py`. `orchestrator.py`'s actual `emergency_shutdown()` + (orchestrator.py:2886) is an independent, simpler, single-attempt reimplementation with no + retries and no confirmation polling (contrast with `EmergencyResponse.emergency_park`'s + 3-retry, poll-for-`is_parked` loop). + - `SafeStateHandler` (watchdog.py:733) is likewise never constructed outside + `tests/unit/test_watchdog.py`; `WatchdogManager.set_safe_state_callback` is never called + anywhere in `orchestrator.py` (confirmed by grep), so `_check_services_once`'s + `if failed_critical and self._safe_state_callback:` branch (watchdog.py:459) never fires. + **Practical consequence: a critical-service failure for `mount`, `weather`, `power`, or + `enclosure` (all marked `critical=True` in `DEFAULT_CONFIGS`, watchdog.py:116-190) that is + NOT specifically a `safety_monitor` heartbeat timeout produces only a `logger.critical` log + line — no automatic park or roof close is triggered.** Only the `SAFETY_MONITOR` service + type has a wired hardware fail-safe (SAFE-004, via `set_safety_veto_callback`, + orchestrator.py:1603). + - Even if `SafeStateHandler` or `EmergencyResponse` were wired up, both call + `self._roof.get_state()` (emergency_response.py:261, watchdog.py:834) to poll for enclosure + closure — but the real `services/enclosure/roof_controller.py` exposes `state` as a + **property**, not a `get_state()` method (roof_controller.py:539). This would raise + `AttributeError` on first use against production hardware; it is currently masked only + because both modules are tested exclusively against `MagicMock()` roof fixtures, which + fabricate any attribute requested (`tests/unit/test_emergency_response.py:49-54`). + - `EventBus` (orchestrator.py:507, a ~350-line pub/sub implementation with subscription + history/stats) and `CommandQueue`/`CommandPriority` (orchestrator.py:109-374, a + priority-ordered command queue meant to let `EMERGENCY` commands preempt in-flight `NORMAL` + ones) are both fully implemented and exported in `__all__`, but `Orchestrator` never + instantiates either — it uses its own smaller ad hoc `_event_listeners` dict instead. The + elaborate emergency-preemption priority model described in `CommandPriority`'s docstring + (orchestrator.py:109-123) does not actually preempt anything in the live system. + - **Net effect for the security/safety auditors:** treat `SafetyInterlock`, `EmergencyResponse`, + `SafeStateHandler`, `EventBus`, and the priority `CommandQueue` as **not part of the + enforced safety boundary**. The actual enforced boundary today is: `services/safety_monitor` + (cross-domain) driving the roof directly + `_on_safety_change`/`_on_safety_veto` + cancellation + the manual park/close blocks inline in `_safe_shutdown`/`end_session`/ + `emergency_shutdown`. + +3. **The "cancel all commands" step of both shutdown paths is a no-op.** + `graceful_shutdown()` (orchestrator.py:2837-2843) and `emergency_shutdown()` + (orchestrator.py:2898-2903) both cancel commands tracked in `self._active_commands` (populated + only by `execute_cancellable`, orchestrator.py:2555). Repo-wide grep confirms + `execute_cancellable` has exactly one call site — its own definition — and is never invoked by + `tool_executor.py` or anything else. The real in-flight command is tracked via + `self._active_context` (a single `CommandContext`, set by `tool_executor.py:351`), which + neither shutdown path references or cancels directly (only the safety-monitor-driven + `_on_safety_change`/`_on_safety_veto` callbacks do). Practically: if `emergency_shutdown()` is + invoked outside of the safety-monitor path (e.g., a future direct operator/API trigger), the + log message "Immediately cancel all commands" is misleading — the actual running voice-tool + command is left untouched while mount and enclosure are being driven underneath it. + `emergency_shutdown()` also unconditionally `return True`s (orchestrator.py:2945) even when + both the mount-park and enclosure-close `except` blocks were hit — callers cannot detect + partial failure from the return value; they must know to check logs. + +4. **Duplicate, incompatible `SafetyInterlockError` classes.** `nightwatch/exceptions.py:263` + defines `SafetyInterlockError(SafetyError)` (part of the `NightwatchError` hierarchy, + constructor `(message, interlock_name, required_state, current_state)`). + `nightwatch/safety_interlock.py:552` independently defines its **own** + `SafetyInterlockError(Exception)` (constructor `(message, status)`) — unrelated to the first + by inheritance. `safety_interlock.py`'s `require_safety_check` decorator (safety_interlock.py: + 514-549) raises the local one. Code that did `except nightwatch.exceptions.SafetyInterlockError` + expecting to catch safety-interlock failures would not catch this. In practice this is currently + low-impact because (a) `SafetyInterlock`/`require_safety_check` aren't wired into production + (see finding 2) and (b) grep confirms nothing in `nightwatch/` or `services/` imports + `SafetyInterlockError` from `exceptions.py` at all — but it is a real footgun for anyone who + does complete the wiring later, and evidence that the exception hierarchy in `exceptions.py` + is largely aspirational (see Quality §, dead-code item). +5. **Config parsing is safe.** `load_config()` uses `yaml.safe_load` (config.py:991), not + `yaml.load`, so no arbitrary-object-deserialization risk from a malicious config file. + Env-var override type coercion (config.py:947-956) only does bool/int/float parsing, no + `eval`/`exec`; no injection vector found. +6. **Filesystem writes are narrow and non-attacker-controlled in the reviewed code.** Log file + path (`logging_config.py:232-245`, `RotatingFileHandler`) and session-log path + (`orchestrator.py:2062-2094`, writes to `Path(self.config.data_dir if hasattr(...) else "logs")` + joined with `session_{self.session.session_id}.json`) both come from operator-controlled + config/CLI, not from voice/LLM input reaching this domain directly — no path-traversal vector + observed here. Note `hasattr(self.config, 'data_dir')` is always `False` in practice: pydantic's + `NightwatchConfig` (config.py:824-866) has no `data_dir` field, so session logs always land in + a `logs/` directory relative to the process's current working directory regardless of + deployment config — a portability/operational gap more than a security one, but worth the + deployment-focused auditor's attention (systemd units should set `WorkingDirectory=` or this + silently writes into `/` or wherever the unit starts). + +### Quality observations + +1. **Recurring "designed, tested-in-isolation, never wired" pattern.** Across this domain alone, + at least five substantial, individually well-documented subsystems have zero production call + sites: `SafetyInterlock`, `EmergencyResponse`, `watchdog.SafeStateHandler`, `EventBus`, and the + `CommandQueue`/`CommandPriority` preemption system (all cited with line numbers under Security + §, finding 2, to avoid duplication). The same pattern shows up in supporting modules: + `nightwatch/constants.py`'s entire safety-threshold section (`WIND_LIMIT_MPH`, + `HUMIDITY_LIMIT_PERCENT`, `TEMP_MIN_F`, etc., constants.py:34-61) is imported by nothing outside + `__init__.py`'s two unrelated convenience names — and it has already **drifted** from the + real source of truth (`SafetyConfig` in config.py has 3-tier warning/park/emergency thresholds + per parameter; `constants.py` has a single flat value per parameter, e.g. + `WIND_LIMIT_MPH = 25.0` vs. `SafetyConfig.wind_limit_warning/park/emergency = 20/25/30`). + Similarly `nightwatch/types.py`'s ~40 shared types/Protocols are used by nothing outside + `nightwatch/__init__.py`'s 5-name convenience re-export — `services/` and `voice/` define their + own local equivalents instead. `nightwatch/exceptions.py`'s device/command/catalog exception + subclasses (`DeviceBusyError`, `CommandTimeoutError`, `ObjectNotFoundError`, etc.) are likewise + unreferenced outside the module itself and docs. This suggests architecture/scaffolding work + consistently outpacing integration — consistent with `10-history.md`'s finding of a 16:1 + feature-to-test commit ratio and single-author bus factor (`10-history.md:301-321`). +2. **Confirmed logic bug in `StartupSequence.run()`'s dependency check** + (`nightwatch/health.py:686-692`): + ```python + for dep in dependencies: + if dep not in self._started_services: + logger.warning(f"Skipping {service_name}: dependency '{dep}' not started") + continue # <-- only continues the inner `for dep in dependencies` loop + # falls through to check service_name's health regardless + ``` + The `continue` only affects the inner dependency loop, not the outer per-service loop, so the + logged "Skipping {service_name}" never actually happens — the service's health check runs + anyway even when a declared dependency (e.g. `guider` depends on `mount`) never started. No + test exercises the failing-dependency path (`tests/integration/test_startup.py`'s fixture + always configures `mount` as a healthy simulator), so this has never been caught. +3. **`nightwatch/orchestrator.py` module docstring references a nonexistent method.** Line 46: + `response = await orchestrator.process_command("slew to M31")` — there is no + `process_command` method anywhere on `Orchestrator` (confirmed by grep); command dispatch + actually happens via `tool_executor.py` calling `set_active_context`/`clear_active_context` + directly. Minor, but a new-engineer-facing docstring should not describe an API that doesn't + exist. +4. **Two overlapping health/liveness loops with different mechanisms and cadences.** + `Orchestrator._health_loop` (orchestrator.py:2096) polls `service.is_running` every 30s and + drives the restart-policy state machine (`ServiceRegistry.should_restart`/`get_restart_delay`, + orchestrator.py:1397-1465, a well-designed exponential-backoff restart system). Separately, + `WatchdogManager._check_services` (watchdog.py:420) runs every 5s on a heartbeat/timeout model + that services must explicitly call (`watchdog.heartbeat(service_type)`). Nothing in + `orchestrator.py` calls `self.watchdog.heartbeat(...)` for the standard services (mount, + weather, camera, etc.) — grep shows the only production heartbeat caller pattern is the + SAFE-004 safety_monitor path. So `WatchdogManager`'s per-service `DEFAULT_CONFIGS` entries for + `MOUNT`, `WEATHER`, `CAMERA`, `GUIDER`, `FOCUSER`, `ENCLOSURE`, `POWER` (watchdog.py:116-172) + likely never receive a heartbeat in production and would sit at `ServiceState.UNKNOWN` + forever (their `check_timeout()` returns `False` when `last_heartbeat is None`, + watchdog.py:252-253) — meaning the watchdog's restart/failure callbacks for those services are + also effectively inert, and `_health_loop`'s simpler `is_running`-poll restart mechanism is + the one actually doing the work. Worth confirming with whoever owns the individual `services/` + modules whether `watchdog.heartbeat()` is called from inside those services (out of this + domain's scope to verify further). +5. **Test coverage gaps.** `nightwatch/main.py` (372 lines, the literal CLI entry point) has zero + dedicated unit tests; the only test file touching `nightwatch.main` + (`tests/integration/test_startup.py`) imports `async_main`, `create_parser`, `GracefulShutdown` + directly and never calls `main()` — which is exactly why the `setup_logging(level=...)` bug + above shipped. `nightwatch/health.py`'s `StartupSequence` has 3 tests, none of which exercise a + failing/unstarted dependency. `nightwatch/orchestrator.py`'s `execute_cancellable`/ + `cancel_command`/`cancel_all_commands`/`get_active_commands` have zero tests anywhere in the + repo (confirmed by grep across `tests/`). By contrast, `config.py`'s safety-allowlist logic + (§5) and `watchdog.py`'s SAFE-004 fail-safe path (`tests/unit/test_safe_004_watchdog_failsafe.py`, + 345 lines) are genuinely well tested — coverage quality in this domain is bimodal: the + recently-touched safety-critical paths (SAFE-001/002/004, per `10-history.md`'s May 2026 + hardening push) are carefully tested, while older/peripheral modules (`main.py`, `health.py`, + the pre-ARCH-003 cancellation system) are not. +6. **Dangling documentation reference.** `config.py:81-82` refers to "the CLAUDE.md + prohibited-edits list" as the governance mechanism restricting edits to `safety_monitor`; no + `CLAUDE.md` file exists anywhere in this repository (confirmed via repo-wide search). Either + the file was removed/never committed, or the governance process it describes doesn't actually + exist yet — worth flagging for the chief architect / process reviewer. +7. **`orchestrator.py` is a 3,446-line single file** covering service registry, session state, + event bus, metrics, command timeout/cancellation, restart policy, and shutdown sequencing all + in one class (`Orchestrator`) plus ~10 supporting classes. This is the domain's obvious + complexity hotspot and highest-churn file (10 commits, single author, per `10-history.md:46,99`) + — any future change here has no second reviewer's mental model to check against, and the file + is large enough that (as demonstrated above) entire subsystems within it can go unused without + anyone noticing. +8. **Minor:** `nightwatch/emergency_response.py`'s `emergency_park` and + `watchdog.SafeStateHandler.enter_safe_state` call `self._mount.stop()` / + `self._mount.park()` without `await`. This is correct for the real `LX200Client` (confirmed + synchronous, `services/mount_control/lx200.py:530,580`) but would silently no-op (coroutine + created, never awaited, and a coroutine object is truthy so `if success:` proceeds as if it + succeeded) against an async mount implementation such as + `services/simulators/mount_simulator.py` (confirmed `async def stop/park`, + mount_simulator.py:131,143). Since both modules are unreachable from production today (finding + 2 above), this is latent rather than active, but it would need fixing before either module + could be safely wired up, and it's the kind of duck-typed contract mismatch that unit tests + using `MagicMock()` (not `AsyncMock()`) will never catch. + +--- + +## Cross-domain touchpoints (for other analysts) + +- **Voice & NLP / Command Execution domains:** `nightwatch/voice_pipeline.py` physically lives in + this directory but is audio/STT/TTS/wake-word plumbing (`WakeWordDetector`, `AudioCapture`, + `STTInterface`, `TTSInterface`, `VoicePipeline`) with no direct coupling to `orchestrator.py` + or `cancellation.py` (confirmed by grep) — it is not covered in depth here and should be treated + as that domain's territory despite the directory boundary. +- **`nightwatch/tool_executor.py`** (Command Execution domain) is the sole real caller of + `Orchestrator.set_active_context`/`clear_active_context` — any change to the ARCH-003 + cancellation contract in `orchestrator.py` must be coordinated with that file. +- **`services/safety_monitor/monitor.py`** (Astronomy & Hardware Services domain) is the actual + live safety brain this domain's callbacks (`_on_safety_change`, `register_safety`) depend on; + its `SafetyStatus` duck-typed contract (`.is_safe`, `.action.name`, `.reasons`) is assumed but + not enforced by a shared type in this domain. +- **`services/enclosure/roof_controller.py`** and **`services/mount_control/`** — the interface + mismatches noted above (`get_state()` vs. `state` property; sync vs. async mount clients) are + genuinely cross-domain contract bugs; whoever owns `services/` should be aware their real + `RoofController`/mount APIs don't match what `nightwatch/emergency_response.py` and + `nightwatch/watchdog.py` expect. +- **`nightwatch/llm_client.py`** (LLM Client domain) reads `ANTHROPIC_API_KEY`/`OPENAI_API_KEY` + directly from `os.environ` (llm_client.py:454,572) rather than through `NightwatchConfig`'s + `LLMConfig` (which has no `api_key`/`endpoint` fields at all, contrary to what + `00-inventory.md:291` describes) — API key handling bypasses this domain's config validation + and env-override-allowlist machinery entirely. Worth the security auditor's attention even + though the file itself is out of scope here. 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. 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