feat(cli): establish npm release boundary - #3169
Conversation
Separate the production bin from repository CLI logic and close the package's accidental JavaScript import surface while preserving the current public commands. Fail Maka Eval before trial startup when pinned Python, Docker, or machine prerequisites are unavailable, using the same credential-filtered preparation environment as the trial supervisor. Generated-by: OpenAI Codex
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review. 📝 WalkthroughSummaryThis PR defines the npm release boundary for It also adds Source of truth and designThe PR extends the existing CLI and evaluation paths. It does not create a parallel command implementation. The design is the smallest coherent solution shown by the diff. The thin production entry point enforces the package boundary. The shared core prevents duplicated CLI behavior. The preflight helpers centralize environment preparation, path validation, command execution, timeout handling, cancellation, and error normalization. Canonical path validation prevents symlink-based escapes. No deletion is recommended without further review. The removed ValidationThe diff adds tests for package metadata, command behavior, exit handling, credential filtering, writable and missing paths, dangling symlinks, prerequisite validation, Python and Docker checks, mounts, version mismatches, path traversal, symlink escapes, and cancellation. It also validates failure before trial startup. The stated verification includes affected-workspace tests, linting, formatting, builds, type checking, production-entry smoke tests, and package-boundary checks. Direct current check results are not available here, so required checks remain unverified. Real Harbor/Pier trials, tarball installation matrices, bundled dependency checks, file allowlists, and release automation remain deferred to follow-up work in Review-relevant risks
WalkthroughThe PR extracts CLI behavior into ChangesCLI core extraction
Executor preflight validation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟠 High · up to This change adds Eval installation and path preflight checks, but the current implementation can still allow symlinked paths to escape the configured source directory before execution policies apply, creating a concrete isolation and data-exposure risk. Merge should be blocked until that path-boundary issue is fixed; several lower-impact correctness and maintainability follow-ups also remain. Sequence Diagram(s)sequenceDiagram
participant CLI
participant cliCore
participant CommandHandler
participant Process
CLI->>cliCore: invoke launchMakaCli
cliCore->>cliCore: parse arguments
cliCore->>CommandHandler: dispatch command
CommandHandler-->>cliCore: return exit code
cliCore->>Process: perform deferred exit
sequenceDiagram
participant EvalCLI
participant HarnessExecutor
participant Preflight
participant Python
participant Docker
participant Experiment
EvalCLI->>HarnessExecutor: create built-in executor
HarnessExecutor->>Preflight: validate installation
Preflight->>Python: check framework
Preflight->>Docker: check daemon when required
Python-->>Preflight: return result
Docker-->>Preflight: return result
Preflight-->>EvalCLI: complete validation
EvalCLI->>Experiment: run with validated executor
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoEstablish bin-only Maka CLI release boundary and eval install preflight
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/eval/src/install-preflight.ts (2)
38-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider reusing the decoded executor options instead of re-parsing
spec.executor.config.
packages/eval/src/harness-executor.tsalready decodes and validates the same fields indecodeOptions(frameworkVersion,pythonPathEnv,trialsRootEnv,tasksRootEnv,mounts,egressProxy,preparationEnvironment), including env-name shape, uniqueness, and machine-path presence.cli.tscallsloadExecutorbeforepreflightBuiltinExecutor, so that decoding always runs first. This file adds a second parser (configText,configObject,configArray,configValueText) over the same JSON, so the two can drift.A smaller solution is to expose the decoded
HarnessOptionsfrom the built-in executor and pass it to the preflight, keeping decoding at one source of truth. The preflight then only performs filesystem and process probes.As per path instructions: "Determine whether it is the smallest coherent solution at the existing source of truth. Flag concrete cases where code can be deleted or simplified."
Also applies to: 196-218
Source: Path instructions
80-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe bundled relay root is now computed in two places.
packages/eval/src/harness-executor.tsline 357 computes the same../harborpath relative to the module directory. Export one shared constant or helper and use it in both files, so a future layout change cannot leave the preflight checking a stale location.♻️ Suggested direction
- const relayRoot = resolve(dirname(fileURLToPath(import.meta.url)), '../harbor'); + const relayRoot = bundledRelayRoot();Define
bundledRelayRoot()in a shared module (for exampleharness-environment.ts) and call it fromstartTrialas well.packages/eval/src/harness-environment.ts (1)
3-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an options object for the two adjacent optional string parameters.
egressAllowedHostandnetworkPolicyPathare adjacent optional strings. A swapped call site compiles and produces a wrong sanitized environment silently. A named-field parameter removes that failure mode. This is now a shared, security-relevant helper with two call sites, so the surface is worth pinning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a86c2b89-e57c-4e4e-bbaf-f749c83dde16
📒 Files selected for processing (14)
packages/cli/package.jsonpackages/cli/src/__tests__/cli.test.tspackages/cli/src/__tests__/pi-tui-runner.test.tspackages/cli/src/cli-core.tspackages/cli/src/cli.tspackages/cli/src/dev-cli.tspackages/cli/src/index.tspackages/eval/README.mdpackages/eval/src/__tests__/cli.test.tspackages/eval/src/__tests__/install-preflight.test.tspackages/eval/src/cli.tspackages/eval/src/harness-environment.tspackages/eval/src/harness-executor.tspackages/eval/src/install-preflight.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
There was a problem hiding this comment.
Pull request overview
This PR tightens the npm release boundary for the Maka CLI by separating the production executable entrypoint from reusable CLI logic, and adds an install-state preflight to maka eval so external prerequisites are validated before any trial starts.
Changes:
- Split the CLI into a bin-only
dist/cli.jsentrypoint plus a reusablecli-coremodule, and update tests/exports accordingly. - Add
maka evalinstall preflight checks for machine paths, bundled relay assets, pinned Harbor/Pier Python distributions, and Docker daemon availability. - Deduplicate harness environment construction into a shared helper used by both preflight and execution.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/eval/src/install-preflight.ts | New install-state preflight for Harbor/Pier executors (paths/assets/Python/Docker) before trials start. |
| packages/eval/src/harness-executor.ts | Reuses the new shared harness environment builder instead of inlining it. |
| packages/eval/src/harness-environment.ts | New shared environment-filtering helper for harness/preflight processes. |
| packages/eval/src/cli.ts | Runs install preflight before starting an experiment attempt (builtin executor only). |
| packages/eval/src/tests/install-preflight.test.ts | New tests validating preflight ordering, env filtering, and error reporting. |
| packages/eval/src/tests/cli.test.ts | New CLI test ensuring preflight failure happens before any attempt starts. |
| packages/eval/README.md | Documents the new public CLI preflight behavior for prerequisites. |
| packages/cli/src/index.ts | Re-exports CLI parsing/runner APIs from cli-core instead of the executable entrypoint. |
| packages/cli/src/dev-cli.ts | Dev launcher now imports launchMakaCli from cli-core. |
| packages/cli/src/cli.ts | Production bin entrypoint now delegates to cli-core and always launches. |
| packages/cli/src/cli-core.ts | New module containing the CLI implementation previously in cli.ts. |
| packages/cli/src/tests/pi-tui-runner.test.ts | Updates test harness imports from cli.js to cli-core.js. |
| packages/cli/src/tests/cli.test.ts | Adds assertions for bin-only package surface and updates imports to cli-core. |
| packages/cli/package.json | Removes library entrypoints and sets exports: {} to enforce bin-only boundary. |
Suppressed comments (2)
packages/eval/src/install-preflight.ts:68
- The egressProxy preflight currently resolves composeRelativePath/networkPolicyRelativePath without guarding against ".." escapes and also reports missing fields as executor.config.composeSourceEnv (missing the egressProxy prefix). This can produce confusing diagnostics and allows path traversal-style inputs that are only rejected later by harness-executor.
const proxy = configObject(egressProxy, 'egressProxy');
const sourceEnv = configText(proxy, 'composeSourceEnv');
const source = machinePath(sourceEnv);
await requirePath(source, `machine path ${sourceEnv}`, 'directory');
await requirePath(
packages/eval/src/install-preflight.ts:179
- optionalDirectory rethrows raw stat() errors (e.g., EACCES) without the machine-path label, which makes preflight failures less actionable than requirePath(). Consider wrapping non-ENOENT errors the same way requirePath does.
const metadata = await stat(path);
if (!metadata.isDirectory()) throw new Error(`${label} is not a directory: ${path}`);
} catch (error) {
if ((error as NodeJS.ErrnoException).code === 'ENOENT') return;
throw error;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Run install checks from the Harbor/Pier executor's decoded configuration so schema validation, egress paths, and preparation environments cannot drift. Propagate cancellation through prerequisite probes and remove the library barrel made unreachable by the bin-only CLI boundary. Generated-by: OpenAI Codex
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 321d7990-940a-40f8-89ff-50420f23b102
📒 Files selected for processing (7)
packages/cli/src/__tests__/cli.test.tspackages/cli/src/index.tspackages/eval/src/__tests__/install-preflight.test.tspackages/eval/src/cli.tspackages/eval/src/harness-environment.tspackages/eval/src/harness-executor.tspackages/eval/src/install-preflight.ts
💤 Files with no reviewable changes (1)
- packages/cli/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/tests/cli.test.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/eval/src/install-preflight.ts:140
- This catch labels all failures as “Docker daemon is unavailable”, which is misleading when the Docker CLI itself is missing (e.g. ENOENT) or not executable. Prefer a more accurate message that covers both missing CLI and daemon failures.
throw new Error(`Docker daemon is unavailable: ${errorMessage(error)}`);
Canonicalize the trusted egress root and selected Compose and policy files before accepting them, so lexical containment cannot be bypassed through a symlink. Generated-by: OpenAI Codex
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/eval/src/install-preflight.ts (1)
210-218: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate that an existing trials root is usable.
optionalDirectoryaccepts an existing directory without checking write and search access. Line 58 uses this helper foroptions.trialsRootEnv, butstartTriallater changes its mode and writes the trial configuration below it. A non-writable trials root therefore passes preflight and fails only after evaluation setup starts.For an existing trials root, check
W_OK | X_OK. For a missing trials root, validate the nearest existing parent before accepting the path.
🧹 Nitpick comments (1)
packages/eval/src/harness-environment.ts (1)
57-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep one source of truth for path containment.
Lines [57-72] add shared helpers, but the supplied snippets still show equivalent implementations in
packages/eval/src/install-preflight.tsandpackages/eval/src/harness-executor.ts. Import these helpers frompackages/eval/src/harness-environment.tsand delete the local copies. Otherwise, the symlink and containment fixes can diverge across preflight and trial execution.As per path instructions, flag concrete code that can be deleted or simplified to keep the smallest coherent solution.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a594e6b6-525d-4038-a51e-37b0f3c510ba
📒 Files selected for processing (4)
packages/eval/src/__tests__/install-preflight.test.tspackages/eval/src/harness-environment.tspackages/eval/src/harness-executor.tspackages/eval/src/install-preflight.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/eval/src/tests/install-preflight.test.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/eval/src/harness-environment.ts:72
resolveRealPathWithinRoot()will currently surface rawrealpath()system errors (e.g., ENOENT/EACCES) without the providedlabel, which makes egress path failures harder to diagnose and inconsistent with the clearer messages produced byrequirePath()in the preflight. Wrappingrealpath()failures with a label-specific error keeps diagnostics actionable when egress assets disappear or are unreadable at runtime.
): Promise<string> {
const resolved = resolvePathWithinRoot(root, path, label);
const [canonicalRoot, canonicalPath] = await Promise.all([realpath(root), realpath(resolved)]);
assertPathWithinRoot(canonicalRoot, canonicalPath, label);
return canonicalPath;
Keep install preflight aligned with the filesystem operations required to prepare a trial, including creatable missing roots. Improve prerequisite diagnostics when Docker or canonical egress paths are unavailable. Generated-by: OpenAI Codex
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f8e281b-436a-4fdd-bcad-0c59938a6612
📒 Files selected for processing (3)
packages/eval/src/__tests__/install-preflight.test.tspackages/eval/src/harness-environment.tspackages/eval/src/install-preflight.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/eval/src/harness-environment.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review.
Distinguish a genuinely missing trials directory from a dangling symlink so install preflight cannot approve a path that recursive mkdir will reject. Generated-by: OpenAI Codex
English
Summary
dist/cli.jsexecutable from reusable CLI logic and keep the repository-only dev launcher on its own profile.maka-agentexplicitly bin-only while preservingmaka,maka-agent, TUI,run,activate, Runtime Host commands, and the lazily loadedmaka evalsurface.maka evalinstall-state preflight for machine paths, bundled relay assets, pinned Harbor/Pier Python environments, and Docker availability before any trial starts. The probe reuses the trial supervisor's credential-filtered environment.Refs #3166
Verification
npm --workspace maka-agent test— 262 tests passed.npm --workspace @maka/eval run build && npm --workspace @maka/eval run test:dist— 72 Node tests and all non-Docker Python suites passed; the existing Docker namespace suite skipped because no daemon is available in this environment.npm run lintnpm run format:checknpm run buildnpm run typecheckgit diff --check--help,--version, andeval --help; importingmaka-agentas a JavaScript library is rejected by the package boundary.A real Harbor/Pier trial and install-from-tarball matrix are intentionally left to the packaging and release-gate follow-ups in #3166.
Review focus
Please review the public command boundary and whether the Eval preflight covers the correct external prerequisites without changing experiment semantics. Tarball construction, bundled dependencies, file allowlists, and release automation are out of scope for this PR.
AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex assisted with implementation, tests, documentation, and local verification under M4n5ter's direction. The commit includes the required
Generated-by: OpenAI Codextrailer.Checklist
Does this PR entail a change in behavior?
中文
概述
dist/cli.js可执行入口与可复用 CLI 逻辑分离，并让仓库专用 dev launcher 继续使用独立 profile。maka-agent明确收敛为 bin-only 包，同时保留maka、maka-agent、TUI、run、activate、Runtime Host 命令，以及按需加载的maka eval。maka eval增加安装态预检，在启动任何 trial 前检查机器路径、包内 relay 资源、固定版本的 Harbor/Pier Python 环境和 Docker 可用性。探测过程复用 trial supervisor 的凭证过滤环境。关联 #3166。
验证
npm --workspace maka-agent test：262 项测试通过。npm --workspace @maka/eval run build && npm --workspace @maka/eval run test:dist：72 项 Node 测试及全部非 Docker Python 测试通过；当前环境没有 Docker daemon，既有 namespace 集成测试按设计跳过。git diff --check通过。--help、--version、eval --helpsmoke 通过；将maka-agent作为 JavaScript library 导入会被包边界拒绝。真实 Harbor/Pier trial 和 tarball 隔离安装矩阵留给 #3166 后续的打包与发布门禁 PR。
审查重点
请重点确认公共命令边界，以及 Eval preflight 是否在不改变实验语义的前提下覆盖了正确的外部前置条件。Tarball 构建、bundled dependencies、文件白名单和发布自动化不在本 PR 范围内。
AI 使用
OpenAI Codex 在 M4n5ter 的指导下辅助了实现、测试、文档和本地验证。提交已包含项目要求的
Generated-by: OpenAI Codextrailer。勾选状态以英文区为准。检查清单