From b66e8c8d9c1d9367df61e14f6452d6e154e9536b Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 15:29:21 +0100 Subject: [PATCH 1/6] Replace dirty unwrap() with expect("TODO") This makes it obvious where better error handling is still needed. --- techtree-manager/src/collect.rs | 4 ++-- techtree-manager/src/issue.rs | 8 ++++---- techtree-manager/src/main.rs | 8 ++++---- techtree-manager/src/render.rs | 14 ++++++++++++-- techtree-manager/src/tree.rs | 6 +++--- 5 files changed, 25 insertions(+), 15 deletions(-) diff --git a/techtree-manager/src/collect.rs b/techtree-manager/src/collect.rs index b02afca..7d1dcb8 100644 --- a/techtree-manager/src/collect.rs +++ b/techtree-manager/src/collect.rs @@ -123,9 +123,9 @@ fn element_from_issue(issue: &forgejo_api::structs::Issue) -> anyhow::Result anyh .iter() .filter(|l| l.name.as_deref() == Some("Stale")) .next() - .map(|l| l.id.unwrap()); + .map(|l| l.id.expect("TODO")); if let Some(stale_label_id) = stale_label_id { log::info!("Removing `Stale` label from issue #{issue_number}..."); diff --git a/techtree-manager/src/main.rs b/techtree-manager/src/main.rs index b593511..d9e2f81 100644 --- a/techtree-manager/src/main.rs +++ b/techtree-manager/src/main.rs @@ -60,12 +60,12 @@ async fn run() -> anyhow::Result<()> { "{}/{}", meta.issue.repository.owner, meta.issue.repository.name )) - .unwrap(); + .expect("TODO"); let repo_auth_url = token.as_ref().map(|token| { let mut repo_auth_url = repo_url.clone(); - repo_auth_url.set_username("forgejo-actions").unwrap(); - repo_auth_url.set_password(Some(&token)).unwrap(); + repo_auth_url.set_username("forgejo-actions").expect("TODO"); + repo_auth_url.set_password(Some(&token)).expect("TODO"); repo_auth_url }); @@ -138,7 +138,7 @@ async fn run() -> anyhow::Result<()> { } 'issues: for issue in tree.iter_issues() { - let subtree = tree.subtree_for_issue(issue).unwrap(); + let subtree = tree.subtree_for_issue(issue).expect("TODO"); let hash = subtree.stable_hash(); let (bot_comment, _is_new) = issue::find_or_make_bot_comment(&ctx, issue) diff --git a/techtree-manager/src/render.rs b/techtree-manager/src/render.rs index d8fbcd8..09f0e4b 100644 --- a/techtree-manager/src/render.rs +++ b/techtree-manager/src/render.rs @@ -84,8 +84,18 @@ pub async fn publish(ctx: &crate::Context, rendered: &RenderedTree) -> anyhow::R .arg("-C") .arg(&rendered.repo_dir) .arg("add") - .arg(rendered.dot_file.strip_prefix(&rendered.repo_dir).unwrap()) - .arg(rendered.svg_file.strip_prefix(&rendered.repo_dir).unwrap()) + .arg( + rendered + .dot_file + .strip_prefix(&rendered.repo_dir) + .expect("TODO"), + ) + .arg( + rendered + .svg_file + .strip_prefix(&rendered.repo_dir) + .expect("TODO"), + ) .success() .context("Failed to add generated graph files to git index")?; diff --git a/techtree-manager/src/tree.rs b/techtree-manager/src/tree.rs index db6a93f..e3911fc 100644 --- a/techtree-manager/src/tree.rs +++ b/techtree-manager/src/tree.rs @@ -165,8 +165,8 @@ impl Tree { } pub fn add_dependency_by_issue_number(&mut self, dependant: u64, dependency: u64) { - let a = self.find_element_by_issue_number(dependant).unwrap(); - let b = self.find_element_by_issue_number(dependency).unwrap(); + let a = self.find_element_by_issue_number(dependant).expect("TODO"); + let b = self.find_element_by_issue_number(dependency).expect("TODO"); self.graph.add_edge(a, b, ()); } @@ -422,7 +422,7 @@ impl<'a> Subtree<'a> { .collect(); edges.sort(); - let json_data = serde_json::ser::to_string(&(nodes, edges, HASH_EPOCH)).unwrap(); + let json_data = serde_json::ser::to_string(&(nodes, edges, HASH_EPOCH)).expect("TODO"); sha256::digest(json_data) } } From 6d6fffcb4a35e51644df0d62363ec6a5dd3383b3 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 15:33:57 +0100 Subject: [PATCH 2/6] Reduce error potentials in type label parsing --- techtree-manager/src/collect.rs | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/techtree-manager/src/collect.rs b/techtree-manager/src/collect.rs index 7d1dcb8..ef3eca1 100644 --- a/techtree-manager/src/collect.rs +++ b/techtree-manager/src/collect.rs @@ -111,22 +111,18 @@ fn element_from_issue(issue: &forgejo_api::structs::Issue) -> anyhow::Result = labels .iter() .filter_map(|l| l.name.as_deref()) - .filter(|l| l.starts_with("Type/")) + .filter_map(|l| l.strip_prefix("Type/")) .collect(); - if ty_labels.len() == 0 { - anyhow::bail!("Issue #{issue_number} has no type label!"); - } - if ty_labels.len() > 1 { - anyhow::bail!("Issue #{issue_number} has more than one type label!"); - } - - let ty = ty_labels - .first() - .expect("TODO") - .strip_prefix("Type/") - .expect("TODO") - .to_owned(); + let ty = match &ty_labels[..] { + [ty] => ty.to_string(), + [] => { + anyhow::bail!("Issue #{issue_number} has no type label!"); + } + [..] => { + anyhow::bail!("Issue #{issue_number} has more than one type label!"); + } + }; let has_completed_label = labels .iter() From 3d73094192f6cd995b5e44857fa2ca7e34472145 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 15:44:29 +0100 Subject: [PATCH 3/6] Refactor rendering code to prevent illegal states Store the rendered tree information such that it is impossible to represent a set of data that is incorrect. --- techtree-manager/src/render.rs | 60 +++++++++++++++++----------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/techtree-manager/src/render.rs b/techtree-manager/src/render.rs index 09f0e4b..f18bebb 100644 --- a/techtree-manager/src/render.rs +++ b/techtree-manager/src/render.rs @@ -16,9 +16,19 @@ impl SuccessExt for Command { } pub struct RenderedTree { - repo_dir: std::path::PathBuf, - dot_file: std::path::PathBuf, - svg_file: std::path::PathBuf, + pub repo_dir: std::path::PathBuf, + pub dot_file_name: std::ffi::OsString, + pub svg_file_name: std::ffi::OsString, +} + +impl RenderedTree { + pub fn dot_file(&self) -> std::path::PathBuf { + self.repo_dir.join(&self.dot_file_name) + } + + pub fn svg_file(&self) -> std::path::PathBuf { + self.repo_dir.join(&self.svg_file_name) + } } pub async fn render( @@ -27,33 +37,33 @@ pub async fn render( ) -> anyhow::Result { let repo_dir = std::path::PathBuf::from("render-git"); - if repo_dir.is_dir() { - log::info!("Found old {repo_dir:?} repository, removing..."); - std::fs::remove_dir_all(&repo_dir).context("Failed to remove stale render repository")?; + let info = RenderedTree { + repo_dir, + dot_file_name: "techtree.dot".into(), + svg_file_name: "techtree.svg".into(), + }; + + if info.repo_dir.is_dir() { + log::info!("Found old {:?} repository, removing...", info.repo_dir); + std::fs::remove_dir_all(&info.repo_dir) + .context("Failed to remove stale render repository")?; } - std::fs::create_dir(&repo_dir).context("Failed creating directory for rendered graph")?; - - let dot_file = repo_dir.join("techtree.dot"); - let svg_file = repo_dir.join("techtree.svg"); + std::fs::create_dir(&info.repo_dir).context("Failed creating directory for rendered graph")?; let dot_source = tree.to_dot(); - std::fs::write(&dot_file, dot_source.as_bytes()) + std::fs::write(&info.dot_file(), dot_source.as_bytes()) .context("Failed to write `dot` source file")?; Command::new("dot") .args(["-T", "svg"]) .arg("-o") - .arg(&svg_file) - .arg(&dot_file) + .arg(&info.svg_file()) + .arg(&info.dot_file()) .success() .context("Failed to generate svg graph from dot source")?; - Ok(RenderedTree { - repo_dir, - dot_file, - svg_file, - }) + Ok(info) } pub async fn publish(ctx: &crate::Context, rendered: &RenderedTree) -> anyhow::Result<()> { @@ -84,18 +94,8 @@ pub async fn publish(ctx: &crate::Context, rendered: &RenderedTree) -> anyhow::R .arg("-C") .arg(&rendered.repo_dir) .arg("add") - .arg( - rendered - .dot_file - .strip_prefix(&rendered.repo_dir) - .expect("TODO"), - ) - .arg( - rendered - .svg_file - .strip_prefix(&rendered.repo_dir) - .expect("TODO"), - ) + .arg(&rendered.dot_file_name) + .arg(&rendered.svg_file_name) .success() .context("Failed to add generated graph files to git index")?; From d10b164b61d772d2e3ad6a3dc3c1e1601dc1feb0 Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 16:11:13 +0100 Subject: [PATCH 4/6] Properly catch errors during URL building --- techtree-manager/src/main.rs | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/techtree-manager/src/main.rs b/techtree-manager/src/main.rs index d9e2f81..cba851d 100644 --- a/techtree-manager/src/main.rs +++ b/techtree-manager/src/main.rs @@ -60,14 +60,21 @@ async fn run() -> anyhow::Result<()> { "{}/{}", meta.issue.repository.owner, meta.issue.repository.name )) - .expect("TODO"); + .with_context(|| { + format!("failed building repository URL from GITHUB_SERVER_URL: {server_url}") + })?; - let repo_auth_url = token.as_ref().map(|token| { - let mut repo_auth_url = repo_url.clone(); - repo_auth_url.set_username("forgejo-actions").expect("TODO"); - repo_auth_url.set_password(Some(&token)).expect("TODO"); - repo_auth_url - }); + let repo_auth_url = token + .as_ref() + .map(|token| -> Result<_, ()> { + let mut repo_auth_url = repo_url.clone(); + repo_auth_url.set_username("forgejo-actions")?; + repo_auth_url.set_password(Some(&token))?; + Ok(repo_auth_url) + }) + .transpose() + .map_err(|_e| anyhow::anyhow!("Repo URL does not have correct format")) + .context("Failed adding auth info to repo URL")?; let auth = if let Some(token) = token.as_ref() { forgejo_api::Auth::Token(token) From 83a610134c3205002a8d299a520cd9bc6af05f2a Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 16:18:48 +0100 Subject: [PATCH 5/6] Add better panic messages where relevant --- techtree-manager/src/main.rs | 2 +- techtree-manager/src/tree.rs | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/techtree-manager/src/main.rs b/techtree-manager/src/main.rs index cba851d..eda6053 100644 --- a/techtree-manager/src/main.rs +++ b/techtree-manager/src/main.rs @@ -145,7 +145,7 @@ async fn run() -> anyhow::Result<()> { } 'issues: for issue in tree.iter_issues() { - let subtree = tree.subtree_for_issue(issue).expect("TODO"); + let subtree = tree.subtree_for_issue(issue).expect("issue from tree not found in tree"); let hash = subtree.stable_hash(); let (bot_comment, _is_new) = issue::find_or_make_bot_comment(&ctx, issue) diff --git a/techtree-manager/src/tree.rs b/techtree-manager/src/tree.rs index e3911fc..3ec271f 100644 --- a/techtree-manager/src/tree.rs +++ b/techtree-manager/src/tree.rs @@ -165,8 +165,12 @@ impl Tree { } pub fn add_dependency_by_issue_number(&mut self, dependant: u64, dependency: u64) { - let a = self.find_element_by_issue_number(dependant).expect("TODO"); - let b = self.find_element_by_issue_number(dependency).expect("TODO"); + let a = self + .find_element_by_issue_number(dependant) + .expect("dependant element does not exist"); + let b = self + .find_element_by_issue_number(dependency) + .expect("dependency element does not exist"); self.graph.add_edge(a, b, ()); } @@ -422,7 +426,8 @@ impl<'a> Subtree<'a> { .collect(); edges.sort(); - let json_data = serde_json::ser::to_string(&(nodes, edges, HASH_EPOCH)).expect("TODO"); + let json_data = serde_json::ser::to_string(&(nodes, edges, HASH_EPOCH)) + .expect("serialization for a stable hash failed"); sha256::digest(json_data) } } From c11026c5be5d7447110ea8dcdc533d40b5c5f39c Mon Sep 17 00:00:00 2001 From: Rahix Date: Tue, 9 Dec 2025 16:19:21 +0100 Subject: [PATCH 6/6] Better handling of id errors from issue API calls --- techtree-manager/src/issue.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/techtree-manager/src/issue.rs b/techtree-manager/src/issue.rs index 9777c43..97cf9d9 100644 --- a/techtree-manager/src/issue.rs +++ b/techtree-manager/src/issue.rs @@ -28,7 +28,7 @@ pub async fn make_bot_comment( .await?; Ok(BotCommentInfo { - id: res.id.expect("TODO"), + id: res.id.context("Missing id for the bot comment")?, body: initial_message.to_owned(), }) } @@ -55,12 +55,16 @@ pub async fn find_bot_comment( let maybe_bot_comment = comments .iter() .rev() - .find(|comment| comment.user.as_ref().expect("TODO").id == Some(-2)); + .find(|comment| comment.user.as_ref().and_then(|u| u.id) == Some(-2)); - Ok(maybe_bot_comment.map(|c| BotCommentInfo { - body: c.body.clone().unwrap_or("".to_owned()), - id: c.id.expect("TODO"), - })) + Ok(maybe_bot_comment + .map(|c| -> anyhow::Result<_> { + Ok(BotCommentInfo { + body: c.body.clone().unwrap_or("".to_owned()), + id: c.id.context("Missing id for the bot comment")?, + }) + }) + .transpose()?) } /// Find existing bot comment or create a new one. @@ -155,7 +159,8 @@ pub async fn remove_stale_label(ctx: &crate::Context, issue_number: u64) -> anyh .iter() .filter(|l| l.name.as_deref() == Some("Stale")) .next() - .map(|l| l.id.expect("TODO")); + .map(|l| l.id.ok_or(anyhow::anyhow!("`Stale` label has no ID"))) + .transpose()?; if let Some(stale_label_id) = stale_label_id { log::info!("Removing `Stale` label from issue #{issue_number}...");