From 6740de16871265877e411877760c1c4ff9e723ca Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 25 Mar 2026 15:49:09 +0000 Subject: [PATCH] Add AST-based symbol extraction using syn, with regex fallback Instead of regex-matching unified diff lines, the enrichment pipeline now fetches the full before/after file contents from GitHub, parses them with syn::parse_file, walks the AST to extract all fn items with their fully-qualified paths (including impl block context), and diffs the two symbol sets to classify Added/Modified/Deleted. This eliminates entire classes of regex bugs: - impl Trait for Type: syn's ItemImpl has separate trait_ and self_ty fields, so the implementing type is always correct - where clauses: parsed into generics.where_clause, never in the type - nested generics: syn handles all nesting correctly - #[cfg(test)] modules: checked via attributes, not path heuristics - Duplicate symbols: set-based diffing produces each symbol exactly once Falls back to regex per-file when: - AST parsing fails (syntax errors, macro-heavy files) - File contents can't be fetched (missing metadata, API errors) - File didn't exist at either ref (handled as all-Added or all-Deleted) New files: - src/ast_differ.rs: AST visitor + symbol set diffing (8 tests) Changes: - src/github.rs: Added parent_sha, owner, repo to PatchDiff; added fetch_file_contents() for raw file retrieval - src/diff_analyzer.rs: extract_symbols() now tries AST first; regex path renamed to extract_symbols_regex() - Cargo.toml: Added quote dependency for token stream hashing https://claude.ai/code/session_01P1LKP6aqGt68rQAXrF6kSE --- Cargo.lock | 1 + Cargo.toml | 1 + src/ast_differ.rs | 460 +++++++++++++++++++++++++++++++++++++++++++ src/diff_analyzer.rs | 172 ++++++++++------ src/github.rs | 97 ++++++++- src/main.rs | 5 +- 6 files changed, 673 insertions(+), 63 deletions(-) create mode 100644 src/ast_differ.rs diff --git a/Cargo.lock b/Cargo.lock index 73b0570..1738227 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -104,6 +104,7 @@ dependencies = [ "anyhow", "clap", "git2", + "quote", "regex", "reqwest", "semver", diff --git a/Cargo.toml b/Cargo.toml index 33de946..7db90dc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,6 +15,7 @@ walkdir = "2" anyhow = "1" semver = "1" syn = { version = "2", features = ["full", "visit"] } +quote = "1" [dev-dependencies] tempfile = "3" diff --git a/src/ast_differ.rs b/src/ast_differ.rs new file mode 100644 index 0000000..42cd295 --- /dev/null +++ b/src/ast_differ.rs @@ -0,0 +1,460 @@ +//! AST-based symbol extraction using `syn`. +//! +//! Instead of regex-matching unified diff lines, this module parses +//! the before/after Rust source files into ASTs and diffs the function +//! sets to precisely identify Added, Modified, and Deleted symbols. + +use std::collections::{HashMap, HashSet}; + +use syn::visit::Visit; +use syn::{ItemFn, ItemImpl, ItemMod, ImplItemFn}; + +use crate::diff_analyzer::{ChangeType, VulnerableSymbol}; + +/// A function symbol extracted from a parsed AST. +#[derive(Debug, Clone)] +struct FnSymbol { + /// Fully qualified name: `module::Type::method` or `module::free_fn` + qualified_name: String, + /// Hash of the function body tokens for detecting modifications + body_hash: u64, +} + +/// Extract all function symbols from a Rust source string. +/// +/// Parses the source with `syn::parse_file` and walks the AST to find +/// all `fn` items, including methods inside `impl` blocks. Returns +/// `None` if parsing fails. +pub fn extract_fn_symbols(source: &str, module_path: &str) -> Option> { + let file = syn::parse_file(source).ok()?; + let mut visitor = SymbolVisitor { + module_path: module_path.to_string(), + impl_type_stack: Vec::new(), + in_test: false, + symbols: Vec::new(), + }; + visitor.visit_file(&file); + Some(visitor.symbols) +} + +/// Diff two sets of function symbols to produce VulnerableSymbol entries. +/// +/// - Functions in `after` but not `before`: Added +/// - Functions in `before` but not `after`: Deleted +/// - Functions in both but with different body hashes: Modified +/// - Functions unchanged: omitted +pub fn diff_symbols( + before: &[FnSymbol], + after: &[FnSymbol], + file_path: &str, +) -> Vec { + let before_map: HashMap<&str, &FnSymbol> = before + .iter() + .map(|s| (s.qualified_name.as_str(), s)) + .collect(); + let after_map: HashMap<&str, &FnSymbol> = after + .iter() + .map(|s| (s.qualified_name.as_str(), s)) + .collect(); + + let before_names: HashSet<&str> = before_map.keys().copied().collect(); + let after_names: HashSet<&str> = after_map.keys().copied().collect(); + + let mut results = Vec::new(); + + // Added: in after but not before + for name in after_names.difference(&before_names) { + results.push(VulnerableSymbol { + file: file_path.to_string(), + function: name.to_string(), + change_type: ChangeType::Added, + }); + } + + // Deleted: in before but not after + for name in before_names.difference(&after_names) { + results.push(VulnerableSymbol { + file: file_path.to_string(), + function: name.to_string(), + change_type: ChangeType::Deleted, + }); + } + + // Modified: in both but with different body hash + for name in before_names.intersection(&after_names) { + let b = before_map[name]; + let a = after_map[name]; + if b.body_hash != a.body_hash { + results.push(VulnerableSymbol { + file: file_path.to_string(), + function: name.to_string(), + change_type: ChangeType::Modified, + }); + } + } + + results +} + +/// Extract symbols from before/after source and diff them. +/// Returns `None` if either file fails to parse. +pub fn ast_diff_symbols( + before_source: Option<&str>, + after_source: Option<&str>, + module_path: &str, + file_path: &str, +) -> Option> { + match (before_source, after_source) { + (Some(before), Some(after)) => { + let before_syms = extract_fn_symbols(before, module_path)?; + let after_syms = extract_fn_symbols(after, module_path)?; + Some(diff_symbols(&before_syms, &after_syms, file_path)) + } + (None, Some(after)) => { + // New file — all functions are Added + let after_syms = extract_fn_symbols(after, module_path)?; + Some( + after_syms + .into_iter() + .map(|s| VulnerableSymbol { + file: file_path.to_string(), + function: s.qualified_name, + change_type: ChangeType::Added, + }) + .collect(), + ) + } + (Some(before), None) => { + // Deleted file — all functions are Deleted + let before_syms = extract_fn_symbols(before, module_path)?; + Some( + before_syms + .into_iter() + .map(|s| VulnerableSymbol { + file: file_path.to_string(), + function: s.qualified_name, + change_type: ChangeType::Deleted, + }) + .collect(), + ) + } + (None, None) => Some(Vec::new()), + } +} + +// ── AST Visitor ────────────────────────────────────────────────────── + +struct SymbolVisitor { + module_path: String, + /// Stack of impl type names (for nested impls, though rare in practice) + impl_type_stack: Vec, + /// Whether we're inside a #[cfg(test)] module + in_test: bool, + symbols: Vec, +} + +impl SymbolVisitor { + /// Build the fully qualified name for a function. + fn qualify(&self, fn_name: &str) -> String { + let mut parts: Vec<&str> = Vec::new(); + if !self.module_path.is_empty() { + parts.push(&self.module_path); + } + if let Some(impl_type) = self.impl_type_stack.last() { + parts.push(impl_type); + } + parts.push(fn_name); + parts.join("::") + } + + /// Check if an item has #[test] or #[cfg(test)] attributes. + fn has_test_attr(attrs: &[syn::Attribute]) -> bool { + for attr in attrs { + if attr.path().is_ident("test") { + return true; + } + if attr.path().is_ident("cfg") { + // Check for #[cfg(test)] + let tokens = attr.meta.to_token_stream().to_string(); + if tokens.contains("test") { + return true; + } + } + } + false + } + + /// Extract a simple type name from a syn::Type, stripping generics. + fn type_name(ty: &syn::Type) -> Option { + match ty { + syn::Type::Path(type_path) => { + // Use the last segment's ident (e.g., for `std::convert::From` → `From`, + // but for `MyStruct` → `MyStruct`) + let last = type_path.path.segments.last()?; + Some(last.ident.to_string()) + } + syn::Type::Reference(type_ref) => Self::type_name(&type_ref.elem), + _ => None, + } + } + + /// Hash function body tokens for modification detection. + fn hash_body(block: &syn::Block) -> u64 { + use std::hash::{Hash, Hasher}; + // Use the token stream string as the hash input. + // This normalizes whitespace but preserves semantics. + let tokens = block.to_token_stream().to_string(); + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + tokens.hash(&mut hasher); + hasher.finish() + } + + fn record_fn(&mut self, name: &str, attrs: &[syn::Attribute], body: &syn::Block) { + // Skip test functions + if Self::has_test_attr(attrs) || name.starts_with("test_") { + return; + } + if self.in_test { + return; + } + + self.symbols.push(FnSymbol { + qualified_name: self.qualify(name), + body_hash: Self::hash_body(body), + }); + } +} + +impl<'ast> Visit<'ast> for SymbolVisitor { + fn visit_item_fn(&mut self, node: &'ast ItemFn) { + let name = node.sig.ident.to_string(); + self.record_fn(&name, &node.attrs, &node.block); + // Don't recurse into nested items — we handle those at the top level + } + + fn visit_item_impl(&mut self, node: &'ast ItemImpl) { + // For `impl Trait for Type`, use Type (self_ty), not Trait. + // For `impl Type`, also use self_ty. syn gives us exactly the right field. + if let Some(type_name) = Self::type_name(&node.self_ty) { + self.impl_type_stack.push(type_name); + + // Visit each method in the impl block + for item in &node.items { + if let syn::ImplItem::Fn(method) = item { + self.visit_impl_item_fn(method); + } + } + + self.impl_type_stack.pop(); + } + // Don't call the default visit — we manually visited methods above + } + + fn visit_impl_item_fn(&mut self, node: &'ast ImplItemFn) { + let name = node.sig.ident.to_string(); + self.record_fn(&name, &node.attrs, &node.block); + } + + fn visit_item_mod(&mut self, node: &'ast ItemMod) { + // Skip #[cfg(test)] modules entirely + if Self::has_test_attr(&node.attrs) { + return; + } + + // Recurse into inline modules (those with `mod foo { ... }`) + if let Some((_, items)) = &node.content { + let old_module = self.module_path.clone(); + let mod_name = node.ident.to_string(); + self.module_path = if self.module_path.is_empty() { + mod_name + } else { + format!("{}::{}", self.module_path, mod_name) + }; + + for item in items { + self.visit_item(item); + } + + self.module_path = old_module; + } + } +} + +use quote::ToTokens; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_extract_simple_fn() { + let source = r#" + pub fn hello() { + println!("hello"); + } + "#; + let symbols = extract_fn_symbols(source, "mymod").unwrap(); + assert_eq!(symbols.len(), 1); + assert_eq!(symbols[0].qualified_name, "mymod::hello"); + } + + #[test] + fn test_extract_impl_methods() { + let source = r#" + struct Request; + impl Request { + pub fn parse(buf: &[u8]) -> Self { Request } + fn internal() {} + } + "#; + let symbols = extract_fn_symbols(source, "http::request").unwrap(); + assert_eq!(symbols.len(), 2); + let names: Vec<&str> = symbols.iter().map(|s| s.qualified_name.as_str()).collect(); + assert!(names.contains(&"http::request::Request::parse")); + assert!(names.contains(&"http::request::Request::internal")); + } + + #[test] + fn test_trait_impl_uses_type_not_trait() { + // This is the key test — impl Trait for Type should use Type + let source = r#" + struct PyObject; + impl From for PyObject { + fn from(val: T) -> Self { PyObject } + } + "#; + let symbols = extract_fn_symbols(source, "instance").unwrap(); + assert_eq!(symbols.len(), 1); + assert_eq!( + symbols[0].qualified_name, "instance::PyObject::from", + "Should use implementing type (PyObject), not trait (From)" + ); + } + + #[test] + fn test_complex_generics_no_leak() { + // impl From> for PyObject where T: AsRef + let source = r#" + struct PyObject; + struct Py(T); + trait AsRef {} + impl From> for PyObject where T: AsRef { + fn from(val: Py) -> Self { PyObject } + } + "#; + let symbols = extract_fn_symbols(source, "instance").unwrap(); + assert_eq!(symbols.len(), 1); + let name = &symbols[0].qualified_name; + assert!( + !name.contains("for "), + "No 'for' leak: got '{}'", name + ); + assert!( + !name.contains("where"), + "No 'where' leak: got '{}'", name + ); + assert_eq!(name, "instance::PyObject::from"); + } + + #[test] + fn test_test_functions_skipped() { + let source = r#" + pub fn real_function() {} + + #[test] + fn test_something() {} + + fn test_other_thing() {} + "#; + let symbols = extract_fn_symbols(source, "").unwrap(); + assert_eq!(symbols.len(), 1); + assert_eq!(symbols[0].qualified_name, "real_function"); + } + + #[test] + fn test_cfg_test_module_skipped() { + let source = r#" + pub fn real_function() {} + + #[cfg(test)] + mod tests { + fn helper() {} + #[test] + fn test_it() {} + } + "#; + let symbols = extract_fn_symbols(source, "mymod").unwrap(); + assert_eq!(symbols.len(), 1); + assert_eq!(symbols[0].qualified_name, "mymod::real_function"); + } + + #[test] + fn test_diff_added_deleted_modified() { + let before = vec![ + FnSymbol { qualified_name: "foo".into(), body_hash: 100 }, + FnSymbol { qualified_name: "bar".into(), body_hash: 200 }, + FnSymbol { qualified_name: "old_fn".into(), body_hash: 300 }, + ]; + let after = vec![ + FnSymbol { qualified_name: "foo".into(), body_hash: 100 }, // unchanged + FnSymbol { qualified_name: "bar".into(), body_hash: 999 }, // modified + FnSymbol { qualified_name: "new_fn".into(), body_hash: 400 }, // added + ]; + + let result = diff_symbols(&before, &after, "src/lib.rs"); + + let added: Vec<_> = result.iter().filter(|s| matches!(s.change_type, ChangeType::Added)).collect(); + let deleted: Vec<_> = result.iter().filter(|s| matches!(s.change_type, ChangeType::Deleted)).collect(); + let modified: Vec<_> = result.iter().filter(|s| matches!(s.change_type, ChangeType::Modified)).collect(); + + assert_eq!(added.len(), 1); + assert_eq!(added[0].function, "new_fn"); + + assert_eq!(deleted.len(), 1); + assert_eq!(deleted[0].function, "old_fn"); + + assert_eq!(modified.len(), 1); + assert_eq!(modified[0].function, "bar"); + + // foo is unchanged — should NOT appear + assert!(!result.iter().any(|s| s.function == "foo")); + } + + #[test] + fn test_ast_diff_end_to_end() { + let before_src = r#" + struct Decoder; + impl Decoder { + pub fn decode(buf: &[u8]) -> Self { Decoder } + fn old_helper() {} + } + "#; + let after_src = r#" + struct Decoder; + impl Decoder { + pub fn decode(buf: &[u8]) -> Self { + let x = 1; + Decoder + } + fn new_helper() {} + } + "#; + + let result = ast_diff_symbols( + Some(before_src), + Some(after_src), + "proto::h1", + "src/proto/h1/decode.rs", + ) + .unwrap(); + + let names: Vec<(&str, &ChangeType)> = result + .iter() + .map(|s| (s.function.as_str(), &s.change_type)) + .collect(); + + assert!(names.iter().any(|(n, t)| *n == "proto::h1::Decoder::decode" && matches!(t, ChangeType::Modified))); + assert!(names.iter().any(|(n, t)| *n == "proto::h1::Decoder::old_helper" && matches!(t, ChangeType::Deleted))); + assert!(names.iter().any(|(n, t)| *n == "proto::h1::Decoder::new_helper" && matches!(t, ChangeType::Added))); + } +} diff --git a/src/diff_analyzer.rs b/src/diff_analyzer.rs index 92640e3..03c7028 100644 --- a/src/diff_analyzer.rs +++ b/src/diff_analyzer.rs @@ -22,14 +22,82 @@ pub enum ChangeType { Deleted, } -/// Extract function signatures from a patch diff. +/// Extract function signatures using AST parsing where possible, with regex fallback. /// -/// Strategy: -/// 1. Look at unified diff hunk headers (`@@ ... @@ fn ...`) which often contain -/// the enclosing function name. -/// 2. Look at added/removed lines containing `fn ` declarations. -/// 3. Infer module path from the file path. -pub fn extract_symbols(diff: &PatchDiff) -> Vec { +/// For each changed `.rs` file, fetches the full before/after contents from GitHub, +/// parses them with `syn`, and diffs the function sets to find Added/Modified/Deleted +/// symbols. Falls back to regex-based extraction per-file when AST parsing fails +/// (syntax errors, macro-heavy files) or when file contents can't be fetched. +pub fn extract_symbols( + diff: &PatchDiff, + gh: &crate::github::GithubClient, +) -> Vec { + let mut all_symbols = Vec::new(); + let has_metadata = !diff.owner.is_empty() && !diff.repo.is_empty(); + + for file_patch in &diff.files { + if is_test_file(&file_patch.filename) { + continue; + } + + let module_path = file_path_to_module(&file_patch.filename); + + // Try AST-based extraction if we have the metadata to fetch files + if has_metadata { + if let Some(parent) = &diff.parent_sha { + let before = gh + .fetch_file_contents( + &diff.owner, + &diff.repo, + &file_patch.filename, + parent, + ) + .ok() + .flatten(); + + let after = gh + .fetch_file_contents( + &diff.owner, + &diff.repo, + &file_patch.filename, + &diff.commit_sha, + ) + .ok() + .flatten(); + + if let Some(ast_symbols) = crate::ast_differ::ast_diff_symbols( + before.as_deref(), + after.as_deref(), + &module_path, + &file_patch.filename, + ) { + if !ast_symbols.is_empty() || before.is_some() || after.is_some() { + // AST succeeded — use its results (even if empty means no changes) + all_symbols.extend(ast_symbols); + continue; + } + } + } + } + + // Fallback: regex-based extraction for this file + let file_diff = PatchDiff { + commit_sha: diff.commit_sha.clone(), + owner: String::new(), + repo: String::new(), + parent_sha: None, + files: vec![file_patch.clone()], + }; + all_symbols.extend(extract_symbols_regex(&file_diff)); + } + + all_symbols +} + +/// Regex-based fallback for extracting function signatures from unified diffs. +/// +/// Used when AST parsing fails or file contents can't be fetched. +pub(crate) fn extract_symbols_regex(diff: &PatchDiff) -> Vec { let mut symbols: Vec = Vec::new(); let fn_decl_re = Regex::new( r"(?:pub\s+(?:\(crate\)\s+)?)?(?:unsafe\s+)?(?:async\s+)?fn\s+([a-zA-Z_][a-zA-Z0-9_]*)", @@ -507,9 +575,8 @@ mod tests { fn test_consecutive_hunks_preserve_fn_context() { // When consecutive hunks are in the same function and hunk 1 header // shows `fn foo`, hunk 2 (with `impl` header) should preserve context. - let diff = PatchDiff { - commit_sha: "abc123".to_string(), - files: vec![crate::github::FilePatch { + let diff = PatchDiff::for_test("abc123", + vec![crate::github::FilePatch { filename: "src/keccak/xof.rs".to_string(), patch: concat!( "@@ -100,6 +100,8 @@ pub(crate) fn squeeze(out: &mut [u8]) {\n", @@ -524,9 +591,9 @@ mod tests { ) .to_string(), }], - }; + ); - let symbols = extract_symbols(&diff); + let symbols = extract_symbols_regex(&diff); assert!( symbols.iter().any(|s| s.function.contains("squeeze")), "Expected squeeze to be found, got: {:?}", @@ -539,9 +606,8 @@ mod tests { // Reproduces the real xof.rs scenario: BOTH hunk headers show `impl`, // fn declaration is too far above for git to include. Changes should // still be captured at the impl-type level. - let diff = PatchDiff { - commit_sha: "6bbe15ec".to_string(), - files: vec![crate::github::FilePatch { + let diff = PatchDiff::for_test("6bbe15ec", + vec![crate::github::FilePatch { filename: "crates/algorithms/sha3/src/generic_keccak/xof.rs".to_string(), patch: concat!( "@@ -290,6 +290,12 @@ impl KeccakState\n", @@ -563,9 +629,9 @@ mod tests { ) .to_string(), }], - }; + ); - let symbols = extract_symbols(&diff); + let symbols = extract_symbols_regex(&diff); assert!( !symbols.is_empty(), "Expected symbols from impl body changes, got none" @@ -579,9 +645,8 @@ mod tests { #[test] fn test_extract_fn_from_diff() { - let diff = PatchDiff { - commit_sha: "abc123".to_string(), - files: vec![crate::github::FilePatch { + let diff = PatchDiff::for_test("abc123", + vec![crate::github::FilePatch { filename: "src/http/request.rs".to_string(), patch: r#"@@ -10,6 +10,8 @@ impl Request { pub fn parse(buf: &[u8]) -> Result { @@ -592,9 +657,9 @@ mod tests { "# .to_string(), }], - }; + ); - let symbols = extract_symbols(&diff); + let symbols = extract_symbols_regex(&diff); assert!(!symbols.is_empty()); assert!(symbols.iter().any(|s| s.function.contains("parse"))); } @@ -647,9 +712,8 @@ mod tests { #[test] fn test_for_keyword_no_longer_leaks_into_symbol() { // Reproduces the pyo3 RUSTSEC-2020-0074 bug - let diff = PatchDiff { - commit_sha: "abc123".to_string(), - files: vec![crate::github::FilePatch { + let diff = PatchDiff::for_test("abc123", + vec![crate::github::FilePatch { filename: "src/instance.rs".to_string(), patch: concat!( "@@ -495,7 +495,9 @@ impl std::convert::From> for PyObject\n", @@ -660,9 +724,9 @@ mod tests { ) .to_string(), }], - }; + ); - let symbols = extract_symbols(&diff); + let symbols = extract_symbols_regex(&diff); for s in &symbols { assert!( !s.function.contains("for "), @@ -681,9 +745,8 @@ mod tests { #[test] fn test_where_clause_no_longer_leaks_into_symbol() { // Reproduces the lock_api RUSTSEC-2020-0070 bug - let diff = PatchDiff { - commit_sha: "abc123".to_string(), - files: vec![crate::github::FilePatch { + let diff = PatchDiff::for_test("abc123", + vec![crate::github::FilePatch { filename: "lock_api/src/mutex.rs".to_string(), patch: concat!( "@@ -100,6 +100,8 @@ impl Mutex where R: Send {\n", @@ -692,9 +755,9 @@ mod tests { ) .to_string(), }], - }; + ); - let symbols = extract_symbols(&diff); + let symbols = extract_symbols_regex(&diff); for s in &symbols { assert!( !s.function.contains("where"), @@ -714,9 +777,8 @@ mod tests { #[test] fn test_test_functions_filtered_by_attribute() { // #[test] functions in library source should be skipped - let diff = PatchDiff { - commit_sha: "abc123".to_string(), - files: vec![crate::github::FilePatch { + let diff = PatchDiff::for_test("abc123", + vec![crate::github::FilePatch { filename: "src/pycell/impl_.rs".to_string(), patch: concat!( "@@ -100,6 +100,12 @@ impl PyCell {\n", @@ -731,9 +793,9 @@ mod tests { ) .to_string(), }], - }; + ); - let symbols = extract_symbols(&diff); + let symbols = extract_symbols_regex(&diff); assert!( !symbols.iter().any(|s| s.function.contains("test_inherited")), "test_ functions should be filtered out, got: {:?}", @@ -750,9 +812,8 @@ mod tests { #[test] fn test_test_functions_filtered_by_name_prefix() { // test_ prefix functions should be filtered even without #[test] attribute - let diff = PatchDiff { - commit_sha: "abc123".to_string(), - files: vec![crate::github::FilePatch { + let diff = PatchDiff::for_test("abc123", + vec![crate::github::FilePatch { filename: "src/lib.rs".to_string(), patch: concat!( "@@ -10,6 +10,8 @@\n", @@ -765,9 +826,9 @@ mod tests { ) .to_string(), }], - }; + ); - let symbols = extract_symbols(&diff); + let symbols = extract_symbols_regex(&diff); assert!( !symbols.iter().any(|s| s.function.contains("test_combine")), "test_ prefix functions should be filtered, got: {:?}", @@ -783,9 +844,8 @@ mod tests { #[test] fn test_cfg_test_functions_filtered() { // Functions after #[cfg(test)] should be filtered - let diff = PatchDiff { - commit_sha: "abc123".to_string(), - files: vec![crate::github::FilePatch { + let diff = PatchDiff::for_test("abc123", + vec![crate::github::FilePatch { filename: "src/core.rs".to_string(), patch: concat!( "@@ -200,6 +200,10 @@\n", @@ -796,9 +856,9 @@ mod tests { ) .to_string(), }], - }; + ); - let symbols = extract_symbols(&diff); + let symbols = extract_symbols_regex(&diff); assert!( !symbols.iter().any(|s| s.function.contains("helper_for_tests")), "#[cfg(test)] functions should be filtered, got: {:?}", @@ -809,9 +869,8 @@ mod tests { #[test] fn test_duplicate_fn_deduped_as_modified() { // When a fn is both removed (-) and added (+), it should appear once as Modified - let diff = PatchDiff { - commit_sha: "abc123".to_string(), - files: vec![crate::github::FilePatch { + let diff = PatchDiff::for_test("abc123", + vec![crate::github::FilePatch { filename: "src/reader.rs".to_string(), patch: concat!( "@@ -10,6 +10,8 @@ impl Reader {\n", @@ -823,9 +882,9 @@ mod tests { ) .to_string(), }], - }; + ); - let symbols = extract_symbols(&diff); + let symbols = extract_symbols_regex(&diff); let open_mmap_syms: Vec<_> = symbols .iter() .filter(|s| s.function.contains("open_mmap")) @@ -884,9 +943,8 @@ mod tests { fn test_trait_as_type_not_in_symbols() { // End-to-end: hunk header with trait impl truncated should not // produce symbols with trait name as type - let diff = PatchDiff { - commit_sha: "abc123".to_string(), - files: vec![crate::github::FilePatch { + let diff = PatchDiff::for_test("abc123", + vec![crate::github::FilePatch { filename: "src/util.rs".to_string(), patch: concat!( "@@ -10,6 +10,8 @@ impl From\n", @@ -897,9 +955,9 @@ mod tests { ) .to_string(), }], - }; + ); - let symbols = extract_symbols(&diff); + let symbols = extract_symbols_regex(&diff); for s in &symbols { assert!( !s.function.contains("From::"), diff --git a/src/github.rs b/src/github.rs index 50c7756..a13aa97 100644 --- a/src/github.rs +++ b/src/github.rs @@ -13,8 +13,14 @@ pub struct GithubClient { } #[derive(Debug, Deserialize)] -struct PrResponse { +struct PrDetailResponse { merge_commit_sha: Option, + base: Option, +} + +#[derive(Debug, Deserialize)] +struct PrBranch { + sha: String, } #[derive(Debug, Deserialize)] @@ -27,6 +33,11 @@ struct CommitFile { #[derive(Debug, Clone)] pub struct PatchDiff { pub commit_sha: String, + /// Owner/repo needed for fetching full file contents for AST parsing. + pub owner: String, + pub repo: String, + /// Parent commit SHA, used to fetch the "before" version of files. + pub parent_sha: Option, pub files: Vec, } @@ -36,6 +47,20 @@ pub struct FilePatch { pub patch: String, } +impl PatchDiff { + /// Create a PatchDiff for testing (no owner/repo/parent metadata). + #[cfg(test)] + pub fn for_test(commit_sha: &str, files: Vec) -> Self { + Self { + commit_sha: commit_sha.to_string(), + owner: String::new(), + repo: String::new(), + parent_sha: None, + files, + } + } +} + impl GithubClient { pub fn new(token: Option) -> Self { Self { @@ -88,12 +113,12 @@ impl GithubClient { /// Fetch all .rs file changes from a PR using the PR files endpoint. /// This captures all changes across the PR, not just a single commit. fn fetch_pr_diff(&self, owner: &str, repo: &str, number: u64) -> Result { - // Get merge commit SHA for the entry metadata + // Get merge commit SHA and base ref for the entry metadata let pr_url = format!( "https://api.github.com/repos/{}/{}/pulls/{}", owner, repo, number ); - let pr_resp: PrResponse = self + let pr_resp: PrDetailResponse = self .get(&pr_url) .send() .context("fetching PR metadata")? @@ -106,6 +131,8 @@ impl GithubClient { .merge_commit_sha .unwrap_or_else(|| format!("pr-{}", number)); + let parent_sha = pr_resp.base.map(|b| b.sha); + // Fetch all files changed in the PR (paginated, up to 300 files) let files_url = format!( "https://api.github.com/repos/{}/{}/pulls/{}/files?per_page=100", @@ -131,7 +158,13 @@ impl GithubClient { }) .collect(); - Ok(PatchDiff { commit_sha, files }) + Ok(PatchDiff { + commit_sha, + owner: owner.to_string(), + repo: repo.to_string(), + parent_sha, + files, + }) } fn fetch_commit_diff(&self, owner: &str, repo: &str, sha: &str) -> Result { @@ -150,11 +183,23 @@ impl GithubClient { #[derive(Deserialize)] struct CommitResponse { sha: String, + parents: Option>, files: Option>, } + #[derive(Deserialize)] + struct CommitParent { + sha: String, + } + let commit: CommitResponse = resp.json().context("parsing commit response")?; + let parent_sha = commit + .parents + .as_ref() + .and_then(|p| p.first()) + .map(|p| p.sha.clone()); + let files = commit .files .unwrap_or_default() @@ -170,7 +215,51 @@ impl GithubClient { Ok(PatchDiff { commit_sha: commit.sha, + owner: owner.to_string(), + repo: repo.to_string(), + parent_sha, files, }) } + + /// Fetch the raw contents of a file at a specific ref (commit SHA, branch, tag). + /// Returns `Ok(None)` if the file doesn't exist at that ref (404). + pub fn fetch_file_contents( + &self, + owner: &str, + repo: &str, + path: &str, + ref_: &str, + ) -> Result> { + let url = format!( + "https://api.github.com/repos/{}/{}/contents/{}?ref={}", + owner, repo, path, ref_ + ); + + let resp = self + .client + .get(&url) + .header(USER_AGENT, "cargo-deep-audit/0.1") + .header(ACCEPT, "application/vnd.github.v3.raw"); + + let resp = if let Some(ref token) = self.token { + resp.header(AUTHORIZATION, format!("Bearer {}", token)) + } else { + resp + }; + + let resp = resp.send().context("fetching file contents")?; + + if resp.status() == reqwest::StatusCode::NOT_FOUND { + return Ok(None); + } + + let body = resp + .error_for_status() + .context("file contents API error")? + .text() + .context("reading file contents")?; + + Ok(Some(body)) + } } diff --git a/src/main.rs b/src/main.rs index 1f31f14..4e41863 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,5 +1,6 @@ mod advisory; mod analyzer; +mod ast_differ; mod db; mod diff_analyzer; mod github; @@ -245,7 +246,7 @@ fn run_enrich(args: EnrichArgs) -> Result<()> { diff.files.len() ); new_sha = Some(diff.commit_sha.clone()); - new_symbols = extract_symbols(&diff); + new_symbols = extract_symbols(&diff, &gh); break; } Ok(None) => { @@ -368,7 +369,7 @@ fn run_enrich(args: EnrichArgs) -> Result<()> { diff.files.len() ); entry.commit_sha = Some(diff.commit_sha.clone()); - let symbols = extract_symbols(&diff); + let symbols = extract_symbols(&diff, &gh); if symbols.is_empty() { println!(" No function signatures extracted from diff"); } else {