refactor(sdk): introduce shared tree-sitter-bash entry point by Fieldnote-Echo · Pull Request #3307 · OpenHands/software-agent-sdk

Fieldnote-Echo · 2026-05-19T18:29:15Z

Summary

Phase 2a foundation for #2721. Creates a shared tree-sitter-bash parse
entry point inside openhands-sdk so the security analyzers can
import a parser without reaching across packages.

Specifically:

Adds openhands-sdk/openhands/sdk/security/shell_parser.py with a
single public function parse(command: str) -> ParseResult and a
frozen ParseResult dataclass carrying tree and has_error.
Declares tree-sitter>=0.25 and tree-sitter-bash>=0.25 in
openhands-sdk/pyproject.toml. The 0.25 floor matches what the
workspace lockfile already resolves and avoids the documented ABI
mismatch between tree-sitter==0.24.0 and tree-sitter-bash==0.25.0.
openhands-tools keeps its direct tree-sitter declarations
(unchanged here, per chore(tools): replace bashlex (GPLv3) with tree-sitter-bash (MIT) #3237) because it still imports those libraries
directly via terminal/utils/command.py. Tightening that spec is a
separate cleanup.
Regenerates uv.lock to reflect the new declaration site.

Out of scope (deliberate)

No consumer is migrated. openhands-tools/openhands/tools/terminal/utils/command.py
keeps its inline parser construction; a follow-up PR will switch it
to shell_parser.parse and confirm tests/tools/terminal/test_terminal_parsing.py
passes unchanged.
No iter_* convenience accessors. The Phase 2c direction note on
Proposal: Replace regex-based shell command analysis with tree-sitter-bash #2721 floats iter_simple_commands / iter_pipelines etc.; those
are easier to design once at least one detector exists to validate
them, so they land with Phase 2b or shortly after.
No ERROR-node policy. Pattern and rail detectors are unchanged.

Why the substrate change goes first

Following the suggestion to "divide the scope of the second part even
further": this is the smallest landable piece that unblocks Phase 2b
(security analyzers can now import a parser from the SDK without
acquiring a transitive dependency on openhands-tools) and Phase 2a
consumer refactor (a single import line change in command.py).
Reviewing one ~30-line module plus a dependency move is meaningfully
cheaper than reviewing the substrate together with either consumer
refactor.

Phase 2 roadmap

This PR is the first link in a chain. Each later PR is small and independently mergeable.

PR	Scope	Depends on
PR-A (this PR)	Shared `shell_parser.py` module + dep declaration. No consumer touched.	(rests on #3237)
PR-B	Refactor `openhands-tools/openhands/tools/terminal/utils/command.py` to consume `shell_parser.parse`.	PR-A
PR-D	Migrate `defense_in_depth/pattern.py` shell-destructive detectors to AST. The xfail bypass corpus from #3304 flips to passing under `strict=True`.	PR-A
PR-E	Migrate `defense_in_depth/policy_rails.py` rails (fetch-to-exec, raw-disk-op, catastrophic-delete) to AST.	PR-D
PR-F	Add `root_node` and `iter_*` convenience accessors to `ParseResult` once a detector validates the surface.	PR-D

Collaboration on lanes PR-B onward is welcome. /cc @eeee2345 in case the analyzer-side work aligns with the Agent Threat Rules direction in #3176.

Test plan

uv sync resolves cleanly; uv.lock regenerated with
tree-sitter / tree-sitter-bash declared by both packages.
tests/sdk/security/test_shell_parser.py passes (new file, 18
tests across success shape, error recovery, edge cases, immutability,
adversarial bytes, byte-offset semantics, determinism).
tests/tools/terminal/test_terminal_parsing.py passes unchanged
(no source changes in openhands-tools).
pre-commit run --all-files clean (ruff format, ruff lint,
pyright, pycodestyle).

Issue Number

Refs #2721

Add `openhands-sdk/openhands/sdk/security/shell_parser.py` exposing `parse(command: str) -> ParseResult`. Declare `tree-sitter` and `tree-sitter-bash` in `openhands-sdk/pyproject.toml`; the existing declarations in `openhands-tools/pyproject.toml` are unchanged because both packages directly import these libraries and each should declare what it imports rather than rely on transitives. `uv.lock` is regenerated. No consumer is refactored in this PR. `openhands-tools.terminal.utils.command` keeps its inline parser construction unchanged. A follow-up will migrate it to `shell_parser.parse`, and a later PR will add the AST-aware security detector that OpenHands#2721 Phase 2b targets. Scope: 1 new module (~30 LOC), 1 test module, 1 pyproject edit, regenerated lockfile. The `ParseResult` surface is intentionally minimal (`tree`, `has_error`) so the first move is a substrate change with no API surface enlargement. Convenience iterators and a public `root_node` field are deferred per the direction comment on OpenHands#2721. Refs OpenHands#2721 Coding-Agent: claude-code Model: claude-opus-4-7 AI-assisted: drafting, adversarial audit synthesis, test generation

all-hands-bot

Taste Rating: 🟢 Good taste. Focused shared parser entry point with real coverage; dependency upload times are outside the 7-day guardrail.

Verified locally: uv run pytest tests/sdk/security/test_shell_parser.py -q and uv run pre-commit run --files openhands-sdk/openhands/sdk/security/shell_parser.py tests/sdk/security/test_shell_parser.py openhands-sdk/pyproject.toml both passed.

Risk Assessment: 🟢 LOW — additive SDK module/dependency declaration; no existing consumers or agent behavior changed.

VERDICT: ✅ Worth merging.

This review was created by an AI agent (OpenHands) on behalf of the user.

VascoSch92

LGTM!

Just left a couple of comments.

VascoSch92 · 2026-05-20T08:16:26Z

+shared substrate rather than an API surface enlargement.
+
+The ``Parser`` is constructed per call. ``Language`` is built once at
+import. This mirrors the convention established by ``#3237`` in


please don't refer to the issue number.

VascoSch92 · 2026-05-20T08:16:49Z

+object is safely reusable.
+"""
+
+from __future__ import annotations


do we need this import?

VascoSch92 · 2026-05-20T08:17:09Z

+_BASH_LANGUAGE = Language(tree_sitter_bash.language())
+
+
+@dataclass(frozen=True)


you can also add slots=True

VascoSch92 · 2026-05-20T08:18:19Z

+    return ParseResult(tree=tree, has_error=tree.root_node.has_error)
+
+
+__all__ = ["ParseResult", "parse"]


This should be at the top of the file

VascoSch92 · 2026-05-20T08:20:26Z

+whose ``has_error`` flag faithfully reflects ``tree.root_node.has_error``.
+"""
+
+from __future__ import annotations


Do we need that?

eeee2345 · 2026-05-21T12:33:52Z

@Fieldnote-Echo — thanks for the cc. The substrate split is the right call; reviewing a ~30-line module + dep declaration in isolation is materially easier than reviewing it against either consumer.

For the ATR adapter path (#3176), PR-A landing is what we want — the adapter eventually lives at the detector layer (sibling to defense_in_depth/pattern.py), so a shared shell_parser.parse entry point in openhands-sdk/security/ is exactly the right shape to consume from. Will track PR-A → PR-D for the migration sequence; the ATR adapter's bash-pattern matchers will benefit from the same AST upgrade as defense_in_depth/pattern.py once that lands.

No review blockers from my side on this PR; just watching the chain.

- drop issue-number references from the module docstrings - remove the unneeded `from __future__ import annotations` (project is py>=3.12) - add `slots=True` to the frozen `ParseResult` - move `__all__` to the top of the module

all-hands-bot approved these changes May 19, 2026

View reviewed changes

VascoSch92 self-requested a review May 19, 2026 19:15

VascoSch92 approved these changes May 20, 2026

View reviewed changes

VascoSch92 reviewed May 20, 2026

View reviewed changes

refactor(sdk): address review nits on shared shell parser

c2159cd

- drop issue-number references from the module docstrings - remove the unneeded `from __future__ import annotations` (project is py>=3.12) - add `slots=True` to the frozen `ParseResult` - move `__all__` to the top of the module

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(sdk): introduce shared tree-sitter-bash entry point#3307

refactor(sdk): introduce shared tree-sitter-bash entry point#3307
Fieldnote-Echo wants to merge 2 commits into
OpenHands:mainfrom
Fieldnote-Echo:ast-migration

Fieldnote-Echo commented May 19, 2026

Uh oh!

all-hands-bot left a comment

Uh oh!

VascoSch92 left a comment

Uh oh!

VascoSch92 May 20, 2026

Uh oh!

VascoSch92 May 20, 2026

Uh oh!

VascoSch92 May 20, 2026

Uh oh!

VascoSch92 May 20, 2026

Uh oh!

VascoSch92 May 20, 2026

Uh oh!

eeee2345 commented May 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		_BASH_LANGUAGE = Language(tree_sitter_bash.language())


		@dataclass(frozen=True)

		return ParseResult(tree=tree, has_error=tree.root_node.has_error)


		__all__ = ["ParseResult", "parse"]

Conversation

Fieldnote-Echo commented May 19, 2026

Summary

Out of scope (deliberate)

Why the substrate change goes first

Phase 2 roadmap

Test plan

Issue Number

Uh oh!

all-hands-bot left a comment

Choose a reason for hiding this comment

Uh oh!

VascoSch92 left a comment

Choose a reason for hiding this comment

Uh oh!

VascoSch92 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

VascoSch92 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

VascoSch92 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

VascoSch92 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

VascoSch92 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

eeee2345 commented May 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants