fix(runtime): separate Runtime CLI authority from init - #266
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (8)
|
| Layer / File(s) | Summary |
|---|---|
Persisted Runtime status projection src/local-runtime/types.ts, src/local-runtime/status.ts, src/local-runtime/status.test.ts, src/local-runtime/index.ts |
Adds a bounded status type, persisted-state reader, human-readable formatters, public exports, and non-mutating status tests. |
Runtime command wiring and init decoupling src/cli.ts, src/cli.test.ts, src/init.ts, src/init.test.ts |
Adds runtime ensure and runtime status. Rejects init --runtime and removes Runtime provisioning and reporting from initialization. |
CLI documentation and packaged validation README.md, docs/local-runtime.md, scripts/smoke-test.mjs |
Updates lifecycle documentation and verifies packed-package Runtime help, JSON status, absent state, and state-directory behavior. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Merge Risk: ⚪ Minimal · up to ffb02
This change separates Runtime lifecycle commands from initialization and adds bounded, read-only status reporting; no actionable merge-blocking risk remains after normal checks and review.
Possibly related issues
- refactor(runtime): complete the four-route declarative deployment ladder #232 — Defines the broader requirement for
runtime ensure/statusas the canonical Runtime lifecycle and for removinginit --runtime.
Possibly related PRs
- yohn-jp/mottainai#15 — Earlier changes to the shared
mottainai initimplementation and CLI workflow. - yohn-jp/mottainai#68 — Related packed-package smoke coverage for Runtime lifecycle commands.
- yohn-jp/mottainai#117 — Related Runtime status and identity diagnostics.
Suggested reviewers: claude
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly states that Runtime CLI authority is separated from init, which matches the primary change. |
| Description check | ✅ Passed | The description directly covers the Runtime CLI separation, included changes, exclusions, behavior, and validation. |
| Linked Issues check | ✅ Passed | The changes satisfy Issue #258 by removing init Runtime coupling, adding runtime ensure/status, preserving output contracts, and updating docs and smoke tests. |
| Out of Scope Changes check | ✅ Passed | The changes remain within Issue #258 scope and contain no unrelated Runtime artifact, hardware integration, Nix, or remote-placement work. |
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
- Create stacked PR
- Commit on current branch
🛠️ Fix failing CI checks 💡
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
fix/258-resolve-258
Comment @coderabbitai help to get the list of available commands.
yohnark
left a comment
yohnark
left a comment
There was a problem hiding this comment.
Merge blockers
The implementation direction matches #258, but this PR is not merge-ready yet.
- Governance rejects the PR contract because
runtimeis not an allowed conventional scope inscripts/governance-rules.json. Runtime is now a first-class subsystem, so prefer fixing the governance vocabulary once (addruntimeas a supported scope) rather than disguising these PRs under an unrelated scope. - Linked Issue #258 is currently governance-invalid / needs specification and must be repaired to the current Issue contract.
- This changes the shipped CLI/package surface, while Node compatibility smoke and built-dist/package CI are failing. #258 explicitly requires packed CLI/package smoke evidence, so this is an acceptance blocker.
- The branch predates current main (#267); rebase after the governance/spec repair and rerun required evidence.
I did not find an additional code-level blocker in the runtime namespace/status implementation itself. Re-review after these gates are green.
yohnark
left a comment
yohnark
left a comment
There was a problem hiding this comment.
Follow-up after baseline comparison: the broad built dist e2e/package and Node 24 smoke failures are not unique to #266; merged PR #267 showed the same CI failures on the adjacent baseline. Do not absorb an unrelated repository-wide CI repair into #266. Instead, restore main CI in a separate prerequisite action and keep #266 responsible for targeted evidence that the new runtime ensure/status commands are actually present and callable in the packed artifact.
1 participant
Summary
Separate local Runtime lifecycle from workspace/MCP initialization.
Linked issue
Fixes #258
Scope
Issue #258 only; sibling #232 child Issues remain excluded.
Included
Remove Runtime provisioning from top-level init; add runtime ensure/status commands; add bounded state projection, tests, docs, and package smoke coverage.
Excluded
QEMU artifact production, Nix image validation, real KVM/HVF/WHPX integration, and remote placement (#233).
Implementation
Reuse the PR #257 LocalRuntimeProvisioner for runtime ensure. Add a read-only status projection over the persisted state schema, with no state-directory creation or secret path exposure. Keep init focused on MCP configuration and reject the removed --runtime option.
Behavioral changes
mottainai init never provisions Runtime. mottainai runtime ensure reconciles the local Runtime; runtime status reports absent or persisted lifecycle state and is non-mutating. Both commands preserve JSON and human output paths with deterministic error exits.
Validation
Test layers:
pnpm testpnpm run test:integrationpnpm run test:e2e,pnpm run test:packagepnpm run verify:standardspnpm run test:coveragepnpm run test:effectivenesspnpm run verifyTest contract
change type: CLI authority separation and read-only persisted-state projection; required layers: unit/contract, process/integration, package smoke, lint/architecture; not-applicable: real VM integration is explicitly excluded by Issue #258 and package suite's unrelated Nawabari-managed-task check is environment-blocked.
Regression proof
status: reviewer-attested plus automated; reason: PR #257 coupled Runtime ensure to init; test path: src/cli.test.ts and src/local-runtime/status.test.ts; test identifier: top-level init rejects --runtime, runtime status absent/persisted projections; pre-fix observed failure: init owned --runtime and no runtime namespace; post-fix result: init is MCP-only and runtime commands are callable; command id: pnpm test; base result: not run as a base comparison; head result: 456 tests passed; isolation: dedicated managed worktree and isolated package smoke state root; reviewer attestation: required for base comparison.
Validation evidence
class: unit/contract; status: pass; command: pnpm test; target: src/cli.test.ts, src/init.test.ts, src/local-runtime/status.test.ts; result: 456 tests passed; artifact: TAP exit 0
class: process/integration; status: pass; command: node --import tsx --test src/local-runtime/status.test.ts src/cli.test.ts src/init.test.ts; target: CLI dispatch and state projection; result: 23 tests passed; artifact: TAP exit 0
class: package smoke; status: pass; command: pnpm run smoke-test; target: packed npm launcher; result: runtime ensure --help and isolated runtime status pass; artifact: smoke-test exit 0
class: fault injection; status: not-applicable; command: none; target: Runtime lifecycle fault injection; result: excluded by Issue #258; scenario: no VM lifecycle mutation was claimed; artifact: Issue scope
class: lint/architecture; status: pass; command: pnpm run lint and pnpm exec prettier --check ...; target: changed TypeScript/JS/Markdown; result: pass; artifact: exit 0
class: release; status: not-applicable; command: none; target: release/publish; result: no release metadata changed; artifact: package version unchanged
class: security/negative; status: pass; command: node --import tsx --test src/local-runtime/status.test.ts; target: read-only status projection; result: no state mutation and no hostKey/endpoint exposure; scenario: persisted state status read; artifact: TAP exit 0
Release impact
impact: none; reason: package command surface and dist output are covered by build and package smoke; package version and publish metadata are unchanged.
Risks
Runtime ensure remains host-accelerator/artifact dependent by design; status is intentionally read-only and can report absent without provisioning. The existing package suite has an unrelated Nawabari ownership ambiguity in this shared environment.
Breaking changes
No. The removed init --runtime option now fails with migration guidance; use runtime ensure.
Migration / compatibility
Existing users invoking init --runtime must use mottainai runtime ensure. Existing init/MCP configuration behavior remains unchanged. Persisted Runtime state schema is reused unchanged.
Security impact
Status reads the validated persisted schema without creating or changing state and omits private SSH host keys, private key paths, and QMP endpoints from its public projection.
Review focus
Verify init has no Runtime side effect, runtime ensure calls the PR #257 reconciler, runtime status is non-mutating and bounded, and package smoke covers the shipped CLI routes.