From 795aa2f71307b5f5bbb434e20dc168f97388e85f Mon Sep 17 00:00:00 2001 From: 44r0n7 <44r0n7@users.noreply.git.44r0n.cc> Date: Sat, 18 Apr 2026 11:55:18 -0400 Subject: [PATCH] refactor: harden internal daemon entrypoint and cleanup observations Remove the internal daemon subcommand from the public CLI surface, start the daemon via an internal env trigger, and ensure generated completions/help never expose internal entrypoints. Also finish the pending observation cleanups and docs updates, including config/key deduplication, registry matching cleanup, and remaining roadmap/project map staleness fixes. --- .gitignore | 2 ++ PROJECT_MAP.md | 3 +- ROADMAP.md | 2 +- claude-observations-plan.md | 50 ------------------------------- src/adapters/mod.rs | 41 ++++++++++++++++++++++++++ src/adapters/roku/mod.rs | 2 ++ src/api/mod.rs | 2 +- src/cli/mod.rs | 59 ++++++++++++++++++++++++++++--------- src/daemon/config.rs | 28 +++++++++++++----- src/daemon/mod.rs | 43 ++------------------------- src/daemon/registry.rs | 13 ++------ src/main.rs | 11 ++++++- tests/http_api.rs | 4 +-- 13 files changed, 131 insertions(+), 129 deletions(-) delete mode 100644 claude-observations-plan.md diff --git a/.gitignore b/.gitignore index 4ccbd93..5a9fe33 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,5 @@ /cache/ /tvctl-agent-base.zip .DS_Store +node-compile-cache/ +phantomjs/ diff --git a/PROJECT_MAP.md b/PROJECT_MAP.md index 707998f..1d85c0a 100644 --- a/PROJECT_MAP.md +++ b/PROJECT_MAP.md @@ -343,8 +343,9 @@ roku_password = "" ## What Has NOT Been Started -- HTTP route handlers and request validation - CI/CD configuration + - Must pin Rust toolchain >= 1.85 because `edition = "2024"` is in use. +- Cross-compile validation for `x86_64` and `aarch64` - Release/packaging --- diff --git a/ROADMAP.md b/ROADMAP.md index 3c29921..ea80a72 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -125,7 +125,7 @@ _Goal: Ready for real use._ - [x] 2026-04-15 — `cargo clippy` clean - [x] 2026-04-15 — `cargo test` passing - [ ] Cross-compile test (x86_64 + aarch64) -- [ ] GitHub Actions CI (build + clippy + test) +- [ ] GitHub Actions CI (build + clippy + test, Rust toolchain >= 1.85 for edition 2024) - [ ] First binary release --- diff --git a/claude-observations-plan.md b/claude-observations-plan.md deleted file mode 100644 index 0b56656..0000000 --- a/claude-observations-plan.md +++ /dev/null @@ -1,50 +0,0 @@ -# Claude Observations Plan - -Reference source: `claude-observations.txt` - -This file is a fact-checked implementation plan based on the numbered observations -in `claude-observations.txt`. It reflects the current worktree state on 2026-04-18, -not just the last committed `main`. - -## Fact Check Summary - -| Observation | Status | Fact Check | Planned Action | -|---|---|---|---| -| 1. `format_tv_key` duplicates normalized key mapping | Confirmed | `src/daemon/mod.rs` still has a 33-arm `format_tv_key` helper and one call site in the `SendKey` action detail path. `src/adapters/mod.rs` still has a separate `parse_normalized_tv_key` table and `TvKey` keeps `#[serde(rename_all = "kebab-case")]`. | Remove the duplicate reverse table. Preferred fix: add a canonical string conversion on `TvKey` and use it from the daemon response text. | -| 2. `DeviceRegistry::remove` duplicates `find` matching logic | Confirmed | `src/daemon/registry.rs` has `find()` with inline UUID/name matching and `remove()` with separate `matches_target(...)` logic. | Refactor `remove()` to reuse `find()` or otherwise share one target-matching implementation. Delete the duplicate matcher after the refactor. | -| 3. `parse_bool` is a no-value wrapper | Confirmed | `src/daemon/config.rs` still has `fn parse_bool(...) { parse_value(...) }` and uses it in `set_value(...)`. | Delete `parse_bool` and switch the bool assignments to `parse_value(...)`. | -| 4. HTTP API loops back through the Unix socket even though it is in-process | Completed locally on 2026-04-18 | Refactored `src/api/mod.rs` to call `daemon::execute_request(...)` directly and added an in-process HTTP regression test that succeeds without starting the daemon Unix socket server. | No further change needed unless follow-up regressions are found. | -| 5. `PROJECT_MAP.md` is stale about Milestone 5 / HTTP status | Stale in current worktree | In the current worktree, `PROJECT_MAP.md` is already locally updated to `Last updated: 2026-04-18` and `Phase: Milestone 6 in progress`. This observation was valid against an older snapshot, not the current tree. | No repo change needed for this item unless those local doc edits are reverted before commit. | -| 6. Missing cleanup for `node-compile-cache/` and `phantomjs/` | Partially confirmed | Neither directory exists in the current repo tree, but `.gitignore` does not currently list them. The observation appears archive-related rather than evidence of a current tracked repo problem. | Add preventive ignore entries later if desired. No tracked-file cleanup is needed unless those directories reappear. | -| 7. `get_value()` allocates a full `BTreeMap` for one lookup | Confirmed | `src/daemon/config.rs` still implements `get_value()` as `self.entries().remove(key)`. | Rewrite `get_value()` as a direct `match` over stable keys so it no longer allocates the full map for single-key reads. | -| 8. `roku_key_paths` unsupported-key intent is unclear | Confirmed | `src/adapters/roku/mod.rs` still falls through from valid `Ok(...)` arms into a bottom branch where `Stop`, `Skip`, `Power`, `PowerOn`, and `Options` are only used to build `InvalidKey(...)`. | Add a clarifying comment above the fall-through block. No behavior change needed. | -| 9. `RefreshAppsBody` optional `clear` handling can be simplified | Cosmetic | `src/api/mod.rs` still uses `body.map(|value| value.clear).unwrap_or(false)`. | Fold into a smaller expression while touching the HTTP module for higher-priority work. Do not treat this as a standalone task. | -| 10. Rust 2024 edition note | Informational only | `Cargo.toml` already uses `edition = "2024"`. No code issue is implied. | No repo change needed now. Keep the note in mind when CI is added so the toolchain is new enough. | - -## Execution Order - -The future implementation order should stay low-risk first and defer the heavier -HTTP refactor until the small cleanups are merged or at least locally stable. - -1. Remove duplication and low-risk cleanup - - Observation 3: delete `parse_bool` - - Observation 8: add unsupported-key comment in `roku_key_paths` - - Observation 9: simplify `RefreshAppsBody` boolean handling if already editing `src/api/mod.rs` - -2. Remove duplicated matching / key-string tables - - Observation 2: unify `DeviceRegistry` target matching - - Observation 1: replace `format_tv_key` with one canonical `TvKey` string path - - Observation 7: rewrite `get_value()` to avoid `entries()` allocation - -3. Perform the medium-risk HTTP architecture cleanup - - Observation 4: replace Unix-socket loopback in the HTTP API with direct `execute_request(...)` - - Add focused regression coverage around HTTP handlers after this change because it changes the transport path, not just formatting - -4. Preventive hygiene - - Observation 6: add `.gitignore` entries for `node-compile-cache/` and `phantomjs/` if we want to harden against future archive/worktree leakage - -## Constraints For Future Work - -- Keep this work separate from the current uncommitted logging-related changes already in the worktree unless the user explicitly asks to batch them together. -- Do not assume all Claude observations are still valid against the live tree. Re-check each item immediately before implementation. -- Preserve current user-facing behavior where possible, especially for daemon/API error payloads and CLI help/output. diff --git a/src/adapters/mod.rs b/src/adapters/mod.rs index 7a451c1..5938d6d 100644 --- a/src/adapters/mod.rs +++ b/src/adapters/mod.rs @@ -174,6 +174,47 @@ pub enum TvKey { Literal(String), } +impl TvKey { + /// Return the normalized key name used by CLI and API payloads. + pub fn normalized_name(&self) -> String { + match self { + TvKey::Home => "home".to_string(), + TvKey::Back => "back".to_string(), + TvKey::Up => "up".to_string(), + TvKey::Down => "down".to_string(), + TvKey::Left => "left".to_string(), + TvKey::Right => "right".to_string(), + TvKey::Select => "select".to_string(), + TvKey::Play => "play".to_string(), + TvKey::Pause => "pause".to_string(), + TvKey::PlayPause => "play-pause".to_string(), + TvKey::Stop => "stop".to_string(), + TvKey::Rewind => "rewind".to_string(), + TvKey::FastForward => "fast-forward".to_string(), + TvKey::Replay => "replay".to_string(), + TvKey::Skip => "skip".to_string(), + TvKey::ChannelUp => "channel-up".to_string(), + TvKey::ChannelDown => "channel-down".to_string(), + TvKey::VolumeUp => "volume-up".to_string(), + TvKey::VolumeDown => "volume-down".to_string(), + TvKey::Mute => "mute".to_string(), + TvKey::Power => "power".to_string(), + TvKey::PowerOn => "power-on".to_string(), + TvKey::PowerOff => "power-off".to_string(), + TvKey::InputHdmi1 => "input-hdmi1".to_string(), + TvKey::InputHdmi2 => "input-hdmi2".to_string(), + TvKey::InputHdmi3 => "input-hdmi3".to_string(), + TvKey::InputHdmi4 => "input-hdmi4".to_string(), + TvKey::InputAv => "input-av".to_string(), + TvKey::InputTuner => "input-tuner".to_string(), + TvKey::Search => "search".to_string(), + TvKey::Info => "info".to_string(), + TvKey::Options => "options".to_string(), + TvKey::Literal(text) => format!("literal:{text}"), + } + } +} + /// A structured error produced by adapter implementations. #[derive(Debug, Error)] pub enum TvError { diff --git a/src/adapters/roku/mod.rs b/src/adapters/roku/mod.rs index 2e0f4b8..bc530f7 100644 --- a/src/adapters/roku/mod.rs +++ b/src/adapters/roku/mod.rs @@ -762,6 +762,8 @@ fn roku_key_paths(key: &TvKey) -> Result> { .map(|character| format!("Lit_{}", urlencoding::encode(&character.to_string()))) .collect()); } + // These keys are part of the normalized key model but Roku ECP does not + // provide documented equivalents for them. TvKey::Stop => "stop", TvKey::Skip => "skip", TvKey::Power => "power", diff --git a/src/api/mod.rs b/src/api/mod.rs index dad8137..58f292a 100644 --- a/src/api/mod.rs +++ b/src/api/mod.rs @@ -154,7 +154,7 @@ async fn refresh_apps( daemon, DaemonRequest::RefreshApps { device: Some(id), - clear: body.map(|value| value.clear).unwrap_or(false), + clear: body.is_some_and(|value| value.clear), }, ) .await diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 80caffe..fbf3e2a 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -103,9 +103,6 @@ pub enum Command { #[arg(value_enum)] shell: CompletionShell, }, - /// Internal daemon entry point used by `tvctl daemon start`. - #[command(hide = true, name = "__daemon_serve")] - InternalDaemonServe, } #[derive(Debug, Clone, Copy, ValueEnum)] @@ -323,9 +320,6 @@ pub async fn run() -> Result<(), CliError> { }; match command { - Command::InternalDaemonServe => daemon::serve().await.map_err(|error| { - CliError::new(error.to_string(), "Inspect the daemon logs and retry.") - }), Command::Daemon { command } => handle_daemon_command(&cli, command).await, Command::Device { command } => handle_device_command(&cli, command).await, Command::App { command } => handle_app_command(&cli, command).await, @@ -348,12 +342,15 @@ fn handle_completion_command(shell: CompletionShell) -> Result<(), CliError> { } fn print_completions(generator: G, command: &mut clap::Command) { - generate( - generator, - command, - command.get_name().to_string(), - &mut std::io::stdout(), - ); + write_completions(generator, command, &mut std::io::stdout()); +} + +fn write_completions( + generator: G, + command: &mut clap::Command, + writer: &mut W, +) { + generate(generator, command, command.get_name().to_string(), writer); } fn build_cli_command() -> clap::Command { @@ -690,7 +687,7 @@ async fn daemon_start(cli: &Cli) -> Result<(), CliError> { })?; TokioCommand::new(exe) - .arg("__daemon_serve") + .env(daemon::INTERNAL_DAEMON_ENV, "1") .stdin(Stdio::null()) .stdout(Stdio::null()) .stderr(Stdio::null()) @@ -1456,7 +1453,8 @@ fn default_marker(device: &Device) -> &'static str { fn render_systemd_unit(exe: &std::path::Path) -> String { format!( - "[Unit]\nDescription=tvctl daemon\nAfter=network.target\n\n[Service]\nType=simple\nExecStart={} __daemon_serve\nRestart=on-failure\nRestartSec=2\n\n[Install]\nWantedBy=default.target\n", + "[Unit]\nDescription=tvctl daemon\nAfter=network.target\n\n[Service]\nType=simple\nEnvironment={}=1\nExecStart={}\nRestart=on-failure\nRestartSec=2\n\n[Install]\nWantedBy=default.target\n", + daemon::INTERNAL_DAEMON_ENV, exe.display() ) } @@ -1473,3 +1471,36 @@ fn parse_tv_key(input: &str) -> Result { ) }) } + +#[cfg(test)] +mod tests { + use super::*; + + fn completion_output(shell: Shell) -> String { + let mut command = build_cli_command(); + let mut output = Vec::new(); + write_completions(shell, &mut command, &mut output); + String::from_utf8(output).expect("completion output should be utf-8") + } + + #[test] + fn completions_do_not_expose_internal_daemon_entrypoint() { + for shell in [Shell::Bash, Shell::Zsh, Shell::Fish] { + let output = completion_output(shell); + assert!( + !output.contains("__daemon_serve"), + "completion for {shell:?} should not include internal daemon entrypoint" + ); + } + } + + #[test] + fn help_does_not_expose_internal_daemon_entrypoint() { + let mut command = build_cli_command(); + let help = command.render_help().to_string(); + assert!( + !help.contains("__daemon_serve"), + "help output should not include internal daemon entrypoint" + ); + } +} diff --git a/src/daemon/config.rs b/src/daemon/config.rs index c42ba46..14f25fb 100644 --- a/src/daemon/config.rs +++ b/src/daemon/config.rs @@ -84,21 +84,37 @@ impl TvctlConfig { /// Return one flattened config value by stable key. pub fn get_value(&self, key: &str) -> Option { - self.entries().remove(key) + match key { + "daemon.socket" => Some(self.daemon.socket.clone()), + "daemon.http_enabled" => Some(self.daemon.http_enabled.to_string()), + "daemon.http_port" => Some(self.daemon.http_port.to_string()), + "daemon.http_host" => Some(self.daemon.http_host.clone()), + "daemon.log_level" => Some(self.daemon.log_level.clone()), + "discovery.auto_discover" => Some(self.discovery.auto_discover.to_string()), + "discovery.interval_secs" => Some(self.discovery.interval_secs.to_string()), + "discovery.timeout_secs" => Some(self.discovery.timeout_secs.to_string()), + "devices.default" => Some(self.devices.default.clone()), + "remote.roku_key_mode" => Some(self.remote.roku_key_mode.clone()), + "remote.roku_press_duration_ms" => Some(self.remote.roku_press_duration_ms.to_string()), + "dev.enabled" => Some(self.dev.enabled.to_string()), + "dev.roku_username" => Some(self.dev.roku_username.clone()), + "dev.roku_password" => Some(self.dev.roku_password.clone()), + _ => None, + } } /// Set one flattened config value by stable key. pub fn set_value(&mut self, key: &str, value: &str) -> anyhow::Result<()> { match key { "daemon.socket" => self.daemon.socket = value.to_string(), - "daemon.http_enabled" => self.daemon.http_enabled = parse_bool(key, value)?, + "daemon.http_enabled" => self.daemon.http_enabled = parse_value(key, value)?, "daemon.http_port" => self.daemon.http_port = parse_value(key, value)?, "daemon.http_host" => self.daemon.http_host = value.to_string(), "daemon.log_level" => { validate_log_level(value)?; self.daemon.log_level = value.to_string(); } - "discovery.auto_discover" => self.discovery.auto_discover = parse_bool(key, value)?, + "discovery.auto_discover" => self.discovery.auto_discover = parse_value(key, value)?, "discovery.interval_secs" => self.discovery.interval_secs = parse_value(key, value)?, "discovery.timeout_secs" => self.discovery.timeout_secs = parse_value(key, value)?, "devices.default" => self.devices.default = value.to_string(), @@ -106,7 +122,7 @@ impl TvctlConfig { "remote.roku_press_duration_ms" => { self.remote.roku_press_duration_ms = parse_value(key, value)? } - "dev.enabled" => self.dev.enabled = parse_bool(key, value)?, + "dev.enabled" => self.dev.enabled = parse_value(key, value)?, "dev.roku_username" => self.dev.roku_username = value.to_string(), "dev.roku_password" => self.dev.roku_password = value.to_string(), other => bail!("unknown config key '{other}'"), @@ -301,10 +317,6 @@ fn current_uid() -> u32 { unsafe { libc::geteuid() } } -fn parse_bool(key: &str, value: &str) -> anyhow::Result { - parse_value(key, value) -} - fn parse_value(key: &str, value: &str) -> anyhow::Result where T: std::str::FromStr, diff --git a/src/daemon/mod.rs b/src/daemon/mod.rs index a491f75..c10d2a2 100644 --- a/src/daemon/mod.rs +++ b/src/daemon/mod.rs @@ -34,11 +34,12 @@ use tracing::warn; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; -use crate::adapters::{Device, TvKey}; +use crate::adapters::Device; use crate::api; use crate::logging; pub type SharedDaemon = Arc>; +pub const INTERNAL_DAEMON_ENV: &str = "TVCTL_INTERNAL_DAEMON"; /// The long-lived tvctld process. #[derive(Debug)] @@ -644,7 +645,7 @@ pub(crate) async fn execute_request( Ok(device) => device, Err(response) => return (response, false), }; - let detail = format!("Sent key '{}' to {}.", format_tv_key(&key), device.name); + let detail = format!("Sent key '{}' to {}.", key.normalized_name(), device.name); match guard.adapters.key(&device, key).await { Ok(()) => (action_success(device, detail), false), Err(error) => ( @@ -834,44 +835,6 @@ async fn send_key_sequence( Ok(()) } -fn format_tv_key(key: &TvKey) -> String { - match key { - TvKey::Home => "home".to_string(), - TvKey::Back => "back".to_string(), - TvKey::Up => "up".to_string(), - TvKey::Down => "down".to_string(), - TvKey::Left => "left".to_string(), - TvKey::Right => "right".to_string(), - TvKey::Select => "select".to_string(), - TvKey::Play => "play".to_string(), - TvKey::Pause => "pause".to_string(), - TvKey::PlayPause => "play-pause".to_string(), - TvKey::Stop => "stop".to_string(), - TvKey::Rewind => "rewind".to_string(), - TvKey::FastForward => "fast-forward".to_string(), - TvKey::Replay => "replay".to_string(), - TvKey::Skip => "skip".to_string(), - TvKey::ChannelUp => "channel-up".to_string(), - TvKey::ChannelDown => "channel-down".to_string(), - TvKey::VolumeUp => "volume-up".to_string(), - TvKey::VolumeDown => "volume-down".to_string(), - TvKey::Mute => "mute".to_string(), - TvKey::Power => "power".to_string(), - TvKey::PowerOn => "power-on".to_string(), - TvKey::PowerOff => "power-off".to_string(), - TvKey::InputHdmi1 => "input-hdmi1".to_string(), - TvKey::InputHdmi2 => "input-hdmi2".to_string(), - TvKey::InputHdmi3 => "input-hdmi3".to_string(), - TvKey::InputHdmi4 => "input-hdmi4".to_string(), - TvKey::InputAv => "input-av".to_string(), - TvKey::InputTuner => "input-tuner".to_string(), - TvKey::Search => "search".to_string(), - TvKey::Info => "info".to_string(), - TvKey::Options => "options".to_string(), - TvKey::Literal(text) => format!("literal:{text}"), - } -} - async fn run_discovery(daemon: &mut Daemon) -> anyhow::Result> { let discovery = daemon.discovery.clone(); let devices = discovery.discover_all(&mut daemon.registry).await?; diff --git a/src/daemon/registry.rs b/src/daemon/registry.rs index 8011770..e34ef29 100644 --- a/src/daemon/registry.rs +++ b/src/daemon/registry.rs @@ -97,10 +97,8 @@ impl DeviceRegistry { /// Remove a device by UUID or case-insensitive name. pub fn remove(&mut self, target: &str) -> Option { - let index = self - .devices - .iter() - .position(|device| matches_target(device, target))?; + let id = self.find(target)?.id; + let index = self.devices.iter().position(|device| device.id == id)?; let removed = self.devices.remove(index); self.ensure_default(); Some(removed) @@ -319,13 +317,6 @@ impl AdapterRegistry { } } -fn matches_target(device: &Device, target: &str) -> bool { - let target_uuid = Uuid::parse_str(target).ok(); - let normalized = target.to_ascii_lowercase(); - target_uuid.map(|uuid| device.id == uuid).unwrap_or(false) - || device.name.to_ascii_lowercase() == normalized -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/main.rs b/src/main.rs index 73bcaa5..5a77cb7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -6,7 +6,16 @@ async fn main() { std::process::exit(1); } - if let Err(error) = tvctl::cli::run().await { + let result = if std::env::var(tvctl::daemon::INTERNAL_DAEMON_ENV) + .map(|value| value == "1") + .unwrap_or(false) + { + tvctl::daemon::serve().await + } else { + tvctl::cli::run().await.map_err(anyhow::Error::from) + }; + + if let Err(error) = result { eprintln!("{error}"); std::process::exit(1); } diff --git a/tests/http_api.rs b/tests/http_api.rs index 53d1033..e6fa03e 100644 --- a/tests/http_api.rs +++ b/tests/http_api.rs @@ -14,7 +14,7 @@ use tokio::{net::TcpListener as TokioTcpListener, sync::Mutex, task::JoinHandle, use tvctl::{ adapters::Device, daemon::{ - Daemon, SharedDaemon, + Daemon, INTERNAL_DAEMON_ENV, SharedDaemon, cache::AppCacheStore, config::{DaemonConfig, DevConfig, DiscoveryConfig, RuntimePaths, TvctlConfig}, discovery::DiscoveryService, @@ -102,7 +102,7 @@ impl TestDaemon { let binary = std::env::var("CARGO_BIN_EXE_tvctl").expect("binary path should exist"); let child = Command::new(binary) - .arg("__daemon_serve") + .env(INTERNAL_DAEMON_ENV, "1") .env("HOME", &home) .env("XDG_CONFIG_HOME", &config_home) .env("XDG_DATA_HOME", &data_home)