Skip to content

test(cli): restore the run-command process-boundary contracts - #3710

Merged
Astro-Han merged 1 commit into
apache:mainfrom
UncertaintyDeterminesYou4ndMe:test/2476-process-boundary-contracts
Aug 27, 2026
Merged

test(cli): restore the run-command process-boundary contracts#3710
Astro-Han merged 1 commit into
apache:mainfrom
UncertaintyDeterminesYou4ndMe:test/2476-process-boundary-contracts

Conversation

@UncertaintyDeterminesYou4ndMe

Copy link
Copy Markdown
Contributor

Summary

Restores the six process-boundary contracts that were retained on a real subprocess in #2476 and lost when 802855c removed the process fixture — as invited in the closing review of #2476 (Refs #2476, refs #2387).

The injected MakaRunDeps seam covers the semantics of stdinIsTTY/readStdin, but not the boundary itself: a real pipe feeding the prompt, a SIGINT delivered by the operating system observed as exit 130 with nothing on stdout, and the fail-closed sandbox path traversing a real child process. Current main asserts none of these (stdinIsTTY/readStdin are exercised in process only; no test anywhere asserts SIGINT → 130; the remaining spawn in cli.test.ts covers profile/identity files, not the run path).

  • run-command-fixture.ts (new) — a subprocess entry over today's runMakaTextCliCore with default environment deps (real stdin, real SIGINT handler, real exit code) and a minimal scripted adapter: four scenarios (echo, sandbox-boundary, slow, graph-wait), no env-var assertion matrix, none of the in-process duplication that 802855c removed. fixture-ready is written only from inside the run (after the core has installed its SIGINT handler), so the test's signal cannot race the default disposition.

  • run-command.test.ts — one describe('maka run process contract') block with the six tests, assertions unchanged from the pre-deletion suite:

    Contract Tests
    Piped non-TTY stdin (run -, implicit stdin prompt, positional + stdin context) 3
    SIGINT → exit 130, empty stdout through a real pipe (plain run; during Graph completion wait) 2
    Fail-closed sandbox boundary reaching a non-interactive run 1

    Both signal tests carry bounded SIGKILL guards (never-ready, never-exiting) so a regression fails the suite instead of hanging it.

No production changes.

Verification

  • packages/cli: node --test dist/__tests__/run-command.test.js — 11/11, three consecutive rounds (the six subprocess tests add ~0.8s total); full CLI suite 442/442.
  • Fault injection (in dist/, reverted): disabling if (interrupted) return 130 fails exactly the two SIGINT tests; disabling the implicit stdin read fails exactly the two non-run - stdin tests.
  • Biome clean on both files; ASF header audit clean; tsc clean.
  • Not run: workspaces this change does not touch.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code authored the fixture and tests (ported from the pre-deletion suite) and ran the verification; commits carry Generated-by: Claude Code trailers. Reviewed and submitted by a human.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@UncertaintyDeterminesYou4ndMe

Copy link
Copy Markdown
Contributor Author

The failing check is Check ASF source headers, and the five files it names are all from d807c0078 (#3250) on current main — none are touched by this PR (both files here carry the header; the audit passes locally against this branch's own changes). The in-flight fix is #3708; once it lands I will rebase to re-run CI on a base where the audit is green.

@UncertaintyDeterminesYou4ndMe
UncertaintyDeterminesYou4ndMe force-pushed the test/2476-process-boundary-contracts branch from a7aab6a to f5fff10 Compare August 24, 2026 11:55
@M4n5ter
M4n5ter force-pushed the test/2476-process-boundary-contracts branch from f5fff10 to b8bb8cb Compare August 26, 2026 09:01
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
@M4n5ter
M4n5ter force-pushed the test/2476-process-boundary-contracts branch from b8bb8cb to 5be8d6f Compare August 26, 2026 10:02
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 27, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds a 157-line subprocess entry and four scripted scenarios covering three maka run contracts that only hold across a real process boundary. SIGINT currently appears nowhere in the repo's tests.

No P-graded issues. I checked why these were removed originally: duplication and fake-only coverage, not flakiness. Fault injection failed in exactly the right cases both times.

Two ungraded notes. sandbox-boundary is entirely within the seam and doesn't need a subprocess, and the three stdin scenarios compress to one — the two SIGINT cases carry nearly all the value, and ~160 lines would do. More substantively, this exercises a synthetic process boundary: it bypasses production's resolveMakaCliExitCode and the exit grace timer, so it can't catch a regression in either.


AI-assisted review: a Claude Code subagent analysed the change; I verified the deletion rationale from history and the fault-injection results myself. No tests run. AI review is not independent human review.

@Astro-Han
Astro-Han merged commit 1bd116b into apache:main Aug 27, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants