[PR #5821] [MERGED] test: assert Result reasons instead of bare ok checks #7084

Closed
opened 2026-11-01 21:45:16 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

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

Base: main ← Head: cursor/result-ok-assertion-tightening-553a


📝 Commits (2)

  • 785920c test: assert Result reasons instead of bare ok checks
  • 717a67e merge: update from origin/main

📊 Changes

4 files changed (+87 additions, -24 deletions)

View changed files

📝 src/domain/dosMz.test.ts (+24 -6)
📝 src/domain/rawBinary.test.ts (+4 -1)
📝 src/evm/EvmWorkerLimits.test.ts (+41 -10)
📝 src/ghidra/GhidraSessionValues.test.ts (+18 -7)

📄 Description

Summary

A few module tests rejected malformed inputs by checking only Result.ok. docs/testing.md says a preceding .ok check adds no coverage and that a successful Result should throw the typed error before asserting the value. These cases now name the producer error, or the admitted Ghidra language and compiler.

Merged origin/main at f5410bdc (head 717a67e2). The pre-existing inspect-wasm-artifact schema mismatch is still present on that main and is not changed here.

Problem and expected behavior

Before, any failure kept the test green. A parser that rejected a DOS MZ page count for the wrong reason, or a Ghidra handshake that succeeded without the real-mode language, still passed.

After, each case expects the stable producer string or the admitted target fields:

  • parseRawBinaryEntryPoint rejects every malformed spelling with entry_point must be a hexadecimal address of the form 0x….
  • parseDosMzHeader names the page, header, entry, and relocation failure for each damaged fixture.
  • selectEvmWorkerLimits names the text budget, soft/hard, missing row, duplicate row, or malformed unit.
  • parseGhidraSessionInfo throws on failure, then the success case checks language_id and compiler_spec_id. Both rejections expect Ghidra bridge handshake identity is invalid.

Change and scope

Test-only, plus a merge from origin/main. No production code, workflow, shard, or concurrency change in this branch's own commit. Not applicable for a maintainer scope decision: this does not add a provider or any target execution, injection, mutation, or protection recovery.

Intentionally not included: the inspect-wasm-artifact commandTimeoutMs schema mismatch. It fails on current main and on the previous base 52415975. This PR does not fix it.

Follow the contribution scope.

Contract and boundary impact

  • Semantic owner and earliest changed stage: module tests for parseDosMzHeader, parseRawBinaryEntryPoint, selectEvmWorkerLimits, and parseGhidraSessionInfo. No production stage changes in this branch's own commit. The merge brings existing main changes for SQLite deadlines and Apple plist scalars.
  • Contribution scope fit and linked maintainer decision: ordinary test-oracle fix inside existing parsers. Maintainer scope decision not applicable.
  • CLI and MCP/tool-catalog contract: none from this branch's own commit.
  • Provider, bridge, target-format, or platform compatibility: none. Assertions match the current producer strings.
  • Evidence, artifact, provenance, or reconstruction contract: none.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): none.

No target interaction from this branch's own commit. None of the effect boxes below apply to that commit.

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added or updated: src/domain/dosMz.test.ts, src/domain/rawBinary.test.ts, src/evm/EvmWorkerLimits.test.ts, src/ghidra/GhidraSessionValues.test.ts.
  • Base reproduction or other evidence: on the base revision those cases called expect(result.ok).toBe(false) or toBe(true) and did not read error or value. The updated expectations fail if the producer returns a different reason.
  • User-visible CLI/MCP output (if applicable): none.
  • Remaining proof gaps: page-count and final-page DOS MZ damage share one producer message, as do the two EVM soft-limit rows and the two Ghidra handshake rejections. Distinct inputs stay. No real Ghidra or EVM tool was run; these are in-process parsers. tests/boundary/cli/cliInputSchemaRequired.test.ts still fails on merged main; see validation.

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

On head 717a67e2 after merging origin/main f5410bdc:

  • npm run check:fast — passed (turbo typecheck and lint, 26s, 2 tasks).
  • npm run test:focused -- tests/boundary/cli/cliInputSchemaRequired.test.ts src/domain/rawBinary.test.ts src/domain/dosMz.test.ts src/evm/EvmWorkerLimits.test.ts src/ghidra/GhidraSessionValues.test.ts — 4 files passed (92 tests). cliInputSchemaRequired.test.ts failed one test: require exactly the inputs whose omission the parser rejects, with inspect-wasm-artifact options: advertised [], parser rejects [commandTimeoutMs]. The other five cases in that file passed.

That schema failure is the same one CI reported on 785920cf and on base 52415975. It is not introduced by the assertion edits.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: not run. Ghidra coverage here is the handshake parser, not a Ghidra process.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: not applicable.

Author readiness

Human owner: @morluto

  • 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

Before: expect(result.ok).toBe(false) or toBe(true) accepted any failure or any success. After: the assertion is the producer error string, or target.language_id / target.compiler_spec_id after throwing the typed error. Rationale: docs/testing.md says a preceding .ok check adds no coverage.

  • 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.

No linked issue: this is test-oracle debt found in the Vitest audit, not a product defect. Docs, catalogs, and CLI output are unchanged by this branch's own commit, so those two checklist lines are satisfied by absence of those changes. The wasm schema mismatch remains on main and is left unfixed.

Open in Web Open in Cursor 

🔄 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/5821 **Author:** [@morluto](https://github.com/morluto) **Created:** 10/21/2026 **Status:** ✅ Merged **Merged:** 10/21/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `cursor/result-ok-assertion-tightening-553a` --- ### 📝 Commits (2) - [`785920c`](https://github.com/morluto/rea/commit/785920cf93802891ef93563d16392d97d12c15d9) test: assert Result reasons instead of bare ok checks - [`717a67e`](https://github.com/morluto/rea/commit/717a67e2fc3a59fa1e827abce277dc3a4aecead3) merge: update from origin/main ### 📊 Changes **4 files changed** (+87 additions, -24 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/dosMz.test.ts` (+24 -6) 📝 `src/domain/rawBinary.test.ts` (+4 -1) 📝 `src/evm/EvmWorkerLimits.test.ts` (+41 -10) 📝 `src/ghidra/GhidraSessionValues.test.ts` (+18 -7) </details> ### 📄 Description <!-- CURSOR_AGENT_PR_BODY_BEGIN --> ## Summary A few module tests rejected malformed inputs by checking only `Result.ok`. `docs/testing.md` says a preceding `.ok` check adds no coverage and that a successful `Result` should throw the typed error before asserting the value. These cases now name the producer error, or the admitted Ghidra language and compiler. Merged `origin/main` at `f5410bdc` (head `717a67e2`). The pre-existing `inspect-wasm-artifact` schema mismatch is still present on that main and is not changed here. ## Problem and expected behavior Before, any failure kept the test green. A parser that rejected a DOS MZ page count for the wrong reason, or a Ghidra handshake that succeeded without the real-mode language, still passed. After, each case expects the stable producer string or the admitted target fields: - `parseRawBinaryEntryPoint` rejects every malformed spelling with `entry_point must be a hexadecimal address of the form 0x…`. - `parseDosMzHeader` names the page, header, entry, and relocation failure for each damaged fixture. - `selectEvmWorkerLimits` names the text budget, soft/hard, missing row, duplicate row, or malformed unit. - `parseGhidraSessionInfo` throws on failure, then the success case checks `language_id` and `compiler_spec_id`. Both rejections expect `Ghidra bridge handshake identity is invalid`. ## Change and scope Test-only, plus a merge from `origin/main`. No production code, workflow, shard, or concurrency change in this branch's own commit. Not applicable for a maintainer scope decision: this does not add a provider or any target execution, injection, mutation, or protection recovery. Intentionally not included: the `inspect-wasm-artifact` `commandTimeoutMs` schema mismatch. It fails on current main and on the previous base `52415975`. This PR does not fix it. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). ## Contract and boundary impact - Semantic owner and earliest changed stage: module tests for `parseDosMzHeader`, `parseRawBinaryEntryPoint`, `selectEvmWorkerLimits`, and `parseGhidraSessionInfo`. No production stage changes in this branch's own commit. The merge brings existing main changes for SQLite deadlines and Apple plist scalars. - Contribution scope fit and linked maintainer decision: ordinary test-oracle fix inside existing parsers. Maintainer scope decision not applicable. - CLI and MCP/tool-catalog contract: none from this branch's own commit. - Provider, bridge, target-format, or platform compatibility: none. Assertions match the current producer strings. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): none. No target interaction from this branch's own commit. None of the effect boxes below apply to that commit. - [ ] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: `src/domain/dosMz.test.ts`, `src/domain/rawBinary.test.ts`, `src/evm/EvmWorkerLimits.test.ts`, `src/ghidra/GhidraSessionValues.test.ts`. - Base reproduction or other evidence: on the base revision those cases called `expect(result.ok).toBe(false)` or `toBe(true)` and did not read `error` or `value`. The updated expectations fail if the producer returns a different reason. - User-visible CLI/MCP output (if applicable): none. - Remaining proof gaps: page-count and final-page DOS MZ damage share one producer message, as do the two EVM soft-limit rows and the two Ghidra handshake rejections. Distinct inputs stay. No real Ghidra or EVM tool was run; these are in-process parsers. `tests/boundary/cli/cliInputSchemaRequired.test.ts` still fails on merged main; see validation. 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 On head `717a67e2` after merging `origin/main` `f5410bdc`: - `npm run check:fast` — passed (turbo typecheck and lint, 26s, 2 tasks). - `npm run test:focused -- tests/boundary/cli/cliInputSchemaRequired.test.ts src/domain/rawBinary.test.ts src/domain/dosMz.test.ts src/evm/EvmWorkerLimits.test.ts src/ghidra/GhidraSessionValues.test.ts` — 4 files passed (92 tests). `cliInputSchemaRequired.test.ts` failed one test: `require exactly the inputs whose omission the parser rejects`, with `inspect-wasm-artifact options: advertised [], parser rejects [commandTimeoutMs]`. The other five cases in that file passed. That schema failure is the same one CI reported on `785920cf` and on base `52415975`. It is not introduced by the assertion edits. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not run. Ghidra coverage here is the handshake parser, not a Ghidra process. - Package or release metadata impact: none. - Security, privacy, process, or containment review: not applicable. ## Author readiness Human owner: @morluto - [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 Before: `expect(result.ok).toBe(false)` or `toBe(true)` accepted any failure or any success. After: the assertion is the producer error string, or `target.language_id` / `target.compiler_spec_id` after throwing the typed error. Rationale: `docs/testing.md` says a preceding `.ok` check adds no coverage. - [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. No linked issue: this is test-oracle debt found in the Vitest audit, not a product defect. Docs, catalogs, and CLI output are unchanged by this branch's own commit, so those two checklist lines are satisfied by absence of those changes. The wasm schema mismatch remains on main and is left unfixed. <!-- CURSOR_AGENT_PR_BODY_END --> <div><a href="https://cursor.com/agents/bc-a548061f-fe96-589c-a908-7523f1b4553a?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a>&nbsp;<a href="https://cursor.com/background-agent?bcId=bc-a548061f-fe96-589c-a908-7523f1b4553a&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a>&nbsp;</div> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-11-01 21:45:16 +00:00
Sign in to join this conversation.
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#7084
No description provided.