refactor(skills): report touched paths on observations for path-rule injection#4053
refactor(skills): report touched paths on observations for path-rule injection#4053VascoSch92 wants to merge 2 commits into
Conversation
9edec9e to
88126b7
Compare
…injection Path-rule injection read the touched file via getattr(action, "path"), an implicit untyped contract. It missed tools whose action names the field differently (Gemini file tools use file_path; apply_patch carries only a patch string) and could fire on a directory a tool merely searched. Add an `affected_paths` property to the Observation base (empty by default) that file-touching observations override to declare the paths they modified. The conversation seam now reads observation.affected_paths directly, dropping the action-correlation lookup and the string-name guess and supporting multiple paths (edits plus renames from a single apply_patch). Producers: file_editor, Gemini read/write/edit, and apply_patch (changed files plus rename destinations). grep/glob/list report nothing, so rules no longer fire on a directory that was only searched. The property is derived from stored fields rather than serialized, so it needs no schema migration and recomputes correctly after deserialization. Closes OpenHands#4038
88126b7 to
b5f1c90
Compare
|
|
||
| def _conversation(tmp_path: Path, rule: Skill) -> LocalConversation: | ||
| class _FileObservation(Observation): | ||
| """Minimal path-bearing observation (stands in for a file-tool result). |
There was a problem hiding this comment.
Kept it as a property on the Observation base rather than a Protocol.
A Protocol makes sense when types don't share a base, but every observation already subclasses Observation, and the seam already gets it typed (ObservationEvent.observation: Observation).
The base default (return []) makes affected_paths total, so the consumer just iterates with no isinstance/hasattr guard.
Or wdyt?
Move workspace-relative path normalization for path-rule injection out of LocalConversation._normalize_rule_path into a free function normalize_rule_path in skills/skill.py, beside path_matches_glob (the matcher it feeds). The injection seam now calls it with working_dir passed in; logic is unchanged. Add direct unit tests for the function.
|
[Automatic Post]: It has been a while since there was any activity on this PR. @VascoSch92, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @VascoSch92, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. This comment was generated by an AI agent (OpenHands) on behalf of the user. |
3 participants
HUMAN:
Refactor path-rule injection.
AGENT:
Why
Path-rule injection discovered the touched file via
getattr(action_event.action, "path", None)inLocalConversation._touched_rule_path— an implicit, untyped contract flaggedwhile reviewing #3994. It fails in two directions with tools that ship today:
file_path, andapply_patchcarries only a patch string — sogetattr(action, "path")returns
Noneand rules silently never fire.grep/glob/list_directorycarry apaththatis a search directory, not a modified file, so a directory a tool merely
searched can trigger
{dir}/**or**rules.It also can't express an operation that touches several files (a rename touches
source and destination; a patch can touch many).
Summary
affected_pathsproperty to theObservationbase (empty by default);file-touching observations override it to declare the paths they modified.
observation.affected_pathsdirectly — droppingthe action-correlation lookup and the
getattrstring-name guess, andsupporting multiple paths.
file_editor, Geminiread/write/edit, andapply_patch(changed files + rename destinations).grep/glob/listreport nothing, so rules no longer fire on a directory that was only searched.
This follows Option C from the issue, adapted: the value is a derived
property, not a stored/serialized field, so it needs no event-schema migration
and recomputes correctly after deserialization (the base is frozen with
extra="forbid", so a serialized computed field would break round-tripvalidation).
Issue Number
Closes #4038
How to Test
Unit + producer coverage:
Broader regression sweep (base-schema + producer changes):
End-to-end (the previously-missed case) — drive a real Gemini
write_fileobservation through a live
LocalConversationand confirm the rule fires:pre-commit run --all-filesscoped to the changed files passes (ruff, pyright,import-dependency rules, tool-registration).
Type
Notes
affected_paths, any file-mutating observation that doesn't override theproperty reports nothing — so all producers are updated in this PR. Third-party
tools that want path rules should override
affected_pathson theirobservation.
read_file/ file-editorviewreport their path(consistent with the pre-existing file-editor behaviour). Erroring ops report
nothing. Happy to gate reads out if reviewers prefer writes-only.
openhands-sdk→openhands-toolslayering is preserved (no newcross-package imports; verified by the import-rules hook).