test(cli): run ordinary run-command semantics in process - #2476
test(cli): run ordinary run-command semantics in process#2476UncertaintyDeterminesYou4ndMe wants to merge 1 commit into
Conversation
Part of apache#2387. The maka run test suite paid a Node subprocess startup per assertion, including for scenarios that only exercise argument handling, session selection, and outcome-to-exit-code mapping through the injectable MakaRunDeps seam. - Extract the scenario fake from run-command-fixture.ts into run-command-fake.ts, parameterized by an options object instead of environment variables; the fixture becomes a thin subprocess wrapper over the same fake, so the two routes cannot drift apart. - Move 21 ordinary-semantics tests in process through runMakaTextCli with injected stdin/stdout/stderr; assertions are unchanged. - Retain real-subprocess coverage for every distinct process contract: piped non-TTY stdin (3), SIGINT delivery with exit 130 (2), and the fail-closed sandbox boundary path plus exit codes and stdout observed through a real process boundary (1). - The graph-runtime-error scenario's "graph wait must not run" marker now travels through the thrown error message so the negative assertion stays observable on the captured stderr channel. Timing (node --test dist/__tests__/run-command.test.js, local): before 15.06s, after 3.60s. 34/34 tests pass, 5 consecutive rounds.
f69d390 to
e335ba3
Compare
|
Rebased onto main after #2462. One deliberate resolution to note: #2462 added a |
|
Thanks — the split here looks right. Ordinary command semantics now use the existing in-process dependency seam, while piped stdin, SIGINT, and the sandbox fail-closed boundary remain real subprocess tests. Sharing one scenario fake across both routes also avoids creating parallel behavior. I did not find a code or test-design issue that requires changing the approach. The only remaining step is resolving the current conflict with When rebasing, please keep the deletion from #2478 of the low-value “names the mode the same way...” help-copy test. This branch migrated that old test in process, but it should not be reintroduced during conflict resolution. After the rebase, please rerun CI on the new merge head. Assuming it stays green, this should be ready to merge. Thanks! |
There was a problem hiding this comment.
Thanks — this is a thoughtful test refactor. Moving ordinary command semantics onto the existing in-process seam while retaining real subprocess coverage for stdin, SIGINT, and fail-closed behavior is the right split. Sharing one fake between both routes also avoids parallel test implementations.
One small non-blocking suggestion: please keep an assertion that a successful real subprocess produces no unexpected stderr output. The injected in-process stderr cannot observe code that writes directly to process.stderr, but this can be covered by adding the existing filtered-empty assertion to one retained successful subprocess test without adding another spawn.
The current head conflicts with main, so it still needs a rebase. During resolution, please preserve the current run-command-core / MakaRunOutcome architecture, the newer Session-name normalization test, and the deletion of the old help-copy test from #2478. After that, a green CI run should be sufficient.
Approved — thanks for the clear process-boundary reasoning and timing evidence!
|
/agentic_review |
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more' |
|
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 |
|
This branch is 739 commits behind main, and both files it restructures — Could you rebase onto current main, paying particular attention to keeping the cases main added to 简体中文这条落后 main 739 个提交，而且它重构的两个文件（ 麻烦在当前 main 上重新 rebase 一次，重点确认 |
|
Closing this as superseded — the goal was reached on
Thank you for the work, and sorry it did not land first. The measurement in your description was the useful part of it, and the outcome matches what you set out to get. One thing from this PR did not survive the alternative, and may be worth a follow-up. You deliberately kept six tests on a real subprocess for contracts the seam cannot express:
I looked for equivalent coverage on current So the fast in-process route landed and the process-boundary guard did not. If you would like to open a small PR restoring just those contracts on top of today's 简体中文这个 PR 已被取代，我把它关掉。目标在
感谢你的工作，很抱歉它没能先落地。你描述里的那份测量是很有价值的部分，而最终结果和你想要的一致。 这个 PR 里有一样东西没有在替代方案中保留下来，可能值得做个后续。 你当时特意把六个测试留在真实子进程上，因为那几条契约是注入缝表达不了的：
我在当前 也就是说，进程内的提速落地了，进程边界的守卫没有。如果你愿意基于今天的 |
|
Opened #3710 restoring exactly the six process-boundary contracts enumerated above, on top of today's |
Restores the six contracts apache#2476 kept on a real subprocess and 802855c dropped with the process fixture: piped non-TTY stdin (run -, implicit stdin prompt, positional plus stdin context), SIGINT delivery observed as exit 130 with empty stdout, and the fail-closed sandbox boundary reaching a non-interactive run. The injected MakaRunDeps seam covers these semantics in process but not the boundary itself. The new fixture is a minimal subprocess entry over runMakaTextCliCore with default environment deps and four scripted scenarios — none of the in-process duplication the removal targeted. Generated-by: Claude Code
3 participants
Part of #2387 (CLI workspace; the Headless workspace follows in a separate PR per the issue).
Problem
run-command.test.tspaid one Node subprocess startup per assertion — 29 spawns across 27 tests — including for scenarios that only exercise argument handling, session selection, and outcome-to-exit-code mapping. Those semantics flow entirely through the injectableMakaRunDepsseam (createContext,listSessions,stdinIsTTY,readStdin,writeStdout,writeStderr), so the subprocess added cost without adding a distinct failure signal.Change
run-command-fixture.tsintorun-command-fake.ts, parameterized by an options object instead ofMAKA_RUN_*environment variables. The fixture becomes a thin subprocess wrapper over the same fake, so the in-process and subprocess routes cannot drift apart.runMakaTextCliwith injected stdin/stdout/stderr. Assertions are unchanged.Retained process contracts (real subprocess, unchanged assertions)
run -, implicit stdin prompt, positional + stdin context)Exit codes 0/1/2 and stdout content through a real pipe remain asserted by these six;
runtime-bootstrap.test.ts(real background ShellRun lifecycle) and thepi-tui-runnersignal/fatal probes are intentionally untouched — their subject is the real process boundary.One observable-signal fix
The
graph-runtime-errorscenario asserted "graph wait was never called" via a marker the fixture wrote to the realprocess.stderr. In process, that channel is not captured, which would have made the negative assertion vacuous. The marker now travels through the thrown error message (graph-wait-called: unexpected graph wait after failed invocation), which the CLI's error path surfaces on the injected stderr — the assertion keeps a live failure signal on both routes.Timing
node --test dist/__tests__/run-command.test.js, same machine, warm build:34/34 pass across 5 consecutive rounds; full
packages/clisuite 516/516. Typecheck, biome, and knip are clean (knip delta verified against a stashed baseline: the only change is the new fake counting as used).cc @Astro-Han