[PR #5615] [MERGED] fix(evaluation): prepare and retain native ownership helpers #6998

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/5094-darwin-ownership-preparation

📝 Commits (2)

a6251f7 fix(evaluation): prepare and retain native ownership helpers
6e54c01 refactor(evaluation): expose the shared resource scope contract

📊 Changes

3 files changed (+158 additions, -49 deletions)

View changed files

📝 docs/client-verification.md (+3 -2)
📝 scripts/lib/agent-evaluation-commands.mjs (+43 -45)
📝 tests/boundary/process/agentEvaluationCommands.test.ts (+112 -2)

📄 Description

Summary

Fixes #5094; follow-up to the two actionable Codex findings on #5613. Fresh Darwin ownership-helper compilation was inside the version probe's 10-second command deadline, and the evaluation helper retained only command supervisors, losing direct/nested preparation cleanup owners.

Prepare native ownership with its independently bounded deadline before entering each command budget. Replace the custom supervisor Map with existing ArtifactResourceScope, AnalysisCleanupLease and retainOwnedCommandCleanup, shared with provider prerequisite probes. The same owner now handles direct preparation failure, nested OwnedCommandFailure.preparationFailure, ordinary command cleanup, retry/admission and final close. File removal remains gated on confirmed cleanup; primary failures and exact owners survive.

Existing manual evaluation authority, caller inputs, diagnostic budgets, CLI/MCP contracts and Evidence semantics remain unchanged. No provider, execution capability, dependency or cache is added; additional maintainer scope approval is not applicable. Command adapters consume the shared cleanup interpretation instead of maintaining another one.

Executed validation

Pinned main 2a6fc712e, Linux x64, Node 24.18.0, npm 11.16.0:

18 focused tests pass: five new actual-entrypoint injection cases plus the existing real native-child/output, retained command-owner, file-permission and evaluation-report tests. Preparation is awaited before command launch; direct and nested preparation failures retry the exact owner, remove files only after release, and retain inert auth/run files on unresolved cleanup. Original failure and resource labels remain visible.
The five new cases all fail on pinned main with the new tests and unchanged production source. The control uses independent paths and immutable compiled process/domain artifacts matching the affected baseline; only the test file is replaced.
Real Linux authored children still cover timeout, inherited descendant pipes, Unicode event parsing, ordinary success/nonzero exits and diagnostic overflow. Actual evaluation entrypoint with an authored version executable again confirms child exit/output closure before removing run files. No real authentication or model request is used.
Full npm run check and docs:check pass, including 5 documentation consistency tests. Final head 6e54c01eb passes a further full check and the same 18 focused tests after completing the shared resource-scope contract.

No real macOS/Swift compilation or Windows native ownership was exercised. Injected preparation cases establish ordering, error/owner transfer and file-removal gating, not native compiler performance. A first resource run was refused because host availability was below the configured floor; it is excluded rather than reported as a test result. Local receipts remain under /tmp/rea-5094-darwin-proof and are not committed.

Performance and context

One CPU, 8-GiB hard / 7-GiB early family bounds, no swap; wait for at least 12 GiB available before heavy checks. Equivalent completed six-command workflows (two version/build/scenario triplets, 3,000 JSON events per scenario):

Revision	Elapsed	Family CPU	Peak family memory	Cold / warm helper workflow
Merged baseline	5.571 s	4.192 s	96.5 MiB	2.672 / 2.324 s
Follow-up	5.271 s	4.188 s	107.7 MiB	2.394 / 2.267 s

Each row includes Node startup and all six command completions. Imports/OS caches warm on the second triplet; there is no result cache. Shared-host load and caches are uncontrolled; no speedup is claimed. Completed runs confirm cleanup and no OOM at payload exit while the observer remains alive, not final whole-scope counters.

The evaluator retains the same event data: 910,891 bytes / estimated 143,003 cl100k_base tokens per scenario, identical to #5613. This is local evaluator input, not model context/usage. No agent schema, prompt or response grows; actual model-token costs and cold Darwin preparation costs remain unmeasured.

CI/review assessment and authorized bypass

Latest head 6e54c01eb: Codex completes with no findings, Greptile completes with 5/5, and no review thread is unresolved. CI classification is running; readiness/title remain queued. No unfinished CI is claimed as passing.

The task owner explicitly authorized bypass once independently qualified. Merge uses completed final-head checks, 18 focused tests, five exact-base failures, real native command/public-entrypoint controls and measured costs. Pending CI remains subject to assessment. The two original #5613 findings are accepted and explained; their threads will be resolved only after this repair merges. Original native macOS/Windows proof gaps remain explicit.

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

Both known findings addressed at the owning boundary; redundant cleanup implementation removed.
Focused regression and native command evidence recorded; scope and limitations explicit.
Final checks and available latest-head review/CI assessed; unfinished CI and authorized bypass recorded above.

_{🔄 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/5615 **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-darwin-ownership-preparation` --- ### 📝 Commits (2) - [`a6251f7`](https://github.com/morluto/rea/commit/a6251f7ad80e49328e38833beb0c7a51a6f6411a) fix(evaluation): prepare and retain native ownership helpers - [`6e54c01`](https://github.com/morluto/rea/commit/6e54c01eb29dc6a5c32235b87e7d7f674de45619) refactor(evaluation): expose the shared resource scope contract ### 📊 Changes **3 files changed** (+158 additions, -49 deletions) <details> <summary>View changed files</summary> 📝 `docs/client-verification.md` (+3 -2) 📝 `scripts/lib/agent-evaluation-commands.mjs` (+43 -45) 📝 `tests/boundary/process/agentEvaluationCommands.test.ts` (+112 -2) </details> ### 📄 Description ## Summary Fixes #5094; follow-up to the two actionable Codex findings on #5613. Fresh Darwin ownership-helper compilation was inside the version probe's 10-second command deadline, and the evaluation helper retained only command supervisors, losing direct/nested preparation cleanup owners. Prepare native ownership with its independently bounded deadline before entering each command budget. Replace the custom supervisor Map with existing `ArtifactResourceScope`, `AnalysisCleanupLease` and `retainOwnedCommandCleanup`, shared with provider prerequisite probes. The same owner now handles direct preparation failure, nested `OwnedCommandFailure.preparationFailure`, ordinary command cleanup, retry/admission and final close. File removal remains gated on confirmed cleanup; primary failures and exact owners survive. Existing manual evaluation authority, caller inputs, diagnostic budgets, CLI/MCP contracts and Evidence semantics remain unchanged. No provider, execution capability, dependency or cache is added; additional maintainer scope approval is not applicable. Command adapters consume the shared cleanup interpretation instead of maintaining another one. ## Executed validation Pinned main **2a6fc712e**, Linux x64, Node **24.18.0**, npm **11.16.0**: - **18 focused tests pass**: five new actual-entrypoint injection cases plus the existing real native-child/output, retained command-owner, file-permission and evaluation-report tests. Preparation is awaited before command launch; direct and nested preparation failures retry the exact owner, remove files only after release, and retain inert auth/run files on unresolved cleanup. Original failure and resource labels remain visible. - The five new cases **all fail on pinned main** with the new tests and unchanged production source. The control uses independent paths and immutable compiled process/domain artifacts matching the affected baseline; only the test file is replaced. - Real Linux authored children still cover timeout, inherited descendant pipes, Unicode event parsing, ordinary success/nonzero exits and diagnostic overflow. Actual evaluation entrypoint with an authored version executable again confirms child exit/output closure before removing run files. No real authentication or model request is used. - Full `npm run check` and `docs:check` pass, including **5 documentation consistency tests**. Final head **6e54c01eb** passes a further full check and the same **18 focused tests** after completing the shared resource-scope contract. No real macOS/Swift compilation or Windows native ownership was exercised. Injected preparation cases establish ordering, error/owner transfer and file-removal gating, not native compiler performance. A first resource run was refused because host availability was below the configured floor; it is excluded rather than reported as a test result. Local receipts remain under `/tmp/rea-5094-darwin-proof` and are not committed. ## Performance and context One CPU, 8-GiB hard / 7-GiB early family bounds, no swap; wait for at least 12 GiB available before heavy checks. Equivalent completed six-command workflows (two version/build/scenario triplets, 3,000 JSON events per scenario): | Revision | Elapsed | Family CPU | Peak family memory | Cold / warm helper workflow | | --- | ---: | ---: | ---: | ---: | | Merged baseline | 5.571 s | 4.192 s | 96.5 MiB | 2.672 / 2.324 s | | Follow-up | 5.271 s | 4.188 s | 107.7 MiB | 2.394 / 2.267 s | Each row includes Node startup and all six command completions. Imports/OS caches warm on the second triplet; there is no result cache. Shared-host load and caches are uncontrolled; no speedup is claimed. Completed runs confirm cleanup and no OOM at payload exit while the observer remains alive, not final whole-scope counters. The evaluator retains the same event data: **910,891 bytes / estimated 143,003 cl100k_base tokens per scenario**, identical to #5613. This is local evaluator input, not model context/usage. No agent schema, prompt or response grows; actual model-token costs and cold Darwin preparation costs remain unmeasured. ## CI/review assessment and authorized bypass Latest head `6e54c01eb`: Codex completes with **no findings**, Greptile completes with **5/5**, and no review thread is unresolved. CI classification is running; readiness/title remain queued. No unfinished CI is claimed as passing. The task owner explicitly authorized bypass once independently qualified. Merge uses completed final-head checks, 18 focused tests, five exact-base failures, real native command/public-entrypoint controls and measured costs. Pending CI remains subject to assessment. The two original #5613 findings are accepted and explained; their threads will be resolved only after this repair merges. Original native macOS/Windows proof gaps remain explicit. ## 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] Both known findings addressed at the owning boundary; redundant cleanup implementation removed. - [x] Focused regression and native command evidence recorded; scope and limitations explicit. - [x] Final checks and available latest-head review/CI assessed; unfinished CI and authorized bypass recorded above. --- <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:36 +00:00