morluto/rea

Fork 0

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

[PR #1700] [MERGED] fix(process): observe already-due trailing callbacks before settling the capture journal #4764

New issue

Closed

opened 2026-10-31 00:54:48 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:54:48 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1700
Author: @JasonBuildAI
Created: 10/11/2026
Status: ✅ Merged
Merged: 10/11/2026
Merged by: @morluto

Base: main ← Head: fix/process-capture-journal-settle-6f2c

📝 Commits (1)

1658564 fix(process): observe already-due trailing callbacks before settling the capture journal

📊 Changes

2 files changed (+32 additions, -1 deletions)

View changed files

📝 src/process/capture/ProcessCaptureLifecycle.ts (+5 -1)
📝 src/process/capture/ProcessHarness.test.ts (+27 -0)

📄 Description

Summary

Fixes #1697. Process-capture Evidence can silently drop the terminal tail when the host stalls the event loop around exit: settleProcessCaptureJournal judged the journal quiet by wall-clock time alone, so a trailing observation callback that was already due ran after the settle returned and never reached the immutable capture. PTY capture results now retain the trailing observations they were always meant to keep.

Problem and expected behavior

When the event loop stalls longer than the quiet window (quietMs, 25 ms by default) and then resumes, the settle's own wake-up timer and a trailing observation callback are both overdue. Due timers run in expiry order, so the earlier wake-up evaluates the journal before the trailing callback has appended its observation, decides the journal is quiet, and finalizes the capture. The trailing observation is dropped without any truncation flag.

Smallest trigger (unit level, deterministic): a trailing 12 ms timer that appends to the journal, plus a 40 ms synchronous stall right after starting settleProcessCaptureJournal(journal, 20, 200).

Expected: the settle does not declare silence while callbacks that were already due are still queued; trailing observations delivered before the capture becomes immutable are retained.

Change and scope

settleProcessCaptureJournal now awaits setImmediate() after each quiet-window sleep, so timer and poll callbacks that were already due run before silence is judged.
Regression test: does not settle while a trailing observation is already due behind a stalled event loop in src/process/capture/ProcessHarness.test.ts.

Intentionally not included: no change to quiet/max-wait defaults, no rework of capture finalization, no new APIs.

Contract and boundary impact

Semantic owner and earliest changed stage: process capture lifecycle; the earliest affected stage is finalization after process exit.
CLI and MCP/tool-catalog contract: none; tool schemas and results are unchanged.
Provider, bridge, target-format, or platform compatibility: none; the logic is platform-independent.
Evidence, artifact, provenance, or reconstruction contract: capture Evidence keeps trailing observations that it could previously drop; no format change.
Process execution, authorization, cleanup, or containment impact: none.
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Evidence and regression coverage

Evidence is executed on the change branch and on the base revision.

Tests added or updated: new deterministic regression in src/process/capture/ProcessHarness.test.ts. The existing waits for terminal observations delivered after the exit callback started failing 1 in 5 standalone runs (and reliably in a 65-file run) on Windows; it passes 6/6 runs after this change.
Base reproduction (base ee46323b with only the new test applied, fix reverted):

FAIL  |adapters| src/process/capture/ProcessHarness.test.ts > does not settle while a trailing observation is already due behind a stalled event loop
AssertionError: expected [ { capture_order: +0, …(2) } ] to have a length of 2 but got 1
 Test Files  1 failed (1)
      Tests  1 failed | 30 skipped (31)

After the change:

 Test Files  1 passed (1)
      Tests  1 passed | 30 skipped (31)

Project focused lane after the change:

npm run test:focused -- src/process/capture/ProcessHarness.test.ts src/process/capture/ProcessCaptureLifecycle.test.ts
 Test Files  2 passed (2)
      Tests  55 passed (55)

User-visible CLI/MCP output (if applicable): none; result shape is unchanged.
Remaining proof gaps: no whole-harness PTY stall repro (would need a controlled host stall during a real capture); the unit-level regression pins the settle contract directly.

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

npm run test:focused -- src/process/capture/ProcessHarness.test.ts src/process/capture/ProcessCaptureLifecycle.test.ts — 55 passed
npm run check:fast — typecheck and lint passed (also ran in the pre-push hook)
npx oxfmt --check src/process/capture/ProcessHarness.test.ts src/process/capture/ProcessCaptureLifecycle.ts — clean
Note: a broader Windows source-test run shows 12 pre-existing platform failures unrelated to this change (symlink creation EPERM, simulated win32 platform paths, missing locally built native artifact). This change removes the one failure it targets.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable; the changed code is the platform-independent capture settle path.
Package or release metadata impact: none.
Security, privacy, process, or containment review: none; the change adds one event-loop turn per quiet-window iteration.

Review checklist

This PR addresses a concrete problem or an agreed enhancement.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed (none needed).
User-visible CLI/MCP changes include representative output (not applicable).
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

AI disclosure: this change was drafted with AI assistance at my request, under my direction and review.

_{🔄 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/1700 **Author:** [@JasonBuildAI](https://github.com/JasonBuildAI) **Created:** 10/11/2026 **Status:** ✅ Merged **Merged:** 10/11/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/process-capture-journal-settle-6f2c` --- ### 📝 Commits (1) - [`1658564`](https://github.com/morluto/rea/commit/1658564d904560b6e05cbf886bc3785504f2b9b8) fix(process): observe already-due trailing callbacks before settling the capture journal ### 📊 Changes **2 files changed** (+32 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/process/capture/ProcessCaptureLifecycle.ts` (+5 -1) 📝 `src/process/capture/ProcessHarness.test.ts` (+27 -0) </details> ### 📄 Description ## Summary Fixes #1697. Process-capture Evidence can silently drop the terminal tail when the host stalls the event loop around exit: `settleProcessCaptureJournal` judged the journal quiet by wall-clock time alone, so a trailing observation callback that was already due ran after the settle returned and never reached the immutable capture. PTY capture results now retain the trailing observations they were always meant to keep. ## Problem and expected behavior When the event loop stalls longer than the quiet window (`quietMs`, 25 ms by default) and then resumes, the settle's own wake-up timer and a trailing observation callback are both overdue. Due timers run in expiry order, so the earlier wake-up evaluates the journal before the trailing callback has appended its observation, decides the journal is quiet, and finalizes the capture. The trailing observation is dropped without any truncation flag. Smallest trigger (unit level, deterministic): a trailing 12 ms timer that appends to the journal, plus a 40 ms synchronous stall right after starting `settleProcessCaptureJournal(journal, 20, 200)`. Expected: the settle does not declare silence while callbacks that were already due are still queued; trailing observations delivered before the capture becomes immutable are retained. ## Change and scope - `settleProcessCaptureJournal` now awaits `setImmediate()` after each quiet-window sleep, so timer and poll callbacks that were already due run before silence is judged. - Regression test: `does not settle while a trailing observation is already due behind a stalled event loop` in `src/process/capture/ProcessHarness.test.ts`. Intentionally not included: no change to quiet/max-wait defaults, no rework of capture finalization, no new APIs. ## Contract and boundary impact - Semantic owner and earliest changed stage: process capture lifecycle; the earliest affected stage is finalization after process exit. - CLI and MCP/tool-catalog contract: none; tool schemas and results are unchanged. - Provider, bridge, target-format, or platform compatibility: none; the logic is platform-independent. - Evidence, artifact, provenance, or reconstruction contract: capture Evidence keeps trailing observations that it could previously drop; no format change. - Process execution, authorization, cleanup, or containment impact: none. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. ## Evidence and regression coverage Evidence is executed on the change branch and on the base revision. - Tests added or updated: new deterministic regression in `src/process/capture/ProcessHarness.test.ts`. The existing `waits for terminal observations delivered after the exit callback` started failing 1 in 5 standalone runs (and reliably in a 65-file run) on Windows; it passes 6/6 runs after this change. - Base reproduction (base ee46323b with only the new test applied, fix reverted): ``` FAIL |adapters| src/process/capture/ProcessHarness.test.ts > does not settle while a trailing observation is already due behind a stalled event loop AssertionError: expected [ { capture_order: +0, …(2) } ] to have a length of 2 but got 1 Test Files 1 failed (1) Tests 1 failed | 30 skipped (31) ``` - After the change: ``` Test Files 1 passed (1) Tests 1 passed | 30 skipped (31) ``` - Project focused lane after the change: ``` npm run test:focused -- src/process/capture/ProcessHarness.test.ts src/process/capture/ProcessCaptureLifecycle.test.ts Test Files 2 passed (2) Tests 55 passed (55) ``` - User-visible CLI/MCP output (if applicable): none; result shape is unchanged. - Remaining proof gaps: no whole-harness PTY stall repro (would need a controlled host stall during a real capture); the unit-level regression pins the settle contract directly. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npm run test:focused -- src/process/capture/ProcessHarness.test.ts src/process/capture/ProcessCaptureLifecycle.test.ts` — 55 passed - `npm run check:fast` — typecheck and lint passed (also ran in the pre-push hook) - `npx oxfmt --check src/process/capture/ProcessHarness.test.ts src/process/capture/ProcessCaptureLifecycle.ts` — clean - Note: a broader Windows source-test run shows 12 pre-existing platform failures unrelated to this change (symlink creation EPERM, simulated `win32` platform paths, missing locally built native artifact). This change removes the one failure it targets. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; the changed code is the platform-independent capture settle path. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none; the change adds one event-loop turn per quiet-window iteration. ## Review checklist - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed (none needed). - [x] User-visible CLI/MCP changes include representative output (not applicable). - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. AI disclosure: this change was drafted with AI assistance at my request, under my direction and review. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 00:54:48 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-10-31 01:14:09 +00:00

[PR #4764] [MERGED] fix(evidence): verify staged output identity after its final write #6397

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

No description provided.

Rows
Columns