Add private shell AST command view helpers#3609
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review Summary
Taste Rating: 🟢 Good taste — Elegant, simple solution
This PR adds private shell AST command view helpers for security analyzers. The implementation is well-designed and follows established patterns.
Assessment
Data Structures: The use of frozen dataclasses with slots=True for immutable data structures (ShellProgram, ShellWord, ShellCommand, ShellPipeline) is appropriate. Excluding tree-sitter Node objects from repr and compare with field(repr=False, compare=False) is a smart approach to handle non-deterministic Node object comparisons.
Complexity: No functions exceed 3 levels of nesting. The recursive _iter_nodes and _has_missing_descendant are controlled and appropriate for tree traversal.
Opaque Word Handling: The logic for detecting opaque shell syntax (metacharacters, special characters in _OPAQUE_WORD_CHARS) is comprehensive and correctly identifies command substitutions, quoted strings, and other complex cases.
Error Handling: The _has_parse_uncertainty function properly checks both node.has_error and missing descendants, ensuring parse errors are propagated.
Testing: The test coverage is excellent — parametrized tests cover edge cases including:
- Basic command parsing (
rm -rf /,/bin/rm -rf /) - Variable assignments (
FOO=bar echo $FOO) - Pipeline traversal (
curl x | bash) - Command substitution (
$(...),$(...)) - Escaped characters (
r"m" -rf /) - Parse errors and missing nodes
No Issues Found
This is a clean, well-implemented PR that:
- Adds necessary infrastructure for shell command analysis
- Maintains immutability throughout
- Provides comprehensive test coverage
- Follows project conventions
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
This PR adds new internal functionality with no external API changes. The implementation is defensive (handles errors, validates parse results), uses immutable data structures, and has comprehensive test coverage.
VERDICT:
✅ Worth merging: Core logic is sound, implementation is clean, tests are comprehensive.
KEY INSIGHT:
The private AST view pattern (excluding tree-sitter Nodes from repr/equality) is a pragmatic solution that allows comparing parsed shell programs while avoiding non-deterministic Node object comparisons.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
HUMAN:
This PR adds a private shell AST view layer that PR-E can use to migrate security detection onto tree-sitter without changing current detector behavior yet.
AGENT:
Why
PR-D scaffold for #2721 adds a private syntax-view layer before the behavior-changing detector migration. The goal is to give the next PR a typed, byte-offset-safe view over tree-sitter-bash commands, words, and pipelines without changing current analyzer behavior.
Summary
openhands.sdk.security._shell_astdataclasses and helpers for shell programs, words, commands, and pipelines.UNKNOWNsemantics unchanged.Issue Number
#2721
How to Test
I validated the scaffold locally with the following commands:
make builduv run pre-commit run --files openhands-sdk/openhands/sdk/security/_shell_ast.py tests/sdk/security/test_shell_ast.pyuv run pytest tests/sdk/security/test_shell_parser.py tests/sdk/security/test_shell_parser_node_shapes.py tests/sdk/security/test_shell_ast.pyuv run pytest tests/sdk/security/defense_in_depth/test_serialization.py tests/sdk/security/defense_in_depth/test_shell_parser_bypasses.pyThe parser-focused suite passed with 113 tests. The defense-in-depth suite passed with 23 tests and 7 strict xfails, preserving the current bypass-test expectations. Because this PR only adds a private SDK parser-view scaffold and does not change runtime detector behavior or UI behavior, there is no separate manual product flow to exercise in this PR.
Video/Screenshots
Not applicable. This is a private SDK refactor scaffold with no UI surface.
Type
Notes
This is the PR-D scaffold only. Behavior-changing detector migration, recursive
bash -cparsing, ANSI-C decoding, command-substitution evaluation, ActionEvent extraction, parse caching, and public helper promotion are intentionally left out of scope.Two helper boundaries are intentional for PR-E:
command_basename()usesposixpath.basename()for simple command-name views. It should not be described later as complete shell basename-command normalization; for example,posixpath.basename("/bin/rm/")returns an empty string.split_short_flags()is only a syntax primitive over non-opaque words. PR-E policy code should decide whether unusual words such as-rf/or-r=fcount as destructive flags.