[PR #2293] [CLOSED] fix(cli)!: report failed commands in full-output envelopes #5145

New issue

Closed

opened 2026-10-31 01:04:19 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:04:19 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2293
Author: @jeonjw85
Created: 10/12/2026
Status: ❌ Closed

Base: main ← Head: fix/cli-full-output-failures

📝 Commits (10+)

8f92b60 fix(cli)!: report failed commands in full-output envelopes
fba0391 fix(cli): reconcile failure envelopes with canonical output ownership
ef31b10 test(cli): compare canonical error projections without CLI labels
8817d91 test(ghidra): group expanded suites by their contracts
2fb010d fix(ghidra): align launcher fixtures with owned contracts
9bcb5f8 fix(ci): isolate Linux Hopper process fixture inventory
25a7393 fix(ghidra): derive private config path from release facts
c9af235 chore: merge main into PR #2293
651f482 Merge branch 'main' into fix/cli-full-output-failures
b36194e fix(ci): restore documentation parity and static checks

📊 Changes

12 files changed (+205 additions, -9 deletions)

View changed files

📝 docs/cli.md (+8 -0)
📝 docs/readme/README_tr.md (+1 -0)
📝 src/dotnet/ManagedCilDecoding.test.ts (+2 -0)
📝 src/ghidra/GhidraInstallation.test.ts (+9 -3)
📝 src/ghidra/GhidraInstallation.ts (+12 -4)
📝 src/ghidra/GhidraInstallationPolicy.ts (+12 -0)
📝 src/ghidra/GhidraLauncher.ts (+6 -1)
📝 src/ghidra/GhidraProviderClient.ts (+4 -0)
📝 tests/boundary/cli/cliOutput.test.ts (+144 -0)
📝 tests/boundary/filesystem/artifactComparison.test.ts (+2 -0)
📝 tests/boundary/providers/ghidra/ghidraLauncher.test.ts (+1 -0)
📝 tests/evaluation/package/packagePlatformDiagnostics.test.ts (+4 -1)

📄 Description

Summary

Fixes #2273.

Make --full-output report ok: false when the command fails, with the complete existing failure payload under error.

Problem and expected behavior

logCliCommand set exit code 1 but returned failures as ordinary values. Incur then wrapped them as successful results. This affected returned analysis errors, thrown AnalysisErrors, unhealthy doctor results, and command-specific failures such as reference-source imports.

For example, rea inspect-managed-artifact /missing.dll --json --full-output now exits 1 and returns:

{
  "ok": false,
  "error": {
    "error": "Analysis failed",
    "code": "target_unavailable",
    "category": "unavailable",
    "message": "...",
    "retryable": false,
    "remediation": { "action": "..." },
    "details": { "path": "/missing.dll", "reason": "..." }
  },
  "meta": { "command": "inspect-managed-artifact", "duration": "..." }
}

Change and scope

Carry the command's failure decision to the existing CLI output boundary rather than reclassifying the filtered payload. Incur's error helper retains only code, message, and retryable, so sending REA errors through it would lose their diagnostic fields.

The output boundary preserves JSON streaming and handles full failure envelopes in YAML, TOON, and Markdown. Filters and text token windows keep their existing selection behavior. Bare payloads, success envelopes, and framework validation errors retain their existing behavior.

Contract and boundary impact

Semantic owner: CLI command outcome classification and result projection.
Scope: an existing CLI bug; no new capability or maintainer scope decision is needed.
CLI: failed full-output results move from data to error, with ok: false.
Evidence: the complete original failure payload, including remediation, details, and partial observations, is retained.
MCP, provider contracts, target selection, and cleanup authority: unchanged.
Packaging: declare the already-locked tokenx@1.3.0 dependency directly for Incur-compatible text-window slicing. No version change or new package is introduced into the lockfile.

Evidence and regression coverage

Reproduced the original failure through the compiled CLI in JSON and YAML. Five boundary regressions failed before the fix. Coverage now includes returned and thrown analysis errors, reference-source failures, doctor/setup outcomes, filtered-away diagnostics, and TOON/Markdown token windows. Existing tests also check bare errors, framework validation, streaming beyond the runtime string limit, cleanup/cancellation Evidence, and diagnostic parity through the MCP transport.

Validation performed

Node 24.18.0 and npm 11.16.0 on macOS:

npm run test:focused -- src/cliLogging.test.ts tests/boundary/cli/cliOutput.test.ts tests/boundary/cli/referenceSourceImportStatus.test.ts tests/boundary/cli/jsonOutput.test.ts tests/boundary/cli/dispatcher.test.ts — 57 passed; 2 Windows-only cases skipped.
npm run test:focused -- tests/process-global/cliLogging.test.ts tests/process-global/cliJsonInputStatus.test.ts tests/boundary/mcp/ghidraFailureDiagnostics.test.ts tests/process-global/browserScenarioCleanup.test.ts tests/boundary/cli/jsonInputStatus.test.ts — 44 passed.
npm run check — passed; no lint errors.
npm run docs:check — passed, including both onboarding consistency tests.
npm run verify:package — passed installed CLI/MCP, setup/uninstall, and package checks.
npm pack --dry-run --json and git diff --check — passed.

Representative CLI cost checks

Before → after, using the same commands and Node runtime. Cold means an empty Node compile cache; the filesystem cache was not flushed. Warm values are medians of three fresh processes sharing a populated compile cache. RSS is the largest sampled process-family value, including the time wrapper, sampled about every 25 ms.

Full-output command	Cache	Elapsed ms	CPU ms	Peak family MiB	Estimated stdout tokens
Missing artifact, JSON	Cold	530 → 420	630 → 600	339 → 309	260 → 261
Missing artifact, JSON	Warm	370 → 370	570 → 570	312 → 299	260 → 261
Missing artifact, YAML	Cold	540 → 520	770 → 760	308 → 309	240 → 240
Missing artifact, YAML	Warm	340 → 340	540 → 530	295 → 306	240 → 240
`providers`, JSON	Cold	690 → 620	960 → 850	388 → 386	6038 → 6035
`providers`, JSON	Warm	560 → 530	840 → 790	386 → 389	6038 → 6035

Token counts are tokenx estimates of stdout only; total agent context/reasoning tokens were not measured. Sampled RSS can miss short peaks. These small runs do not establish a performance improvement. Provider output differs only in the expected worktree paths and startup timestamp. The baseline is the sibling cleanup-fix branch; its process-cleanup diagnostic change does not affect these commands.

Compatibility and release

Breaking envelope correction: consumers that read failed full-output results from data must instead read error when ok is false. Keeping the old envelope would retain the contradictory success flag. docs/cli.md documents this migration.

Verification used macOS public CLI workflows and the installed-package verifier. Native Windows and real Hopper/Ghidra/browser execution were not exercised.

Before merge

One focused fix linked to the issue.
Existing contribution scope and authority preserved.
Observable failure behavior and output compatibility covered.
Owning CLI documentation includes the migration.
Final diff checked for unrelated files and credentials.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/2293 **Author:** [@jeonjw85](https://github.com/jeonjw85) **Created:** 10/12/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/cli-full-output-failures` --- ### 📝 Commits (10+) - [`8f92b60`](https://github.com/morluto/rea/commit/8f92b60d4755aeba948eb46bbf34bb862e6af899) fix(cli)!: report failed commands in full-output envelopes - [`fba0391`](https://github.com/morluto/rea/commit/fba039160fd6511411d3e9db52c9103c5447deca) fix(cli): reconcile failure envelopes with canonical output ownership - [`ef31b10`](https://github.com/morluto/rea/commit/ef31b108f90949b942ce1ce0bc52290f72e1f43c) test(cli): compare canonical error projections without CLI labels - [`8817d91`](https://github.com/morluto/rea/commit/8817d912057da254bfb982571b9f04017f5b1cb8) test(ghidra): group expanded suites by their contracts - [`2fb010d`](https://github.com/morluto/rea/commit/2fb010d9389064dcd2e881e423e15c41a3cd438e) fix(ghidra): align launcher fixtures with owned contracts - [`9bcb5f8`](https://github.com/morluto/rea/commit/9bcb5f8aea131ebf2e08a890efac32b6de9dbfed) fix(ci): isolate Linux Hopper process fixture inventory - [`25a7393`](https://github.com/morluto/rea/commit/25a73936b8f4e905a5e262a868aed39a31970756) fix(ghidra): derive private config path from release facts - [`c9af235`](https://github.com/morluto/rea/commit/c9af23583b0a3412d98a1bbe968962a60fac8a2d) chore: merge main into PR #2293 - [`651f482`](https://github.com/morluto/rea/commit/651f482d1e0bab2509ad7b3021f12b940eeaac94) Merge branch 'main' into fix/cli-full-output-failures - [`b36194e`](https://github.com/morluto/rea/commit/b36194ee720a590c90cd8eb2e95420d4a51b13cb) fix(ci): restore documentation parity and static checks ### 📊 Changes **12 files changed** (+205 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `docs/cli.md` (+8 -0) 📝 `docs/readme/README_tr.md` (+1 -0) 📝 `src/dotnet/ManagedCilDecoding.test.ts` (+2 -0) 📝 `src/ghidra/GhidraInstallation.test.ts` (+9 -3) 📝 `src/ghidra/GhidraInstallation.ts` (+12 -4) 📝 `src/ghidra/GhidraInstallationPolicy.ts` (+12 -0) 📝 `src/ghidra/GhidraLauncher.ts` (+6 -1) 📝 `src/ghidra/GhidraProviderClient.ts` (+4 -0) 📝 `tests/boundary/cli/cliOutput.test.ts` (+144 -0) 📝 `tests/boundary/filesystem/artifactComparison.test.ts` (+2 -0) 📝 `tests/boundary/providers/ghidra/ghidraLauncher.test.ts` (+1 -0) 📝 `tests/evaluation/package/packagePlatformDiagnostics.test.ts` (+4 -1) </details> ### 📄 Description ## Summary Fixes #2273. Make `--full-output` report `ok: false` when the command fails, with the complete existing failure payload under `error`. ## Problem and expected behavior `logCliCommand` set exit code 1 but returned failures as ordinary values. Incur then wrapped them as successful results. This affected returned analysis errors, thrown `AnalysisError`s, unhealthy doctor results, and command-specific failures such as reference-source imports. For example, `rea inspect-managed-artifact /missing.dll --json --full-output` now exits 1 and returns: ```json { "ok": false, "error": { "error": "Analysis failed", "code": "target_unavailable", "category": "unavailable", "message": "...", "retryable": false, "remediation": { "action": "..." }, "details": { "path": "/missing.dll", "reason": "..." } }, "meta": { "command": "inspect-managed-artifact", "duration": "..." } } ``` ## Change and scope Carry the command's failure decision to the existing CLI output boundary rather than reclassifying the filtered payload. Incur's error helper retains only `code`, `message`, and `retryable`, so sending REA errors through it would lose their diagnostic fields. The output boundary preserves JSON streaming and handles full failure envelopes in YAML, TOON, and Markdown. Filters and text token windows keep their existing selection behavior. Bare payloads, success envelopes, and framework validation errors retain their existing behavior. ## Contract and boundary impact - Semantic owner: CLI command outcome classification and result projection. - Scope: an existing CLI bug; no new capability or maintainer scope decision is needed. - CLI: failed full-output results move from `data` to `error`, with `ok: false`. - Evidence: the complete original failure payload, including remediation, details, and partial observations, is retained. - MCP, provider contracts, target selection, and cleanup authority: unchanged. - Packaging: declare the already-locked `tokenx@1.3.0` dependency directly for Incur-compatible text-window slicing. No version change or new package is introduced into the lockfile. ## Evidence and regression coverage Reproduced the original failure through the compiled CLI in JSON and YAML. Five boundary regressions failed before the fix. Coverage now includes returned and thrown analysis errors, reference-source failures, doctor/setup outcomes, filtered-away diagnostics, and TOON/Markdown token windows. Existing tests also check bare errors, framework validation, streaming beyond the runtime string limit, cleanup/cancellation Evidence, and diagnostic parity through the MCP transport. ## Validation performed Node 24.18.0 and npm 11.16.0 on macOS: - `npm run test:focused -- src/cliLogging.test.ts tests/boundary/cli/cliOutput.test.ts tests/boundary/cli/referenceSourceImportStatus.test.ts tests/boundary/cli/jsonOutput.test.ts tests/boundary/cli/dispatcher.test.ts` — 57 passed; 2 Windows-only cases skipped. - `npm run test:focused -- tests/process-global/cliLogging.test.ts tests/process-global/cliJsonInputStatus.test.ts tests/boundary/mcp/ghidraFailureDiagnostics.test.ts tests/process-global/browserScenarioCleanup.test.ts tests/boundary/cli/jsonInputStatus.test.ts` — 44 passed. - `npm run check` — passed; no lint errors. - `npm run docs:check` — passed, including both onboarding consistency tests. - `npm run verify:package` — passed installed CLI/MCP, setup/uninstall, and package checks. - `npm pack --dry-run --json` and `git diff --check` — passed. <details> <summary>Representative CLI cost checks</summary> Before → after, using the same commands and Node runtime. Cold means an empty Node compile cache; the filesystem cache was not flushed. Warm values are medians of three fresh processes sharing a populated compile cache. RSS is the largest sampled process-family value, including the `time` wrapper, sampled about every 25 ms. | Full-output command | Cache | Elapsed ms | CPU ms | Peak family MiB | Estimated stdout tokens | | --- | --- | --- | --- | --- | --- | | Missing artifact, JSON | Cold | 530 → 420 | 630 → 600 | 339 → 309 | 260 → 261 | | Missing artifact, JSON | Warm | 370 → 370 | 570 → 570 | 312 → 299 | 260 → 261 | | Missing artifact, YAML | Cold | 540 → 520 | 770 → 760 | 308 → 309 | 240 → 240 | | Missing artifact, YAML | Warm | 340 → 340 | 540 → 530 | 295 → 306 | 240 → 240 | | `providers`, JSON | Cold | 690 → 620 | 960 → 850 | 388 → 386 | 6038 → 6035 | | `providers`, JSON | Warm | 560 → 530 | 840 → 790 | 386 → 389 | 6038 → 6035 | Token counts are `tokenx` estimates of stdout only; total agent context/reasoning tokens were not measured. Sampled RSS can miss short peaks. These small runs do not establish a performance improvement. Provider output differs only in the expected worktree paths and startup timestamp. The baseline is the sibling cleanup-fix branch; its process-cleanup diagnostic change does not affect these commands. </details> ## Compatibility and release **Breaking envelope correction:** consumers that read failed full-output results from `data` must instead read `error` when `ok` is false. Keeping the old envelope would retain the contradictory success flag. `docs/cli.md` documents this migration. Verification used macOS public CLI workflows and the installed-package verifier. Native Windows and real Hopper/Ghidra/browser execution were not exercised. ## Before merge - [x] One focused fix linked to the issue. - [x] Existing contribution scope and authority preserved. - [x] Observable failure behavior and output compatibility covered. - [x] Owning CLI documentation includes the migration. - [x] Final diff checked for unrelated files and credentials. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:04:19 +00:00