Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion libs/edge-toolkit/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,8 @@ impl Language {
/// to branch on the returned bool -- they don't repeat the message themselves.
#[must_use]
pub fn mise_env_includes(language: Language) -> bool {
let Ok(value) = std::env::var("MISE_ENV") else {
const MISE_ENV: &str = "MISE_ENV";
let Ok(value) = std::env::var(MISE_ENV) else {
return true;
};
let included = !value.is_empty() && value.split(',').any(|seg| seg.trim() == language.as_str());
Expand Down
13 changes: 7 additions & 6 deletions libs/et-otlp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,11 @@ pub struct OtelHandles {
impl OtelHandles {
/// Flush any buffered spans/logs/metrics and tear down the exporters.
pub fn shutdown(self) {
// Errors here are non-fatal -- the process is exiting anyway.
drop(self.tracer_provider.shutdown());
drop(self.logger_provider.shutdown());
drop(self.meter_provider.shutdown());
// Errors here are non-fatal -- the process is exiting anyway, and there is no caller to
// propagate to, so each teardown result is intentionally discarded.
let _tracer = self.tracer_provider.shutdown();
let _logger = self.logger_provider.shutdown();
let _meter = self.meter_provider.shutdown();
}
}

Expand All @@ -60,8 +61,8 @@ impl OtelHandles {
/// invalid, or the global subscriber is already set.
pub fn init(config: &OtlpConfig) -> Result<OtelHandles, Box<dyn std::error::Error + Send + Sync>> {
// tracing_log forwards `log` crate records (used by transitive deps)
// through the tracing subscriber.
drop(tracing_log::LogTracer::init());
// through the tracing subscriber. A second init (global logger already set) is a real error here.
tracing_log::LogTracer::init()?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

Suggestion: Changed from silent ignore (drop) to error propagation (?). While this is a robustness improvement that prevents running with broken telemetry, it will cause initialization to fail if a global logger was already set elsewhere. Verify that callers are prepared to handle this change in return type.


let mut headers = std::collections::HashMap::new();
if let Some(auth) = &config.auth {
Expand Down
3 changes: 2 additions & 1 deletion libs/path/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ pub fn find_project_root(start: &Path) -> PathBuf {
/// start, or `edge_toolkit::config::get_project_root`, instead.
#[must_use]
pub fn find_project_root_from_manifest() -> PathBuf {
let manifest = std::env::var("CARGO_MANIFEST_DIR").unwrap_or_default();
const CARGO_MANIFEST_DIR: &str = "CARGO_MANIFEST_DIR";
let manifest = std::env::var(CARGO_MANIFEST_DIR).unwrap_or_default();
find_project_root(Path::new(&manifest))
}

Expand Down
14 changes: 9 additions & 5 deletions libs/test-helpers/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ impl ChildGuard {

/// Kill the child and reap it now (also runs on drop; errors are ignored).
pub fn shutdown(&mut self) {
drop(self.child.kill());
drop(self.child.wait());
// Best-effort teardown, also invoked from Drop -- no caller to propagate to, so discard.
let _kill = self.child.kill();
let _wait = self.child.wait();
}
}

Expand All @@ -73,10 +74,13 @@ pub fn drain_stderr(child: &mut Child) -> Arc<Mutex<String>> {
let stderr = child.stderr.take().unwrap();
let log = Arc::new(Mutex::new(String::new()));
let sink = Arc::clone(&log);
drop(std::thread::spawn(move || {
// Detached drainer: nothing joins the handle, so neither the read result nor the thread's own
// outcome has a caller to propagate to -- both are intentionally discarded (partial output on a
// read error is still worth keeping for diagnostics).
let _drainer = std::thread::spawn(move || {
let mut buffer = String::new();
drop(std::io::BufReader::new(stderr).read_to_string(&mut buffer));
let _read = std::io::BufReader::new(stderr).read_to_string(&mut buffer);
*sink.lock().unwrap() = buffer;
}));
});
log
}
7 changes: 1 addition & 6 deletions services/ws-modules/har1/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ impl MotionReading {
}

#[wasm_bindgen]
#[derive(Default)]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ LOW RISK

Suggestion: Manual Default implementation replaced by derive macro. This is a clean refactor that maintains the expected initialization state.

pub struct DeviceSensors {
active: bool,
orientation_state: Rc<RefCell<Option<OrientationReadingState>>>,
Expand All @@ -165,12 +166,6 @@ pub struct DeviceSensors {
motion_listener: Option<Closure<dyn FnMut(Event)>>,
}

impl Default for DeviceSensors {
fn default() -> Self {
Self::new()
}
}

#[wasm_bindgen]
impl DeviceSensors {
#[must_use]
Expand Down
7 changes: 1 addition & 6 deletions services/ws-modules/sensor1/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ impl MotionReading {
}

#[wasm_bindgen]
#[derive(Default)]
pub struct DeviceSensors {
active: bool,
orientation_state: Rc<RefCell<Option<OrientationReadingState>>>,
Expand All @@ -141,12 +142,6 @@ pub struct DeviceSensors {
motion_listener: Option<Closure<dyn FnMut(Event)>>,
}

impl Default for DeviceSensors {
fn default() -> Self {
Self::new()
}
}

#[wasm_bindgen]
impl DeviceSensors {
#[must_use]
Expand Down
24 changes: 14 additions & 10 deletions services/ws-pyo3-runner/src/agent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,18 +156,18 @@ pub async fn run(agent: InitializedAgent) -> Result<(), RunnerError> {
// Populate the slot before `on_connect` so Python sees a valid
// `storage.agent_id` from the first instant it can act.
*agent_id_slot.lock().unwrap_or_else(PoisonError::into_inner) = Some(agent_id.clone());
drop(inbound_tx.send(InboundEvent::Connect(agent_id)));
let _connect_sent = inbound_tx.send(InboundEvent::Connect(agent_id));

let result = drive(&mut socket, &inbound_tx, &mut outbound_rx).await;

// Queue `on_shutdown` (the worker drains any frames ahead of it first),
// then drop our sender so the worker's recv loop ends. Join before aborting
// the storage task so an `on_shutdown` that persists state can still reach
// it; only then close the socket and stop storage.
drop(inbound_tx.send(InboundEvent::Shutdown));
let _shutdown_sent = inbound_tx.send(InboundEvent::Shutdown);
drop(inbound_tx);
drop(worker.join());
drop(socket.send(tungstenite::Message::Close(None)).await);
let _joined = worker.join();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

Suggestion: Swallowing the join result hides panics in the dispatcher thread. Propagating the panic ensures that the agent fails visibly if the Python worker crashes.

Suggested change
let _joined = worker.join();
worker.join().expect("python worker thread panicked");

Comment on lines +159 to +169

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

sed -n '1,360p' services/ws-pyo3-runner/src/agent.rs

Repository: edge-toolkit/core

Length of output: 13589


🏁 Script executed:

rg -n "enum RunnerError|impl From<.*Join|SendError|worker.join|unbounded_channel|InboundEvent" services/ws-pyo3-runner/src -S

Repository: edge-toolkit/core

Length of output: 1770


🏁 Script executed:

sed -n '1,220p' services/ws-pyo3-runner/src/error.rs

Repository: edge-toolkit/core

Length of output: 1160


Propagate dispatch-worker failure instead of silently dropping events. If inbound_tx.send(...) fails, the Python worker is already gone, but run() keeps the socket alive and inbound frames are lost. Surface that error, and do the same for worker.join() so a worker panic does not look like a successful run.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/ws-pyo3-runner/src/agent.rs` around lines 159 - 169, Update the
dispatch flow around the Connect and Shutdown sends in run() to propagate
inbound_tx.send failures instead of ignoring them, while preserving shutdown
ordering. Also propagate worker.join() failures so worker termination or panic
causes run() to return an error rather than appearing successful.

let _close_sent = socket.send(tungstenite::Message::Close(None)).await;
storage_task.abort();
result
}
Expand Down Expand Up @@ -195,12 +195,16 @@ fn python_worker(
}
}
InboundEvent::Text(text) => match dispatcher.on_text_frame(&text) {
Ok(Some(reply)) => drop(outbound_tx.send(OutboundFrame::Text(reply))),
Ok(Some(reply)) => {
let _sent = outbound_tx.send(OutboundFrame::Text(reply));
}
Ok(None) => {}
Err(err) => warn!("on_text_frame raised: {err}"),
},
InboundEvent::Binary(bytes) => match dispatcher.on_binary_frame(&bytes) {
Ok(Some(reply)) => drop(outbound_tx.send(OutboundFrame::Binary(reply))),
Ok(Some(reply)) => {
let _sent = outbound_tx.send(OutboundFrame::Binary(reply));
}
Ok(None) => {}
Err(err) => warn!("on_binary_frame raised: {err}"),
},
Expand Down Expand Up @@ -233,7 +237,7 @@ async fn storage_worker(http_base: String, mut rx: mpsc::UnboundedReceiver<Stora
Err(et_rest_client::Error::ErrorResponse(_)) => Ok(None),
Err(source) => Err(StorageError::get(&agent_id, &key, source.to_string())),
};
drop(reply.send(outcome));
let _reply_sent = reply.send(outcome);
}
StorageOp::Put {
agent_id,
Expand All @@ -245,7 +249,7 @@ async fn storage_worker(http_base: String, mut rx: mpsc::UnboundedReceiver<Stora
Ok(_) => Ok(()),
Err(source) => Err(StorageError::put(&agent_id, &key, source.to_string())),
};
drop(reply.send(outcome));
let _reply_sent = reply.send(outcome);
}
}
}
Expand Down Expand Up @@ -273,10 +277,10 @@ async fn drive(
// pushes to via WsSender, so multi-send + reply compose in order.
frame = socket.next() => match frame {
Some(Ok(tungstenite::Message::Binary(bytes))) => {
drop(inbound_tx.send(InboundEvent::Binary(bytes)));
let _forwarded = inbound_tx.send(InboundEvent::Binary(bytes));
}
Some(Ok(tungstenite::Message::Text(text))) => {
drop(inbound_tx.send(InboundEvent::Text(text)));
let _forwarded = inbound_tx.send(InboundEvent::Text(text));
}
Some(Ok(tungstenite::Message::Close(_))) => {
info!("server closed connection");
Expand Down
4 changes: 2 additions & 2 deletions services/ws-pyo3-runner/tests/modules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,8 @@ async fn module_behaves(
};
let outcome = tokio::time::timeout(budget, run_exchange(&mut control, &control_id, &exchange)).await;

drop(runner.kill());
drop(runner.wait());
runner.kill().unwrap();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

This unwrap() introduces a race condition that can cause flaky tests if the runner exits just before kill() is called. Since wait() follows immediately, you can safely ignore the result of kill() to avoid unnecessary panics if the process is already dead.

Suggested change
runner.kill().unwrap();
let _ = runner.kill();

let _status = runner.wait().unwrap();

match outcome {
Ok(result) => result,
Expand Down
5 changes: 3 additions & 2 deletions services/ws-test-server/tests/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ use tokio_tungstenite::{accept_async, connect_async};
async fn scripted_server(frames: Vec<Message>) -> String {
let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
let port = listener.local_addr().unwrap().port();
drop(tokio::spawn(async move {
// Detached scripted server; the handle is intentionally not joined (the task runs until the test ends).
let _server = tokio::spawn(async move {
let Ok((stream, _)) = listener.accept().await else {
return;
};
Expand All @@ -30,7 +31,7 @@ async fn scripted_server(frames: Vec<Message>) -> String {
}
// Keep the connection open so the client can finish reading rather than seeing an early close.
std::future::pending::<()>().await;
}));
});
format!("ws://127.0.0.1:{port}")
}

Expand Down
5 changes: 3 additions & 2 deletions services/ws-wasi-runner/src/host/ws.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,11 @@ impl WsBackend {
let (tx, rx) = mpsc::unbounded_channel::<ServerMessage>();
// The handshake consumed the et-connect-ack frame; re-surface it to the
// guest's `recv()` so guests that read it still see it as the first message.
drop(tx.send(ServerMessage::ConnectAck {
// Cannot fail: `rx` (created just above) is still alive, so the channel is open.
let _seeded = tx.send(ServerMessage::ConnectAck {
agent_id: assigned_id.clone(),
status,
}));
});

let agent_id = Arc::new(Mutex::new(Some(assigned_id)));
let connection_state = Arc::new(Mutex::new(State::Connected));
Expand Down
2 changes: 1 addition & 1 deletion services/ws-wasm-agent/tests/web.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ async fn test_websocket_connection() {
.set_timeout_with_callback_and_timeout_and_arguments_0(&resolve, 1000)
.unwrap();
});
drop(JsFuture::from(promise).await);
let _resolved = JsFuture::from(promise).await.unwrap();

// Assert connection state is successfully connected or at least it didn't fail
let state = client.get_state();
Expand Down
16 changes: 8 additions & 8 deletions services/ws/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -328,13 +328,13 @@ impl Connection {
"Direct message {} delivered from {} to {}",
message_id, from_agent_id, to_agent_id
);
drop(recipient.send(SessionMessage::Json(ServerMessage::AgentMessage {
let _sent = recipient.send(SessionMessage::Json(ServerMessage::AgentMessage {
message_id: message_id.clone(),
from_agent_id,
scope: MessageScope::Direct,
server_received_at: pending.server_received_at,
message: pending.message,
})));
}));
Comment on lines +331 to +337

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

set -euo pipefail

# Show the relevant file and line ranges.
sed -n '300,360p' services/ws/src/lib.rs
printf '\n====\n'
sed -n '500,540p' services/ws/src/lib.rs
printf '\n====\n'
# Find delivery/status related enums or message variants in the same file.
rg -n "Delivered|Broadcast|Direct|send\\(" services/ws/src/lib.rs

Repository: edge-toolkit/core

Length of output: 6305


🏁 Script executed:

set -euo pipefail
printf 'Need tool output\n'

Repository: edge-toolkit/core

Length of output: 172


Don't mark direct or broadcast delivery as successful when send can fail. recipient.send(...) is ignored here, so a disconnect after the registry snapshot can still produce a false Delivered/broadcast status. Handle the error or count only successful sends.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@services/ws/src/lib.rs` around lines 331 - 337, Update the direct-message
delivery path around recipient.send in the AgentMessage handling flow so its
Result is checked rather than discarded. Only report or count delivery as
successful when send succeeds, and handle failed sends consistently with the
existing delivery-status behavior, including broadcast delivery where
applicable.

self.send_status(
Some(message_id),
MessageDeliveryStatus::Delivered,
Expand Down Expand Up @@ -367,7 +367,7 @@ impl Connection {
recipients.len()
);
for (_, recipient) in recipients {
drop(recipient.send(SessionMessage::Text(text.to_string())));
let _sent = recipient.send(SessionMessage::Text(text.to_string()));
}
}

Expand All @@ -381,7 +381,7 @@ impl Connection {
recipients.len()
);
for (_, recipient) in recipients {
drop(recipient.send(SessionMessage::Binary(bytes.clone())));
let _sent = recipient.send(SessionMessage::Binary(bytes.clone()));
}
}

Expand Down Expand Up @@ -509,13 +509,13 @@ impl Connection {
recipients.len()
);
for (_, recipient) in &recipients {
drop(recipient.send(SessionMessage::Json(ServerMessage::AgentMessage {
let _sent = recipient.send(SessionMessage::Json(ServerMessage::AgentMessage {
message_id: message_id.clone(),
from_agent_id: from_agent_id.clone(),
scope: MessageScope::Broadcast,
server_received_at: server_received_at.clone(),
message: message.clone(),
})));
}));
}
self.send_status(
Some(message_id),
Expand Down Expand Up @@ -545,11 +545,11 @@ impl Connection {
)
.await;
if let Some(sender) = sender_session {
drop(sender.send(SessionMessage::Json(ServerMessage::MessageStatus {
let _sent = sender.send(SessionMessage::Json(ServerMessage::MessageStatus {
message_id: Some(message_id),
status: MessageDeliveryStatus::Acknowledged,
detail: format!("agent {recipient_agent_id} acknowledged receipt"),
})));
}));
}
}
Err(error) => {
Expand Down
Loading