Skip to content

Conversation

@Justar96
Copy link
Owner

@Justar96 Justar96 commented Nov 7, 2025

User description

This pull request introduces significant improvements to testing, documentation, and CI/CD workflows, along with updates for compatibility and code quality. The main changes include expanded test coverage and reporting, improved installation instructions and troubleshooting for ast-grep, enhanced documentation of tool limitations and best practices, and updates to configuration and versioning files to support these enhancements.

Testing and CI/CD improvements:

  • Added new test suites for CLI flag mapping and binary manager, with coverage reporting and artifact uploads in .github/workflows/ci.yaml. Integration tests are now forced in CI via the INTEGRATION_TESTS environment variable. [1] [2] [3] [4]
  • Updated .github/workflows/publish.yaml to install the ast-grep binary explicitly and ensure integration tests run with the correct environment.

Documentation enhancements:

  • Expanded the README.md to include detailed installation instructions for ast-grep, troubleshooting guidance, a new section on test infrastructure, and clearer usage notes for configuration flags and environment variables. Added a link to the Pattern Library and clarified requirements. [1] [2] [3] [4] [5] [6]
  • Updated CHANGELOG.md with new features, improvements, and fixes for versions 1.2.0 through 1.2.5, including simple text search detection, enhanced tool descriptions, and compatibility fixes for ast-grep v0.39.7. [1] [2]
  • Removed the PUBLIC_READY_CHECKLIST.md file, as the project is now publicly ready and professionally configured.

Configuration and code quality:

  • Added .prettierrc and eslint.config.js for consistent code formatting and linting across the project. [1] [2]

Versioning and compatibility:

  • Bumped the package version in package.json to 1.2.6 to reflect the new features and fixes.
  • Updated version comparison links in CHANGELOG.md for accurate tracking of changes between releases.

Minor usability improvements:

  • Improved CLI usage instructions in .github/QUICK_SETUP.md for easier installation verification.

These changes collectively enhance reliability, usability, and maintainability for both users and contributors.


PR Type

Enhancement, Tests, Documentation


Description

  • Enhanced ScanTool with dual execution modes: Added support for both run mode (simple patterns) and scan mode (structural rules) with automatic mode detection, comprehensive constraint operators (not_regex, not_equals, kind), and improved parameter validation

  • Comprehensive test coverage expansion: Added 1600+ lines of CLI flag mapping tests covering all tools (SearchTool, ReplaceTool, ScanTool, ExplainTool), integration tests with absolute path validation, binary manager tests, and enhanced constraint validation

  • Code quality and formatting standardization: Implemented Prettier and ESLint configurations for consistent code style (double quotes, 2-space indentation, 100-character line width), applied formatting across all TypeScript and JavaScript files

  • CI/CD workflow improvements: Updated publish workflow to install ast-grep binary v0.39.7 and enable integration tests via INTEGRATION_TESTS environment variable; enhanced CI workflow with coverage reporting and artifact uploads

  • Documentation enhancements: Expanded README with detailed ast-grep installation instructions, troubleshooting guidance, test infrastructure documentation, and clarified configuration flags; updated CHANGELOG for versions 1.2.0-1.2.5

  • TypeScript and tooling updates: Updated tsconfig.json for Bun compatibility, improved type safety in rules.ts with Record<string, unknown> instead of any, added ExplainTool export, and enhanced validation utilities

  • Test fixtures and validation updates: Refactored integration tests with absolute path requirements, updated fixture files with proper formatting and unused variable suppression, improved constraint validation tests


Diagram Walkthrough

flowchart LR
  A["Code Quality<br/>Prettier + ESLint"] --> B["Formatted Codebase<br/>Double quotes, consistent style"]
  C["Enhanced ScanTool<br/>Dual modes + constraints"] --> D["Improved Tool Capabilities<br/>run/scan modes"]
  E["Test Suite Expansion<br/>1600+ lines CLI mapping"] --> F["Comprehensive Coverage<br/>All tools validated"]
  G["CI/CD Updates<br/>ast-grep binary + env vars"] --> H["Reliable Workflows<br/>Integration tests enabled"]
  I["Documentation Expansion<br/>README + CHANGELOG"] --> J["Better User Guidance<br/>Installation & troubleshooting"]
  B --> K["Production Ready"]
  D --> K
  F --> K
  H --> K
  J --> K
Loading

File Walkthrough

Relevant files
Tests
3 files
integration.test.ts
Extensive integration test refactoring with absolute path validation
and new test coverage

tests/integration.test.ts

  • Converted all single quotes to double quotes for consistent code style
    (Prettier formatting)
  • Added path module import for handling absolute file paths in tests
  • Removed unused error type imports (ExecutionError, BinaryError)
  • Added non-null assertion operators (!) to tool variables to satisfy
    TypeScript strict mode
  • Added absolute path resolution using path.join(process.cwd(), ...) for
    all file-based test operations
  • Added new test suites for relative path validation, timeout handling,
    stdin vs file mode behavior, context parameters, dry-run behavior, and
    JSON stream format verification
  • Updated Windows path handling tests to use absolute paths and added
    system directory blocking validation
  • Updated cross-platform path resolution tests to reject relative paths
    and validate absolute path requirements
  • Added comprehensive error handling and validation tests for path
    escaping, metavariable placement, and language requirements
+1522/-697
sample.ts
TypeScript fixture cleanup with unused variable suppression

tests/fixtures/ts/sample.ts

  • Added ESLint disable comment to suppress unused variable warnings
  • Prefixed all variable and class names with underscore to mark them as
    intentionally unused (e.g., AdminManager_AdminManager, userName
    _userName)
+6/-5     
cli-flag-mapping.test.ts
Comprehensive CLI flag mapping test suite for all tools   

tests/cli-flag-mapping.test.ts

  • Created comprehensive test suite with 1600+ lines covering CLI flag
    mapping for all tools (SearchTool, ReplaceTool, ScanTool, ExplainTool)
  • Tests verify correct mapping of MCP parameters to ast-grep CLI flags
    including --pattern, --lang, --json=stream, --stdin, --rule,
    --rewrite, --update-all
  • Added YAML generation validation tests for rule structure,
    constraints, escaping, and language normalization
  • Implemented enhanced constraint tests for not_regex, not_equals, and
    kind operators with proper YAML nesting
  • Added temp file lifecycle tests verifying correct file creation,
    extension matching, and cleanup
  • Comprehensive language normalization tests for all supported aliases
    (javascript→js, typescript→ts, python→py, etc.)
  • Path handling tests validating absolute path requirements and
    rejection of relative paths with clear error messages
+1605/-0
Enhancement
2 files
index.ts
Tool exports update with new ExplainTool and formatting   

src/tools/index.ts

  • Converted all single quotes to double quotes for consistent code style
    (Prettier formatting)
  • Added export for new ExplainTool class
+4/-3     
scan.ts
Enhanced ScanTool with dual modes, constraint operators, and
validation

src/tools/scan.ts

  • Added comprehensive TypeScript interfaces (WhereConstraint,
    ScanParams, FindingLocation, Finding, ScanResult) for type safety and
    runtime validation
  • Implemented extensive parameter validation with explicit type checking
    for all fields including where array constraints and kind format
    validation
  • Added support for dual execution modes: run mode for simple patterns
    and scan mode for structural rules, with automatic mode detection
  • Enhanced constraint handling with support for not_regex, not_equals,
    and kind operators, generating proper nested YAML structures
  • Implemented extractAllMetavariables() method to recursively extract
    metavariables from complex nested rules
  • Added path validation with security checks to prevent scanning from
    home/user directories without explicit paths
  • Improved error messages and added support for C# language
    normalization
  • Refactored code formatting to use double quotes consistently and
    improved code organization
+793/-196
Formatting
3 files
sample.js
JavaScript fixture arrow function formatting update           

tests/fixtures/js/sample.js

  • Converted arrow function parameter from implicit to explicit
    parentheses: x => x * 2(x) => x * 2
+1/-1     
validation.test.ts
Code formatting standardization and constraint validation updates

tests/validation.test.ts

  • Converted all single quotes to double quotes for consistent code
    formatting throughout the test file
  • Added ESLint disable comment for @typescript-eslint/no-explicit-any at
    the top of the file
  • Updated test descriptions and assertions to use double quotes
    consistently
  • Modified constraint validation tests to expect errors when both regex
    and equals are provided simultaneously
  • Added new test for simple text search detection warning
  • Improved test comments and documentation formatting
+748/-658
rules.ts
Code formatting and type safety improvements                         

src/types/rules.ts

  • Converted all string literal types from single quotes to double quotes
    for consistency with Prettier configuration
  • Reformatted nthChild type definition for improved readability with
    proper line breaks
  • Updated Transform interface to use Record[] instead
    of any[] for better type safety
  • Updated RuleConfig metadata field to use Record
    instead of Record
+24/-14 
Configuration changes
4 files
eslint.config.js
Add ESLint configuration for TypeScript code quality         

eslint.config.js

  • New ESLint configuration file for TypeScript projects
  • Configured TypeScript parser and ESLint plugin for .ts and .tsx files
  • Set rules for TypeScript-specific linting including warnings for
    explicit any types
  • Configured to ignore build artifacts, node_modules, and cache
    directories
+27/-0   
publish.yaml
Install ast-grep binary and enable integration tests in CI

.github/workflows/publish.yaml

  • Added step to install ast-grep binary version 0.39.7 from GitHub
    releases
  • Downloads, extracts, and moves binary to /usr/local/bin/ with
    executable permissions
  • Added INTEGRATION_TESTS environment variable set to "1" for test
    execution
  • Ensures integration tests run with proper environment configuration
    during publish workflow
+11/-0   
tsconfig.json
Update TypeScript configuration for Bun compatibility       

tsconfig.json

  • Changed moduleResolution from "node" to "bundler" for better Bun
    compatibility
  • Added "types": ["bun-types"] to include Bun type definitions
  • Maintains strict TypeScript checking and other existing compiler
    options
+2/-1     
.prettierrc
Add Prettier code formatting configuration                             

.prettierrc

  • Added new Prettier configuration file with formatting rules
  • Configured for double quotes, 2-space indentation, 100-character line
    width, and trailing commas in ES5 format
+9/-0     
Documentation
1 files
QUICK_SETUP.md
Update npx command with non-interactive flag                         

.github/QUICK_SETUP.md

  • Updated npx command to include -y flag for non-interactive
    installation
+1/-1     
Additional files
30 files
ci.yaml +49/-0   
AST_GREP_DOCUMENTS.md +16636/-0
CHANGELOG.md +76/-1   
PATTERN_LIBRARY.md +507/-0 
PUBLIC_READY_CHECKLIST.md +0/-105 
README.md +144/-19
plan-test-mcp-tools-on-large-codebases-and-stress-test-edge-cases-0.md +0/-1166
package.json +23/-7   
debug-cache.ts +179/-0 
binary-manager.ts +488/-169
workspace-manager.ts +157/-91
index.ts +69/-79 
explain.ts +413/-0 
replace.ts +293/-110
search.ts +288/-98
errors.ts +13/-14 
validation.ts +366/-195
sample.ts +0/-98   
test-advanced.ts +0/-294 
test-direct.ts +0/-224 
test-mcp-vs-cli.ts +0/-256 
binary-manager.test.ts +1362/-0
diff-parsing.test.ts +176/-125
edge-cases.test.ts +205/-204
enhanced-constraints.test.ts +977/-0 
explain.test.ts +513/-0 
stderr-capture.ts +310/-22
setup.ts +76/-3   
structural-rules.test.ts +285/-244
warnings.test.ts +210/-155

Summary by CodeRabbit

  • New Features

    • Added ExplainTool for analyzing and understanding AST patterns with detailed metavariable capture information.
  • Improvements

    • Enhanced binary management with improved caching and version detection.
    • Expanded validation and error messages for better debugging.
    • Streamlined CI/CD with improved test coverage and binary installation handling.
  • Documentation

    • Updated README with comprehensive installation steps and troubleshooting guidance for ast-grep binary issues.
    • Clarified configuration options and default behaviors.
  • Chores

    • Version bumped to 1.2.6.
    • Added code formatting and linting standards.

- Add ESLint and Prettier configuration
- Add comprehensive test coverage (binary-manager, cli-flag-mapping tests)
- Update CI/CD workflows for improved automation
- Refactor core tools and utilities for better maintainability
- Remove obsolete test-manual files and documentation
- Update dependencies and configurations
Introduces AST_GREP_DOCUMENTS.md and PATTERN_LIBRARY.md for comprehensive ast-grep usage and rule writing documentation. Adds src/tools/explain.ts and corresponding tests for explain functionality. Updates CLI usage in QUICK_SETUP.md, expands README.md, and modifies core and tool modules to support new features. Includes new and updated tests for enhanced constraints and explain tool.
chore: release version 1.2.6 with improved workspace detection and validation

- Enhanced workspace root detection to reject user directories (home, Downloads, Documents, Desktop) and require explicit paths for safer operations
- Removed informational stderr logging (binary version, server ready messages) to prevent MCP client warnings
- Updated path depth limit from 10 to 6 levels for better performance
- Added validation to prevent scanning/replacing in user directories without explicit paths
-
@coderabbitai
Copy link

coderabbitai bot commented Nov 7, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

This pull request introduces a new ExplainTool for AST pattern explanation, overhauls the binary manager with cross-platform path handling and caching, refactors tool parameter validation with stricter typing across SearchTool, ReplaceTool, ScanTool, and ReplaceTool, adds comprehensive test suites for binary management and CLI flag mapping, enhances CI/CD workflows with ast-grep binary caching, and updates development tooling with ESLint and Prettier configuration. Version bumped from 1.1.1 to 1.2.6.

Changes

Cohort / File(s) Summary
CI/CD & Workflows
.github/QUICK_SETUP.md, .github/workflows/ci.yaml, .github/workflows/publish.yaml
Added -y flag to npx invocation; introduced INTEGRATION_TESTS environment variable; added ast-grep binary caching, global CLI installation, and consolidated test steps in CI; added binary installation and integration test variable to publish workflow.
Configuration & Tooling
.prettierrc, eslint.config.js, tsconfig.json
Added Prettier configuration (semi, trailingComma, printWidth 100, etc.); introduced ESLint config for TypeScript with parser and rules (no-explicit-any, no-unused-vars); changed moduleResolution to "bundler" and added bun-types.
Package & Version Management
package.json, CHANGELOG.md
Version bumped to 1.2.6; added test scripts (cli-mapping, binary-manager) and format/lint tasks; extended devDependencies with TypeScript, ESLint, Prettier tooling; removed --auto-install flag from MCP server config; documented v1.2.6 CI improvements and v1.2.5 features in changelog.
Documentation & Manifest
README.md, .gitignore
Expanded README with multi-step installation guide, ast-grep binary setup, troubleshooting section, and development/testing details; removed auto-install argument from config; added test-manual/ and .augment/ to .gitignore.
Documentation Removal
SECURITY.md, PUBLIC_READY_CHECKLIST.md, docs/plan/plan-test-mcp-tools-on-large-codebases-and-stress-test-edge-cases-0.md
Removed security policy documentation, public readiness checklist, and large-repo stress-testing plan.
Core Binary Management
src/core/binary-manager.ts
Introduced getDefaultCacheDir(), resolveExistingBinary(), cross-platform path normalization; enhanced binary discovery with version validation and caching; reworked download/install flow with multi-step validation; added manual ZIP extraction fallback and PowerShell extraction retry logic; exposed getBinaryPath(): string | null.
Workspace Management
src/core/workspace-manager.ts
Reduced maxDepth from 10 to 6; refactored detection with strong/secondary/tertiary indicators; enhanced validateWorkspaceRoot() with strict path handling, OS-native separators, and blocked paths; strengthened validation with home directory and common user directory guards.
Tool Infrastructure & Index
src/index.ts, src/tools/index.ts
Added ExplainTool registration and invocation in MCP server; integrated ExplainTool.getSchema() into tool registry; updated tool argument casting to Record<string, unknown>; standardized double-quoted strings throughout.
New Tool: Explain
src/tools/explain.ts
Introduced ExplainTool class with pattern validation, language normalization, ast-grep invocation, JSON-stream output parsing for metavariables/AST kinds, optional showAst debug output, and error handling via ValidationError/ExecutionError.
Tool Parameter Refactoring
src/tools/search.ts, src/tools/replace.ts, src/tools/scan.ts
Introduced strict parameter typing (SearchParams, ReplaceParams, ScanParams, corresponding Result types); added comprehensive runtime validation for pattern, code, language, timeout; implemented path normalization and workspace confinement; added language alias mapping; enhanced error messages; refactored result parsing with typed outputs.
Type System Tightening
src/types/errors.ts, src/types/rules.ts
Changed context?: any to context?: Record<string, unknown> in AstGrepMCPError; normalized error code constants to double-quoted strings; updated ValidationDiagnostics and ValidationResult typing; removed deprecated InstallationOptions fields; standardized rule type unions to double-quoted literals; changed rewriters type to Record<string, unknown>\[\].
Validation & Utilities
src/utils/validation.ts
Added PathValidator.isAbsolutePath() for cross-platform absolute path detection; added PatternValidator.detectSimpleTextSearch() to warn on non-AST patterns; updated ParameterValidator signatures to accept unknown instead of any; refined complexity typing to use literal type union.
Binary Integration Test Support
scripts/debug-cache.ts
Added diagnostic script for Windows binary download/extraction, ZIP parsing, manual extraction fallback, AstGrepBinaryManager instantiation, and stderr capture with retry counting.
Binary Manager Test Suite
tests/binary-manager.test.ts
Comprehensive test suite covering version detection, parsing, comparison, system binary discovery across platforms, custom path handling, installation logic, caching, retry logging, offline scenarios, initialization priority, PowerShell/CMD execution, and edge cases.
CLI Flag Mapping Test Suite
tests/cli-flag-mapping.test.ts
Validates MCP parameter-to-CLI-flag mappings for all tools via mocked binary execution; covers flag ordering, language normalization, YAML generation, constraints, path handling, and error cases without real executions.
Explain Tool Test Suite
tests/explain.test.ts
Integration tests for ExplainTool covering success cases (metavariables, multi-node patterns), failure cases with suggestions, multi-language support, validation/error handling, optional parameters (showAst, timeout), and edge cases with environment-driven gating.
Enhanced Constraints Test Suite
tests/enhanced-constraints.test.ts
New comprehensive test suite for not_regex, not_equals, kind constraints, combined constraints, validation, YAML structure verification, cross-language support, nested patterns, and error handling.
Existing Test Updates
tests/diff-parsing.test.ts, tests/edge-cases.test.ts, tests/structural-rules.test.ts, tests/warnings.test.ts
Added environment-driven integration test gating via INTEGRATION_TESTS flag; conditional setup and describeOrSkip logic; normalized string quotes to double; updated type assertions for conditional initialization; enhanced test infrastructure for binary availability checks.
Test Helpers
tests/helpers/stderr-capture.ts, tests/setup.ts
Added assertion utilities for binary version logging, stage logging, CLI flag validation, YAML field/structure checks, temp file management; added temp directory lifecycle helpers (createTempTestDir, cleanupTempTestDir) with exit/beforeExit cleanup handlers; extended WarningPatterns regex.
Test Fixtures
tests/fixtures/js/sample.js, tests/fixtures/ts/sample.ts
Updated arrow function parameter syntax in JS sample; renamed TypeScript class and variables with leading underscores (_AdminManager, _userName, etc.); added ESLint disable comment.
Manual Test Removal
test-manual/sample.ts, test-manual/test-advanced.ts, test-manual/test-direct.ts, test-manual/test-mcp-vs-cli.ts
Removed all manual test files including sample TypeScript fixtures, advanced MCP tests, direct CLI tests, and MCP-vs-CLI comparison tests.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant MCP as MCP Server
    participant ExplainTool
    participant BinaryMgr as Binary Manager
    participant AstGrep as ast-grep CLI

    User->>MCP: Call ast_explain_pattern
    MCP->>ExplainTool: execute(params)
    ExplainTool->>ExplainTool: Validate pattern, code, language
    ExplainTool->>ExplainTool: Normalize language alias
    ExplainTool->>BinaryMgr: executeAstGrep(["explain", ...])
    BinaryMgr->>AstGrep: Run explain command
    AstGrep-->>BinaryMgr: JSON-stream output
    BinaryMgr-->>ExplainTool: stdout/stderr
    ExplainTool->>ExplainTool: Parse JSON output
    ExplainTool->>ExplainTool: Extract metavariables, AST kinds
    Note over ExplainTool: If showAst=true,<br/>run secondary explain --show-ast
    ExplainTool->>ExplainTool: Generate suggestions if no match
    ExplainTool-->>MCP: ExplainResult {matched, metavariables, astNodes, suggestions}
    MCP-->>User: Return result
Loading
sequenceDiagram
    participant Test
    participant SearchTool
    participant Validator as Parameter Validator
    participant PathMgr as Path Manager
    participant BinaryMgr as Binary Manager

    Test->>SearchTool: execute({pattern, code, context, ...})
    SearchTool->>Validator: validatePattern(pattern)
    alt Pattern Invalid
        Validator-->>SearchTool: ValidationError
        SearchTool-->>Test: Error
    end
    SearchTool->>Validator: validateCode(code)
    SearchTool->>Validator: validateContext(context)
    SearchTool->>PathMgr: Normalize language alias
    alt Inline Code Mode
        SearchTool->>PathMgr: Validate language provided
    else File Mode
        SearchTool->>PathMgr: Enforce absolute paths
        PathMgr->>PathMgr: Validate workspace confinement
        PathMgr->>PathMgr: Check against blocked dirs
    end
    SearchTool->>BinaryMgr: executeAstGrep([...flags])
    BinaryMgr-->>SearchTool: Matches
    SearchTool->>SearchTool: Parse results, normalize paths
    SearchTool-->>Test: SearchResult {matches, summary}
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Key areas requiring extra attention:

  • Binary manager cross-platform path handling (src/core/binary-manager.ts): Windows path normalization, extraction fallbacks (manual ZIP, tar, PowerShell variants), and cache validation logic are non-trivial and platform-specific.
  • Tool parameter validation and type safety (src/tools/search.ts, src/tools/replace.ts, src/tools/scan.ts): Systematic refactoring with stricter typing, path normalization, and workspace confinement checks across multiple tools—ensure consistent error handling and boundary validation.
  • ExplainTool JSON parsing (src/tools/explain.ts): Verify correctness of JSON-stream output parsing for metavariables, AST kinds, and optional debug output.
  • Workspace detection logic (src/core/workspace-manager.ts): Reduced maxDepth, strong/secondary/tertiary indicator prioritization, and blocked-path logic—ensure depth limits and early-exit conditions prevent traversal issues.
  • Test infrastructure gating (tests/, tests/helpers/, tests/setup.ts): Integration test environment variable handling, conditional initialization, and temp directory lifecycle management should be verified for correctness across CI environments.
  • New binary and CLI-flag test suites (tests/binary-manager.test.ts, tests/cli-flag-mapping.test.ts): Extensive mocking and assertion helpers—verify test isolation and that spy/mock behaviors are consistent.

Poem

🐰 Hopping through patterns with explanations so clear,
Binary paths now dance without fear,
Type safety shields us, validation runs deep,
Tests multiply like carrots we reap!
From v1.1 to v1.2.6, the garden is grown,
This astute tool collection is proudly now shown! 🌱


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Free

📥 Commits

Reviewing files that changed from the base of the PR and between b89df77 and a6871d0.

📒 Files selected for processing (3)
  • .github/workflows/ci.yaml (5 hunks)
  • CHANGELOG.md (2 hunks)
  • SECURITY.md (0 hunks)
💤 Files with no reviewable changes (1)
  • SECURITY.md

Note

🎁 Summarized by CodeRabbit Free

Your organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login.

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-code-review
Copy link

qodo-code-review bot commented Nov 7, 2025

PR Compliance Guide 🔍

(Compliance updated until commit a6871d0)

Below is a summary of compliance checks for this PR:

Security Compliance
Broad scan scope

Description: Defaulting to scanning the entire workspace when paths are omitted could unintentionally
scan large or sensitive directories unless guarded, though there are checks against home
and common user dirs; this broad scan behavior may expose more code than intended and
should require explicit confirmation.
scan.ts [462-536]

Referred Code
if (!pathsProvided) {
  const workspaceRoot = this.workspaceManager.getWorkspaceRoot();
  const home = process.env.HOME || process.env.USERPROFILE || "";

  // Prevent scanning from home directory or common user directories
  if (home && path.resolve(workspaceRoot) === path.resolve(home)) {
    throw new ValidationError(
      `Cannot scan from home directory without explicit paths. Please provide absolute paths to specific directories.`
    );
  }

  const normalizedRoot = workspaceRoot.toLowerCase();
  const userDirPatterns = [
    /[/\\]downloads[/\\]?$/i,
    /[/\\]documents[/\\]?$/i,
    /[/\\]desktop[/\\]?$/i,
  ];
  if (userDirPatterns.some((pattern) => pattern.test(normalizedRoot))) {
    throw new ValidationError(
      `Cannot scan from user directory without explicit paths. Please provide absolute paths to specific directories.`
    );


 ... (clipped 54 lines)
Sensitive log exposure

Description: Logging raw malformed JSON lines to stderr may leak snippets of source paths/content in CI
logs; redact or truncate more aggressively to avoid sensitive information exposure.
scan.ts [1003-1007]

Referred Code
if (skippedLines > 0) {
  console.error(
    `Warning: Skipped ${skippedLines} malformed finding lines out of ${lines.length} total lines`
  );
}
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logs: The new code adds extensive validation and execution paths but does not introduce or
update any audit logging for critical actions, and it is unclear whether higher-level
logging exists elsewhere.

Referred Code
async execute(paramsRaw: Record<string, unknown>): Promise<ScanResult> {
  // Runtime parameter validation with explicit type checking for all fields
  // Validate required parameters
  if (!paramsRaw.id || typeof paramsRaw.id !== "string") {
    throw new ValidationError("id is required and must be a string");
  }
  if (!paramsRaw.language || typeof paramsRaw.language !== "string") {
    throw new ValidationError("language is required and must be a string");
  }

  // Validate optional parameters with explicit type checks
  if (paramsRaw.pattern !== undefined && typeof paramsRaw.pattern !== "string") {
    throw new ValidationError("pattern must be a string");
  }
  if (
    paramsRaw.rule !== undefined &&
    (typeof paramsRaw.rule !== "object" ||
      paramsRaw.rule === null ||
      Array.isArray(paramsRaw.rule))
  ) {
    throw new ValidationError("rule must be an object");


 ... (clipped 913 lines)
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Console warnings: Multiple console.error calls output raw warning and error context (e.g., skipped malformed
JSON lines), which may expose internal details if surfaced to end users depending on
runtime environment.

Referred Code
      console.error(`Warning: ${warning}`);
    }
  }
} else if (typeof pattern === "object" && pattern !== null) {
  // Pattern object validation (selector, context, strictness)
  const patternObj = pattern as Record<string, unknown>;
  if (patternObj.selector && typeof patternObj.selector !== "string") {
    throw new ValidationError("Pattern object selector must be a string");
  }
  if (patternObj.context && typeof patternObj.context !== "string") {
    throw new ValidationError("Pattern object context must be a string");
  }
  if (patternObj.strictness) {
    const validStrictness = ["cst", "smart", "ast", "relaxed", "signature"];
    if (
      typeof patternObj.strictness === "string" &&
      !validStrictness.includes(patternObj.strictness)
    ) {
      throw new ValidationError(
        `Invalid strictness: ${patternObj.strictness}. Must be one of: ${validStrictness.join(", ")}`
      );


 ... (clipped 731 lines)
Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Unstructured logs: The code logs warnings and cleanup issues via console.error with free-form strings rather
than structured logs and may include file paths, which could be sensitive depending on
deployment.

Referred Code
      console.error(`Warning: ${warning}`);
    }
  }
} else if (typeof pattern === "object" && pattern !== null) {
  // Pattern object validation (selector, context, strictness)
  const patternObj = pattern as Record<string, unknown>;
  if (patternObj.selector && typeof patternObj.selector !== "string") {
    throw new ValidationError("Pattern object selector must be a string");
  }
  if (patternObj.context && typeof patternObj.context !== "string") {
    throw new ValidationError("Pattern object context must be a string");
  }
  if (patternObj.strictness) {
    const validStrictness = ["cst", "smart", "ast", "relaxed", "signature"];
    if (
      typeof patternObj.strictness === "string" &&
      !validStrictness.includes(patternObj.strictness)
    ) {
      throw new ValidationError(
        `Invalid strictness: ${patternObj.strictness}. Must be one of: ${validStrictness.join(", ")}`
      );


 ... (clipped 731 lines)
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Previous compliance checks

Compliance check up to commit b89df77
Security Compliance
Broad scan default

Description: Scanning defaults to the entire workspace when no paths are provided, which could
inadvertently scan large directories and leak file structure via logs; although
mitigations exist, this behavior may still be risky and should require explicit
confirmation or stricter limits.
scan.ts [462-506]

Referred Code
if (!pathsProvided) {
  const workspaceRoot = this.workspaceManager.getWorkspaceRoot();
  const home = process.env.HOME || process.env.USERPROFILE || "";

  // Prevent scanning from home directory or common user directories
  if (home && path.resolve(workspaceRoot) === path.resolve(home)) {
    throw new ValidationError(
      `Cannot scan from home directory without explicit paths. Please provide absolute paths to specific directories.`
    );
  }

  const normalizedRoot = workspaceRoot.toLowerCase();
  const userDirPatterns = [
    /[/\\]downloads[/\\]?$/i,
    /[/\\]documents[/\\]?$/i,
    /[/\\]desktop[/\\]?$/i,
  ];
  if (userDirPatterns.some((pattern) => pattern.test(normalizedRoot))) {
    throw new ValidationError(
      `Cannot scan from user directory without explicit paths. Please provide absolute paths to specific directories.`
    );


 ... (clipped 24 lines)
Log information exposure

Description: The parser logs details about skipped malformed JSON lines to stderr, which could expose
parts of input content; consider rate-limiting or redacting to prevent information
disclosure in logs.
scan.ts [1003-1007]

Referred Code
if (skippedLines > 0) {
  console.error(
    `Warning: Skipped ${skippedLines} malformed finding lines out of ${lines.length} total lines`
  );
}
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Limited Auditing: The new code performs extensive validation and executes scans but does not add audit
logging of critical actions (e.g., which user initiated scans, parameters used, outcomes)
beyond console warnings, so it’s unclear if sufficient audit trails exist elsewhere.

Referred Code
async execute(paramsRaw: Record<string, unknown>): Promise<ScanResult> {
  // Runtime parameter validation with explicit type checking for all fields
  // Validate required parameters
  if (!paramsRaw.id || typeof paramsRaw.id !== "string") {
    throw new ValidationError("id is required and must be a string");
  }
  if (!paramsRaw.language || typeof paramsRaw.language !== "string") {
    throw new ValidationError("language is required and must be a string");
  }

  // Validate optional parameters with explicit type checks
  if (paramsRaw.pattern !== undefined && typeof paramsRaw.pattern !== "string") {
    throw new ValidationError("pattern must be a string");
  }
  if (
    paramsRaw.rule !== undefined &&
    (typeof paramsRaw.rule !== "object" ||
      paramsRaw.rule === null ||
      Array.isArray(paramsRaw.rule))
  ) {
    throw new ValidationError("rule must be an object");


 ... (clipped 488 lines)
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Console Details: The code logs detailed warnings and cleanup errors to console (including file paths and
malformed JSON snippets), which may expose internal details if surfaced to end users;
verify logs are not user-facing.

Referred Code
      console.error(`Warning: ${warning}`);
    }
  }
} else if (typeof pattern === "object" && pattern !== null) {
  // Pattern object validation (selector, context, strictness)
  const patternObj = pattern as Record<string, unknown>;
  if (patternObj.selector && typeof patternObj.selector !== "string") {
    throw new ValidationError("Pattern object selector must be a string");
  }
  if (patternObj.context && typeof patternObj.context !== "string") {
    throw new ValidationError("Pattern object context must be a string");
  }
  if (patternObj.strictness) {
    const validStrictness = ["cst", "smart", "ast", "relaxed", "signature"];
    if (
      typeof patternObj.strictness === "string" &&
      !validStrictness.includes(patternObj.strictness)
    ) {
      throw new ValidationError(
        `Invalid strictness: ${patternObj.strictness}. Must be one of: ${validStrictness.join(", ")}`
      );


 ... (clipped 311 lines)
Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Console Logging: Multiple console.error statements output raw paths and content snippets (e.g., malformed
JSON lines and cleanup errors), which might inadvertently log sensitive data depending on
inputs and environment.

Referred Code
      console.error(`Warning: ${warning}`);
    }
  }
} else if (typeof pattern === "object" && pattern !== null) {
  // Pattern object validation (selector, context, strictness)
  const patternObj = pattern as Record<string, unknown>;
  if (patternObj.selector && typeof patternObj.selector !== "string") {
    throw new ValidationError("Pattern object selector must be a string");
  }
  if (patternObj.context && typeof patternObj.context !== "string") {
    throw new ValidationError("Pattern object context must be a string");
  }
  if (patternObj.strictness) {
    const validStrictness = ["cst", "smart", "ast", "relaxed", "signature"];
    if (
      typeof patternObj.strictness === "string" &&
      !validStrictness.includes(patternObj.strictness)
    ) {
      throw new ValidationError(
        `Invalid strictness: ${patternObj.strictness}. Must be one of: ${validStrictness.join(", ")}`
      );


 ... (clipped 732 lines)

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@qodo-code-review
Copy link

qodo-code-review bot commented Nov 7, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Mandating absolute paths is a breaking change

The PR mandates absolute paths, which is a breaking change. This should be
documented as such, or backward compatibility should be maintained by resolving
relative paths.

Examples:

tests/integration.test.ts [395-409]
  test("Rejects relative paths with ValidationError", async () => {
    // Validates Phase 2 implementation - tools reject relative paths with real binary
    try {
      await searchTool!.execute({
        pattern: "console.log($ARGS)",
        paths: ["tests/fixtures/js/"],
        language: "javascript",
      });
      throw new Error("Expected ValidationError but none was thrown");
    } catch (error) {

 ... (clipped 5 lines)
src/tools/scan.ts [1151-1156]
6. "Invalid paths"
    Use absolute paths like '/workspace/src/' or 'C:/workspace/src/'
    Relative paths are not supported (will be rejected with validation error)
    Paths validated against workspace root for security
    Omit paths to scan entire workspace (defaults to current directory)

Solution Walkthrough:

Before:

// in ScanTool.execute()
const pathsProvided = params.paths && params.paths.length > 0;
const inputPaths = pathsProvided ? params.paths : ["."];

for (const p of inputPaths) {
  if (!path.isAbsolute(p)) {
    if (p === "." && !pathsProvided) {
      // allow default "."
      continue;
    }
    throw new ValidationError(
      `Path must be absolute. Use '/workspace/src/'...`
    );
  }
}
// ... uses inputPaths which are validated to be absolute

After:

// in ScanTool.execute()
const pathsProvided = params.paths && params.paths.length > 0;
const inputPaths = pathsProvided ? params.paths : ["."];
const resolvedPaths = [];

for (const p of inputPaths) {
  if (path.isAbsolute(p)) {
    resolvedPaths.push(p);
  } else {
    // Resolve relative paths against the workspace root
    const absolutePath = path.resolve(this.workspaceManager.getWorkspaceRoot(), p);
    resolvedPaths.push(absolutePath);
  }
}

// Then validate the resolvedPaths for security
const { valid, errors } = this.workspaceManager.validatePaths(resolvedPaths);
// ... use resolvedPaths
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a significant, undocumented breaking change where relative paths are no longer supported, which directly impacts user workflows and the tool's public API.

High
Possible issue
Use cross-platform process checking

Replace the non-portable ps aux command with the cross-platform ps-list package
to reliably check for lingering processes in the timeout test.

tests/integration.test.ts [1579-1684]

 test("Process cleanup on timeout (error expected, no lingering processes)", async () => {
   if (!searchTool) throw new Error("Tools not initialized");
 
   // Create a large temporary directory with many files to force timeout
   const { mkdtemp, writeFile, rm } = await import("fs/promises");
   const { join } = await import("path");
   const { tmpdir } = await import("os");
-  const { spawnSync } = await import("child_process");
+  const psList = (await import("ps-list")).default;
 
   const tempDir = await mkdtemp(join(tmpdir(), "astgrep-timeout-test-"));
 
   try {
     // Create 1000 JavaScript files to slow down ast-grep
     const fileCount = 1000;
     const fileCreationPromises: Promise<void>[] = [];
 
     for (let i = 0; i < fileCount; i++) {
       const filePath = join(tempDir, `file${i}.js`);
-      const content = `
+      const content =
+        `
 // File ${i}
 const x${i} = 1;
 const y${i} = 2;
 const z${i} = 3;
 function test${i}() {
   return x${i} + y${i} + z${i};
 }
 `.repeat(50); // Make each file large
       fileCreationPromises.push(writeFile(filePath, content));
     }
 
     await Promise.all(fileCreationPromises);
 
     // Count ast-grep processes before test
-    let beforeCount = 0;
-    try {
-      const psResult = spawnSync("ps", ["aux"], { encoding: "utf-8" });
-      if (psResult.stdout) {
-        beforeCount = psResult.stdout
-          .split("\n")
-          .filter((line) => line.includes("ast-grep")).length;
-      }
-    } catch {
-      // ps command might not be available on all platforms
-    }
+    const beforeProcesses = await psList();
+    const beforeCount = beforeProcesses.filter((p) => p.name.includes("ast-grep")).length;
 
     // Execute search with very short timeout (1-2 seconds)
     let timeoutOccurred = false;
     try {
       await searchTool.execute({
         pattern: "const $NAME = $VALUE",
         paths: [tempDir],
         language: "javascript",
         timeoutMs: 1500, // Very short timeout to force failure
       });
     } catch (error) {
       // Timeout error expected
       timeoutOccurred = true;
       expect(error).toBeDefined();
       const errorMessage = error instanceof Error ? error.message : String(error);
       // Should mention timeout or termination
       expect(errorMessage.toLowerCase()).toMatch(/timeout|terminated|killed|signal/i);
     }
 
     // Give OS time to clean up processes
     await new Promise((resolve) => setTimeout(resolve, 500));
 
     // Count ast-grep processes after test
-    let afterCount = 0;
-    try {
-      const psResult = spawnSync("ps", ["aux"], { encoding: "utf-8" });
-      if (psResult.stdout) {
-        afterCount = psResult.stdout
-          .split("\n")
-          .filter((line) => line.includes("ast-grep")).length;
-      }
-    } catch {
-      // ps command might not be available
-    }
+    const afterProcesses = await psList();
+    const afterCount = afterProcesses.filter((p) => p.name.includes("ast-grep")).length;
 
     // Verify no lingering processes (allow small tolerance for timing)
     expect(afterCount).toBeLessThanOrEqual(beforeCount + 1);
 
     // Verify subsequent quick search succeeds immediately (no blocking)
     const quickResult = await searchTool.execute({
       pattern: "const $NAME = $VALUE",
       code: "const x = 1;",
       language: "javascript",
       timeoutMs: 5000,
     });
 
     expect(quickResult.matches.length).toBeGreaterThan(0);
     expect(quickResult.summary.totalMatches).toBeGreaterThan(0);
 
     // If CI or strict environment, assert timeout actually occurred
     if (process.env.CI === "1" || process.env.INTEGRATION_TESTS === "1") {
       expect(timeoutOccurred).toBe(true);
     }
   } finally {
     // Cleanup temp directory
     try {
       await rm(tempDir, { recursive: true, force: true });
     } catch {
       // Best effort cleanup
     }
   }
 });
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that using ps aux is not cross-platform and will fail on Windows, which is contrary to the PR's goal of improving Windows compatibility. Proposing ps-list is an excellent, robust solution.

Medium
General
Assert a more specific error

Update the test for an empty regex string to assert a more specific error
message, clarifying that the operator's value is invalid because it's empty.

tests/validation.test.ts [487-506]

 test("throws ValidationError when constraint has empty regex string", async () => {
   const params = {
     id: "test-rule",
     language: "javascript",
     pattern: "console.log($ARG)",
     where: [
       { metavariable: "ARG", regex: "   " }, // Empty/whitespace regex
     ],
   };
 
   try {
     await scanTool.execute(params);
     expect(true).toBe(false); // Should not reach here
   } catch (error) {
     expect(error).toBeInstanceOf(ValidationError);
     expect((error as ValidationError).message).toContain(
-      "must specify at least one operator"
+      "Constraint for metavariable 'ARG' has an empty 'regex' operator. The value cannot be empty or contain only whitespace."
     );
   }
 });
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that the test's error message assertion is too generic and could be improved to be more specific, which enhances test quality and robustness.

Medium
Log error details in catch block

Improve error handling in parseFindings by logging the specific error in the
catch block when a malformed JSON line is encountered.

src/tools/scan.ts [953-1010]

     private parseFindings(stdout: string): { findings: Finding[]; skippedLines: number } {
       const findings: Finding[] = [];
       let skippedLines = 0;
 
       if (!stdout.trim()) return { findings, skippedLines: 0 };
 
       const lines = stdout.trim().split("\n");
       for (const line of lines) {
         if (!line.trim()) continue;
         try {
           const finding = JSON.parse(line) as {
             ruleId?: string;
             severity?: string;
             message?: string;
             file?: string;
             range?: {
               start?: { line?: number; column?: number };
               end?: { line?: number; column?: number };
             };
             fix?: string;
           };
           const startLine = (finding.range?.start?.line || 0) + 1;
           const startColumn = finding.range?.start?.column || 0;
           const resolvedFile = this.resolveFilePath(finding.file || "");
           findings.push({
             ruleId: finding.ruleId || "unknown",
             severity: finding.severity || "info",
             message: finding.message || "",
             file: resolvedFile,
             line: startLine,
             column: startColumn,
             range: {
               file: resolvedFile,
               start: {
                 line: startLine,
                 column: startColumn,
               },
               end: {
                 line: (finding.range?.end?.line || 0) + 1,
                 column: finding.range?.end?.column || 0,
               },
             },
             fix: finding.fix,
           });
-        } catch {
+        } catch (e) {
           skippedLines++;
-          console.error(`Warning: Skipped malformed JSON line: ${line.substring(0, 100)}...`);
+          console.error(`Warning: Skipped malformed JSON line due to error: ${e instanceof Error ? e.message : String(e)}. Line: ${line.substring(0, 100)}...`);
         }
       }
 
       if (skippedLines > 0) {
         console.error(
           `Warning: Skipped ${skippedLines} malformed finding lines out of ${lines.length} total lines`
         );
       }
 
       return { findings, skippedLines };
     }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the empty catch block swallows errors, and proposes logging the error message, which improves debuggability.

Medium
Assert the exact error message

Change the test assertion from checking for a substring in the error message to
asserting the full, exact error message for improved test accuracy.

tests/validation.test.ts [466-485]

 test("throws ValidationError when constraint has neither regex nor equals", async () => {
   const params = {
     id: "test-rule",
     language: "javascript",
     pattern: "console.log($ARG)",
     where: [
       { metavariable: "ARG" }, // Missing all operators
     ],
   };
 
   try {
     await scanTool.execute(params);
     expect(true).toBe(false); // Should not reach here
   } catch (error) {
     expect(error).toBeInstanceOf(ValidationError);
-    expect((error as ValidationError).message).toContain(
-      "must specify at least one operator: regex, equals, not_regex, not_equals, or kind"
+    expect((error as ValidationError).message).toBe(
+      "Constraint for metavariable 'ARG' must specify at least one operator: regex, equals, not_regex, not_equals, or kind."
     );
   }
 });
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion to use an exact match for the error message is a good practice for improving test precision, though the current toContain check is still valid and functional.

Low
  • Update

@openhands-ai
Copy link

openhands-ai bot commented Nov 7, 2025

Looks like there are a few issues preventing this PR from being merged!

  • GitHub Actions are failing:
    • CI Tests

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #1 at branch `feat/improvements`

Feel free to include any additional details that might help me get this PR into a better state.

You can manage your notification settings

- Consolidated test execution to 5 stable suites (validation, diff-parsing, explain, structural-rules, warnings)
- Removed environment-dependent test jobs (binary-manager, cli-flag-mapping, integration)
- Unified coverage reporting for stable tests only
- Deleted unused PATTERN_LIBRARY.md documentation file
@Justar96 Justar96 merged commit a58b1e3 into main Nov 7, 2025
10 checks passed
@Justar96 Justar96 deleted the feat/improvements branch November 7, 2025 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants