morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[GH-ISSUE #5911] [Bug] Provider-status CLI commands report success after session cleanup fails #3588

New issue

Closed

opened 2026-10-31 00:42:09 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-31 00:42:09 +00:00

Owner

Originally created by @coygeek on GitHub (Oct 21, 2026).
Original GitHub issue: https://github.com/morluto/rea/issues/5911

Before submitting

I searched the open and closed issues for a related report.
I removed credentials, tokens, private paths, and sensitive target data.

Area

CLI

Observed behavior

Both rea capabilities --json and rea providers --json can return success with cleanup_pending: false even though their final session close fails and an owned descriptor remains open.

A focused test executed the registered Incur command parsers and handlers with production createDirectAnalysis composition, a real session and a valid IDA registration file. With persistent injected descriptor-close denial, each command returned JSON without a top-level error and left the parser's exit status at 0. The real BinarySession.close() returned an error with cleanupIncomplete: true; one registration descriptor remained open.

The IDA candidate was correctly marked unavailable, and its nested configuration_details.cleanup contained incomplete. The defect is the successful outer command classification, false outer pending flag, and loss of the final close result. This was fault injection at a real filesystem ownership boundary, not a naturally occurring OS failure or a live IDA run. No separately spawned packaged CLI process was tested.

Reproduction

From this source revision with locked dependencies installed, save the following test as tests/boundary/process/statusCleanup.test.ts. Provide an empty disposable directory through REA_STATUS_REPRO_DIR, then run:

REA_STATUS_REPRO_DIR="$(mktemp -d)" ./node_modules/.bin/vitest run tests/boundary/process/statusCleanup.test.ts --maxWorkers 1

The four cases ran successfully once in the final focused run. The assertions characterize the current defect. The finalizers restore close behavior, retry the exact retained session, verify descriptor release and remove the registration file. Remove the test and its empty disposable directory afterward.

import { mkdir, writeFile, rm } from 'node:fs/promises';
import { join } from 'node:path';
import { expect, it, vi } from 'vitest';
import { Cli } from 'incur';
import { createDirectAnalysis } from '../../../src/composition/directAnalysis.js';
import { registerUtilityCommands } from '../../../src/cli/utilityCommands.js';
import { silentLogger } from '../../../src/logger.js';
import { BinarySession } from '../../../src/application/binary/BinarySession.js';
import { OwnedFileHandle } from '../../../src/filesystem/OwnedFileHandle.js';

it.each([
  ['capabilities', 'none'],
  ['capabilities', 'once'],
  ['capabilities', 'persistent'],
  ['providers', 'persistent'],
])('%s with %s descriptor close denial', async (command, mode) => {
  const root = process.env.REA_STATUS_REPRO_DIR!;
  await mkdir(root, { recursive: true });
  const registration = join(root, 'registration.json');
  await writeFile(registration, JSON.stringify({ command: 'python' }));
  const environment = { REA_IDA_MCP_CONFIG: registration, PATH: '' };
  const originalClose = OwnedFileHandle.prototype.close;
  const originalSessionClose = BinarySession.prototype.close;
  const held = new Set<OwnedFileHandle>();
  let session: BinarySession | undefined;
  let closeResult: Awaited<ReturnType<BinarySession['close']>> | undefined;
  const ownerSpy = vi.spyOn(OwnedFileHandle.prototype, 'close').mockImplementation(function(this: OwnedFileHandle) {
    if (mode === 'none' || (mode === 'once' && held.size > 0)) return originalClose.call(this);
    held.add(this);
    return Promise.reject(new Error('simulated descriptor close denial'));
  });
  const sessionSpy = vi.spyOn(BinarySession.prototype, 'close').mockImplementation(async function(this: BinarySession, options) {
    session = this;
    closeResult = await originalSessionClose.call(this, options);
    return closeResult;
  });
  const savedExitCode = process.exitCode;
  process.exitCode = undefined;
  try {
    const cli = Cli.create('rea', { sync: false });
    registerUtilityCommands(cli, silentLogger, environment, createDirectAnalysis(environment));
    let stdout = '';
    const exits: unknown[] = [];
    await cli.serve([command, '--json'], { env: {}, stdout: text => { stdout += text; }, exit: code => { exits.push(code); } });
    const result = JSON.parse(stdout);
    const descriptorsOpen = [...held].filter(owner => owner.handle.fd >= 0).length;
    const exitCode = process.exitCode ?? 0;
    expect(result).toMatchObject({ cleanup_pending: false });
    expect(result).not.toHaveProperty('error');
    expect(exitCode).toBe(0);
    expect(exits.every(code => code === 0 || code === undefined)).toBe(true);
    if (mode === 'persistent') {
      expect(closeResult).toMatchObject({ ok: false, error: { cleanupIncomplete: true } });
      expect(descriptorsOpen).toBe(1);
      const ida = result.analysis_provider_candidates.find((candidate: any) => candidate.provider.id === 'ida');
      expect(ida.availability.status).toBe('unavailable');
      expect(ida.availability.diagnostics.configuration_details).toMatchObject({ cleanup: 'incomplete' });
    } else {
      expect(closeResult).toEqual({ ok: true, value: null });
      expect(descriptorsOpen).toBe(0);
    }
    console.log(JSON.stringify({ command, mode, exit_code: exitCode, returned_cleanup_pending: result.cleanup_pending, top_level_error: Object.hasOwn(result, 'error'), close_ok: closeResult!.ok, descriptors_open: descriptorsOpen }));
  } finally {
    ownerSpy.mockRestore();
    sessionSpy.mockRestore();
    process.exitCode = savedExitCode;
    if (session) expect((await originalSessionClose.call(session)).ok).toBe(true);
    for (const handle of held) {
      await originalClose.call(handle);
      expect(handle.handle.fd).toBe(-1);
    }
    await rm(registration, { force: true });
  }
});

Expected behavior

Finalization should preserve the completed provider-status observation while surfacing the unresolved cleanup as a typed failure with the remaining resources and recovery instructions. It should not report a successful command and cleanup_pending: false while the session's own close reports incomplete cleanup.

The one-shot lifecycle contract requires preserving observations, remaining resource ownership and cleanup recovery instructions. The IDA discovery contract allows registration inspection without engine startup, so target-free discovery still needs descriptor ownership.

Impact and scope

P2: scripts and agents can receive a false successful-finalization signal from the two documented provider-discovery commands. The observed resource is one real registration descriptor, under persistent controlled close denial. Both commands are affected; ordinary close and a single transient denial complete cleanup in the controls.

No binary was opened, no IDA connection was established, and no data loss or security-boundary crossing was observed. Natural fault frequency, other provider paths and a public recovery workaround were not established. Nested candidate diagnostics still provide evidence of the original registration cleanup problem.

REA version

rea-agents 6.7.0; source revision 52415975942a4b9c53a817915b313ccc404f280e.

Runtime and platform

Node.js v26.12.0; macOS 27.0.1 arm64; locked Vitest 5.0.3. The repository's Node 24.18.0 pin was not exercised.

Target and artifact context

No analysis target. A valid synthetic local IDA registration containing {"command":"python"}; real file descriptors; controlled close denial. No IDA engine or upstream server required.

Interface

CLI

Earliest failure stage

Cleanup, cancellation, or lifecycle

Relevant output or evidence

Final focused result: one test file passed, four tests passed. The following compact observations came from the executed CLI parser/handler tests:

{"command":"capabilities","mode":"none","exit_code":0,"returned_cleanup_pending":false,"top_level_error":false,"close_ok":true,"descriptors_open":0}
{"command":"capabilities","mode":"once","exit_code":0,"returned_cleanup_pending":false,"top_level_error":false,"close_ok":true,"descriptors_open":0}
{"command":"capabilities","mode":"persistent","exit_code":0,"returned_cleanup_pending":false,"top_level_error":false,"close_ok":false,"descriptors_open":1}
{"command":"providers","mode":"persistent","exit_code":0,"returned_cleanup_pending":false,"top_level_error":false,"close_ok":false,"descriptors_open":1}

The final close wrapper only records the real method's result; it does not manufacture an error. In the two failing-cleanup cases, assertions confirm cleanupIncomplete: true and a still-open descriptor. After the denial is removed, retrying the exact session closes successfully and every retained descriptor has fd === -1.

Typed outcome or recovery expected

An outer typed cleanup_incomplete outcome retaining the completed status, exact unresolved resource information and cleanup owner's recovery instructions. Any final cleanup_pending representation should agree with unresolved ownership. Preserve the nested registration diagnostic as useful context.

Analysis or suspected cause

Source-derived: the two CLI registrations both call runSessionStatus. That helper returns the status and ignores the close Result in finally.

IDA registration inspection retains a failed descriptor close, and candidate readiness converts the configuration error to unavailable status, allowing the status operation to complete. The session pending flag is captured before asynchronous candidate discovery, so the initial false value predates that new retained descriptor. Finally, session closure collects registry cleanup failures and returns an error, which the one-shot status helper discards.

Additional context

Fetched upstream main at c324d31b45fcab77e4ed7320ed60f0bf5a4ba188; the cited status, CLI, session and IDA paths were unchanged from the tested revision. No restoration patch was tested.

Originally created by @coygeek on GitHub (Oct 21, 2026). Original GitHub issue: https://github.com/morluto/rea/issues/5911 ### Before submitting - [x] I searched the open and closed issues for a related report. - [x] I removed credentials, tokens, private paths, and sensitive target data. ### Area CLI ### Observed behavior Both `rea capabilities --json` and `rea providers --json` can return success with `cleanup_pending: false` even though their final session close fails and an owned descriptor remains open. A focused test executed the registered Incur command parsers and handlers with production `createDirectAnalysis` composition, a real session and a valid IDA registration file. With persistent injected descriptor-close denial, each command returned JSON without a top-level error and left the parser's exit status at 0. The real `BinarySession.close()` returned an error with `cleanupIncomplete: true`; one registration descriptor remained open. The IDA candidate was correctly marked unavailable, and its nested `configuration_details.cleanup` contained `incomplete`. The defect is the successful outer command classification, false outer pending flag, and loss of the final close result. This was fault injection at a real filesystem ownership boundary, not a naturally occurring OS failure or a live IDA run. No separately spawned packaged CLI process was tested. ### Reproduction From this source revision with locked dependencies installed, save the following test as `tests/boundary/process/statusCleanup.test.ts`. Provide an empty disposable directory through `REA_STATUS_REPRO_DIR`, then run: ```sh REA_STATUS_REPRO_DIR="$(mktemp -d)" ./node_modules/.bin/vitest run tests/boundary/process/statusCleanup.test.ts --maxWorkers 1 ``` The four cases ran successfully once in the final focused run. The assertions characterize the current defect. The finalizers restore close behavior, retry the exact retained session, verify descriptor release and remove the registration file. Remove the test and its empty disposable directory afterward. ```ts import { mkdir, writeFile, rm } from 'node:fs/promises'; import { join } from 'node:path'; import { expect, it, vi } from 'vitest'; import { Cli } from 'incur'; import { createDirectAnalysis } from '../../../src/composition/directAnalysis.js'; import { registerUtilityCommands } from '../../../src/cli/utilityCommands.js'; import { silentLogger } from '../../../src/logger.js'; import { BinarySession } from '../../../src/application/binary/BinarySession.js'; import { OwnedFileHandle } from '../../../src/filesystem/OwnedFileHandle.js'; it.each([ ['capabilities', 'none'], ['capabilities', 'once'], ['capabilities', 'persistent'], ['providers', 'persistent'], ])('%s with %s descriptor close denial', async (command, mode) => { const root = process.env.REA_STATUS_REPRO_DIR!; await mkdir(root, { recursive: true }); const registration = join(root, 'registration.json'); await writeFile(registration, JSON.stringify({ command: 'python' })); const environment = { REA_IDA_MCP_CONFIG: registration, PATH: '' }; const originalClose = OwnedFileHandle.prototype.close; const originalSessionClose = BinarySession.prototype.close; const held = new Set<OwnedFileHandle>(); let session: BinarySession | undefined; let closeResult: Awaited<ReturnType<BinarySession['close']>> | undefined; const ownerSpy = vi.spyOn(OwnedFileHandle.prototype, 'close').mockImplementation(function(this: OwnedFileHandle) { if (mode === 'none' || (mode === 'once' && held.size > 0)) return originalClose.call(this); held.add(this); return Promise.reject(new Error('simulated descriptor close denial')); }); const sessionSpy = vi.spyOn(BinarySession.prototype, 'close').mockImplementation(async function(this: BinarySession, options) { session = this; closeResult = await originalSessionClose.call(this, options); return closeResult; }); const savedExitCode = process.exitCode; process.exitCode = undefined; try { const cli = Cli.create('rea', { sync: false }); registerUtilityCommands(cli, silentLogger, environment, createDirectAnalysis(environment)); let stdout = ''; const exits: unknown[] = []; await cli.serve([command, '--json'], { env: {}, stdout: text => { stdout += text; }, exit: code => { exits.push(code); } }); const result = JSON.parse(stdout); const descriptorsOpen = [...held].filter(owner => owner.handle.fd >= 0).length; const exitCode = process.exitCode ?? 0; expect(result).toMatchObject({ cleanup_pending: false }); expect(result).not.toHaveProperty('error'); expect(exitCode).toBe(0); expect(exits.every(code => code === 0 || code === undefined)).toBe(true); if (mode === 'persistent') { expect(closeResult).toMatchObject({ ok: false, error: { cleanupIncomplete: true } }); expect(descriptorsOpen).toBe(1); const ida = result.analysis_provider_candidates.find((candidate: any) => candidate.provider.id === 'ida'); expect(ida.availability.status).toBe('unavailable'); expect(ida.availability.diagnostics.configuration_details).toMatchObject({ cleanup: 'incomplete' }); } else { expect(closeResult).toEqual({ ok: true, value: null }); expect(descriptorsOpen).toBe(0); } console.log(JSON.stringify({ command, mode, exit_code: exitCode, returned_cleanup_pending: result.cleanup_pending, top_level_error: Object.hasOwn(result, 'error'), close_ok: closeResult!.ok, descriptors_open: descriptorsOpen })); } finally { ownerSpy.mockRestore(); sessionSpy.mockRestore(); process.exitCode = savedExitCode; if (session) expect((await originalSessionClose.call(session)).ok).toBe(true); for (const handle of held) { await originalClose.call(handle); expect(handle.handle.fd).toBe(-1); } await rm(registration, { force: true }); } }); ``` ### Expected behavior Finalization should preserve the completed provider-status observation while surfacing the unresolved cleanup as a typed failure with the remaining resources and recovery instructions. It should not report a successful command and `cleanup_pending: false` while the session's own close reports incomplete cleanup. The [one-shot lifecycle contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/tool-design.md#L119-L136) requires preserving observations, remaining resource ownership and cleanup recovery instructions. The [IDA discovery contract](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/docs/ida-provider.md#L3-L5) allows registration inspection without engine startup, so target-free discovery still needs descriptor ownership. ### Impact and scope P2: scripts and agents can receive a false successful-finalization signal from the two documented provider-discovery commands. The observed resource is one real registration descriptor, under persistent controlled close denial. Both commands are affected; ordinary close and a single transient denial complete cleanup in the controls. No binary was opened, no IDA connection was established, and no data loss or security-boundary crossing was observed. Natural fault frequency, other provider paths and a public recovery workaround were not established. Nested candidate diagnostics still provide evidence of the original registration cleanup problem. ### REA version rea-agents 6.7.0; source revision `52415975942a4b9c53a817915b313ccc404f280e`. ### Runtime and platform Node.js v26.12.0; macOS 27.0.1 arm64; locked Vitest 5.0.3. The repository's Node 24.18.0 pin was not exercised. ### Target and artifact context No analysis target. A valid synthetic local IDA registration containing `{"command":"python"}`; real file descriptors; controlled close denial. No IDA engine or upstream server required. ### Interface CLI ### Earliest failure stage Cleanup, cancellation, or lifecycle ### Relevant output or evidence Final focused result: one test file passed, four tests passed. The following compact observations came from the executed CLI parser/handler tests: ```json {"command":"capabilities","mode":"none","exit_code":0,"returned_cleanup_pending":false,"top_level_error":false,"close_ok":true,"descriptors_open":0} {"command":"capabilities","mode":"once","exit_code":0,"returned_cleanup_pending":false,"top_level_error":false,"close_ok":true,"descriptors_open":0} {"command":"capabilities","mode":"persistent","exit_code":0,"returned_cleanup_pending":false,"top_level_error":false,"close_ok":false,"descriptors_open":1} {"command":"providers","mode":"persistent","exit_code":0,"returned_cleanup_pending":false,"top_level_error":false,"close_ok":false,"descriptors_open":1} ``` The final close wrapper only records the real method's result; it does not manufacture an error. In the two failing-cleanup cases, assertions confirm `cleanupIncomplete: true` and a still-open descriptor. After the denial is removed, retrying the exact session closes successfully and every retained descriptor has `fd === -1`. ### Typed outcome or recovery expected An outer typed `cleanup_incomplete` outcome retaining the completed status, exact unresolved resource information and cleanup owner's recovery instructions. Any final `cleanup_pending` representation should agree with unresolved ownership. Preserve the nested registration diagnostic as useful context. ### Analysis or suspected cause Source-derived: the [two CLI registrations](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/cli/utilityCommands.ts#L155-L170) both call `runSessionStatus`. That helper [returns the status and ignores the close Result in finally](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/application/DirectAnalysisStatus.ts#L22-L40). [IDA registration inspection retains a failed descriptor close](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/ida/IdaConfiguration.ts#L78-L93), and [candidate readiness converts the configuration error to unavailable status](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/ida/IdaProvider.ts#L180-L197), allowing the status operation to complete. The [session pending flag](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/application/binary/BinarySession.ts#L959-L988) is captured before asynchronous candidate discovery, so the initial false value predates that new retained descriptor. Finally, [session closure collects registry cleanup failures and returns an error](https://github.com/morluto/rea/blob/52415975942a4b9c53a817915b313ccc404f280e/src/application/binary/BinarySession.ts#L819-L851), which the one-shot status helper discards. ### Additional context Fetched upstream main at `c324d31b45fcab77e4ed7320ed60f0bf5a4ba188`; the cited status, CLI, session and IDA paths were unchanged from the tested revision. No restoration patch was tested.

bsakaguchi closed this issue

2026-10-31 00:42:09 +00:00

bsakaguchi commented

2026-10-31 00:42:09 +00:00

Author

Owner

@coygeek commented on GitHub (Oct 24, 2026):

Checked the reported fix on main at d87e73f2db.

runSessionStatus now uses withSessionCleanup and returns directAnalysisCleanupFailure on close failure. That projection emits cleanup_incomplete, non-retryability, the cleanup owner's remediation/resources and the completed status under partial_observation, instead of returning a successful outer status with cleanup_pending:false. Both CLI registrations pass through logCliCommand, which recognizes the typed projection and sets process.exitCode=1.

Implementation: DirectAnalysisStatus.ts, DirectAnalysisCleanup.ts.

The relevant regression checks in directAnalysisStatus.test.ts passed in the focused test run on this revision. This closure is based on the current source and those checks; provider fixtures do not establish a real-engine run. Closing as completed on main.

@coygeek commented on GitHub (Oct 24, 2026): Checked the reported fix on `main` at d87e73f2db50be72d33a4b3c7199af35518b441c. runSessionStatus now uses withSessionCleanup and returns directAnalysisCleanupFailure on close failure. That projection emits cleanup_incomplete, non-retryability, the cleanup owner's remediation/resources and the completed status under partial_observation, instead of returning a successful outer status with cleanup_pending:false. Both CLI registrations pass through logCliCommand, which recognizes the typed projection and sets process.exitCode=1. Implementation: [DirectAnalysisStatus.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/application/DirectAnalysisStatus.ts#L30), [DirectAnalysisCleanup.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/src/application/DirectAnalysisCleanup.ts#L29). The relevant regression checks in [directAnalysisStatus.test.ts](https://github.com/morluto/rea/blob/d87e73f2db50be72d33a4b3c7199af35518b441c/tests/boundary/cli/directAnalysisStatus.test.ts) passed in the focused test run on this revision. This closure is based on the current source and those checks; provider fixtures do not establish a real-engine run. Closing as completed on main.

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#3588

No description provided.

Rows
Columns