morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 07:47:29 +00:00

[PR #498] [MERGED] test: replace implementation lock-in with behavioural assertions #2027

New issue

Closed

opened 2026-10-16 21:13:29 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-16 21:13:29 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: test/replace-slop-with-behaviour-tests

📝 Commits (10+)

2ca56ac fix(process): normalize reactive rendered-frame timestamps
349998a fix(install): add recovery guidance to version-mismatch failure
5878e21 ci: repair stale curated Windows lane and unblind the duplication gate
a59de29 test: replace implementation lock-in with behavioural assertions
81b0d22 ci: keep native-authority suites out of the curated Windows lane
e9bea27 test: stop max-lines from forcing test files to split
fd3790a test: stop creating temp scaffolding the temp helper immediately deletes
8d2a490 test: merge the split process reactive runtime suite
5f06fac test: drop two identity round-trips from node runtime characterization
50a1b31 test: hoist duplicated graph fixtures into the shared fixture module

📊 Changes

95 files changed (+5416 additions, -6147 deletions)

View changed files

📝 .github/workflows/ci.yml (+19 -9)
📝 .jscpd.json (+7 -3)
📝 .oxlintrc.json (+1 -4)
📝 docs/verification/managed-conformance-manifest.json (+2 -2)
📝 install.sh (+1 -1)
📝 package.json (+5 -5)
📝 src/application/ProcessHarness.ts (+36 -0)
📝 src/application/ProcessReactiveCoordinator.test.ts (+3 -0)
📝 src/application/ProcessReactiveCoordinatorFailures.fixture.ts (+5 -54)
📝 src/application/ProcessReactiveCoordinatorFailures.test.ts (+53 -83)
📝 src/application/ProcessReactiveEffects.ts (+15 -3)
📝 src/application/ProcessReactiveHarness.ts (+1 -0)
📝 src/contracts/promptContracts.test.ts (+14 -12)
📝 src/contracts/toolContracts.test.ts (+196 -53)
📝 src/domain/customProtocolCapture.test.ts (+26 -15)
➕ src/domain/eventProcessTree.fixture.ts (+112 -0)
📝 src/domain/eventProcessTree.test.ts (+52 -260)
➖ src/domain/javascriptApplicationGraph.01-defines-the-complete-provider-neutral.test.ts (+0 -251)
➖ src/domain/javascriptApplicationGraph.02-keeps-static-observations-static-inferences.test.ts (+0 -190)
➖ src/domain/javascriptApplicationGraph.03-normalizes-structural-identity-and-evidence.test.ts (+0 -160)

...and 75 more files

📄 Description

Summary

Audits the test suite for slop, then removes the lock-in rather than the coverage. Twenty-two commits, split so Release Please records the behaviour fixes separately from the test work.

Two of these were latent defects rather than test problems: a windows-curated CI job that had been silently skipping 9 of its 15 files, and a check:pr that has been failing on main since #488.

Net: 40 files, +1653 / -1984.

Contract and behaviour changes

Root causes fixed

Reactive capture could lose observations emitted just before exit. The exit path drained the reactive coordinator and then submitted target_lost, but node-pty delivers buffered output through onData callbacks that run outside the coordinator's queue. A trailing chunk not yet delivered produced no journal entry, so the drain had nothing to wait for and the target was declared lost while a terminal trigger was still satisfiable — the multi-source fixture resolved target_lost instead of passed at roughly even odds. The harness now yields to the event loop until the capture journal stops growing before draining, bounded so a pathological stream cannot stall exit.

The malformed-body test asserted post-window outcomes from a zero-length window. observation_ms: 0 raced the response-body fetch phase, so whether the body was fetched at all depended on host speed. The fake browser answers instantly, so a real window removes the race with no product nondeterminism involved.

`fix(process)` — reactive rendered-frame timestamps

A reactive resize action stamped rendered_frames[].at_ms with the raw elapsed time while every other rendered frame used normalizeProcessElapsedTime. Under the default 10ms time bucket the next frame buckets down, appears to move backwards, and parseProcessCapture rejects the whole capture with rendered_frames: timestamps must be ordered.

No test combined a reactive resize with time bucketing, so the path was never exercised. ProcessReactiveEffects.ts now normalizes the renderer call, matching what ProcessCaptureJournal already did for scheduled resizes. dispatched_at_ms/captured_at_ms remain raw on both paths, so only the one inconsistent clock changes.

Verified causally: reverting only the normalization reproduces the failure immediately. Found while converting a wall-clock-scheduled PTY scenario to terminal_text triggers, which is now deterministic.

`fix(install)` — version-mismatch guidance

install.sh:105 was the only installer failure that stated what went wrong without a remedy, contradicting the requirement to keep diagnostics actionable for mismatch locations. The test's exemption is removed, so all ten failures now assert they carry a recovery action — that assertion is what surfaced the gap.

`ci` — repaired Windows lane and unblinded duplication gate

windows-curated referenced 9 non-existent test files, including 6 processOwnership* files moved to src/process/ in 4ecc7a72. Vitest silently drops unmatched paths, so the job ran 6 files and reported success. Paths repointed, plus tests/process-global/curatedWindowsLane.test.ts which fails on any stale path (verified by injecting one). The lane now runs 14 files, up from the 6 it silently ran, all verified green on windows-latest.
Restoring the lane turned it red, which is the more interesting finding. Three restored files — src/ghidra/GhidraProvider.test.ts, ghidraClient.test.ts, providerProcess.test.ts — fail with PrivateRuntimeRootUnavailableError, because they require Job Object process ownership and private-runtime DACL enforcement that windows-latest does not provide by design. They were never running on Windows, so nobody knew. They are excluded with the reason recorded in both ci.yml and the guard test, since those claims belong behind the same approval boundary as the real Ghidra Windows workflow. Two of the three were entries this branch added by mistake.
.jscpd.json ignored **/*.test.ts, so the duplication gate running on every PR reported 0 clones across the test tree. Test files are now included and the threshold is ratcheted to the measured result.
Local runs used 1 worker, costing ~140s wall at ~100% CPU on a 10-core host. Now 2, matching CI's proven budget, with PTY scenarios in a serial project. Full local suite 140s → ~89s.
Regenerated managed-conformance-manifest.json; check:pr has been red on main since #488.

`test` — structural cleanups

max-lines: 500 on test files removed. Its only effect on tests was to force duplication: it split one binary session suite across nine files with names truncated mid-sentence, and it is why the Windows lane referenced six split-apart processOwnership* paths. That suite is merged back to one file (20 composition files → 12), and the processReactiveRuntime five-file split is merged to one (92 domain files → 88).
createTestTempDirectory no longer builds scaffolding it deletes. It called createTestWorkspace, which creates home and xdg/{config,cache}, then deleted two of the three so callers would get an empty root — three mkdir and two rm per call across ~100 files.
Repeated literals replaced with builders. eventProcessTree.test.ts spelled out nineteen thirteen-field event literals inline, varying three or four fields each; fixture builders take it from 422 to 214 lines with all nine cases intact.
Duplicated fixtures hoisted. Nineteen byte-identical copies of six helpers across the javascriptApplicationGraph files now live in javascriptApplicationGraph.fixture.ts (−187 lines), and three more enhancedTools files import helpers already exported from the shared harness instead of re-declaring them (−41 lines).

`test` — lock-in removed, coverage kept

Removed	Replaced with
`toolContracts` group sizes + 50 lines of tool names	4 structural invariants, both mutation-checked
6× `safeParse(fixture).success === true`	5 real rejections + 2 property tests
8× `expect.any(...)` placeholders	exact captured results
2 assertions on strings absent from their own inputs	removed (could never fail)
Concurrency fake returning `? [] : []`	real shared state, lost updates observable
`getTimerCount()` / `getEventListeners()` / exact retry counts	detachment behaviour
Caps that no longer exist (16 MiB, 10,001 files, 100,001 frames, literal `10_201`)	derived relationships valid at any size
2 byte-identical preambles split by `max-lines`	2 shared fixture modules (~1,000 lines)

Profiling the heaviest files rather than only running them:

appleApplication.test.ts 6.4s → 0.28s (1,001 frameworks × 101 pages → 12/4/4/3, with a derived SCRIPT × NATIVE pairing invariant that the literal 10_201 could not check)
referenceSourceImport.test.ts 2.3s → 0.2s

Verification

npm run check:pr — typecheck, lint (42 warnings, unchanged from main), format, knip, jscpd, docs:check, full suite: 372 files, 1804 passed, 2 skipped. All 19 CI checks pass, including the repaired Windows lane.

No real Hopper or Ghidra toolchain verification was performed.

Notes for reviewers

Diff noise is expected in tests/boundary/process/. it( → itWithCaptureCapability( is 21 characters longer and pushes lines past the print width, so Prettier reindents them. git diff -w shows the real change: 891 changed lines are 105 semantic.
The cdpBrowserProvider.sensitive_data change is reasoned, not reproduced. That scenario asserted outcomes produced after the observation window closes while using observation_ms: 0; it now uses a real window. I could not reproduce the failure locally (0/24 at 8× concurrency) — it only appeared under full-suite load — so this removes a genuine timing dependency by inspection rather than by confirmed reproduction. Worth watching on CI.
processCaptureMultisource is flaky and CI may show it. Diagnosed, not masked: the run resolves target_lost instead of passed because the harness can declare the target lost before draining observations emitted just before exit. The fixture already works around this with a 25 ms settle sleep, which is load-sensitive. Raising the reactive deadlines to 30 s did not help, so the deadlines were reverted. The test is deliberately left strict because it is correctly detecting a real harness drain race.
Rejection and retention assertions are now exact. The reactive scenario table asserted only that each of six fixtures failed to parse, so a typo invalidating an unrelated field still passed; each row now names the field it targets and must produce that issue path and code. The large-body replay test asserted the serialized result exceeded five million characters as a proxy for retention, which escaping could satisfy under truncation; it now compares each body byte for byte. Both were verified by mutation: a row valid except for an unknown key now fails, and shaving one byte off the expected body now fails.
Duplication gate ratcheted. Test-tree duplication is down to 2.58% of lines from 3.39% when tests were first included, so the threshold moved from 3.2 to 2.7.
Measurement caveat on the PTY fix. It passes 18 consecutive runs isolated and under four-way concurrency, against roughly even odds before. A control run with the wait disabled did not reproduce the failure, so the machine was not loaded enough for a controlled A/B; the before/after numbers come from separate sessions.
No real Hopper or Ghidra verification was performed. Those lanes were not touched and remain manual per docs/testing.md.

🤖 Generated with Claude Code

_{🔄 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/498 **Author:** [@morluto](https://github.com/morluto) **Created:** 10/3/2026 **Status:** ✅ Merged **Merged:** 10/3/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `test/replace-slop-with-behaviour-tests` --- ### 📝 Commits (10+) - [`2ca56ac`](https://github.com/morluto/rea/commit/2ca56ac5b3bd1db668fa747e2a52db54f4977669) fix(process): normalize reactive rendered-frame timestamps - [`349998a`](https://github.com/morluto/rea/commit/349998a693b377c14055249b93e43824c3b3334c) fix(install): add recovery guidance to version-mismatch failure - [`5878e21`](https://github.com/morluto/rea/commit/5878e21939b0c5aa84d3ee9c59ebb54e19ce2b40) ci: repair stale curated Windows lane and unblind the duplication gate - [`a59de29`](https://github.com/morluto/rea/commit/a59de295bd9d53bda1cd7b5d38009f73a47be917) test: replace implementation lock-in with behavioural assertions - [`81b0d22`](https://github.com/morluto/rea/commit/81b0d226b5e4f79dccad220048220347f92c2a79) ci: keep native-authority suites out of the curated Windows lane - [`e9bea27`](https://github.com/morluto/rea/commit/e9bea27e16960dfee062bd0a782b2b7c5ab36ba3) test: stop max-lines from forcing test files to split - [`fd3790a`](https://github.com/morluto/rea/commit/fd3790ab9756e17fa95d8f5faa12da7f9e5eeec0) test: stop creating temp scaffolding the temp helper immediately deletes - [`8d2a490`](https://github.com/morluto/rea/commit/8d2a4904c039072bc00ce50a73faac840e4b83be) test: merge the split process reactive runtime suite - [`5f06fac`](https://github.com/morluto/rea/commit/5f06facc72692c93b0e6946292f277c503cc21f4) test: drop two identity round-trips from node runtime characterization - [`50a1b31`](https://github.com/morluto/rea/commit/50a1b3149cc9c168bb7575f397d114e8adaacf94) test: hoist duplicated graph fixtures into the shared fixture module ### 📊 Changes **95 files changed** (+5416 additions, -6147 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/ci.yml` (+19 -9) 📝 `.jscpd.json` (+7 -3) 📝 `.oxlintrc.json` (+1 -4) 📝 `docs/verification/managed-conformance-manifest.json` (+2 -2) 📝 `install.sh` (+1 -1) 📝 `package.json` (+5 -5) 📝 `src/application/ProcessHarness.ts` (+36 -0) 📝 `src/application/ProcessReactiveCoordinator.test.ts` (+3 -0) 📝 `src/application/ProcessReactiveCoordinatorFailures.fixture.ts` (+5 -54) 📝 `src/application/ProcessReactiveCoordinatorFailures.test.ts` (+53 -83) 📝 `src/application/ProcessReactiveEffects.ts` (+15 -3) 📝 `src/application/ProcessReactiveHarness.ts` (+1 -0) 📝 `src/contracts/promptContracts.test.ts` (+14 -12) 📝 `src/contracts/toolContracts.test.ts` (+196 -53) 📝 `src/domain/customProtocolCapture.test.ts` (+26 -15) ➕ `src/domain/eventProcessTree.fixture.ts` (+112 -0) 📝 `src/domain/eventProcessTree.test.ts` (+52 -260) ➖ `src/domain/javascriptApplicationGraph.01-defines-the-complete-provider-neutral.test.ts` (+0 -251) ➖ `src/domain/javascriptApplicationGraph.02-keeps-static-observations-static-inferences.test.ts` (+0 -190) ➖ `src/domain/javascriptApplicationGraph.03-normalizes-structural-identity-and-evidence.test.ts` (+0 -160) _...and 75 more files_ </details> ### 📄 Description ## Summary Audits the test suite for slop, then removes the lock-in rather than the coverage. Twenty-two commits, split so Release Please records the behaviour fixes separately from the test work. Two of these were latent defects rather than test problems: a `windows-curated` CI job that had been silently skipping 9 of its 15 files, and a `check:pr` that has been failing on `main` since #488. Net: **40 files, +1653 / -1984**. ## Contract and behaviour changes ### Root causes fixed **Reactive capture could lose observations emitted just before exit.** The exit path drained the reactive coordinator and then submitted `target_lost`, but node-pty delivers buffered output through `onData` callbacks that run *outside* the coordinator's queue. A trailing chunk not yet delivered produced no journal entry, so the drain had nothing to wait for and the target was declared lost while a terminal trigger was still satisfiable — the multi-source fixture resolved `target_lost` instead of `passed` at roughly even odds. The harness now yields to the event loop until the capture journal stops growing before draining, bounded so a pathological stream cannot stall exit. **The malformed-body test asserted post-window outcomes from a zero-length window.** `observation_ms: 0` raced the response-body fetch phase, so whether the body was fetched at all depended on host speed. The fake browser answers instantly, so a real window removes the race with no product nondeterminism involved. ### `fix(process)` — reactive rendered-frame timestamps A reactive `resize` action stamped `rendered_frames[].at_ms` with the **raw** elapsed time while every other rendered frame used `normalizeProcessElapsedTime`. Under the default 10ms time bucket the next frame buckets *down*, appears to move backwards, and `parseProcessCapture` rejects the whole capture with `rendered_frames: timestamps must be ordered`. No test combined a reactive resize with time bucketing, so the path was never exercised. `ProcessReactiveEffects.ts` now normalizes the renderer call, matching what `ProcessCaptureJournal` already did for scheduled resizes. `dispatched_at_ms`/`captured_at_ms` remain raw on both paths, so only the one inconsistent clock changes. Verified causally: reverting only the normalization reproduces the failure immediately. Found while converting a wall-clock-scheduled PTY scenario to `terminal_text` triggers, which is now deterministic. ### `fix(install)` — version-mismatch guidance `install.sh:105` was the only installer failure that stated what went wrong without a remedy, contradicting the requirement to keep diagnostics actionable for mismatch locations. The test's exemption is removed, so all ten failures now assert they carry a recovery action — that assertion is what surfaced the gap. ### `ci` — repaired Windows lane and unblinded duplication gate - **`windows-curated` referenced 9 non-existent test files**, including 6 `processOwnership*` files moved to `src/process/` in `4ecc7a72`. Vitest silently drops unmatched paths, so the job ran 6 files and reported success. Paths repointed, plus `tests/process-global/curatedWindowsLane.test.ts` which fails on any stale path (verified by injecting one). The lane now runs **14 files, up from the 6 it silently ran**, all verified green on `windows-latest`. - **Restoring the lane turned it red, which is the more interesting finding.** Three restored files — `src/ghidra/GhidraProvider.test.ts`, `ghidraClient.test.ts`, `providerProcess.test.ts` — fail with `PrivateRuntimeRootUnavailableError`, because they require Job Object process ownership and private-runtime DACL enforcement that `windows-latest` does not provide by design. They were never running on Windows, so nobody knew. They are excluded with the reason recorded in both `ci.yml` and the guard test, since those claims belong behind the same approval boundary as the real Ghidra Windows workflow. Two of the three were entries this branch added by mistake. - **`.jscpd.json` ignored `**/*.test.ts`**, so the duplication gate running on every PR reported **0 clones** across the test tree. Test files are now included and the threshold is ratcheted to the measured result. - **Local runs used 1 worker**, costing ~140s wall at ~100% CPU on a 10-core host. Now 2, matching CI's proven budget, with PTY scenarios in a serial project. Full local suite **140s → ~89s**. - Regenerated `managed-conformance-manifest.json`; `check:pr` has been red on `main` since #488. ### `test` — structural cleanups - **`max-lines: 500` on test files removed.** Its only effect on tests was to force duplication: it split one binary session suite across nine files with names truncated mid-sentence, and it is why the Windows lane referenced six split-apart `processOwnership*` paths. That suite is merged back to one file (20 composition files → 12), and the `processReactiveRuntime` five-file split is merged to one (92 domain files → 88). - **`createTestTempDirectory` no longer builds scaffolding it deletes.** It called `createTestWorkspace`, which creates `home` and `xdg/{config,cache}`, then deleted two of the three so callers would get an empty root — three `mkdir` and two `rm` per call across ~100 files. - **Repeated literals replaced with builders.** `eventProcessTree.test.ts` spelled out nineteen thirteen-field event literals inline, varying three or four fields each; fixture builders take it from 422 to 214 lines with all nine cases intact. - **Duplicated fixtures hoisted.** Nineteen byte-identical copies of six helpers across the `javascriptApplicationGraph` files now live in `javascriptApplicationGraph.fixture.ts` (−187 lines), and three more `enhancedTools` files import helpers already exported from the shared harness instead of re-declaring them (−41 lines). ### `test` — lock-in removed, coverage kept | Removed | Replaced with | |---|---| | `toolContracts` group sizes + 50 lines of tool names | 4 structural invariants, both mutation-checked | | 6× `safeParse(fixture).success === true` | 5 real rejections + 2 property tests | | 8× `expect.any(...)` placeholders | exact captured results | | 2 assertions on strings absent from their own inputs | removed (could never fail) | | Concurrency fake returning `? [] : []` | real shared state, lost updates observable | | `getTimerCount()` / `getEventListeners()` / exact retry counts | detachment behaviour | | Caps that no longer exist (16 MiB, 10,001 files, 100,001 frames, literal `10_201`) | derived relationships valid at any size | | 2 byte-identical preambles split by `max-lines` | 2 shared fixture modules (~1,000 lines) | Profiling the heaviest files rather than only running them: - `appleApplication.test.ts` **6.4s → 0.28s** (1,001 frameworks × 101 pages → 12/4/4/3, with a derived `SCRIPT × NATIVE` pairing invariant that the literal `10_201` could not check) - `referenceSourceImport.test.ts` **2.3s → 0.2s** ## Verification `npm run check:pr` — typecheck, lint (42 warnings, unchanged from `main`), format, knip, jscpd, docs:check, full suite: **372 files, 1804 passed, 2 skipped**. All 19 CI checks pass, including the repaired Windows lane. No real Hopper or Ghidra toolchain verification was performed. ## Notes for reviewers - **Diff noise is expected** in `tests/boundary/process/`. `it(` → `itWithCaptureCapability(` is 21 characters longer and pushes lines past the print width, so Prettier reindents them. `git diff -w` shows the real change: 891 changed lines are 105 semantic. - **The `cdpBrowserProvider.sensitive_data` change is reasoned, not reproduced.** That scenario asserted outcomes produced *after* the observation window closes while using `observation_ms: 0`; it now uses a real window. I could not reproduce the failure locally (0/24 at 8× concurrency) — it only appeared under full-suite load — so this removes a genuine timing dependency by inspection rather than by confirmed reproduction. Worth watching on CI. - **`processCaptureMultisource` is flaky and CI may show it.** Diagnosed, not masked: the run resolves `target_lost` instead of `passed` because the harness can declare the target lost before draining observations emitted just before exit. The fixture already works around this with a 25 ms settle sleep, which is load-sensitive. Raising the reactive deadlines to 30 s did not help, so the deadlines were reverted. The test is deliberately left strict because it is correctly detecting a real harness drain race. - **Rejection and retention assertions are now exact.** The reactive scenario table asserted only that each of six fixtures failed to parse, so a typo invalidating an unrelated field still passed; each row now names the field it targets and must produce that issue path and code. The large-body replay test asserted the serialized result exceeded five million characters as a proxy for retention, which escaping could satisfy under truncation; it now compares each body byte for byte. Both were verified by mutation: a row valid except for an unknown key now fails, and shaving one byte off the expected body now fails. - **Duplication gate ratcheted.** Test-tree duplication is down to 2.58% of lines from 3.39% when tests were first included, so the threshold moved from 3.2 to 2.7. - **Measurement caveat on the PTY fix.** It passes 18 consecutive runs isolated and under four-way concurrency, against roughly even odds before. A control run with the wait disabled did *not* reproduce the failure, so the machine was not loaded enough for a controlled A/B; the before/after numbers come from separate sessions. - **No real Hopper or Ghidra verification was performed.** Those lanes were not touched and remain manual per `docs/testing.md`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-16 21:13:29 +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#2027

No description provided.

Rows
Columns