refactor: consolidate public APIs and computer-use scripts - #2592
refactor: consolidate public APIs and computer-use scripts#2592jackwener wants to merge 1 commit into
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for doing the hard consolidation work across the public API and Computer Use entry points. I reviewed the current head with four independent review slices plus a read-only ollama-cloud/deepseek-v4-flash:high pass.
The direction is sound: one public package surface and one data-driven Computer Use dispatcher remove parallel paths. The concrete issue is narrower: moving the Runtime prefix digest to the browser-safe helper also changed its memory model from incremental hashing to retaining every encoded row, concatenating them, and then allocating the SHA-256 padded copy. That is avoidable complexity in a hot persistence boundary.
The first-principles seam is a single incremental SHA-256 authority that accepts chunks in both browser-safe and Node contexts. Keep the canonical byte order exactly as it is, but feed the domain tag, identity, sequence, and event payload directly into the digest instead of constructing a whole-prefix buffer. That preserves the consolidation goal without making memory proportional to the serialized history.
I found no P0/P1. The P2 below should be handled, but it does not change the overall PASS gate. The no-subcommand behavior, mapping coverage, stale documentation references, and manifest-only export test are P3 cleanup opportunities rather than production blockers.
No local test suite was run during this review; conclusions are based on source and test inspection. Codex coordinated the independent passes and performed the final adjudication; external-model output was treated as unverified until checked against the code.
中文摘要
感谢这次对公共 API 和 Computer Use 入口的大规模收敛。方向是正确的:单一公共包入口和数据驱动 dispatcher 都在删除并行路径。
当前唯一需要处理的具体问题是 Runtime prefix digest 的内存模型从增量 hash 退化成了保存全部编码行、拼接整段 buffer,再由 SHA-256 创建 padding 副本。更符合第一性原理的方案是提供一个浏览器与 Node 都能使用的增量 SHA-256 权威,保持现有 canonical byte order,但逐块写入 domain tag、identity、sequence 和 event payload,不再构造整段历史 buffer。
未发现 P0/P1。下面的 P2 应处理,但整体 gate 仍为 PASS。其余无子命令行为、mapping 覆盖、文档引用和 manifest-only 测试都属于 P3 清理项,不应阻塞。
本次未在本地运行测试套件;结论来自源码和测试检查。Codex 协调了独立审查并完成最终判断,外部模型输出在核对代码前均视为未验证输入。
| chunks.push(lengthPrefixed(utf8(encodeCanonicalRuntimeEvent(row.event).json))); | ||
| } | ||
| return `sha256:${hash.digest('hex')}`; | ||
| return `sha256:${sha256Hex(concatBytes(...chunks))}`; |
There was a problem hiding this comment.
P2 — Preserve incremental hashing for large Runtime prefixes. This now retains every encoded row in chunks, allocates the concatenated full-prefix buffer, and sha256Hex then allocates its padded working copy. The previous createHash().update(...) path kept extra memory bounded, so long sessions can now see a large avoidable peak on this persistence boundary. Please expose a browser-safe incremental SHA-256/chunked digest seam and feed the existing canonical pieces into it directly; add a focused large-prefix regression check that demonstrates memory does not scale with an additional whole serialized copy.
|
/agentic_review |
Code Review by Qodo
1. Stale Computer Use command
|
| "e2e:computer-use-function-model": "node scripts/cu-real-function-model-e2e.mjs", | ||
| "e2e:computer-use-real-anthropic": "node scripts/cu-real-anthropic-model-e2e.mjs", | ||
| "e2e:computer-use-real-runtime": "node scripts/cu-real-runtime-model-e2e.mjs" | ||
| "e2e:computer-use": "node scripts/computer-use.mjs" |
There was a problem hiding this comment.
1. Stale computer use command 🐞 Bug ≡ Correctness
Fix-now: the consolidation leaves docs/archive/computer-use-process-restart-e2e.md instructing users to invoke the deleted npm run e2e:computer-use-process-restart script, causing npm’s “Missing script” error instead of running restart recovery. The canonical equivalent is now `npm run e2e:computer-use -- process-restart`.
Agent Prompt
## Issue description
The Computer Use script consolidation removes `e2e:computer-use-process-restart`, but the archived restart-E2E runbook still invokes it. Replace that invocation with the canonical dispatcher command; no new behavior, public command, or compatibility alias is needed.
## Issue Context
Use `npm run e2e:computer-use -- process-restart`. The deleted script set `MAKA_CU_AX_MODEL_SCENARIO=restart-recovery`, and the new dispatcher preserves that behavior under the `process-restart` command, so updating the existing documentation is the smallest correction.
## Fix Focus Areas
- docs/archive/computer-use-process-restart-e2e.md[129-133]
- package.json[64-65]
- scripts/computer-use.mjs[8-10]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Astro-Han
left a comment
There was a problem hiding this comment.
Approving at this head, converting my earlier verdict into an explicit approval. My previous review already concluded no P0/P1 and an overall gate of PASS: the one P2 is worth addressing but does not block, and the remaining items — no-subcommand behaviour, mapping coverage, documentation references, and the manifest-only test — are P3 cleanups. Nothing has changed on this head since; I was withholding the state, not the judgement.
|
Hi — this PR conflicts with current I tested a rebase onto current
These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current Thanks for the contribution — happy to help if any conflict is unclear. AI-assisted maintenance note, not a review. It does not count as the required human review under |
|
Heads-up on why this PR has no CI results: it is not waiting for someone to release a run. There are zero workflow runs on the current head, because the PR is currently conflicting with That makes it look identical to "checks not applicable," which it is not. Rebasing onto current Flagging this because a review cannot be closed out against a head that has no gate evidence. |
Summary
Consolidates the duplicated package and script entry surfaces:
@maka/coreexports from 104 to the root product API plus the non-overlapping@maka/core/nodeadapter;@maka/runtimeexports from 54 to the root product API plus its existing test-only observation helper;npm run e2e:computer-use -- <command>and one canonical dispatcher;Verification
npm run build:testnpm run typechecknpm run lintnpm run format:checknpm --workspace @maka/desktop run build:renderernpm --workspace @maka/core test— 815 passednpm --workspace @maka/ui run test:dist— 260 passednpm --workspace @maka/desktop run test:dist— 1131 passedThe full
npm testrun exposed one unrelated existing Storage failure: importing the Storage root emits the Nodenode:sqliteExperimentalWarning through existing static imports insession-bundle-policy.ts/operational-state-backup.ts. The remaining Storage tests passed (772 passed, 12 skipped).Breaking change
@maka/core/permissionand@maka/runtime/model-runtimewith@maka/coreand@maka/runtime.@maka/core/node.npm run e2e:computer-use-realwithnpm run e2e:computer-use -- real.Checklist
Does this PR entail a change in behavior?