Replace TODOs with config-driven defaults and transport-based checks

core/codegen/src/registry.rs

@@ -552,7 +552,9 @@ pub fn all_tools() -> Vec<ToolDef> {
         //
         // Provider, model, and criteria are pipeline config (baked into DAG via
         // LoadPipelineConfig node), NOT CLI flags.
-        // TODO: base_ref should come from repo branching model config.
+        // The CLI default for base_ref mirrors ReviewPipelineConfig::default_branch.
+        // The graph builder's config is the source of truth; this CLI default is
+        // only a fallback for when no config override is provided.
         ToolDef::new(
             "gunbc-lib-review",
             "review",

core/ir/src/transport/tool.rs

@@ -475,17 +475,27 @@ pub static APK: ToolDef = ToolDef {
 };
 
 /// cargo package manager (Rust).
-/// Cargo package manager (Rust).
+///
+/// Cargo is both a package manager (tools use `via: "cargo"`) and a tool that
+/// must be installed. On brew, it comes from the `rustup` package alongside
+/// rustc. On apt, it's a separate package.
 ///
 /// For runtime tool acquisition, use `transport::cli::CARGO` with `.requires()`.
 /// This ToolDef is for platform-aware satisfiability checking.
-///
-/// Note: cargo depends on rust being installed.
 pub static CARGO: ToolDef = ToolDef {
     id: "cargo",
     command: "cargo",
     verify: "cargo --version",
-    install_options: &[], // Base PM once rust is installed
+    install_options: &[
+        InstallOption {
+            via: "brew",
+            inputs: InstallInputs::packages(&["rustup"]),
+        },
+        InstallOption {
+            via: "apt",
+            inputs: InstallInputs::packages(&["cargo"]),
+        },
+    ],
     depends_on: &["rust"],
 };
 
@@ -518,10 +528,18 @@ pub static GIT: ToolDef = ToolDef {
     depends_on: &[],
 };
 
-/// Rust toolchain (rustc, cargo).
+/// Rust toolchain (rustc).
+///
+/// The base of the Rust toolchain dependency chain:
+///
+/// ```text
+/// RUST (rustc) ← CARGO ← CLIPPY
+///                       ← RUSTFMT
+/// ```
 ///
-/// Note: On macOS, rust can be installed via brew (rustup).
-/// On other platforms, proper modeling of shell environments would be needed.
+/// Each level declares its own install constraints per platform. On brew,
+/// all levels resolve to the `rustup` package (idempotent). On apt, each
+/// is a separate system package.
 pub static RUST: ToolDef = ToolDef {
     id: "rust",
     command: "rustc",
@@ -531,25 +549,35 @@ pub static RUST: ToolDef = ToolDef {
             via: "brew",
             inputs: InstallInputs::packages(&["rustup"]),
         },
-        // TODO: apt has rustc package but rustup is preferred
+        InstallOption {
+            via: "apt",
+            inputs: InstallInputs::packages(&["rustc"]),
+        },
     ],
     depends_on: &[],
 };
 
-/// Clippy linter (Rust component).
-///
 /// Clippy linter (Rust component).
 ///
 /// For runtime tool acquisition, use `transport::cli::CLIPPY` with `.requires()`.
 /// This ToolDef is for platform-aware satisfiability checking.
 ///
-/// Clippy is installed as a rustup component, not a standalone package.
-/// It's invoked via `cargo clippy`.
+/// On brew, clippy comes from the `rustup` package (as a default component).
+/// On apt, it's the `rust-clippy` package.
 pub static CLIPPY: ToolDef = ToolDef {
     id: "clippy",
     command: "cargo",
     verify: "cargo clippy --version",
-    install_options: &[], // Installed as rust component via rustup
+    install_options: &[
+        InstallOption {
+            via: "brew",
+            inputs: InstallInputs::packages(&["rustup"]),
+        },
+        InstallOption {
+            via: "apt",
+            inputs: InstallInputs::packages(&["rust-clippy"]),
+        },
+    ],
     depends_on: &["cargo"],
 };
 
@@ -558,12 +586,22 @@ pub static CLIPPY: ToolDef = ToolDef {
 /// For runtime tool acquisition, use `transport::cli::RUSTFMT` with `.requires()`.
 /// This ToolDef is for platform-aware satisfiability checking.
 ///
-/// Rustfmt is installed as a rustup component, not a standalone package.
+/// On brew, rustfmt comes from the `rustup` package (as a default component).
+/// On apt, it's the `rustfmt` package.
 pub static RUSTFMT: ToolDef = ToolDef {
     id: "rustfmt",
     command: "rustfmt",
     verify: "rustfmt --version",
-    install_options: &[], // Installed as rust component via rustup
+    install_options: &[
+        InstallOption {
+            via: "brew",
+            inputs: InstallInputs::packages(&["rustup"]),
+        },
+        InstallOption {
+            via: "apt",
+            inputs: InstallInputs::packages(&["rustfmt"]),
+        },
+    ],
     depends_on: &["cargo"],
 };
 
@@ -850,7 +888,7 @@ mod tests {
     }
 
     #[test]
-    fn test_platform_registry() {
+    fn test_platform_registry_inheritance() {
         static TEST_LINUX: PlatformDef = PlatformDef {
             id: "linux",
             parent: None,
@@ -871,99 +909,11 @@ mod tests {
         assert!(ubuntu_pms.contains("apt"));
     }
 
-    #[test]
-    fn test_builtin_package_managers() {
-        // Verify built-in PMs are defined correctly
-        assert_eq!(super::APT.id, "apt");
-        assert_eq!(super::APT.command, "apt-get");
-        assert!(super::APT.install_options.is_empty()); // Base PM
-
-        assert_eq!(super::BREW.id, "brew");
-        assert!(super::BREW.install_options.is_empty()); // Base PM
-
-        assert_eq!(super::APK.id, "apk");
-        assert!(super::APK.install_options.is_empty()); // Base PM
-
-        assert_eq!(super::CARGO.id, "cargo");
-        assert!(super::CARGO.depends_on.contains(&"rust")); // cargo needs rust
-    }
-
-    #[test]
-    fn test_builtin_platforms() {
-        // Verify built-in platforms are defined correctly
-        assert_eq!(super::UBUNTU.id, "ubuntu");
-        assert_eq!(super::UBUNTU.parent, Some("linux"));
-        assert!(super::UBUNTU.available_pms.contains(&"apt"));
-
-        assert_eq!(super::MACOS.id, "macos");
-        assert_eq!(super::MACOS.parent, None);
-        assert!(super::MACOS.available_pms.contains(&"brew"));
-
-        assert_eq!(super::ALPINE.id, "alpine");
-        assert!(super::ALPINE.available_pms.contains(&"apk"));
-    }
-
-    #[test]
-    fn test_default_registries() {
-        let tool_registry = super::default_tool_registry();
-        assert!(tool_registry.get("apt").is_some());
-        assert!(tool_registry.get("brew").is_some());
-        assert!(tool_registry.get("apk").is_some());
-        assert!(tool_registry.get("cargo").is_some());
-        assert!(tool_registry.get("git").is_some());
-        assert!(tool_registry.get("rust").is_some());
-        assert!(tool_registry.get("gh").is_some());
-
-        let platform_registry = super::default_platform_registry();
-        assert!(platform_registry.get("ubuntu").is_some());
-        assert!(platform_registry.get("macos").is_some());
-        assert!(platform_registry.get("alpine").is_some());
-
-        // Test ubuntu has apt via inheritance
-        let ubuntu_pms = platform_registry.available_pms("ubuntu");
-        assert!(ubuntu_pms.contains("apt"));
-    }
-
-    #[test]
-    fn test_git_tool_definition() {
-        assert_eq!(super::GIT.id, "git");
-        assert_eq!(super::GIT.command, "git");
-        assert_eq!(super::GIT.verify, "git --version");
-
-        // git should be installable via apt, brew, and apk
-        let apt_opt = super::GIT.install_options.iter().find(|o| o.via == "apt");
-        assert!(apt_opt.is_some());
-        assert_eq!(apt_opt.unwrap().inputs.packages, Some(&["git"][..]));
-
-        let brew_opt = super::GIT.install_options.iter().find(|o| o.via == "brew");
-        assert!(brew_opt.is_some());
-
-        let apk_opt = super::GIT.install_options.iter().find(|o| o.via == "apk");
-        assert!(apk_opt.is_some());
-    }
-
-    #[test]
-    fn test_rust_tool_definition() {
-        assert_eq!(super::RUST.id, "rust");
-        assert_eq!(super::RUST.command, "rustc");
-        assert_eq!(super::RUST.verify, "rustc --version");
-
-        // rust can be installed via brew (rustup)
-        let brew_opt = super::RUST.install_options.iter().find(|o| o.via == "brew");
-        assert!(brew_opt.is_some());
-        assert_eq!(brew_opt.unwrap().inputs.packages, Some(&["rustup"][..]));
-    }
-
-    #[test]
-    fn test_cargo_depends_on_rust() {
-        assert!(super::CARGO.depends_on.contains(&"rust"));
-    }
-
     #[test]
     fn test_git_satisfiable_on_ubuntu() {
         let registry = super::default_tool_registry();
         let available: HashSet<&str> = ["apt"].into_iter().collect();
-        
+
         assert!(super::is_satisfiable(&super::GIT, &available, &registry).is_ok());
     }
 
@@ -984,12 +934,26 @@ mod tests {
     }
 
     #[test]
-    fn test_rust_not_satisfiable_on_ubuntu_only_apt() {
+    fn test_rust_toolchain_satisfiable_on_apt() {
         let registry = super::default_tool_registry();
         let available: HashSet<&str> = ["apt"].into_iter().collect();
-
-        // rust only has brew install option, so not satisfiable with just apt
-        let result = super::is_satisfiable(&super::RUST, &available, &registry);
-        assert!(result.is_err());
+
+        // Full Rust toolchain should be satisfiable via apt packages
+        assert!(super::is_satisfiable(&super::RUST, &available, &registry).is_ok());
+        assert!(super::is_satisfiable(&super::CARGO, &available, &registry).is_ok());
+        assert!(super::is_satisfiable(&super::CLIPPY, &available, &registry).is_ok());
+        assert!(super::is_satisfiable(&super::RUSTFMT, &available, &registry).is_ok());
+    }
+
+    #[test]
+    fn test_rust_toolchain_satisfiable_on_brew() {
+        let registry = super::default_tool_registry();
+        let available: HashSet<&str> = ["brew"].into_iter().collect();
+
+        // Full Rust toolchain should be satisfiable via brew (all → rustup)
+        assert!(super::is_satisfiable(&super::RUST, &available, &registry).is_ok());
+        assert!(super::is_satisfiable(&super::CARGO, &available, &registry).is_ok());
+        assert!(super::is_satisfiable(&super::CLIPPY, &available, &registry).is_ok());
+        assert!(super::is_satisfiable(&super::RUSTFMT, &available, &registry).is_ok());
     }
 }

gunbc-dag/src/makegen/registry.rs

@@ -770,16 +770,14 @@ impl ToolRegistry {
 
     /// Check if any codegen is needed.
     ///
-    /// BUILD-TIME EXCEPTION: This function previously checked the filesystem directly.
-    /// Since we've closed the execute_transport escape hatch, this check would need
-    /// to be part of a DAG execution. For now, we always return true (assume codegen needed).
+    /// Returns true if the registry contains tools that require code generation.
+    /// This is a conservative check — it does not verify whether generated files
+    /// are stale, only whether the registry has anything to generate.
     ///
-    /// TODO: Properly integrate this check into a build DAG or use a separate
-    /// build-time mechanism.
+    /// For staleness checking at runtime, the CI graph uses transport-based file
+    /// existence checks (see `CIOp::PrepareCodegenExistsCheck`).
     pub fn needs_codegen(&self) -> bool {
-        // STUB: Always return true until we have proper build-time file checking
-        // This means codegen will always run, which is safe but potentially redundant.
-        true
+        !self.tools.is_empty()
     }
 
     /// Check if any daggen is needed.

lib/review/src/graph.rs

@@ -380,6 +380,8 @@ pub fn build_diff_review_graph_with(
 ) -> Dag<ReviewGraphOp> {
     let mut dag = Dag::new();
 
+    let default_branch = config.default_branch.clone();
+
     // ========================================================================
     // Pipeline Config (zero-input node, emits constants)
     // ========================================================================
@@ -400,8 +402,8 @@ pub fn build_diff_review_graph_with(
     // ========================================================================
 
     // PrepareDiff - builds git diff request.
-    // base_ref default is baked into the op; the optional port allows CLI override.
-    // TODO: base_ref default should come from repo branching model config.
+    // base_ref default comes from pipeline config (GitConfig::default_branch);
+    // the optional port allows CLI override.
     dag.add_node(Node::opaque(
         "prepare_diff",
         vec![
@@ -410,7 +412,7 @@ pub fn build_diff_review_graph_with(
         ],
         vec![port("request", "TransportRequest")],
         ReviewGraphOp::Git(GitOps::PrepareDiff {
-            base_ref: "main".to_string(),
+            base_ref: default_branch,
             extensions: vec![],
         }),
     ));
@@ -896,6 +898,7 @@ mod tests {
             provider: "anthropic".to_string(),
             model: "claude-sonnet-4-20250514".to_string(),
             criteria: crate::graph_mock::default_criteria(),
+            default_branch: "develop".to_string(),
         };
         let dag = build_diff_review_graph_with(config);
         assert_eq!(dag.nodes.len(), 10);

lib/review/src/lib.rs

@@ -187,17 +187,31 @@ pub struct ReviewPipelineConfig {
     pub model: String,
     /// Review criteria to apply.
     pub criteria: Criteria,
+    /// Default branch for diff operations (e.g., "main", "master", "develop").
+    ///
+    /// Sourced from `GitConfig::default_branch` — the single source of truth
+    /// for the repo's branching model. The graph builder uses this instead of
+    /// hardcoding a branch name.
+    #[serde(default = "default_branch")]
+    pub default_branch: String,
+}
+
+fn default_branch() -> String {
+    "main".to_string()
 }
 
 impl ReviewPipelineConfig {
     /// Default pipeline config for gunbc's own repo.
     ///
-    /// TODO: Eventually load from repo config (e.g., `gunbc.toml`).
+    /// For repo-specific config, construct a `ReviewPipelineConfig` directly
+    /// or deserialize from a config file (e.g., `gunbc.toml`). The
+    /// `default_branch` field defaults to `"main"` via `GitConfig::default()`.
     pub fn gunbc_default() -> Self {
         Self {
             provider: "openai".to_string(),
             model: "gpt-4o".to_string(),
             criteria: crate::graph_mock::default_criteria(),
+            default_branch: "main".to_string(),
         }
     }
 }