diff --git a/docs/plans/2026-04-02-004-feat-python-checks-validation-coverage-plan.md b/docs/plans/2026-04-02-004-feat-python-checks-validation-coverage-plan.md index 98dce5e..6082635 100644 --- a/docs/plans/2026-04-02-004-feat-python-checks-validation-coverage-plan.md +++ b/docs/plans/2026-04-02-004-feat-python-checks-validation-coverage-plan.md @@ -1,7 +1,7 @@ --- title: "feat: Python starter checks, fixture test coverage, and real-world validation" type: feat -status: active +status: done date: 2026-04-02 origin: ~/.gstack/projects/brettdavies-agentnative/brett-main-design-20260327-214808.md --- @@ -96,7 +96,7 @@ real-world CLIs outside of self-dogfooding. ## Implementation Units -- [ ] **Unit 1: Implement `code-bare-except` Python source check** +- [x] **Unit 1: Implement `code-bare-except` Python source check** **Goal:** Detect bare `except:` clauses (without exception type) in Python source. Proves the tree-sitter-python pipeline works end-to-end. @@ -144,7 +144,7 @@ pipeline works end-to-end. --- -- [ ] **Unit 2: Implement `p4-sys-exit` Python source check** +- [x] **Unit 2: Implement `p4-sys-exit` Python source check** **Goal:** Detect `sys.exit()` calls outside `if __name__ == "__main__"` blocks. @@ -185,7 +185,7 @@ pipeline works end-to-end. --- -- [ ] **Unit 3: Implement `p6-no-color-source` Python source check** +- [x] **Unit 3: Implement `p6-no-color-source` Python source check** **Goal:** Detect `NO_COLOR` environment variable handling in Python source. @@ -225,7 +225,7 @@ pipeline works end-to-end. --- -- [ ] **Unit 4: Un-ignore and fix integration tests for fixtures** +- [x] **Unit 4: Un-ignore and fix integration tests for fixtures** **Goal:** Make the 3 `#[ignore]` integration tests pass and remove the `#[ignore]` attribute. @@ -269,7 +269,7 @@ pipeline works end-to-end. --- -- [ ] **Unit 5: Real-world validation against bird, xurl-rs, and ripgrep** +- [x] **Unit 5: Real-world validation against bird, xurl-rs, and ripgrep** **Goal:** Run agentnative against 3 real-world CLIs and document results. Satisfies design doc success criteria. @@ -308,7 +308,7 @@ pipeline works end-to-end. --- -- [ ] **Unit 6: Update design doc to reflect revised Python scope** +- [x] **Unit 6: Update design doc to reflect revised Python scope** **Goal:** Formally document that v0.1 ships 2-3 starter Python checks, with full coverage in v0.2. diff --git a/src/checks/behavioral/mod.rs b/src/checks/behavioral/mod.rs index 8510925..ec77a66 100644 --- a/src/checks/behavioral/mod.rs +++ b/src/checks/behavioral/mod.rs @@ -24,9 +24,8 @@ pub fn all_behavioral_checks() -> Vec> { #[cfg(test)] pub(crate) mod tests { - use std::cell::RefCell; - use std::collections::HashMap; use std::path::PathBuf; + use std::sync::OnceLock; use std::time::Duration; use crate::project::Project; @@ -44,7 +43,7 @@ pub(crate) mod tests { .expect("create test runner"), ), include_tests: false, - parsed_files: RefCell::new(HashMap::new()), + parsed_files: OnceLock::new(), } } @@ -103,7 +102,7 @@ pub(crate) mod tests { BinaryRunner::new(script_path, Duration::from_secs(5)).expect("create test runner"), ), include_tests: false, - parsed_files: RefCell::new(HashMap::new()), + parsed_files: OnceLock::new(), } } } diff --git a/src/checks/project/dry_run.rs b/src/checks/project/dry_run.rs index dcf6785..50935e5 100644 --- a/src/checks/project/dry_run.rs +++ b/src/checks/project/dry_run.rs @@ -106,9 +106,9 @@ impl Check for DryRunCheck { #[cfg(test)] mod tests { use super::*; - use std::cell::RefCell; use std::collections::HashMap; use std::path::PathBuf; + use std::sync::OnceLock; use crate::project::{Language, ParsedFile}; @@ -154,7 +154,7 @@ mod tests { manifest_path: Some(dir.join("Cargo.toml")), runner: None, include_tests: false, - parsed_files: RefCell::new(parsed), + parsed_files: OnceLock::from(parsed), } } diff --git a/src/checks/source/python/bare_except.rs b/src/checks/source/python/bare_except.rs new file mode 100644 index 0000000..47cfb2d --- /dev/null +++ b/src/checks/source/python/bare_except.rs @@ -0,0 +1,306 @@ +//! Check: Detect bare `except:` clauses in Python source. +//! +//! Principle: P4 (Actionable Errors) — bare `except:` swallows BaseException +//! (KeyboardInterrupt, SystemExit) and hides programming errors. Always specify +//! the exception type. Analogous to Rust's `code-unwrap`. + +use ast_grep_core::tree_sitter::LanguageExt; +use ast_grep_language::Python; + +use crate::check::Check; +use crate::project::{Language, Project}; +use crate::types::{CheckGroup, CheckLayer, CheckResult, CheckStatus, SourceLocation}; + +/// Check trait implementation for bare-except detection. +pub struct BareExceptCheck; + +impl Check for BareExceptCheck { + fn id(&self) -> &str { + "code-bare-except" + } + + fn group(&self) -> CheckGroup { + CheckGroup::CodeQuality + } + + fn layer(&self) -> CheckLayer { + CheckLayer::Source + } + + fn applicable(&self, project: &Project) -> bool { + project.language == Some(Language::Python) + } + + fn run(&self, project: &Project) -> anyhow::Result { + let parsed = project.parsed_files(); + let mut all_evidence = Vec::new(); + + for (path, parsed_file) in parsed.iter() { + let file_str = path.display().to_string(); + if let CheckStatus::Fail(evidence) = check_bare_except(&parsed_file.source, &file_str) { + all_evidence.push(evidence); + } + } + + let status = if all_evidence.is_empty() { + CheckStatus::Pass + } else { + CheckStatus::Fail(all_evidence.join("\n")) + }; + + Ok(CheckResult { + id: self.id().to_string(), + label: "No bare except: clauses".to_string(), + group: self.group(), + layer: self.layer(), + status, + }) + } +} + +/// Scan a Python source string for bare `except:` clauses. +/// +/// Walks the AST looking for `except_clause` nodes that have no exception type. +/// The tree-sitter-python grammar represents bare except as an `except_clause` +/// whose first non-keyword child is the `:` token (no type expression between +/// `except` and `:`). +pub(crate) fn check_bare_except(source: &str, file: &str) -> CheckStatus { + let root = Python.ast_grep(source); + let mut matches = Vec::new(); + walk(root.root(), file, &mut matches); + + if matches.is_empty() { + CheckStatus::Pass + } else { + let evidence = matches + .iter() + .map(|m| format!("{}:{}:{} — {}", m.file, m.line, m.column, m.text)) + .collect::>() + .join("\n"); + CheckStatus::Fail(evidence) + } +} + +fn walk<'a>( + node: ast_grep_core::Node<'a, ast_grep_core::tree_sitter::StrDoc>, + file: &str, + out: &mut Vec, +) { + if node.kind() == "except_clause" && is_bare_except(&node) { + let pos = node.start_pos(); + let snippet = node + .text() + .lines() + .next() + .unwrap_or("except:") + .trim() + .to_string(); + out.push(SourceLocation { + file: file.to_string(), + line: pos.line() + 1, + column: pos.column(&node) + 1, + text: snippet, + }); + } + for child in node.children() { + walk(child, file, out); + } +} + +/// A bare `except:` has no expression between `except` and `:`. +fn is_bare_except<'a>( + node: &ast_grep_core::Node<'a, ast_grep_core::tree_sitter::StrDoc>, +) -> bool { + let text = node.text(); + let first_line = text.lines().next().unwrap_or(""); + let Some((header, _)) = first_line.split_once(':') else { + return false; + }; + let trimmed = header.trim(); + trimmed == "except" +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn pass_when_typed_except() { + let source = "\ +try: + do_thing() +except ValueError: + handle_it() +"; + let status = check_bare_except(source, "src/foo.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn pass_when_multiple_typed_excepts() { + let source = "\ +try: + do_thing() +except (ValueError, KeyError) as e: + log(e) +except OSError: + cleanup() +"; + let status = check_bare_except(source, "src/foo.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn fail_when_bare_except() { + let source = "\ +try: + do_thing() +except: + pass +"; + let status = check_bare_except(source, "src/foo.py"); + assert!(matches!(status, CheckStatus::Fail(_))); + if let CheckStatus::Fail(evidence) = &status { + assert!(evidence.contains("except:")); + assert!(evidence.contains("src/foo.py")); + } + } + + #[test] + fn fail_when_bare_except_with_pass() { + let source = "\ +try: + risky() +except: pass +"; + let status = check_bare_except(source, "src/foo.py"); + assert!(matches!(status, CheckStatus::Fail(_))); + } + + #[test] + fn fail_counts_multiple_bare_excepts() { + let source = "\ +def a(): + try: + f() + except: + pass + +def b(): + try: + g() + except: + log() +"; + let status = check_bare_except(source, "src/multi.py"); + if let CheckStatus::Fail(evidence) = &status { + assert_eq!(evidence.lines().count(), 2); + } else { + panic!("expected Fail, got {:?}", status); + } + } + + #[test] + fn pass_when_typed_after_bare_in_different_file() { + // Regression guard: a bare except in one file shouldn't be masked by a typed + // one in the same source string. (Covered indirectly above; explicit here.) + let source = "\ +try: + a() +except Exception: + handle() +"; + let status = check_bare_except(source, "src/clean.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn ignores_except_inside_string_literal() { + // ast-grep is AST-aware — the literal `except:` in a string should not match. + let source = "\ +msg = \"never write `except:` in production\" +def main(): + return msg +"; + let status = check_bare_except(source, "src/strings.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn ignores_except_in_comment() { + let source = "\ +# remember: except: is bad style +def main(): + pass +"; + let status = check_bare_except(source, "src/comments.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn pass_with_no_python_files() { + // Empty source = no AST = pass. + let status = check_bare_except("", "src/empty.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn evidence_includes_line_and_column() { + let source = "\ +try: + work() +except: + pass +"; + let status = check_bare_except(source, "src/loc.py"); + if let CheckStatus::Fail(evidence) = &status { + // bare `except:` is on line 3 + assert!( + evidence.contains(":3:"), + "evidence missing line 3: {evidence}" + ); + } else { + panic!("expected Fail"); + } + } + + #[test] + fn applicable_for_python() { + let check = BareExceptCheck; + let dir = std::env::temp_dir().join(format!("anc-bare-except-py-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create test dir"); + std::fs::write( + dir.join("pyproject.toml"), + "[project]\nname = \"test\"\nversion = \"0.1.0\"\n", + ) + .expect("write test pyproject.toml"); + let project = Project::discover(&dir).expect("discover test project"); + assert!(check.applicable(&project)); + } + + #[test] + fn pass_when_typed_except_group() { + let source = "\ +try: + do_thing() +except* ValueError: + handle_it() +"; + let status = check_bare_except(source, "src/foo.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn not_applicable_for_rust() { + let check = BareExceptCheck; + let dir = std::env::temp_dir().join(format!("anc-bare-except-rs-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create test dir"); + std::fs::write( + dir.join("Cargo.toml"), + "[package]\nname = \"test\"\nversion = \"0.1.0\"\n", + ) + .expect("write test Cargo.toml"); + let project = Project::discover(&dir).expect("discover test project"); + assert!(!check.applicable(&project)); + } +} diff --git a/src/checks/source/python/mod.rs b/src/checks/source/python/mod.rs index afc7c11..ed4f9c5 100644 --- a/src/checks/source/python/mod.rs +++ b/src/checks/source/python/mod.rs @@ -1,10 +1,16 @@ +pub mod bare_except; +pub mod no_color; +pub mod sys_exit; + use crate::check::Check; /// Returns all Python source checks. -/// -/// Currently empty — Python checks will be added in a future unit. pub fn all_python_checks() -> Vec> { - vec![] + vec![ + Box::new(bare_except::BareExceptCheck), + Box::new(sys_exit::SysExitCheck), + Box::new(no_color::NoColorPythonCheck), + ] } #[cfg(test)] @@ -12,7 +18,11 @@ mod tests { use super::*; #[test] - fn python_checks_empty() { - assert!(all_python_checks().is_empty()); + fn python_checks_registered() { + let checks = all_python_checks(); + let ids: Vec<&str> = checks.iter().map(|c| c.id()).collect(); + assert!(ids.contains(&"code-bare-except")); + assert!(ids.contains(&"p4-sys-exit")); + assert!(ids.contains(&"p6-no-color")); } } diff --git a/src/checks/source/python/no_color.rs b/src/checks/source/python/no_color.rs new file mode 100644 index 0000000..2d8d732 --- /dev/null +++ b/src/checks/source/python/no_color.rs @@ -0,0 +1,251 @@ +//! Check: Detect NO_COLOR environment variable handling in Python source. +//! +//! Principle: P6 (Composable Structure) — CLIs must respect NO_COLOR. +//! See https://no-color.org/ +//! +//! The behavioral check is the primary gate; this source check returns Warn +//! (not Fail) when NO_COLOR is absent — many libraries (rich, click, colorama) +//! handle NO_COLOR transparently without explicit lookups in user code. + +use ast_grep_core::Pattern; +use ast_grep_core::tree_sitter::LanguageExt; +use ast_grep_language::Python; + +use crate::check::Check; +use crate::project::{Language, Project}; +use crate::source::has_string_literal_in; +use crate::types::{CheckGroup, CheckLayer, CheckResult, CheckStatus}; + +/// Check trait implementation for NO_COLOR detection in Python. +pub struct NoColorPythonCheck; + +impl Check for NoColorPythonCheck { + fn id(&self) -> &str { + "p6-no-color" + } + + fn group(&self) -> CheckGroup { + CheckGroup::P6 + } + + fn layer(&self) -> CheckLayer { + CheckLayer::Source + } + + fn applicable(&self, project: &Project) -> bool { + project.language == Some(Language::Python) + } + + fn run(&self, project: &Project) -> anyhow::Result { + let parsed = project.parsed_files(); + let mut found_any = false; + + for (_path, parsed_file) in parsed.iter() { + if source_handles_no_color(&parsed_file.source) { + found_any = true; + break; + } + } + + let status = if found_any { + CheckStatus::Pass + } else { + CheckStatus::Warn( + "No reference to NO_COLOR found in any Python source file. CLIs should respect \ + the NO_COLOR convention. Many libraries (rich, click, colorama) handle this \ + transparently — verify via the behavioral check. See https://no-color.org/" + .to_string(), + ) + }; + + Ok(CheckResult { + id: self.id().to_string(), + label: "Respects NO_COLOR".to_string(), + group: self.group(), + layer: self.layer(), + status, + }) + } +} + +/// Returns true if the source references NO_COLOR via a recognized Python +/// env-var access pattern, or as a string literal anywhere in the AST. +pub(crate) fn source_handles_no_color(source: &str) -> bool { + let root = Python.ast_grep(source); + + // Common explicit access patterns — parse source once, check all patterns. + let access_patterns = [ + r#"os.environ.get("NO_COLOR")"#, + r#"os.environ.get('NO_COLOR')"#, + r#"os.getenv("NO_COLOR")"#, + r#"os.getenv('NO_COLOR')"#, + r#"os.environ["NO_COLOR"]"#, + r#"os.environ['NO_COLOR']"#, + r#"environ.get("NO_COLOR")"#, + r#"environ.get('NO_COLOR')"#, + r#"getenv("NO_COLOR")"#, + r#"getenv('NO_COLOR')"#, + ]; + for p_str in access_patterns { + if let Ok(pattern) = Pattern::try_new(p_str, Python) { + if root.root().find(&pattern).is_some() { + return true; + } + } + } + + // Fallback: any string literal "NO_COLOR" or 'NO_COLOR' in the AST. + has_string_literal_in(source, "NO_COLOR", Language::Python) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn pass_with_os_environ_get() { + let source = "\ +import os + +def setup(): + if os.environ.get(\"NO_COLOR\"): + disable_color() +"; + assert!(source_handles_no_color(source)); + } + + #[test] + fn pass_with_os_getenv() { + let source = "\ +import os + +if os.getenv('NO_COLOR') is not None: + disable_color() +"; + assert!(source_handles_no_color(source)); + } + + #[test] + fn pass_with_subscript_access() { + let source = "\ +import os +val = os.environ[\"NO_COLOR\"] +"; + assert!(source_handles_no_color(source)); + } + + #[test] + fn pass_with_imported_environ() { + let source = "\ +from os import environ +val = environ.get('NO_COLOR') +"; + assert!(source_handles_no_color(source)); + } + + #[test] + fn pass_with_string_literal_constant() { + // Recognized via the string-literal fallback. + let source = "\ +NO_COLOR_ENV = \"NO_COLOR\" + +def disable(): + import os + return os.environ.get(NO_COLOR_ENV) +"; + assert!(source_handles_no_color(source)); + } + + #[test] + fn warn_when_no_no_color_anywhere() { + let source = "\ +def main(): + print(\"hello\") +"; + assert!(!source_handles_no_color(source)); + } + + #[test] + fn ignored_in_comments() { + // tree-sitter-python doesn't parse comment text as string literals. + let source = "\ +# remember NO_COLOR support +def main(): + print('hi') +"; + assert!(!source_handles_no_color(source)); + } + + #[test] + fn pass_with_bare_string_literal_only() { + let source = "CONST = \"NO_COLOR\"\n"; + assert!(source_handles_no_color(source)); + } + + #[test] + fn applicable_for_python() { + let check = NoColorPythonCheck; + let dir = std::env::temp_dir().join(format!("anc-nocolor-py-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create test dir"); + std::fs::write( + dir.join("pyproject.toml"), + "[project]\nname = \"test\"\nversion = \"0.1.0\"\n", + ) + .expect("write test pyproject.toml"); + let project = Project::discover(&dir).expect("discover test project"); + assert!(check.applicable(&project)); + } + + #[test] + fn not_applicable_for_rust() { + let check = NoColorPythonCheck; + let dir = std::env::temp_dir().join(format!("anc-nocolor-py-rs-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create test dir"); + std::fs::write( + dir.join("Cargo.toml"), + "[package]\nname = \"test\"\nversion = \"0.1.0\"\n", + ) + .expect("write test Cargo.toml"); + let project = Project::discover(&dir).expect("discover test project"); + assert!(!check.applicable(&project)); + } + + #[test] + fn run_emits_pass_when_no_color_present() { + let check = NoColorPythonCheck; + let dir = + std::env::temp_dir().join(format!("anc-nocolor-pass-test-{}", std::process::id())); + std::fs::create_dir_all(dir.join("src")).expect("create test dir"); + std::fs::write( + dir.join("pyproject.toml"), + "[project]\nname = \"test\"\nversion = \"0.1.0\"\n", + ) + .expect("write pyproject"); + std::fs::write( + dir.join("src/app.py"), + "import os\nif os.getenv('NO_COLOR'):\n pass\n", + ) + .expect("write app.py"); + let project = Project::discover(&dir).expect("discover"); + let result = check.run(&project).expect("check ran"); + assert_eq!(result.status, CheckStatus::Pass); + } + + #[test] + fn run_emits_warn_status_when_missing() { + let check = NoColorPythonCheck; + let dir = + std::env::temp_dir().join(format!("anc-nocolor-warn-test-{}", std::process::id())); + std::fs::create_dir_all(dir.join("src")).expect("create test dir"); + std::fs::write( + dir.join("pyproject.toml"), + "[project]\nname = \"test\"\nversion = \"0.1.0\"\n", + ) + .expect("write pyproject"); + std::fs::write(dir.join("src/app.py"), "def main():\n print('hi')\n") + .expect("write app.py"); + let project = Project::discover(&dir).expect("discover"); + let result = check.run(&project).expect("check ran"); + assert!(matches!(result.status, CheckStatus::Warn(_))); + } +} diff --git a/src/checks/source/python/sys_exit.rs b/src/checks/source/python/sys_exit.rs new file mode 100644 index 0000000..71af9c4 --- /dev/null +++ b/src/checks/source/python/sys_exit.rs @@ -0,0 +1,456 @@ +//! Check: Detect `sys.exit()` calls outside `if __name__ == "__main__":` guards. +//! +//! Principle: P4 (Actionable Errors) — library code should `raise` or `return`, +//! not call `sys.exit()`. Reserve `sys.exit()` for the entry-point script under +//! the `__main__` guard. Analogous to Rust's `p4-process-exit`. + +use ast_grep_core::Node; +use ast_grep_core::tree_sitter::{LanguageExt, StrDoc}; +use ast_grep_language::Python; + +use crate::check::Check; +use crate::project::{Language, Project}; +use crate::types::{CheckGroup, CheckLayer, CheckResult, CheckStatus, SourceLocation}; + +/// Check trait implementation for sys.exit() outside __main__ guard. +pub struct SysExitCheck; + +impl Check for SysExitCheck { + fn id(&self) -> &str { + "p4-sys-exit" + } + + fn group(&self) -> CheckGroup { + CheckGroup::P4 + } + + fn layer(&self) -> CheckLayer { + CheckLayer::Source + } + + fn applicable(&self, project: &Project) -> bool { + project.language == Some(Language::Python) + } + + fn run(&self, project: &Project) -> anyhow::Result { + let parsed = project.parsed_files(); + let mut all_evidence = Vec::new(); + + for (path, parsed_file) in parsed.iter() { + let file_str = path.display().to_string(); + // __main__.py is the Python entry point — sys.exit() is expected there, + // just as process::exit() is expected in Rust's main.rs. + if path.file_name().is_some_and(|f| f == "__main__.py") { + continue; + } + if let CheckStatus::Fail(evidence) = check_sys_exit(&parsed_file.source, &file_str) { + all_evidence.push(evidence); + } + } + + let status = if all_evidence.is_empty() { + CheckStatus::Pass + } else { + CheckStatus::Fail(all_evidence.join("\n")) + }; + + Ok(CheckResult { + id: self.id().to_string(), + label: "No sys.exit() outside __main__ guard".to_string(), + group: self.group(), + layer: self.layer(), + status, + }) + } +} + +/// Scan a Python source string for `sys.exit(...)` calls outside the +/// `if __name__ == "__main__":` guard. +pub(crate) fn check_sys_exit(source: &str, file: &str) -> CheckStatus { + let root = Python.ast_grep(source); + let mut matches = Vec::new(); + walk(root.root(), file, false, &mut matches); + + if matches.is_empty() { + CheckStatus::Pass + } else { + let evidence = matches + .iter() + .map(|m| format!("{}:{}:{} — {}", m.file, m.line, m.column, m.text)) + .collect::>() + .join("\n"); + CheckStatus::Fail(evidence) + } +} + +fn walk<'a>( + node: Node<'a, StrDoc>, + file: &str, + inside_main_guard: bool, + out: &mut Vec, +) { + // Detect `if __name__ == "__main__":` and treat its body as guarded. + let entering_guard = node.kind() == "if_statement" && is_main_guard(&node); + + if !inside_main_guard && is_sys_exit_call(&node) { + let pos = node.start_pos(); + let snippet = node + .text() + .lines() + .next() + .unwrap_or("sys.exit(...)") + .trim() + .to_string(); + out.push(SourceLocation { + file: file.to_string(), + line: pos.line() + 1, + column: pos.column(&node) + 1, + text: snippet, + }); + } + + let child_guarded = inside_main_guard || entering_guard; + for child in node.children() { + walk(child, file, child_guarded, out); + } +} + +/// Match `sys.exit(...)` call expressions. +/// +/// In tree-sitter-python a call like `sys.exit(1)` is a `call` node whose +/// `function` child is an `attribute` node with `object=sys` and `attribute=exit`. +fn is_sys_exit_call<'a>(node: &Node<'a, StrDoc>) -> bool { + if node.kind() != "call" { + return false; + } + let Some(func) = node.children().next() else { + return false; + }; + if func.kind() != "attribute" { + return false; + } + let text = func.text(); + text == "sys.exit" || text.replace(char::is_whitespace, "") == "sys.exit" +} + +/// Detect `if __name__ == "__main__":` guards. +/// +/// Handles canonical form, parenthesized, no-spaces, and reversed orderings. +fn is_main_guard<'a>(node: &Node<'a, StrDoc>) -> bool { + let text = node.text(); + let first_line = text.lines().next().unwrap_or("").trim(); + let first_line = first_line.split('#').next().unwrap_or("").trim(); + let header = first_line + .strip_prefix("if") + .unwrap_or("") + .trim() + .trim_end_matches(':') + .trim(); + // Strip outer parentheses: if (__name__ == "__main__"): + let header = header + .strip_prefix('(') + .and_then(|s| s.strip_suffix(')')) + .unwrap_or(header) + .trim(); + // Split on == and check both orderings + let Some((lhs, rhs)) = header.split_once("==") else { + return false; + }; + let (lhs, rhs) = (lhs.trim(), rhs.trim()); + let is_name = |s: &str| s == "__name__"; + let is_main = |s: &str| s == "\"__main__\"" || s == "'__main__'"; + (is_name(lhs) && is_main(rhs)) || (is_main(lhs) && is_name(rhs)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn pass_when_sys_exit_inside_main_guard() { + let source = "\ +import sys + +def main(): + return 0 + +if __name__ == \"__main__\": + sys.exit(main()) +"; + let status = check_sys_exit(source, "src/cli.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn pass_when_main_guard_uses_single_quotes() { + let source = "\ +import sys + +if __name__ == '__main__': + sys.exit(0) +"; + let status = check_sys_exit(source, "src/cli.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn fail_when_sys_exit_at_module_level() { + let source = "\ +import sys +sys.exit(1) +"; + let status = check_sys_exit(source, "src/bad.py"); + assert!(matches!(status, CheckStatus::Fail(_))); + if let CheckStatus::Fail(evidence) = &status { + assert!(evidence.contains("sys.exit")); + assert!(evidence.contains("src/bad.py")); + } + } + + #[test] + fn fail_when_sys_exit_in_function_outside_guard() { + let source = "\ +import sys + +def fail_hard(msg): + print(msg) + sys.exit(2) + +fail_hard('boom') +"; + let status = check_sys_exit(source, "src/lib.py"); + assert!(matches!(status, CheckStatus::Fail(_))); + } + + #[test] + fn evidence_records_line_number() { + let source = "\ +import sys +print('hi') +sys.exit(7) +"; + let status = check_sys_exit(source, "src/loc.py"); + if let CheckStatus::Fail(evidence) = &status { + assert!(evidence.contains(":3:"), "expected line 3, got: {evidence}"); + } else { + panic!("expected Fail"); + } + } + + #[test] + fn pass_when_main_guard_has_inline_comment() { + let source = "\ +import sys + +if __name__ == \"__main__\": # entry point + sys.exit(0) +"; + let status = check_sys_exit(source, "src/cli.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn ignores_builtin_exit() { + // `exit()` (the REPL builtin) is intentionally not flagged — it has + // different semantics and is not what this check targets. + let source = "\ +exit(1) +"; + let status = check_sys_exit(source, "src/builtin.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn ignores_unrelated_sys_calls() { + let source = "\ +import sys +sys.stderr.write('hi') +print(sys.argv) +"; + let status = check_sys_exit(source, "src/sys_other.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn pass_when_no_sys_exit_anywhere() { + let source = "\ +def add(a, b): + return a + b +"; + let status = check_sys_exit(source, "src/clean.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn fail_counts_multiple_unguarded_exits() { + let source = "\ +import sys + +def a(): + sys.exit(1) + +def b(): + sys.exit(2) +"; + let status = check_sys_exit(source, "src/multi.py"); + if let CheckStatus::Fail(evidence) = &status { + assert_eq!(evidence.lines().count(), 2); + } else { + panic!("expected Fail"); + } + } + + #[test] + fn nested_block_inside_guard_is_still_guarded() { + // Calls in nested ifs/loops under the __main__ guard are still considered + // inside the guard (the body of the guard is a CLI entry-point script). + let source = "\ +import sys + +if __name__ == \"__main__\": + try: + run() + except RuntimeError: + sys.exit(1) +"; + let status = check_sys_exit(source, "src/cli.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn ignores_sys_exit_in_string() { + let source = "\ +msg = \"call sys.exit(1) on failure\" +print(msg) +"; + let status = check_sys_exit(source, "src/strings.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn pass_when_main_guard_parenthesized() { + let source = "\ +import sys + +if (__name__ == '__main__'): + sys.exit(0) +"; + let status = check_sys_exit(source, "src/cli.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn pass_when_main_guard_no_spaces() { + let source = "\ +import sys + +if __name__==\"__main__\": + sys.exit(0) +"; + let status = check_sys_exit(source, "src/cli.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn pass_when_main_guard_reversed_double_quotes() { + let source = "\ +import sys + +if \"__main__\" == __name__: + sys.exit(0) +"; + let status = check_sys_exit(source, "src/cli.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn pass_when_main_guard_reversed_single_quotes() { + let source = "\ +import sys + +if '__main__' == __name__: + sys.exit(0) +"; + let status = check_sys_exit(source, "src/cli.py"); + assert_eq!(status, CheckStatus::Pass); + } + + #[test] + fn applicable_for_python() { + let check = SysExitCheck; + let dir = std::env::temp_dir().join(format!("anc-sysexit-py-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create test dir"); + std::fs::write( + dir.join("pyproject.toml"), + "[project]\nname = \"test\"\nversion = \"0.1.0\"\n", + ) + .expect("write test pyproject.toml"); + let project = Project::discover(&dir).expect("discover test project"); + assert!(check.applicable(&project)); + } + + #[test] + fn not_applicable_for_rust() { + let check = SysExitCheck; + let dir = std::env::temp_dir().join(format!("anc-sysexit-rs-{}", std::process::id())); + std::fs::create_dir_all(&dir).expect("create test dir"); + std::fs::write( + dir.join("Cargo.toml"), + "[package]\nname = \"test\"\nversion = \"0.1.0\"\n", + ) + .expect("write test Cargo.toml"); + let project = Project::discover(&dir).expect("discover test project"); + assert!(!check.applicable(&project)); + } + + #[test] + fn run_aggregates_across_files() { + let check = SysExitCheck; + let dir = std::env::temp_dir().join(format!("anc-sysexit-multi-{}", std::process::id())); + let src = dir.join("src"); + std::fs::create_dir_all(&src).expect("create src dir"); + std::fs::write( + dir.join("pyproject.toml"), + "[project]\nname = \"test\"\nversion = \"0.1.0\"\n", + ) + .expect("write pyproject"); + std::fs::write( + src.join("good.py"), + "import sys\nif __name__ == \"__main__\":\n sys.exit(0)\n", + ) + .expect("write good.py"); + std::fs::write(src.join("bad.py"), "import sys\nsys.exit(1)\n").expect("write bad.py"); + let project = Project::discover(&dir).expect("discover"); + let result = check.run(&project).expect("check ran"); + assert!(matches!(result.status, CheckStatus::Fail(_))); + if let CheckStatus::Fail(evidence) = &result.status { + assert!( + evidence.contains("bad.py"), + "evidence should reference bad.py: {evidence}" + ); + assert!( + !evidence.contains("good.py"), + "evidence should not reference good.py: {evidence}" + ); + } + } + + #[test] + fn run_skips_dunder_main_py() { + let check = SysExitCheck; + let dir = + std::env::temp_dir().join(format!("anc-sysexit-skip-main-{}", std::process::id())); + let src = dir.join("src"); + std::fs::create_dir_all(&src).expect("create src dir"); + std::fs::write( + dir.join("pyproject.toml"), + "[project]\nname = \"test\"\nversion = \"0.1.0\"\n", + ) + .expect("write pyproject"); + std::fs::write(src.join("__main__.py"), "import sys\nsys.exit(0)\n") + .expect("write __main__.py"); + let project = Project::discover(&dir).expect("discover"); + let result = check.run(&project).expect("check ran"); + assert_eq!(result.status, CheckStatus::Pass); + } +} diff --git a/src/checks/source/rust/no_color.rs b/src/checks/source/rust/no_color.rs index 50f2a2e..bf9ec8a 100644 --- a/src/checks/source/rust/no_color.rs +++ b/src/checks/source/rust/no_color.rs @@ -13,7 +13,7 @@ use ast_grep_language::Rust; use crate::check::Check; use crate::project::{Language, Project}; -use crate::source::has_pattern; +use crate::source::{has_pattern, has_string_literal_in}; use crate::types::{CheckGroup, CheckLayer, CheckResult, CheckStatus}; /// Check trait implementation for NO_COLOR detection. @@ -79,7 +79,9 @@ pub(crate) fn check_no_color(source: &str, file: &str) -> CheckResult { let found_clap_env = source_contains_no_color_clap_attr(source); - let found_any = found_env_var || found_clap_env || has_string_literal(source, "NO_COLOR"); + let found_any = found_env_var + || found_clap_env + || has_string_literal_in(source, "NO_COLOR", Language::Rust); let status = if found_any { CheckStatus::Pass @@ -114,16 +116,6 @@ fn source_contains_no_color_clap_attr(source: &str) -> bool { false } -/// Fallback: scan for "NO_COLOR" as a string literal anywhere in the AST. -fn has_string_literal(source: &str, needle: &str) -> bool { - let pattern = match Pattern::try_new(&format!(r#""{needle}""#), Rust) { - Ok(p) => p, - Err(_) => return false, - }; - let root = Rust.ast_grep(source); - root.root().find(&pattern).is_some() -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/project.rs b/src/project.rs index 0989bbd..4e30fc6 100644 --- a/src/project.rs +++ b/src/project.rs @@ -1,7 +1,7 @@ -use std::cell::RefCell; use std::collections::HashMap; use std::fs; use std::path::{Path, PathBuf}; +use std::sync::OnceLock; use std::time::Duration; use anyhow::{Context, Result, bail}; @@ -35,7 +35,7 @@ pub struct Project { pub manifest_path: Option, pub runner: Option, pub include_tests: bool, - pub(crate) parsed_files: RefCell>, + pub(crate) parsed_files: OnceLock>, } impl std::fmt::Debug for Project { @@ -47,7 +47,10 @@ impl std::fmt::Debug for Project { .field("manifest_path", &self.manifest_path) .field("has_runner", &self.runner.is_some()) .field("include_tests", &self.include_tests) - .field("parsed_files_count", &self.parsed_files.borrow().len()) + .field( + "parsed_files_count", + &self.parsed_files.get().map_or(0, |m| m.len()), + ) .finish() } } @@ -73,7 +76,7 @@ impl Project { manifest_path: None, runner, include_tests: false, - parsed_files: RefCell::new(HashMap::new()), + parsed_files: OnceLock::new(), }); } @@ -94,7 +97,7 @@ impl Project { manifest_path, runner, include_tests: false, - parsed_files: RefCell::new(HashMap::new()), + parsed_files: OnceLock::new(), }) } @@ -108,10 +111,9 @@ impl Project { .expect("runner must exist when applicable() returns true") } - pub fn parsed_files(&self) -> std::cell::Ref<'_, HashMap> { - // Lazily populate on first access - if self.parsed_files.borrow().is_empty() { - let mut cache = self.parsed_files.borrow_mut(); + pub fn parsed_files(&self) -> &HashMap { + self.parsed_files.get_or_init(|| { + let mut cache = HashMap::new(); if let Some(lang) = self.language { let ext = match lang { Language::Rust => "rs", @@ -127,8 +129,8 @@ impl Project { } } } - } - self.parsed_files.borrow() + cache + }) } } diff --git a/src/source.rs b/src/source.rs index 3039dfc..31521c2 100644 --- a/src/source.rs +++ b/src/source.rs @@ -1,27 +1,84 @@ use ast_grep_core::Pattern; use ast_grep_core::tree_sitter::LanguageExt; -use ast_grep_language::Rust; +use ast_grep_language::{Python, Rust}; +use crate::project::Language; use crate::types::SourceLocation; /// Check whether a Rust source string contains at least one match for the given pattern. pub fn has_pattern(source: &str, pattern_str: &str) -> bool { - let pattern = match Pattern::try_new(pattern_str, Rust) { + has_pattern_with(source, pattern_str, Rust) +} + +/// Parse a Rust source file and find all matches for a pattern. +pub fn find_pattern_matches(source: &str, pattern_str: &str) -> Vec { + find_pattern_matches_with(source, pattern_str, Rust) +} + +/// Check whether `source` contains at least one match for `pattern_str` in `lang`. +pub fn has_pattern_in(source: &str, pattern_str: &str, lang: Language) -> bool { + match lang { + Language::Rust => has_pattern_with(source, pattern_str, Rust), + Language::Python => has_pattern_with(source, pattern_str, Python), + _ => false, + } +} + +/// Find all matches for `pattern_str` in `source`, parsed as `lang`. +/// +/// Currently used by tests; kept as a symmetric counterpart to `has_pattern_in` +/// for future Python checks that need evidence locations rather than a boolean. +#[allow(dead_code)] +pub fn find_pattern_matches_in( + source: &str, + pattern_str: &str, + lang: Language, +) -> Vec { + match lang { + Language::Rust => find_pattern_matches_with(source, pattern_str, Rust), + Language::Python => find_pattern_matches_with(source, pattern_str, Python), + _ => vec![], + } +} + +/// Check whether `source` contains a string literal matching `needle` in `lang`. +/// +/// For Rust, checks `"needle"`. For Python, checks both `"needle"` and `'needle'` +/// (checking both quote forms for Rust is harmless — Rust has no single-quote strings +/// beyond char literals, so the single-quote pattern simply won't match). +pub fn has_string_literal_in(source: &str, needle: &str, lang: Language) -> bool { + // Double-quoted form: works for both Rust and Python + let dq = format!(r#""{needle}""#); + if has_pattern_in(source, &dq, lang) { + return true; + } + // Single-quoted form: meaningful for Python, harmless no-op for Rust + let sq = format!("'{needle}'"); + has_pattern_in(source, &sq, lang) +} + +fn has_pattern_with(source: &str, pattern_str: &str, lang: L) -> bool +where + L: LanguageExt + Copy, +{ + let pattern = match Pattern::try_new(pattern_str, lang) { Ok(p) => p, Err(_) => return false, }; - let root = Rust.ast_grep(source); + let root = lang.ast_grep(source); root.root().find(&pattern).is_some() } -/// Parse a Rust source file and find all matches for a pattern. -pub fn find_pattern_matches(source: &str, pattern_str: &str) -> Vec { - let pattern = match Pattern::try_new(pattern_str, Rust) { +fn find_pattern_matches_with(source: &str, pattern_str: &str, lang: L) -> Vec +where + L: LanguageExt + Copy, +{ + let pattern = match Pattern::try_new(pattern_str, lang) { Ok(p) => p, Err(_) => return vec![], }; - let root = Rust.ast_grep(source); + let root = lang.ast_grep(source); root.root() .find_all(&pattern) .map(|m| { @@ -101,4 +158,64 @@ fn main() { fn test_has_pattern_invalid_pattern() { assert!(!has_pattern("fn main() {}", "<<>>")); } + + #[test] + fn test_python_bare_except_matches() { + let source = "try:\n foo()\nexcept:\n pass\n"; + let matches = find_pattern_matches_in( + source, + "try:\n $$$BODY\nexcept:\n $$$HANDLER", + Language::Python, + ); + assert!(!matches.is_empty(), "bare except pattern should match"); + } + + #[test] + fn test_python_typed_except_does_not_match_bare() { + let source = "try:\n foo()\nexcept ValueError:\n pass\n"; + let bare_matches = find_pattern_matches_in( + source, + "try:\n $$$BODY\nexcept:\n $$$HANDLER", + Language::Python, + ); + assert!( + bare_matches.is_empty(), + "typed except should not match bare except pattern" + ); + } + + #[test] + fn test_python_sys_exit_matches() { + let source = "import sys\nsys.exit(1)\n"; + let matches = find_pattern_matches_in(source, "sys.exit($$$ARGS)", Language::Python); + assert_eq!(matches.len(), 1); + } + + #[test] + fn test_python_pattern_against_rust_source_returns_empty() { + let source = "fn main() { sys.exit(1); }"; + // Rust-shaped source parsed as Python should yield no matches for our Python pattern. + let matches = find_pattern_matches_in(source, "fn main() { $$$BODY }", Language::Python); + assert!(matches.is_empty()); + } + + #[test] + fn test_unsupported_language_returns_empty() { + let source = "package main\nfunc main() {}"; + assert!(find_pattern_matches_in(source, "anything", Language::Go).is_empty()); + assert!(!has_pattern_in(source, "anything", Language::Node)); + } + + #[test] + fn test_supported_language_dispatches_correctly() { + let py_source = "import sys\nsys.exit(1)\n"; + assert!(has_pattern_in( + py_source, + "sys.exit($$$ARGS)", + Language::Python + )); + + let rs_source = "fn main() { x.unwrap(); }"; + assert!(has_pattern_in(rs_source, "$RECV.unwrap()", Language::Rust)); + } } diff --git a/tests/fixtures/broken-python/pyproject.toml b/tests/fixtures/broken-python/pyproject.toml new file mode 100644 index 0000000..e908da6 --- /dev/null +++ b/tests/fixtures/broken-python/pyproject.toml @@ -0,0 +1,3 @@ +[project] +name = "broken-python-fixture" +version = "0.1.0" diff --git a/tests/fixtures/broken-python/src/app.py b/tests/fixtures/broken-python/src/app.py new file mode 100644 index 0000000..dab467a --- /dev/null +++ b/tests/fixtures/broken-python/src/app.py @@ -0,0 +1,10 @@ +import sys + +# bare except (should trigger code-bare-except) +try: + do_thing() +except: + pass + +# sys.exit outside __main__ guard (should trigger p4-sys-exit) +sys.exit(1) diff --git a/tests/integration.rs b/tests/integration.rs index 1eb01f5..22a6d31 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -206,7 +206,6 @@ fn test_binary_only_fixture() { } #[test] -#[ignore] // Requires source-only fixture to have parseable Rust source fn test_source_only_fixture() { let path = fixture_path("source-only"); @@ -244,7 +243,6 @@ fn test_source_only_fixture() { } #[test] -#[ignore] // Requires broken-rust fixture to have parseable Rust source fn test_broken_fixture() { let path = fixture_path("broken-rust"); @@ -276,7 +274,6 @@ fn test_broken_fixture() { } #[test] -#[ignore] // Requires cargo build in the perfect-rust fixture directory fn test_perfect_fixture() { let path = fixture_path("perfect-rust"); @@ -609,3 +606,49 @@ fn test_explicit_completions_subcommand_still_works() { .success() .stdout(predicate::str::is_empty().not()); } + +// ── Python fixture tests ──────────────────────────────────────── + +#[test] +fn test_broken_python_fixture() { + let path = fixture_path("broken-python"); + + let assert = cmd().args(["check", &path, "--output", "json"]).assert(); + + let output = assert.get_output().stdout.clone(); + let json_str = String::from_utf8(output).expect("stdout should be valid UTF-8"); + let parsed: serde_json::Value = + serde_json::from_str(&json_str).expect("output should be valid JSON"); + + let results = parsed["results"] + .as_array() + .expect("results should be an array"); + + // Should have source-layer checks + let has_source = results + .iter() + .any(|r| r["layer"].as_str() == Some("source")); + assert!( + has_source, + "broken-python fixture should have source checks" + ); + + // Should have at least one failure + let fail_count = results + .iter() + .filter(|r| r["status"].as_str() == Some("fail")) + .count(); + assert!( + fail_count > 0, + "broken-python fixture should have at least one failure, got 0" + ); + + // Specifically check bare-except fires + let has_bare_except = results.iter().any(|r| { + r["id"].as_str() == Some("code-bare-except") && r["status"].as_str() == Some("fail") + }); + assert!( + has_bare_except, + "broken-python fixture should trigger code-bare-except check" + ); +}