morluto/rea

Fork 0

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

[PR #5613] [MERGED] fix(evaluation): join owned command cleanup before removing files #6997

New issue

Closed

opened 2026-11-01 21:44:35 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:44:35 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5613
Author: @gensokyo-dev
Created: 10/20/2026
Status: ✅ Merged
Merged: 10/20/2026
Merged by: @gensokyo-dev

Base: main ← Head: fix/5094-evaluation-process-cleanup

📝 Commits (2)

988354f fix(evaluation): join owned command cleanup before removing files
b055ab2 Merge remote-tracking branch 'origin/main' into fix/5094-evaluation-process-cleanup

📊 Changes

4 files changed (+414 additions, -142 deletions)

View changed files

📝 docs/client-verification.md (+10 -2)
➕ scripts/lib/agent-evaluation-commands.mjs (+193 -0)
📝 scripts/verify-agent-experience.mjs (+21 -140)
➕ tests/boundary/process/agentEvaluationCommands.test.ts (+190 -0)

📄 Description

Summary

Fixes #5094. The manual agent-evaluation runner had three copies of “send SIGTERM, immediately reject.” All three reproduce rejection while their exact child remains active. The Codex path also returns before stdout/readline closure; the outer failure path can delete disposable files while the process still uses them.

Move the three command adapters into one run-scoped helper using existing runOwnedCommand supervision, identity checks, process-tree cleanup and output joining. Shebang CLI shims use the existing interpreter-compatible ownership option; launch generation and token still establish ownership. Streaming JSON-line consumption is closed/joined on every outcome. Incomplete cleanup retains the exact supervisor, preserves the primary timeout through cause, retries that owner in finalization and gates file removal on confirmed cleanup.

This changes only the manual evaluation lane, with no new provider, target execution authority, CLI/MCP schema, Evidence semantics, package dependency or cache. A maintainer scope decision for new capabilities is not applicable. Existing target/configuration selections and Codex arguments are preserved. Diagnostic budgets now fail explicitly above 32 KiB stderr / 8 KiB combined version output; setup stdout is drained without retaining it. Complete parsed scenario events retain their existing evaluator semantics.

Executed validation

Pinned main c3d7d0aaf, Linux x64, Node 24.18.0, npm 11.16.0:

13 focused tests pass: real child/output closure for all three timeouts; seven success, nonzero-exit, Unicode JSON and diagnostic-limit controls; exact retained-owner retry/failure; existing independent permission/fixture cleanup and evaluation-report tests. Authored children include delayed SIGTERM exit, ignored SIGTERM and descendants inheriting output pipes. The shared owner may terminate the tree directly; no cooperative-exit assumption is required.
Full npm run check passes (typecheck, lint/module boundaries, formatting, unused-code/metadata and architecture checks); docs:check passes with 5 consistency tests.
Newer-main integration 57d64b6ba, final head b055ab20e: full checks and the same 13 focused tests pass again; the actual entrypoint timeout control again confirms child/output closure before file removal.
Base reproduction evaluates the unchanged original declarations using real spawn/readline and retained exact handles. All three reject before child exit; Codex stdout/readline remain open at rejection. Probe cleanup joins its owned fixtures.
The actual verify-agent-experience.mjs entrypoint runs on both revisions with a disposable authored version executable. Observation hooks record state without changing spawn/removal behavior. Main removes auth/run files with childActive:true and both output streams open. Candidate removes them only with childActive:false and both streams destroyed. Both preserve --version timed out and eventually remove their disposable run.
Injected cleanup failures exercise the actual entrypoint: retry uses the same retained owner; a successful retry permits removal, while a still-incomplete retry preserves inert auth/run files and reports both the original timeout and unresolved cleanup. These injections establish owner/error/file ordering, not real OS cleanup denial.

No real Codex model request, authentication copy, paid API call or reverse-engineering provider runs. Actual vendor signal behavior, complete model evaluation, original macOS/Node 26 and Windows native ownership remain unverified. Existing event history grows with transcript size; a whole-model-run memory bound is not claimed. The manual lane still requires its documented external process-family resource policy.

Local raw receipts/probes remain under /tmp/rea-5094-proof and are not committed.

Performance and context

One CPU, 8-GiB hard / 7-GiB early family memory limits, no swap. Equivalent completed authored command workflows each perform version discovery, a command draining 2 MiB stdout, and a 3,000-event JSON scenario, twice:

Revision	Whole task elapsed	Family CPU	Peak family memory	Cold / warm helper workflow
Main	0.939 s	0.672 s	46.0 MiB	0.285 / 0.278 s
Candidate	5.389 s	4.325 s	105.9 MiB	2.689 / 2.219 s

Ownership/descendant verification adds measurable cost for very short commands; this is a cleanup-correctness tradeoff, not a speedup. Each row includes Node startup and two three-command tasks. “Warm” only reuses imports/OS state, not command results; OS caches/shared-host load are uncontrolled. Actual evaluation/model cost is unmeasured.

Actual entrypoint timeout controls cost 13.160 / 14.437 s, CPU 2.220 / 3.823 s, peak 271.0 / 316.0 MiB (main/candidate); they intentionally differ in cleanup ordering. All completed controls verify cleanup and no OOM using payload-exit counters while the observer remains alive, not final whole-scope counters.

No agent interface or model prompt grows. Each deterministic fixture event list remains 910,891 bytes, estimated 143,003 cl100k_base tokens, on both revisions; it is local evaluator input, not a claim about model context/usage. Only compact verification receipts reach the reviewing agent. Full evaluation input/output token usage and downstream prompts were not measured. No raw stdout duplicate or cache is added; bounded non-JSON diagnostics avoid repeated full-prefix concatenation.

CI/review assessment and authorized bypass

Latest head b055ab20e: Greptile completes with 5/5 and no unresolved threads. Codex delivered two actionable Darwin findings during the final query/merge command. They were not assessed before that command merged; this is a merge-process error. Issue #5094 has been reopened and a focused follow-up is in progress for independently bounded ownership preparation and direct/nested preparation cleanup transfer. No Darwin completion claim is made. Classification/readiness/title jobs remain queued without a runner; neither queued CI nor pending Codex is claimed as passing.

The task owner explicitly authorized bypass once independently qualified. Merge uses completed full checks, base reproductions, real child/descendant/output tests, actual entrypoint ordering controls, measured performance and final-tree newer-main integration. Pending results remain subject to assessment; unrelated main failures will not be mixed into this fix.

Author readiness

Human owner: @gensokyo-dev

The task owner explicitly delegated validation, readiness declarations and merge to this agent. These declarations are supplied under that authorization; no separate new manual human review by the original author is asserted.

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

Focused existing-lifecycle repair; original behavior, producing boundary, affected consumers and related cleanup reviewed.
Real native processes and public entrypoint tested; no credentials, targets, generated outputs or local proofs in the diff.
Available latest-head CI/review state assessed; queued work and bypass authorization recorded above.

Follow-up complete: #5615 merged as 8716b5241125d81f1a16808eb5178fdfdea1ecdb. Both accepted Codex findings are addressed and their threads resolved after verified integration; final-head 18 tests/full checks/docs pass, Codex no findings, Greptile5. Native macOS/Windows proof gaps remain explicit.

_{🔄 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/5613 **Author:** [@gensokyo-dev](https://github.com/gensokyo-dev) **Created:** 10/20/2026 **Status:** ✅ Merged **Merged:** 10/20/2026 **Merged by:** [@gensokyo-dev](https://github.com/gensokyo-dev) **Base:** `main` ← **Head:** `fix/5094-evaluation-process-cleanup` --- ### 📝 Commits (2) - [`988354f`](https://github.com/morluto/rea/commit/988354f2f95e17e4ea5b8ea4cab241a3ea628f34) fix(evaluation): join owned command cleanup before removing files - [`b055ab2`](https://github.com/morluto/rea/commit/b055ab20ec01554a0a820631a22616cae63d8617) Merge remote-tracking branch 'origin/main' into fix/5094-evaluation-process-cleanup ### 📊 Changes **4 files changed** (+414 additions, -142 deletions) <details> <summary>View changed files</summary> 📝 `docs/client-verification.md` (+10 -2) ➕ `scripts/lib/agent-evaluation-commands.mjs` (+193 -0) 📝 `scripts/verify-agent-experience.mjs` (+21 -140) ➕ `tests/boundary/process/agentEvaluationCommands.test.ts` (+190 -0) </details> ### 📄 Description ## Summary Fixes #5094. The manual agent-evaluation runner had three copies of “send SIGTERM, immediately reject.” All three reproduce rejection while their exact child remains active. The Codex path also returns before stdout/readline closure; the outer failure path can delete disposable files while the process still uses them. Move the three command adapters into one run-scoped helper using existing `runOwnedCommand` supervision, identity checks, process-tree cleanup and output joining. Shebang CLI shims use the existing interpreter-compatible ownership option; launch generation and token still establish ownership. Streaming JSON-line consumption is closed/joined on every outcome. Incomplete cleanup retains the exact supervisor, preserves the primary timeout through `cause`, retries that owner in finalization and gates file removal on confirmed cleanup. This changes only the manual evaluation lane, with no new provider, target execution authority, CLI/MCP schema, Evidence semantics, package dependency or cache. A maintainer scope decision for new capabilities is not applicable. Existing target/configuration selections and Codex arguments are preserved. Diagnostic budgets now fail explicitly above 32 KiB stderr / 8 KiB combined version output; setup stdout is drained without retaining it. Complete parsed scenario events retain their existing evaluator semantics. ## Executed validation Pinned main `c3d7d0aaf`, Linux x64, Node **24.18.0**, npm **11.16.0**: - **13 focused tests pass**: real child/output closure for all three timeouts; seven success, nonzero-exit, Unicode JSON and diagnostic-limit controls; exact retained-owner retry/failure; existing independent permission/fixture cleanup and evaluation-report tests. Authored children include delayed SIGTERM exit, ignored SIGTERM and descendants inheriting output pipes. The shared owner may terminate the tree directly; no cooperative-exit assumption is required. - Full `npm run check` passes (typecheck, lint/module boundaries, formatting, unused-code/metadata and architecture checks); `docs:check` passes with **5 consistency tests**. - Newer-main integration **57d64b6ba**, final head **b055ab20e**: full checks and the same **13 focused tests** pass again; the actual entrypoint timeout control again confirms child/output closure before file removal. - Base reproduction evaluates the unchanged original declarations using real spawn/readline and retained exact handles. All three reject before child exit; Codex stdout/readline remain open at rejection. Probe cleanup joins its owned fixtures. - The actual `verify-agent-experience.mjs` entrypoint runs on both revisions with a disposable authored version executable. Observation hooks record state without changing spawn/removal behavior. Main removes auth/run files with `childActive:true` and both output streams open. Candidate removes them only with `childActive:false` and both streams destroyed. Both preserve `--version timed out` and eventually remove their disposable run. - Injected cleanup failures exercise the actual entrypoint: retry uses the same retained owner; a successful retry permits removal, while a still-incomplete retry preserves inert auth/run files and reports both the original timeout and unresolved cleanup. These injections establish owner/error/file ordering, not real OS cleanup denial. No real Codex model request, authentication copy, paid API call or reverse-engineering provider runs. Actual vendor signal behavior, complete model evaluation, original macOS/Node 26 and Windows native ownership remain unverified. Existing event history grows with transcript size; a whole-model-run memory bound is not claimed. The manual lane still requires its documented external process-family resource policy. Local raw receipts/probes remain under `/tmp/rea-5094-proof` and are not committed. ## Performance and context One CPU, 8-GiB hard / 7-GiB early family memory limits, no swap. Equivalent completed authored command workflows each perform version discovery, a command draining 2 MiB stdout, and a 3,000-event JSON scenario, twice: | Revision | Whole task elapsed | Family CPU | Peak family memory | Cold / warm helper workflow | | --- | ---: | ---: | ---: | ---: | | Main | 0.939 s | 0.672 s | 46.0 MiB | 0.285 / 0.278 s | | Candidate | 5.389 s | 4.325 s | 105.9 MiB | 2.689 / 2.219 s | Ownership/descendant verification adds measurable cost for very short commands; this is a cleanup-correctness tradeoff, not a speedup. Each row includes Node startup and two three-command tasks. “Warm” only reuses imports/OS state, not command results; OS caches/shared-host load are uncontrolled. Actual evaluation/model cost is unmeasured. Actual entrypoint timeout controls cost **13.160 / 14.437 s**, CPU **2.220 / 3.823 s**, peak **271.0 / 316.0 MiB** (main/candidate); they intentionally differ in cleanup ordering. All completed controls verify cleanup and no OOM using payload-exit counters while the observer remains alive, not final whole-scope counters. No agent interface or model prompt grows. Each deterministic fixture event list remains **910,891 bytes**, estimated **143,003 cl100k_base tokens**, on both revisions; it is local evaluator input, not a claim about model context/usage. Only compact verification receipts reach the reviewing agent. Full evaluation input/output token usage and downstream prompts were not measured. No raw stdout duplicate or cache is added; bounded non-JSON diagnostics avoid repeated full-prefix concatenation. ## CI/review assessment and authorized bypass Latest head `b055ab20e`: Greptile completes with **5/5** and no unresolved threads. Codex delivered two actionable Darwin findings during the final query/merge command. They were not assessed before that command merged; this is a merge-process error. Issue #5094 has been reopened and a focused follow-up is in progress for independently bounded ownership preparation and direct/nested preparation cleanup transfer. No Darwin completion claim is made. Classification/readiness/title jobs remain queued without a runner; neither queued CI nor pending Codex is claimed as passing. The task owner explicitly authorized bypass once independently qualified. Merge uses completed full checks, base reproductions, real child/descendant/output tests, actual entrypoint ordering controls, measured performance and final-tree newer-main integration. Pending results remain subject to assessment; unrelated main failures will not be mixed into this fix. ## Author readiness Human owner: @gensokyo-dev The task owner explicitly delegated validation, readiness declarations and merge to this agent. These declarations are supplied under that authorization; no separate new manual human review by the original author is asserted. - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] Focused existing-lifecycle repair; original behavior, producing boundary, affected consumers and related cleanup reviewed. - [x] Real native processes and public entrypoint tested; no credentials, targets, generated outputs or local proofs in the diff. - [x] Available latest-head CI/review state assessed; queued work and bypass authorization recorded above. Follow-up complete: #5615 merged as `8716b5241125d81f1a16808eb5178fdfdea1ecdb`. Both accepted Codex findings are addressed and their threads resolved after verified integration; final-head 18 tests/full checks/docs pass, Codex no findings, Greptile5. Native macOS/Windows proof gaps remain explicit. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-01 21:44:35 +00:00

closed this issue
added the
pull-request
label

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#6997

No description provided.

Rows
Columns