Skip to content

Centralized AST walker for consistent bash analysis#29

Merged
ldayton merged 2 commits intomainfrom
lily/centralized-ast-walker
Jan 14, 2026
Merged

Centralized AST walker for consistent bash analysis#29
ldayton merged 2 commits intomainfrom
lily/centralized-ast-walker

Conversation

@ldayton
Copy link
Copy Markdown
Owner

@ldayton ldayton commented Jan 13, 2026

Summary

Replace scattered partial AST traversals with a single recursive walker that analyzes all bash constructs consistently.

Problem

The old code had multiple scattered functions that partially traversed the AST:

  • extract_simple_commands
  • has_output_redirect
  • get_command_substitutions
  • split_pipeline
  • split_command_list

This led to inconsistent handling:

  • Config rules didn't reach inside compound commands
  • Process substitution (>()) was silently allowed without checking inner commands
  • Function definitions weren't analyzed

Solution

New src/dippy/core/analyzer.py with a single analyze() function that recursively walks the entire AST:

def analyze(command: str, config: Config, cwd: Path) -> Decision:
    """Single recursive walker. Unknown nodes default to ask."""
    
    match node.kind:
        case "command": return analyze_command(node, config, cwd)
        case "pipeline": return combine([analyze(cmd) for cmd in node.commands])
        case "list": return combine([analyze(p) for p in parts])
        case "if": return combine([analyze(condition), analyze(then_body), ...])
        case "while" | "until": return combine([analyze(condition), analyze(body)])
        case "for" | "select": return analyze(body)
        case "case": return combine([analyze(pattern.body) for pattern in patterns])
        case "function": return analyze(body)
        case "subshell" | "brace-group": return analyze(body)
        case "time": return analyze(pipeline)
        case _: return Decision("ask", f"unrecognized: {node.kind}")

Key Improvements

Before After
if true; then rm -rf /; fi → ask "if" → matches deny rm -rf /*
diff <(ls) >(tee /etc/passwd)allow → ask (inner cmd analyzed)
foo() { rm -rf /; } → ask "foo()" → matches deny rules
echo > file.txt → "output redirect" → "redirect to file.txt"

Changes

  • Add src/dippy/core/analyzer.py (new file, ~350 lines)
  • Simplify dippy.py from 617 to 286 lines
  • Add read to SIMPLE_SAFE (shell builtin)
  • Handle Parable's time node type
  • Update tests to reflect improved behavior

Test Plan

  • All 9286 tests pass on Python 3.11, 3.12, 3.13, 3.14
  • just check passes (lint, fmt, lock)

@ldayton ldayton force-pushed the lily/centralized-ast-walker branch from f402d0b to f030479 Compare January 14, 2026 05:57
Replace scattered partial AST traversals with a single recursive walker
that analyzes all bash constructs consistently.

Key changes:
- Add src/dippy/core/analyzer.py with unified analyze() entry point
- Remove duplicated traversal code from dippy.py (365 -> 90 lines)
- Config rules now reach inside compound commands (if/while/for/case)
- Process substitution properly analyzed (was silently allowed!)
- Function definitions analyzed (body checked for safety)
- Decisions bubble up correctly (deny > ask > allow)
- Unknown constructs default to ask (fail-safe)

Behavioral improvements:
- `deny rm -rf /*` now catches rm inside `if true; then rm -rf /; fi`
- `diff <(cat a) >(tee /etc/passwd)` now properly asks (inner cmd analyzed)
- Compound commands with safe contents are now approved
- Redirect reasons now show target path (more informative)

Also:
- Add `read` to SIMPLE_SAFE (shell builtin, reads from stdin)
- Handle Parable's `time` node type
- Strip quotes from word values consistently

All 9286 tests pass on Python 3.11-3.14.
- Pass tokens (sans env var prefixes) to handlers instead of words
- Add explicit "cmdsub injection risk" warning for pure cmdsubs in
  handler CLI argument positions
- Add regression tests for both issues
@ldayton ldayton force-pushed the lily/centralized-ast-walker branch from f030479 to c6376ba Compare January 14, 2026 05:59
@ldayton ldayton merged commit 67c4689 into main Jan 14, 2026
0 of 2 checks passed
@ldayton ldayton deleted the lily/centralized-ast-walker branch January 14, 2026 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant