morluto/rea

Fork 0

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

[PR #6342] fix(process): name the fixed finalization delivery grace #7291

New issue

Open

opened 2026-11-01 21:46:48 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:46:48 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6342
Author: @saimmehmood
Created: 10/22/2026
Status: 🔄 Open

Base: main ← Head: fix/process-finalization-grace-disclosure

📝 Commits (1)

1360da2 fix(process): name the fixed finalization delivery grace

📊 Changes

2 files changed (+5 additions, -3 deletions)

View changed files

📝 src/process/capture/ProcessCaptureLifecycle.test.ts (+4 -2)
📝 src/process/capture/ProcessCaptureLifecycle.ts (+1 -1)

📄 Description

Summary

Finalization signal-delivery records cited a bare millisecond figure when a
delivery did not settle, without saying whether that figure is adjustable. An
agent reading exit.finalization.signals[].reason could not tell that the grace
is fixed implementation policy, and could plausibly infer that raising the
caller-selected finalization_ms would extend it.

Relates to #5910 (limit errors omit adjustment controls and leave hard-coded
ceilings unexplained).

Problem and expected behavior

Smallest trigger: run a capture with a positive finalization_ms where the
signal delivery never settles within 1000 ms (ProcessCaptureLifecycle
attempt()). The record is correctly marked delivery: "unverified", but its
reason read:

delivery did not settle within 1000 ms

Violated invariant, from #5910 success criterion 4: for a retained hard-coded
cap, the error must say that this release has no runtime override and must not
imply that an unrelated, caller-selected budget changes it.
PROCESS_FINALIZATION_DELIVERY_GRACE_MS is a module-private constant with no
environment or per-call control, and it is independent of finalization_ms
(which is the SIGTERM→SIGKILL interval, not the per-delivery settle grace).

Expected: the reason names the specific budget, states that it is fixed in this
release, and forecloses the wrong inference about finalization_ms.

Change and scope

One message string, plus the two assertions that pin it.

The new text keeps the local register of this field. Sibling values in the same
function ("delivery had not settled when the record was published",
"delivery wait was interrupted") are lowercase fragments without trailing
periods, so the disclosure is appended as a clause rather than as a separate
capitalised sentence.

Intentionally not included:

No change to the grace value, control flow, or delivery classification.
No new per-call override or configuration setting. #5910 criterion 2 invites
considering one; that is a larger behavioural decision for the owner, not part
of a message-fidelity fix.
fixedMcpPhaseBudgetRecovery (src/mcpStartupPolicy.ts) was deliberately not
reused: it is scoped to MCP startup phases, and importing it into process
capture would cross a module boundary for a string.
No sweep of other boundaries under #5910.

Contract and boundary impact

Semantic owner and earliest changed stage: process capture finalization,
src/process/capture/ProcessCaptureLifecycle.ts — the producing boundary that
records a finalization signal's delivery result.
Contribution scope fit and linked maintainer decision: not applicable;
ordinary fix inside an existing boundary, no new provider or capability.
CLI and MCP/tool-catalog contract: unchanged. signals[].reason is already
z.string().min(1).optional() with no maximum length; only the text differs.
No catalog or schema regeneration.
Provider, bridge, target-format, or platform compatibility: none; the string is
platform-independent.
Evidence, artifact, provenance, or reconstruction contract: unchanged. The
reason still accompanies delivery: "unverified", which
processCaptureFinalizationSchema requires (a reason is permitted only for an
unverified delivery). "Unverified" continues to mean delivery was not
confirmed, not that it failed.
Target selection, authority, upstream behavior, and cleanup details: not
applicable; no change to signalling, ownership, or cleanup.
Generated metadata, package, or installation impact: none.

Expected effects:

Documentation or usability only; no target interaction.

Evidence and regression coverage

Tests added or updated: the two existing assertions in
src/process/capture/ProcessCaptureLifecycle.test.ts that pin this reason
(stops waiting for a hung delivery once the exit is observed, and
stops waiting when the escalation delivery never settles and no exit arrives)
are updated to the new text. Both already exercise the unsettled-delivery path.
Base reproduction or other evidence: executed. Reverting only the source
file to the parent revision while keeping the updated assertions fails the
regression as expected:
```
× stops waiting for a hung delivery once the exit is observed 1125ms
```
The reason is compared by exact equality through toMatchObject, so the
coverage catches a silent revert of the disclosure. The source file was
restored afterwards and the committed diff is unchanged.
User-visible CLI/MCP output: the reason field of an unverified finalization
signal record now reads:

delivery did not settle within REA's fixed 1000 ms signal-delivery grace; this release has no runtime override and a larger finalization_ms does not extend it
Remaining proof gaps:
- No real process capture was driven end to end to observe this message in
  live MCP output; the unsettled-delivery path is exercised through the
  suite's fake terminal, as the pre-existing tests do.
- npm ci in this environment resolved vite 8.3.2 → 8.3.5 as a peer
  dependency and rewrote package-lock.json; that drift was reverted and is
  not part of this PR, but the test run therefore used vite 8.3.5 rather than
  the locked version.

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:local -- src/process/capture/ProcessCaptureLifecycle.test.ts —
1 file passed, 25/25 tests passed (6.98s), including both updated
assertions.
npx vitest run --project process-boundary tests/boundary/process/processCaptureLifecycle.test.ts
— 1 file passed, 24/24 tests passed (24.2s). Verified against the parent
revision as well, which also passes 24/24, so this change leaves the real
process-capture boundary behaviour unchanged.
Base-revision regression check (source reverted, assertions kept) —
failed as intended, 1 test failing; see Evidence above.
npm run check:fast (turbo typecheck + lint) — 2 successful, 2 total,
exit 0.
npm run lint — 4645 files, 0 errors, 2239 pre-existing repo-wide
warnings. The 9 diagnostics touching
src/process/capture/ProcessCaptureLifecycle.ts are pre-existing structural
warnings (max-lines 1585, complexity 38, max-params, and
max-lines-per-function); this change replaces one line with one line and
does not alter them.
npm run verify:module-boundaries — checked 2704 source files, passed.
npm run format:check (oxfmt) — no issues in either changed file.
grep -rn 'did not settle within 1000 ms' . — no matches outside the two
changed files; no stale assertions, snapshots, fixtures, or docs embed the old
text.
Exact-equality check between the source template (constant substituted) and
both test literals — both match byte-for-byte.
The full process-boundary project (12 files, including JADX/Java integration)
was not cleanly established in this environment: interrupted earlier runs
left orphaned process state that produced spurious failures, and some files
need external provider tools. Only the file covering this change was run to a
clean, repeated result. CI coverage is needed for the rest.

Compatibility, safety, and release

Breaking changes or migration steps: none. The field is an optional
human-readable string, not an enum or a stable identifier. Any consumer
matching this reason by exact text would need updating, but the text is not
part of a documented contract.
Real Hopper/Ghidra, browser, or OS coverage: not applicable; no provider or
platform interaction changes.
Package or release metadata impact: none.
Security, privacy, process, or containment review: none. No new data is
disclosed; the message names a constant already visible in public source.

Author readiness

Human owner: @saimmehmood

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

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
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.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

_{🔄 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/6342 **Author:** [@saimmehmood](https://github.com/saimmehmood) **Created:** 10/22/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/process-finalization-grace-disclosure` --- ### 📝 Commits (1) - [`1360da2`](https://github.com/morluto/rea/commit/1360da2ad5a60a9c89d955da06c533b32d7ac320) fix(process): name the fixed finalization delivery grace ### 📊 Changes **2 files changed** (+5 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `src/process/capture/ProcessCaptureLifecycle.test.ts` (+4 -2) 📝 `src/process/capture/ProcessCaptureLifecycle.ts` (+1 -1) </details> ### 📄 Description ## Summary Finalization signal-delivery records cited a bare millisecond figure when a delivery did not settle, without saying whether that figure is adjustable. An agent reading `exit.finalization.signals[].reason` could not tell that the grace is fixed implementation policy, and could plausibly infer that raising the caller-selected `finalization_ms` would extend it. Relates to #5910 (limit errors omit adjustment controls and leave hard-coded ceilings unexplained). ## Problem and expected behavior Smallest trigger: run a capture with a positive `finalization_ms` where the signal delivery never settles within 1000 ms (`ProcessCaptureLifecycle` `attempt()`). The record is correctly marked `delivery: "unverified"`, but its reason read: > delivery did not settle within 1000 ms Violated invariant, from #5910 success criterion 4: for a retained hard-coded cap, the error must say that this release has no runtime override and must not imply that an unrelated, caller-selected budget changes it. `PROCESS_FINALIZATION_DELIVERY_GRACE_MS` is a module-private constant with no environment or per-call control, and it is independent of `finalization_ms` (which is the SIGTERM→SIGKILL interval, not the per-delivery settle grace). Expected: the reason names the specific budget, states that it is fixed in this release, and forecloses the wrong inference about `finalization_ms`. ## Change and scope One message string, plus the two assertions that pin it. The new text keeps the local register of this field. Sibling values in the same function (`"delivery had not settled when the record was published"`, `"delivery wait was interrupted"`) are lowercase fragments without trailing periods, so the disclosure is appended as a clause rather than as a separate capitalised sentence. Intentionally not included: - No change to the grace value, control flow, or `delivery` classification. - No new per-call override or configuration setting. #5910 criterion 2 invites considering one; that is a larger behavioural decision for the owner, not part of a message-fidelity fix. - `fixedMcpPhaseBudgetRecovery` (`src/mcpStartupPolicy.ts`) was deliberately not reused: it is scoped to MCP startup phases, and importing it into process capture would cross a module boundary for a string. - No sweep of other boundaries under #5910. ## Contract and boundary impact - Semantic owner and earliest changed stage: process capture finalization, `src/process/capture/ProcessCaptureLifecycle.ts` — the producing boundary that records a finalization signal's delivery result. - Contribution scope fit and linked maintainer decision: not applicable; ordinary fix inside an existing boundary, no new provider or capability. - CLI and MCP/tool-catalog contract: unchanged. `signals[].reason` is already `z.string().min(1).optional()` with no maximum length; only the text differs. No catalog or schema regeneration. - Provider, bridge, target-format, or platform compatibility: none; the string is platform-independent. - Evidence, artifact, provenance, or reconstruction contract: unchanged. The reason still accompanies `delivery: "unverified"`, which `processCaptureFinalizationSchema` requires (a reason is permitted only for an unverified delivery). "Unverified" continues to mean delivery was not confirmed, not that it failed. - Target selection, authority, upstream behavior, and cleanup details: not applicable; no change to signalling, ownership, or cleanup. - Generated metadata, package, or installation impact: none. Expected effects: - [x] Documentation or usability only; no target interaction. ## Evidence and regression coverage - Tests added or updated: the two existing assertions in `src/process/capture/ProcessCaptureLifecycle.test.ts` that pin this reason (`stops waiting for a hung delivery once the exit is observed`, and `stops waiting when the escalation delivery never settles and no exit arrives`) are updated to the new text. Both already exercise the unsettled-delivery path. - Base reproduction or other evidence: **executed**. Reverting only the source file to the parent revision while keeping the updated assertions fails the regression as expected: ``` × stops waiting for a hung delivery once the exit is observed 1125ms ``` The reason is compared by exact equality through `toMatchObject`, so the coverage catches a silent revert of the disclosure. The source file was restored afterwards and the committed diff is unchanged. - User-visible CLI/MCP output: the `reason` field of an unverified finalization signal record now reads: > delivery did not settle within REA's fixed 1000 ms signal-delivery grace; this release has no runtime override and a larger finalization_ms does not extend it - Remaining proof gaps: - No real process capture was driven end to end to observe this message in live MCP output; the unsettled-delivery path is exercised through the suite's fake terminal, as the pre-existing tests do. - `npm ci` in this environment resolved `vite` 8.3.2 → 8.3.5 as a peer dependency and rewrote `package-lock.json`; that drift was reverted and is not part of this PR, but the test run therefore used vite 8.3.5 rather than the locked version. 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:local -- src/process/capture/ProcessCaptureLifecycle.test.ts` — **1 file passed, 25/25 tests passed** (6.98s), including both updated assertions. - `npx vitest run --project process-boundary tests/boundary/process/processCaptureLifecycle.test.ts` — **1 file passed, 24/24 tests passed** (24.2s). Verified against the parent revision as well, which also passes 24/24, so this change leaves the real process-capture boundary behaviour unchanged. - Base-revision regression check (source reverted, assertions kept) — **failed as intended**, 1 test failing; see Evidence above. - `npm run check:fast` (turbo `typecheck` + `lint`) — **2 successful, 2 total**, exit 0. - `npm run lint` — **4645 files, 0 errors**, 2239 pre-existing repo-wide warnings. The 9 diagnostics touching `src/process/capture/ProcessCaptureLifecycle.ts` are pre-existing structural warnings (`max-lines` 1585, `complexity` 38, `max-params`, and `max-lines-per-function`); this change replaces one line with one line and does not alter them. - `npm run verify:module-boundaries` — **checked 2704 source files**, passed. - `npm run format:check` (`oxfmt`) — **no issues in either changed file**. - `grep -rn 'did not settle within 1000 ms' .` — no matches outside the two changed files; no stale assertions, snapshots, fixtures, or docs embed the old text. - Exact-equality check between the source template (constant substituted) and both test literals — both match byte-for-byte. - The full `process-boundary` project (12 files, including JADX/Java integration) was **not cleanly established** in this environment: interrupted earlier runs left orphaned process state that produced spurious failures, and some files need external provider tools. Only the file covering this change was run to a clean, repeated result. CI coverage is needed for the rest. ## Compatibility, safety, and release - Breaking changes or migration steps: none. The field is an optional human-readable string, not an enum or a stable identifier. Any consumer matching this reason by exact text would need updating, but the text is not part of a documented contract. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; no provider or platform interaction changes. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none. No new data is disclosed; the message names a constant already visible in public source. ## Author readiness Human owner: @saimmehmood  - [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] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [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. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-11-01 21:46:48 +00:00

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

No description provided.

Rows
Columns