Fix component issues

[AlexMikhalev]

No description provided.

[Copilot]

Pull Request Overview

Copilot reviewed 81 out of 717 changed files in this pull request and generated 7 comments.

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

Loading a version ignores the provided storage key. VersionedLessons::new(self.get_version_key(...)) constructs an instance that doesn't retain/use that key (see Persistable::new), so load() will derive a different key and fail to fetch the intended version. Fix by either (a) making VersionedLessons::new(key) initialize agent_id and timestamp from the key, or (b) changing load_version to set agent_id and timestamp directly (or adding a load_by_key that uses the provided key).

Suggested change

	let mut versioned_lessons = VersionedLessons::new(self.get_version_key(timestamp));
	let mut versioned_lessons = VersionedLessons {
	agent_id: self.agent_id.clone(),
	timestamp,
	state: LessonsState::default(),
	};

[Copilot]

Persistable::new discards the passed key and initializes an empty agent_id and timestamp = now(). This breaks any subsequent get_key() or load() that rely on version identity. Parse the key (e.g., agent_{id}/lessons/v_{ts}) to set agent_id and timestamp, or store the key and use it in load() instead of get_key().

[Copilot]

start() does not set start_time, and because it takes &self (not &mut self), it can't update internal state. As a result, get_status() will always report running: false and uptime_seconds: 0. Change the trait and impl to use &mut self for start/stop (and set self.start_time = Some(SystemTime::now()) in start() and None in stop()), or wrap start_time in interior mutability (e.g., Mutex<Option<SystemTime>>).

[Copilot]

start() does not set start_time, and because it takes &self (not &mut self), it can't update internal state. As a result, get_status() will always report running: false and uptime_seconds: 0. Change the trait and impl to use &mut self for start/stop (and set self.start_time = Some(SystemTime::now()) in start() and None in stop()), or wrap start_time in interior mutability (e.g., Mutex<Option<SystemTime>>).

[Copilot]

ComponentType is used as a HashMap key in ReloadStatus, but it doesn't implement Eq and Hash, which will fail to compile. Add Eq and Hash derives: #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Hash)].

[Copilot]

ComponentType is used as a HashMap key in ReloadStatus, but it doesn't implement Eq and Hash, which will fail to compile. Add Eq and Hash derives: #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Hash)].

[Copilot]

Configuration precedence is inverted: defaults are added last and thus override file and environment values. To honor the expected precedence (defaults < file < env), add the default source first, then the file, then the environment.

Suggested change

	let mut config_builder = Config::builder()
	.add_source(File::from(config_path).required(false))
	.add_source(Environment::with_prefix("TERRAPHIM"));
	// Add default configuration
	let default_config = ApplicationConfig::default();
	let default_toml = toml::to_string(&default_config)
	.map_err(|e| ApplicationError::ConfigurationError(e.to_string()))?;
	config_builder = config_builder.add_source(config::File::from_str(
	&default_toml,
	config::FileFormat::Toml,
	));
	// Add default configuration first
	let default_config = ApplicationConfig::default();
	let default_toml = toml::to_string(&default_config)
	.map_err(|e| ApplicationError::ConfigurationError(e.to_string()))?;
	let mut config_builder = Config::builder()
	.add_source(config::File::from_str(
	&default_toml,
	config::FileFormat::Toml,
	))
	.add_source(File::from(config_path).required(false))
	.add_source(Environment::with_prefix("TERRAPHIM"));

[Copilot]

Pull Request Overview

Copilot reviewed 81 out of 718 changed files in this pull request and generated 9 comments.

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

Configuration source precedence is reversed: defaults are added last and will override user file and environment values. In config-rs, later sources win. Fix by adding defaults first, then the file (optional), and finally the environment so user/env values take precedence.

[Copilot]

Persistable::new discards the provided key, and get_key derives from empty fields, breaking load_by_key semantics. Store the provided key (e.g., add a key: String field and have get_key return it) or parse the key into agent_id and timestamp in new().

[Copilot]

start()/stop() take &self, so start_time is never updated and running stays false; get_status reports incorrect state. Either change the trait to use &mut self for start/stop or use interior mutability (e.g., Mutex/Atomic) so start/stop can set/clear start_time.

[Copilot]

start()/stop() take &self, so start_time is never updated and running stays false; get_status reports incorrect state. Either change the trait to use &mut self for start/stop or use interior mutability (e.g., Mutex/Atomic) so start/stop can set/clear start_time.

[Copilot]

Spawned periodic tasks are not tracked or cancelled on stop()/restart(), which can leak tasks and duplicate work across restarts. Store JoinHandles (e.g., in a Vec<JoinHandle<()>>) and abort them in stop(), or use a shutdown signal to gracefully terminate the loops.

[Copilot]

Spawned periodic tasks are not tracked or cancelled on stop()/restart(), which can leak tasks and duplicate work across restarts. Store JoinHandles (e.g., in a Vec<JoinHandle<()>>) and abort them in stop(), or use a shutdown signal to gracefully terminate the loops.

[Copilot]

Spawned periodic tasks are not tracked or cancelled on stop()/restart(), which can leak tasks and duplicate work across restarts. Store JoinHandles (e.g., in a Vec<JoinHandle<()>>) and abort them in stop(), or use a shutdown signal to gracefully terminate the loops.

[Copilot]

Spawned periodic tasks are not tracked or cancelled on stop()/restart(), which can leak tasks and duplicate work across restarts. Store JoinHandles (e.g., in a Vec<JoinHandle<()>>) and abort them in stop(), or use a shutdown signal to gracefully terminate the loops.

[Copilot]

Spawned periodic tasks are not tracked or cancelled on stop()/restart(), which can leak tasks and duplicate work across restarts. Store JoinHandles (e.g., in a Vec<JoinHandle<()>>) and abort them in stop(), or use a shutdown signal to gracefully terminate the loops.

[Copilot]

Pull Request Overview

Copilot reviewed 81 out of 733 changed files in this pull request and generated 3 comments.

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

start_time is never set and cannot be mutated because the trait methods take &self. As a result, running is always false and uptime is 0. Use interior mutability to update start_time (e.g., wrap it in a Mutex/RwLock) and set it in start(), clear it in stop(). Alternatively, adjust the trait to take &mut self for start/stop.

[Copilot]

Configuration source precedence is inverted: defaults are added last and will override file and environment values. Defaults should be lowest precedence. Fix by adding the default source first, then the file, then environment (env highest). For example: let mut builder = Config::builder().add_source(config::File::from_str(&default_toml, config::FileFormat::Toml)).add_source(File::from(config_path).required(false)).add_source(Environment::with_prefix("TERRAPHIM"));

[Copilot]

Unnecessary allocation: converting "" to String before into(). pulldown_cmark accepts Cow, so "".into() is sufficient. Prefer id: "".into() to avoid allocation.

Suggested change

	id: "".to_string().into(),
	id: "".into(),

[Copilot]

Pull Request Overview

Copilot reviewed 81 out of 733 changed files in this pull request and generated 1 comment.

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

libxar artifacts are copied to /xar earlier in the stage, but configure points to --with-libxar=/libxar. This mismatch will cause configure to fail to find libxar. Either change the COPY to /libxar or update the flag to --with-libxar=/xar.

Suggested change

	--with-libxar=/libxar \
	--with-libxar=/xar \

[Copilot]

Pull Request Overview

Copilot reviewed 81 out of 735 changed files in this pull request and generated 1 comment.

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

This uses Arc::try_unwrap on a freshly cloned Arc (almost always fails) and falls back to cloning the inner ConfigurationManager, which doesn't implement Clone and contains non-cloneable fields (file watcher). This will not compile and is conceptually wrong. Use Arc::get_mut(&mut self.config_manager) since you have &mut self, or wrap ConfigurationManager in a Mutex/RwLock and lock mutably. Example: if let Some(cm) = Arc::get_mut(&mut self.config_manager) { cm.start_hot_reload().await?; } else { return Err(ApplicationError::ConfigurationError("config manager is shared".into())); }

Suggested change

	let mut config_manager =
	Arc::try_unwrap(self.config_manager.clone()).unwrap_or_else(|arc| (*arc).clone());
	config_manager.start_hot_reload().await?;
	self.config_manager = Arc::new(config_manager);
	if let Some(config_manager) = Arc::get_mut(&mut self.config_manager) {
	config_manager.start_hot_reload().await?;
	} else {
	return Err(ApplicationError::ConfigurationError("config manager is shared".into()));
	}

[Copilot]

Pull Request Overview

Copilot reviewed 82 out of 736 changed files in this pull request and generated 4 comments.

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

LifecycleStatus.running will always be false because start_time is never set in start() nor cleared in stop(). To reflect actual state, set self.start_time = Some(SystemTime::now()) on start and reset it to None on stop; this requires mutability. Either change the trait methods to take &mut self or wrap start_time in a Mutex/RwLock for interior mutability.

[Copilot]

LifecycleStatus.running will always be false because start_time is never set in start() nor cleared in stop(). To reflect actual state, set self.start_time = Some(SystemTime::now()) on start and reset it to None on stop; this requires mutability. Either change the trait methods to take &mut self or wrap start_time in a Mutex/RwLock for interior mutability.

[Copilot]

Potential division by zero if max_cpu_cores or max_memory_mb is set to 0, which would panic at runtime. Add guards to handle zero-capacity configurations (e.g., return 0.0 when the denominator is zero).

Suggested change

	(total_cpu_usage / self.config.resources.max_cpu_cores as f64) * 100.0;
	let memory_utilization_percent =
	(total_memory_usage_mb as f64 / self.config.resources.max_memory_mb as f64) * 100.0;
	if self.config.resources.max_cpu_cores == 0 {
	0.0
	} else {
	(total_cpu_usage / self.config.resources.max_cpu_cores as f64) * 100.0
	};
	let memory_utilization_percent =
	if self.config.resources.max_memory_mb == 0 {
	0.0
	} else {
	(total_memory_usage_mb as f64 / self.config.resources.max_memory_mb as f64) * 100.0
	};

[Copilot]

The imported Arc is unused in this file. Removing unused imports keeps the module clean and avoids lint warnings.

Suggested change

use std::sync::Arc;