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.
This commit is contained in:
44r0n7
2026-04-18 11:55:18 -04:00
parent 274844b558
commit 795aa2f713
13 changed files with 131 additions and 129 deletions
+2
View File
@@ -2,3 +2,5 @@
/cache/ /cache/
/tvctl-agent-base.zip /tvctl-agent-base.zip
.DS_Store .DS_Store
node-compile-cache/
phantomjs/
+2 -1
View File
@@ -343,8 +343,9 @@ roku_password = ""
## What Has NOT Been Started ## What Has NOT Been Started
- HTTP route handlers and request validation
- CI/CD configuration - 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 - Release/packaging
--- ---
+1 -1
View File
@@ -125,7 +125,7 @@ _Goal: Ready for real use._
- [x] 2026-04-15 — `cargo clippy` clean - [x] 2026-04-15 — `cargo clippy` clean
- [x] 2026-04-15 — `cargo test` passing - [x] 2026-04-15 — `cargo test` passing
- [ ] Cross-compile test (x86_64 + aarch64) - [ ] 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 - [ ] First binary release
--- ---
-50
View File
@@ -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.
+41
View File
@@ -174,6 +174,47 @@ pub enum TvKey {
Literal(String), 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. /// A structured error produced by adapter implementations.
#[derive(Debug, Error)] #[derive(Debug, Error)]
pub enum TvError { pub enum TvError {
+2
View File
@@ -762,6 +762,8 @@ fn roku_key_paths(key: &TvKey) -> Result<Vec<String>> {
.map(|character| format!("Lit_{}", urlencoding::encode(&character.to_string()))) .map(|character| format!("Lit_{}", urlencoding::encode(&character.to_string())))
.collect()); .collect());
} }
// These keys are part of the normalized key model but Roku ECP does not
// provide documented equivalents for them.
TvKey::Stop => "stop", TvKey::Stop => "stop",
TvKey::Skip => "skip", TvKey::Skip => "skip",
TvKey::Power => "power", TvKey::Power => "power",
+1 -1
View File
@@ -154,7 +154,7 @@ async fn refresh_apps(
daemon, daemon,
DaemonRequest::RefreshApps { DaemonRequest::RefreshApps {
device: Some(id), device: Some(id),
clear: body.map(|value| value.clear).unwrap_or(false), clear: body.is_some_and(|value| value.clear),
}, },
) )
.await .await
+45 -14
View File
@@ -103,9 +103,6 @@ pub enum Command {
#[arg(value_enum)] #[arg(value_enum)]
shell: CompletionShell, shell: CompletionShell,
}, },
/// Internal daemon entry point used by `tvctl daemon start`.
#[command(hide = true, name = "__daemon_serve")]
InternalDaemonServe,
} }
#[derive(Debug, Clone, Copy, ValueEnum)] #[derive(Debug, Clone, Copy, ValueEnum)]
@@ -323,9 +320,6 @@ pub async fn run() -> Result<(), CliError> {
}; };
match command { 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::Daemon { command } => handle_daemon_command(&cli, command).await,
Command::Device { command } => handle_device_command(&cli, command).await, Command::Device { command } => handle_device_command(&cli, command).await,
Command::App { command } => handle_app_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<G: Generator>(generator: G, command: &mut clap::Command) { fn print_completions<G: Generator>(generator: G, command: &mut clap::Command) {
generate( write_completions(generator, command, &mut std::io::stdout());
generator, }
command,
command.get_name().to_string(), fn write_completions<G: Generator, W: std::io::Write>(
&mut std::io::stdout(), generator: G,
); command: &mut clap::Command,
writer: &mut W,
) {
generate(generator, command, command.get_name().to_string(), writer);
} }
fn build_cli_command() -> clap::Command { fn build_cli_command() -> clap::Command {
@@ -690,7 +687,7 @@ async fn daemon_start(cli: &Cli) -> Result<(), CliError> {
})?; })?;
TokioCommand::new(exe) TokioCommand::new(exe)
.arg("__daemon_serve") .env(daemon::INTERNAL_DAEMON_ENV, "1")
.stdin(Stdio::null()) .stdin(Stdio::null())
.stdout(Stdio::null()) .stdout(Stdio::null())
.stderr(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 { fn render_systemd_unit(exe: &std::path::Path) -> String {
format!( 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() exe.display()
) )
} }
@@ -1473,3 +1471,36 @@ fn parse_tv_key(input: &str) -> Result<TvKey, CliError> {
) )
}) })
} }
#[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"
);
}
}
+20 -8
View File
@@ -84,21 +84,37 @@ impl TvctlConfig {
/// Return one flattened config value by stable key. /// Return one flattened config value by stable key.
pub fn get_value(&self, key: &str) -> Option<String> { pub fn get_value(&self, key: &str) -> Option<String> {
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. /// Set one flattened config value by stable key.
pub fn set_value(&mut self, key: &str, value: &str) -> anyhow::Result<()> { pub fn set_value(&mut self, key: &str, value: &str) -> anyhow::Result<()> {
match key { match key {
"daemon.socket" => self.daemon.socket = value.to_string(), "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_port" => self.daemon.http_port = parse_value(key, value)?,
"daemon.http_host" => self.daemon.http_host = value.to_string(), "daemon.http_host" => self.daemon.http_host = value.to_string(),
"daemon.log_level" => { "daemon.log_level" => {
validate_log_level(value)?; validate_log_level(value)?;
self.daemon.log_level = value.to_string(); 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.interval_secs" => self.discovery.interval_secs = parse_value(key, value)?,
"discovery.timeout_secs" => self.discovery.timeout_secs = parse_value(key, value)?, "discovery.timeout_secs" => self.discovery.timeout_secs = parse_value(key, value)?,
"devices.default" => self.devices.default = value.to_string(), "devices.default" => self.devices.default = value.to_string(),
@@ -106,7 +122,7 @@ impl TvctlConfig {
"remote.roku_press_duration_ms" => { "remote.roku_press_duration_ms" => {
self.remote.roku_press_duration_ms = parse_value(key, value)? 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_username" => self.dev.roku_username = value.to_string(),
"dev.roku_password" => self.dev.roku_password = value.to_string(), "dev.roku_password" => self.dev.roku_password = value.to_string(),
other => bail!("unknown config key '{other}'"), other => bail!("unknown config key '{other}'"),
@@ -301,10 +317,6 @@ fn current_uid() -> u32 {
unsafe { libc::geteuid() } unsafe { libc::geteuid() }
} }
fn parse_bool(key: &str, value: &str) -> anyhow::Result<bool> {
parse_value(key, value)
}
fn parse_value<T>(key: &str, value: &str) -> anyhow::Result<T> fn parse_value<T>(key: &str, value: &str) -> anyhow::Result<T>
where where
T: std::str::FromStr, T: std::str::FromStr,
+3 -40
View File
@@ -34,11 +34,12 @@ use tracing::warn;
#[cfg(unix)] #[cfg(unix)]
use std::os::unix::fs::PermissionsExt; use std::os::unix::fs::PermissionsExt;
use crate::adapters::{Device, TvKey}; use crate::adapters::Device;
use crate::api; use crate::api;
use crate::logging; use crate::logging;
pub type SharedDaemon = Arc<Mutex<Daemon>>; pub type SharedDaemon = Arc<Mutex<Daemon>>;
pub const INTERNAL_DAEMON_ENV: &str = "TVCTL_INTERNAL_DAEMON";
/// The long-lived tvctld process. /// The long-lived tvctld process.
#[derive(Debug)] #[derive(Debug)]
@@ -644,7 +645,7 @@ pub(crate) async fn execute_request(
Ok(device) => device, Ok(device) => device,
Err(response) => return (response, false), 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 { match guard.adapters.key(&device, key).await {
Ok(()) => (action_success(device, detail), false), Ok(()) => (action_success(device, detail), false),
Err(error) => ( Err(error) => (
@@ -834,44 +835,6 @@ async fn send_key_sequence(
Ok(()) 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<Vec<Device>> { async fn run_discovery(daemon: &mut Daemon) -> anyhow::Result<Vec<Device>> {
let discovery = daemon.discovery.clone(); let discovery = daemon.discovery.clone();
let devices = discovery.discover_all(&mut daemon.registry).await?; let devices = discovery.discover_all(&mut daemon.registry).await?;
+2 -11
View File
@@ -97,10 +97,8 @@ impl DeviceRegistry {
/// Remove a device by UUID or case-insensitive name. /// Remove a device by UUID or case-insensitive name.
pub fn remove(&mut self, target: &str) -> Option<Device> { pub fn remove(&mut self, target: &str) -> Option<Device> {
let index = self let id = self.find(target)?.id;
.devices let index = self.devices.iter().position(|device| device.id == id)?;
.iter()
.position(|device| matches_target(device, target))?;
let removed = self.devices.remove(index); let removed = self.devices.remove(index);
self.ensure_default(); self.ensure_default();
Some(removed) 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)] #[cfg(test)]
mod tests { mod tests {
use super::*; use super::*;
+10 -1
View File
@@ -6,7 +6,16 @@ async fn main() {
std::process::exit(1); 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}"); eprintln!("{error}");
std::process::exit(1); std::process::exit(1);
} }
+2 -2
View File
@@ -14,7 +14,7 @@ use tokio::{net::TcpListener as TokioTcpListener, sync::Mutex, task::JoinHandle,
use tvctl::{ use tvctl::{
adapters::Device, adapters::Device,
daemon::{ daemon::{
Daemon, SharedDaemon, Daemon, INTERNAL_DAEMON_ENV, SharedDaemon,
cache::AppCacheStore, cache::AppCacheStore,
config::{DaemonConfig, DevConfig, DiscoveryConfig, RuntimePaths, TvctlConfig}, config::{DaemonConfig, DevConfig, DiscoveryConfig, RuntimePaths, TvctlConfig},
discovery::DiscoveryService, discovery::DiscoveryService,
@@ -102,7 +102,7 @@ impl TestDaemon {
let binary = std::env::var("CARGO_BIN_EXE_tvctl").expect("binary path should exist"); let binary = std::env::var("CARGO_BIN_EXE_tvctl").expect("binary path should exist");
let child = Command::new(binary) let child = Command::new(binary)
.arg("__daemon_serve") .env(INTERNAL_DAEMON_ENV, "1")
.env("HOME", &home) .env("HOME", &home)
.env("XDG_CONFIG_HOME", &config_home) .env("XDG_CONFIG_HOME", &config_home)
.env("XDG_DATA_HOME", &data_home) .env("XDG_DATA_HOME", &data_home)