feat(windows): add native maka.cu/2 executor - #8
Conversation
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Review of d722afda8517cd3f7557b895b2603cb6087c4278
This is the correct architectural direction: a Windows direct-COM executor behind the existing maka.cu/2 protocol, with no second model-facing schema or supervisor. The current revision is not merge-ready yet.
Blocking findings:
- The advertised snapshot/image lifecycle is not implemented. The handshake promises a 120-second snapshot TTL and a 256 MiB image-directory budget, but snapshots have no timestamp, no expiry/eviction path exists, and image files are not tracked or deleted when a snapshot is spent, superseded, expired, evicted, or released by
session.end. After 64 unspent observations the executor permanently returnssnapshot_registry_full, and image files accumulate until the host restarts. This violates the shared protocol's resource and stale-image guarantees. limits.maxResponseBytesis advertised as 6 MiB while the sharedmaka.cu/2contract fixes this limit at 1 MiB. The implementation also replaces an oversized response with an RPC error instead of reducing/truncating the observation to the declared bound. Please use the shared 1 MiB limit and add conformance coverage for bounded observations.- The stated local validation is not reproducible on this revision:
cargo test --all-targetspasses 7 tests, not 8, andcargo clippy --all-targets -- -D warningsfails on the current checkout. This repository currently has no GitHub checks for the PR. Please make fmt/clippy/test green on both the normal Windows build and the non-Windows CI analysis target, then add the checks as an actual merge gate.
The implementation is otherwise aligned with the agreed boundary: semantic element actions use UIA patterns, keyboard dispatch is observation/focus-bound, and dispatch.point is fail-closed. I recommend fixing the lifecycle contract first, then rerunning the real Windows matrix against the new head.
Production code that can be removed or simplified: the #[cfg(test)] compatibility authorization/input subsystem is historical experiment code and does not exercise the production maka.cu/2 path. Move any useful vectors into protocol tests and delete the inactive production-file implementation.
Test quality: keep lifecycle and real-application tests, but make the authoritative tests run against this repository revision and report the exact head/artifact digest. Current downstream reports alone cannot replace repository CI.
Verdict: not ready to merge. A deeper architectural rewrite is not required; focused lifecycle/resource accounting, protocol-limit conformance, test cleanup, and CI are required.
| .extract_if(|_, snapshot| snapshot.session == session) | ||
| .count(); | ||
| Ok( | ||
| json!({"ok":true,"session":session,"released":{"snapshots":released,"images":0,"streams":0}}), |
There was a problem hiding this comment.
session.end reports zero released images and only removes snapshot records. The shared protocol requires it to delete every image owned by the session and report the real count. Because Snapshot stores no image path and store_capture is untracked, spent/expired/session-ended snapshots leak files. Please add executor-owned image accounting and cleanup for every snapshot lifecycle transition.
| }, | ||
| "limits": { | ||
| "snapshotsPerSession": MAX_SNAPSHOTS, | ||
| "snapshotTtlMs": 120000, |
There was a problem hiding this comment.
The executor advertises snapshotTtlMs: 120000, but snapshots contain no creation/expiry time and registry admission only checks len() >= 64. Sixty-four observe-only calls therefore make the process permanently refuse further observations until sessions are explicitly ended. Implement TTL expiry and bounded eviction before advertising this limit.
| "snapshotTtlMs": 120000, | ||
| "maxElements": MAX_ELEMENTS, | ||
| "maxDepth": 64, | ||
| "maxTextChars": 500, |
There was a problem hiding this comment.
The shared maka.cu/2 contract fixes maxResponseBytes at 1 MiB; this executor advertises and enforces 6 MiB. Please align to the shared limit and truncate/reduce observation payloads to fit instead of returning an oversized-response RPC error.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Re-review of 47972926c8c938f4ccaa6563d8241849d1d161a1
The revision makes substantial progress on the previous findings: it adds explicit live/spent/superseded/expired/evicted states, 120-second cleanup, eight-live-snapshot enforcement, image accounting, the 1 MiB advertised limit, focused lifecycle tests, removal of the compatibility experiment, and a CI workflow. The current revision is still not merge-ready.
Blocking findings:
- Snapshot IDs are still deterministic process-local counters (
s000...) and contain no 128-bit per-process nonce. A restarted executor will mint the same IDs again, so a stale request from the previous generation can resolve to a fresh snapshot instead of returningsnapshot_unknown. This violates the explicit restart-isolation requirement inHOST_PROTOCOL.mdsection 4.1. - Terminal snapshots retain their full
elementsmaps indefinitely untilsession.end. The eight-snapshot limit counts only live entries, so a long session can accumulate an unbounded number of spent, superseded, expired, and evicted snapshots, each retaining up to 512 element records. Keep only compact, bounded tombstones for terminal error classification and release the heavy snapshot payload immediately. - Oversized responses are made to fit by recursively truncating every JSON string and then dropping elements/tree nodes. The protocol requires retrying observation with a reduced
maxElementsand, if it still does not fit, returningresponse_too_largewith{bytes, limit}; it explicitly says fields must not be dropped to fit. The current transformation can also change element text after its digest was computed and does not mark each changed field inelement.truncated, so the returned snapshot is not a faithful dispatch authority. - The new merge gate is not green on this exact head. I reran
cargo clippy --locked --all-targets --manifest-path apps/OpenComputerUseWindows/native/Cargo.toml -- -D warningson the Linux analysis target and it fails with 16 errors (unusedOnce, Windows-only constants and fields, readback symbols, and the non-Windows worker stub). GitHub currently reports no checks for this fork head, while the README and execution plan claim clippy passed.cargo fmt --check,cargo test(10/10), and the release build do pass locally.
The previous lifecycle/image-leak and 6 MiB declaration findings are directionally addressed. The architecture remains appropriate: one shared maka.cu/2 contract and a native Windows executor, with no new model-facing schema. No deeper product-side rewrite is required, but the executor still needs protocol-conformant response generation, bounded tombstones, nonce-based IDs, and a genuinely green Windows/Linux gate.
Recommended next revision:
- Add one random 128-bit generation nonce at process startup and include it in every snapshot ID; add a two-generation collision test.
- Split live snapshot payloads from compact terminal tombstones and bound/prune both explicitly.
- Enforce the response budget while constructing/retrying the observation, preserving the protocol's truncation semantics and exact digests.
- Fix cfg scoping/dead-code warnings, make the fork workflow run on the PR head, and attach the exact Windows artifact digest.
Verdict: not ready to merge. After these focused fixes and a green exact-head Windows/Linux run, the next review can move to real Windows application and packaged Maka qualification.
| .next | ||
| .fetch_add((MAX_ELEMENTS as u64) + 1, Ordering::Relaxed) | ||
| .wrapping_add(1); | ||
| let snapshot_id = format!("s{:016x}", token_seed); |
There was a problem hiding this comment.
[P1] Add a per-process generation nonce to snapshot IDs. This counter restarts from zero, so a new executor generation can mint the same s000... identifier as the previous process. A delayed stale request can then bind to a fresh snapshot instead of failing snapshot_unknown, contrary to HOST_PROTOCOL.md section 4.1. Generate one random 128-bit nonce at startup, prefix every snapshot ID with it, and test two independently initialized registries/generations for non-collision.
|
|
||
| fn reap_expired(&mut self, now: Instant) { | ||
| let mut retired_images = Vec::new(); | ||
| for snapshot in self.snapshots.values_mut() { |
There was a problem hiding this comment.
[P1] Terminal snapshots still retain the complete elements map. This loop changes only the state and image path; spent, superseded, expired, and evicted entries are excluded from the eight-live-snapshot limit but remain in registry.snapshots until session.end. A long conversation can therefore retain an unbounded number of up-to-512-element snapshots. Move terminal classification into compact bounded tombstones and drop the heavy element payload as soon as a snapshot leaves Live.
| } | ||
|
|
||
| fn shrink_oversized_observation(response: &mut Value) { | ||
| truncate_json_strings(response, 256); |
There was a problem hiding this comment.
[P1] Do not mutate arbitrary response strings to satisfy the byte limit. HOST_PROTOCOL.md section 7.5 requires reducing maxElements and rebuilding/retrying the observation, then returning response_too_large with byte/limit detail if it still cannot fit; it explicitly forbids dropping fields to fit. This pass can truncate title/value text after element digests were computed, without updating per-field element.truncated, leaving the wire snapshot inconsistent with its dispatch authority.
| run: cargo fmt --check | ||
|
|
||
| - name: Run clippy | ||
| run: cargo clippy --locked --all-targets -- -D warnings |
There was a problem hiding this comment.
[P1] This exact command is not green on the Ubuntu analysis target at 4797292. It fails with 16 -D warnings errors from Windows-only imports/constants/fields and non-Windows stubs. GitHub also currently reports zero check runs for this fork head, so the workflow has not established a merge gate yet. Please fix the cfg boundaries and show a successful exact-head Windows and Ubuntu run before describing clippy/CI as passed.
2 participants
Summary
maka.cu/2snapshot lifecycle: TTL, one-use spending, supersede/evict states, session cleanup, and distinct refusal codes.distributionReadyremains false.Validation
cargo fmt -- --checkcargo test --locked --all-targets— 10 passedcargo clippy --locked --all-targets -- -D warningscargo build --locked --releaseClean-machine validation, Authenticode signing, and packaged conversation E2E are intentionally not claimed by this PR; they must qualify the exact immutable artifact in a later release pipeline.
中文翻译
摘要
maka.cu/2snapshot 生命周期：TTL、一次性消费、替换/驱逐状态、session 清理和区分明确的拒绝码。distributionReady保持 false。验证
cargo fmt -- --checkcargo test --locked --all-targets—— 10 个通过cargo clippy --locked --all-targets -- -D warningscargo build --locked --release本 PR 不声称已经完成 clean-machine 验证、Authenticode 签名或 packaged conversation E2E；这些工作必须在后续发布流水线中针对同一个 immutable artifact 完成资格验证。