Feat/rpa harness - #20
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces cross-platform support for OpenChronicle by adding Windows UI Automation (UIA) as a capture source and implementing a new RPA harness for pluggable automation providers, including an Android ADB provider. The changes include updates to configuration paths, daemon management for Windows, and robust file I/O operations. I have provided feedback regarding the use of read_bytes() for JSON parsing to improve encoding handling and a minor null-check optimization for the Windows UIA provider.
| """Rewrite a capture JSON without its ``screenshot`` field. Returns True if stripped.""" | ||
| try: | ||
| raw = path.read_text() | ||
| raw = path.read_text(encoding="utf-8") |
There was a problem hiding this comment.
When parsing JSON files, it is recommended to use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions.
| raw = path.read_text(encoding="utf-8") | |
| raw = path.read_bytes() |
References
- When parsing JSON files, use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions, which are a type of ValueError and may not be caught by OSError handlers.
| return {} | ||
| try: | ||
| data = json.loads(path.read_text()) | ||
| data = json.loads(path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
When parsing JSON files, it is recommended to use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions.
| data = json.loads(path.read_text(encoding="utf-8")) | |
| data = json.loads(path.read_bytes()) |
References
- When parsing JSON files, use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions, which are a type of ValueError and may not be caught by OSError handlers.
| return {} | ||
| try: | ||
| data = json.loads(path.read_text()) | ||
| data = json.loads(path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
When parsing JSON files, it is recommended to use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions.
| data = json.loads(path.read_text(encoding="utf-8")) | |
| data = json.loads(path.read_bytes()) |
References
- When parsing JSON files, use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions, which are a type of ValueError and may not be caught by OSError handlers.
| for p in files: | ||
| try: | ||
| data = json.loads(p.read_text()) | ||
| data = json.loads(p.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
When parsing JSON files, it is recommended to use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions.
| data = json.loads(p.read_text(encoding="utf-8")) | |
| data = json.loads(p.read_bytes()) |
References
- When parsing JSON files, use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions, which are a type of ValueError and may not be caught by OSError handlers.
| def _load_capture(path: Path) -> dict[str, Any] | None: | ||
| try: | ||
| return json.loads(path.read_text()) | ||
| return json.loads(path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
When parsing JSON files, it is recommended to use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions.
| return json.loads(path.read_text(encoding="utf-8")) | |
| return json.loads(path.read_bytes()) |
References
- When parsing JSON files, use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions, which are a type of ValueError and may not be caught by OSError handlers.
| def load_manifest(path: str | Path) -> ProviderManifest: | ||
| manifest_path = Path(path) | ||
| try: | ||
| data = json.loads(manifest_path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
When parsing JSON files, it is recommended to use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions.
| data = json.loads(manifest_path.read_text(encoding="utf-8")) | |
| data = json.loads(manifest_path.read_bytes()) |
References
- When parsing JSON files, use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions, which are a type of ValueError and may not be caught by OSError handlers.
| def load_workflow(path: str | Path) -> dict[str, Any]: | ||
| workflow_path = Path(path) | ||
| try: | ||
| data = json.loads(workflow_path.read_text(encoding="utf-8")) |
There was a problem hiding this comment.
When parsing JSON files, it is recommended to use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions.
| data = json.loads(workflow_path.read_text(encoding="utf-8")) | |
| data = json.loads(workflow_path.read_bytes()) |
References
- When parsing JSON files, use read_bytes() and pass the result to json.loads() to let the JSON library handle encoding detection and prevent unhandled UnicodeDecodeError exceptions, which are a type of ValueError and may not be caught by OSError handlers.
| if _control_identity(focused) != _control_identity(foreground): | ||
| focused_el = _element_from_control(focused, depth=min(self._depth, 3), budget=budget) |
There was a problem hiding this comment.
The focused control can be None if _safe_focused_control fails. While _element_from_control handles None gracefully by returning None, it is more efficient to check for None before calling it.
| if _control_identity(focused) != _control_identity(foreground): | |
| focused_el = _element_from_control(focused, depth=min(self._depth, 3), budget=budget) | |
| if focused is not None and _control_identity(focused) != _control_identity(foreground): | |
| focused_el = _element_from_control(focused, depth=min(self._depth, 3), budget=budget) |
Xiao-ao-jiang-hu
left a comment
Xiao-ao-jiang-hu
left a comment
There was a problem hiding this comment.
Thanks for the work here. I appreciate the ambition behind this PR, but I cannot approve or merge it in its current form.
This PR is far beyond the acceptable scope for a single OpenChronicle change. It introduces multiple major product directions at once. That makes it very difficult to review safely, reason about the product impact, or validate the security model.
OpenChronicle’s current positioning is intentionally lightweight: a local-first screen-context memory system for local agent software, exposed through MCP. The core value is observing local context, storing it, compressing it, and letting agents retrieve it through a small MCP surface. This PR changes that product boundary from “observe and remember” into “operate devices and replay UI workflows.” That is a much larger and riskier product direction.
The RPA harness in particular does not belong in the core project as submitted. RPA means recording, replaying, and executing UI actions. That is an actuation/control system, not just a memory MCP. It introduces new trust boundaries, safety requirements, permission models, audit requirements, and failure modes. Those need a separate design discussion before any code lands in core.
There are several concrete issues with the current PR:
-
The PR violates the minimum-scope principle.
A PR should fix one bug, add one feature, or change one coherent module. This PR combines many unrelated or only loosely related changes:
- Windows UIA capture support
- Android ADB control
- A separate ADB MCP server
- Generic RPA provider abstractions
- RPA workflow schemas, runner, recorder, replayer, trace store, skill builder
- A mock-heavy React/Vite UI application
- Large PNG UI assets
- zipped UI source archives and patch files
- installation script changes
- documentation and examples
- modifications to existing capture, CLI, config, paths, store, and tests
These should not be reviewed or merged as one unit.
-
The PR changes the product direction too much.
OpenChronicle is currently a lightweight local memory/context MCP. Adding ADB control and RPA replay means agents can actively operate devices. That is a qualitatively different product surface.
A read/search MCP can be small, local, and low-risk. A control MCP needs a much stronger security model: explicit user consent, capability scoping, action approval, default-off behavior, audit logs, emergency stops, and very clear documentation of what agents are allowed to do.
This PR does include some safety code, but the design is not mature enough for this kind of capability to enter the core project.
-
Large parts of the RPA system are still mock/MVP/skeleton code.
Several files indicate that this is not a finished production feature:
src/openchronicle/rpa/replayer.pydescribes itself as an MVP using a mock provider.src/openchronicle/rpa/observer.pyis a mock observer.src/openchronicle/rpa/providers/windows_uia/adapter.pyregisters actions like click/type/hotkey, but returns “not implemented yet” for most of them.ui/src/lib/rpaApi.tsreads from mock data rather than a real backend API.- The UI appears to be a mock dashboard/prototype rather than an integrated product surface.
Experimental scaffolding is fine in a lab branch or separate package, but it should not be merged into the main lightweight MCP project.
-
The UI artifacts do not belong in this PR.
The PR adds a full Vite/React UI tree, mock data, large PNG images, UI specs, PRD documents, zip archives, and patch files. This is not appropriate for a core Python MCP package PR.
If a UI is desired, it should be discussed as a separate product surface and probably live in a separate repository or clearly isolated package. It should not be mixed with capture providers, ADB control, RPA workflow execution, and core CLI changes.
-
The code does not currently pass the project’s own quality checks.
Running Ruff on the branch reports multiple issues, including unused variables/imports, unsorted imports, style violations, and modernization warnings. Examples include:
- unused local variable in
src/openchronicle/rpa/replayer.py - import sorting issues in
src/openchronicle/rpa/skill_store.py - suppressible
try/except/passpatterns in RPA storage/report code datetime.UTCmodernization warnings- unused imports in tests
A PR of this size must at minimum pass the existing lint and test standards before it can be reviewed seriously.
- unused local variable in
-
The security model for ADB/RPA control is not sufficient for core inclusion.
The ADB MCP exposes action tools such as tap, swipe, text input, app launch, logcat access, pairing, and wireless connect. Even with denylist checks, this is a sensitive control surface.
A denylist is not enough by itself for an agent-facing control MCP. We would need a clearer capability model, default-off configuration, explicit user approval flows, scope restrictions, documented threat model, and stronger separation from the read-only memory MCP.
-
The architecture is not cleanly separated.
RPA, ADB, Windows capture, UI, skill generation, trace storage, and MCP server additions are all mixed into the main package. This makes the core project heavier and harder to maintain.
If this direction is pursued, it should probably be split into optional packages or experimental modules, for example:
openchronicle-windows-captureopenchronicle-adbopenchronicle-rpa-labs- or a plugin/experimental directory that is not installed by default
The current PR makes these capabilities feel like part of the default core product, which is not appropriate.
Suggested path forward
I recommend closing or substantially rewriting this PR into smaller, reviewable PRs.
A reasonable split would be:
-
Windows AX/UIA capture support as a standalone PR.
This is the one part of the PR that does fit the existing OpenChronicle product direction. Windows capture is analogous to the existing macOS AX capture path: it improves local context observation and keeps the product within the “memory/context MCP” boundary.
That PR should include only:
- the Windows UIA capture provider
- minimal changes to provider selection
- tests for Windows UIA behavior with mocked UIA objects
- documentation for Windows capture setup
- no RPA harness
- no ADB control
- no React UI
- no workflow runner/recorder
- no large binary UI assets
-
ADB MCP as a separate proposal or experimental PR.
This needs its own design review because it adds agent-controlled device actions. It should be default-off, optional, and reviewed primarily as a security-sensitive control surface.
-
RPA harness as a separate RFC before implementation.
The project should first decide whether OpenChronicle wants to become an automation/control platform at all. If yes, the RPA system should probably live outside the core lightweight MCP package or under an explicitly experimental/plugin namespace.
-
UI dashboard as a separate project or later PR.
The mock UI should not be merged with backend automation primitives. It needs its own product decision, integration plan, and build/package strategy.
Decision
I am requesting changes and do not think this PR should be merged as-is.
The main reason is not that every individual idea is bad. Some pieces, especially Windows UIA capture, may be valuable. The issue is that this PR combines too many unrelated product directions, expands the project far beyond its lightweight MCP positioning, introduces a sensitive device-control surface, and includes unfinished/mock-heavy RPA and UI code.
Please split out Windows AX/UIA capture into a focused PR first. That would be much easier to review and is aligned with OpenChronicle’s current mission.
2 participants
Summary
Verification