[PR #4563] [CLOSED] test(firmware): assert composed publication cleanup failures #6309

Closed
opened 2026-10-31 01:13:28 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4563
Author: @jayinvers
Created: 10/16/2026
Status: ❌ Closed

Base: main ← Head: fix/firmware-cleanup-flake


📝 Commits (2)

  • c21c019 test(firmware): assert composed publication cleanup failures
  • 7d991ca ci: retrigger checks

📊 Changes

1 file changed (+32 additions, -5 deletions)

View changed files

📝 tests/boundary/firmware/firmwareIntegration.test.ts (+32 -5)

📄 Description

Summary

Fixes #4558.

Two Linux firmware integration tests have failed deterministically in every CI run that executed tests/boundary/firmware/firmwareIntegration.test.ts since 2d163c12 (current main included), blocking unrelated PRs (observed on PR #4551). They still asserted the pre-2d163c12 publication cleanup error shape; this PR migrates those assertions to the composed failure contract that commit introduced.

Despite the "flaky" appearance, every run that actually executed the file failed: the green main run 38943231467 only re-ran the Copilot reviewer job (event dynamic) and executed no tests.

Problem and expected behavior

Smallest trigger: on Linux, run the firmware integration file at 4838f444. Both tests fail together:

  • "retains a setup-failed publication tree for provider cleanup retry" — expected error.diagnostics.previous_error (flat) to contain "injected post-acquisition setup failure";
  • "retains the complete firmware result when published output cleanup stays uncertain" — expected result.error to be a ProviderCleanupError instance; received Error: Provider unblob adapter failed during extract_firmware.

Violated invariant (test-side): 2d163c12 changed publishFirmwareExtraction's rollback-failure path to compose the primary failure with the cleanup failure through analysisErrorWithCleanupFailure (diagnostics {primary_error, cleanup_error}), matching docs/tool-design.md "Own the lifecycle" ("…preserving the original failure and completed observations") and the sibling tests/process-global/firmwareFileLifetime.test.ts added in that commit. The two integration tests pinned the superseded bare-ProviderCleanupError shape and were not migrated, so they reject the intended contract.

Change and scope

Test-only change to the two assertions (and removal of the now-unused ProviderCleanupError import):

  • Setup-failure test: assert cleanupIncomplete, the retained fixture.output resource, diagnostics.primary_error carrying the setup failure reason, and diagnostics.cleanup_error carrying the rollback failure reason plus previous_error.
  • Close-uncertainty test: keep the existing projectAnalysisError assertions (cleanup_incomplete with partial_observation), and additionally assert diagnostics.primary_error.code === "cleanup_incomplete" with the retained partial observation and a cleanup_error classification, replacing the superseded instanceof ProviderCleanupError check.

No production code, CLI, MCP contract, or docs change. All other assertions in both tests (retention of the output tree, retry succeeding at fixture.close() with cleanupAttempts === 2, closeFailure.attempts() >= 4, published file contents surviving close) are unchanged and still pass.

Contribution scope: not applicable (test-only fix within existing boundaries).

Contract and boundary impact

  • Semantic owner and earliest changed stage: tests/boundary/firmware/firmwareIntegration.test.ts assertions only; no production stage changed.
  • Contribution scope fit and linked maintainer decision: not applicable (ordinary test fix; no new capability).
  • CLI and MCP/tool-catalog contract: none.
  • Provider, bridge, target-format, or platform compatibility: none (Linux-only test lane).
  • Evidence, artifact, provenance, or reconstruction contract: none; the assertions now pin the composed error's preserved primary/cleanup projections and partial observation.
  • Target selection, authority, upstream behavior, and cleanup details: not applicable.
  • Generated metadata, package, or installation impact: none.

Expected effects: test-only change with no target interaction; no runtime behavior is added or altered, so no effect boxes are checked.

  • 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: the two firmware integration tests updated to the composed-contract assertions described above.
  • Base reproduction or other evidence: executed (see Validation). Pre-fix at upstream main 4838f444 and at 2d163c12, the file fails exactly these two assertions; at 2b7b698d (parent of 2d163c12) it passes 35/35 — establishing 2d163c12 as the commit whose contract change the tests missed, i.e. the updated tests would have failed on that base revision at its introduction.
  • User-visible CLI/MCP output: not applicable (test-only).
  • Remaining proof gaps: firmwareWorkspaceRecovery.test.ts was skipped locally (requires non-root user; WSL ran as root) and the full boundary project/CI was not run locally.

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

All test executions ran on WSL Ubuntu 24.04.5, Node 24.18.0, x64 (the file is Linux-only: it.skipIf(process.platform !== "linux")); npm ci from the locked lockfile. Commands and observed results:

  • npx vitest run tests/boundary/firmware/firmwareIntegration.test.ts --project boundary — pre-fix at 4838f444: 2 failed | 33 passed (35) (2 runs); at 2d163c12: same 2 failures (1 run); at 2b7b698d: 35 passed (1 run); with this change: 35 passed in 6 consecutive runs.
  • npm run test:focused -- tests/boundary/firmware/firmwareIntegration.test.ts — 35 passed. (npm run test:local -- <boundary path> itself executes nothing; it directs built-project paths to test:focused.)
  • npx vitest run tests/boundary/firmware/ --project boundary — 41 passed, 2 skipped (firmwareWorkspaceRecovery.test.ts skips as uid 0; environmental).
  • npx vitest run tests/process-global/firmwareFileLifetime.test.ts --project process-global — 24 passed.
  • npm run check:fast — typecheck + lint successful (also re-run by the pre-push hook on Windows).
  • npx oxfmt --check tests/boundary/firmware/firmwareIntegration.test.ts — clean.

Compatibility, safety, and release

  • Breaking changes or migration steps: none (test-only).
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable; firmware fixtures are synthetic producers (tests/fixtures/firmware/).
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: not applicable (no production change).

Author readiness

Human owner:

  • 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/4563 **Author:** [@jayinvers](https://github.com/jayinvers) **Created:** 10/16/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/firmware-cleanup-flake` --- ### 📝 Commits (2) - [`c21c019`](https://github.com/morluto/rea/commit/c21c0191fce24915e0af9f91d75e95e5cf9d5324) test(firmware): assert composed publication cleanup failures - [`7d991ca`](https://github.com/morluto/rea/commit/7d991cacc28a501e8a9b5c415acd864925b28e6e) ci: retrigger checks ### 📊 Changes **1 file changed** (+32 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `tests/boundary/firmware/firmwareIntegration.test.ts` (+32 -5) </details> ### 📄 Description ## Summary Fixes #4558. Two Linux firmware integration tests have failed deterministically in every CI run that executed `tests/boundary/firmware/firmwareIntegration.test.ts` since `2d163c12` (current `main` included), blocking unrelated PRs (observed on PR #4551). They still asserted the pre-`2d163c12` publication cleanup error shape; this PR migrates those assertions to the composed failure contract that commit introduced. Despite the "flaky" appearance, every run that actually executed the file failed: the green main run [38943231467](https://github.com/morluto/rea/actions/runs/38943231467) only re-ran the Copilot reviewer job (event `dynamic`) and executed no tests. ## Problem and expected behavior Smallest trigger: on Linux, run the firmware integration file at `4838f444`. Both tests fail together: - "retains a setup-failed publication tree for provider cleanup retry" — expected `error.diagnostics.previous_error` (flat) to contain `"injected post-acquisition setup failure"`; - "retains the complete firmware result when published output cleanup stays uncertain" — expected `result.error` to be a `ProviderCleanupError` instance; received `Error: Provider unblob adapter failed during extract_firmware`. Violated invariant (test-side): `2d163c12` changed `publishFirmwareExtraction`'s rollback-failure path to compose the primary failure with the cleanup failure through `analysisErrorWithCleanupFailure` (diagnostics `{primary_error, cleanup_error}`), matching docs/tool-design.md "Own the lifecycle" ("…preserving the original failure and completed observations") and the sibling `tests/process-global/firmwareFileLifetime.test.ts` added in that commit. The two integration tests pinned the superseded bare-`ProviderCleanupError` shape and were not migrated, so they reject the intended contract. ## Change and scope Test-only change to the two assertions (and removal of the now-unused `ProviderCleanupError` import): - Setup-failure test: assert `cleanupIncomplete`, the retained `fixture.output` resource, `diagnostics.primary_error` carrying the setup failure reason, and `diagnostics.cleanup_error` carrying the rollback failure `reason` plus `previous_error`. - Close-uncertainty test: keep the existing `projectAnalysisError` assertions (`cleanup_incomplete` with `partial_observation`), and additionally assert `diagnostics.primary_error.code === "cleanup_incomplete"` with the retained partial observation and a `cleanup_error` classification, replacing the superseded `instanceof ProviderCleanupError` check. No production code, CLI, MCP contract, or docs change. All other assertions in both tests (retention of the output tree, retry succeeding at `fixture.close()` with `cleanupAttempts === 2`, `closeFailure.attempts() >= 4`, published file contents surviving close) are unchanged and still pass. Contribution scope: not applicable (test-only fix within existing boundaries). ## Contract and boundary impact - Semantic owner and earliest changed stage: `tests/boundary/firmware/firmwareIntegration.test.ts` assertions only; no production stage changed. - Contribution scope fit and linked maintainer decision: not applicable (ordinary test fix; no new capability). - CLI and MCP/tool-catalog contract: none. - Provider, bridge, target-format, or platform compatibility: none (Linux-only test lane). - Evidence, artifact, provenance, or reconstruction contract: none; the assertions now pin the composed error's preserved primary/cleanup projections and partial observation. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. Expected effects: test-only change with no target interaction; no runtime behavior is added or altered, so no effect boxes are checked. - [ ] 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: the two firmware integration tests updated to the composed-contract assertions described above. - Base reproduction or other evidence: executed (see Validation). Pre-fix at upstream `main` 4838f444 and at `2d163c12`, the file fails exactly these two assertions; at `2b7b698d` (parent of `2d163c12`) it passes 35/35 — establishing `2d163c12` as the commit whose contract change the tests missed, i.e. the updated tests would have failed on that base revision at its introduction. - User-visible CLI/MCP output: not applicable (test-only). - Remaining proof gaps: `firmwareWorkspaceRecovery.test.ts` was skipped locally (requires non-root user; WSL ran as root) and the full `boundary` project/CI was not run locally. 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 All test executions ran on WSL Ubuntu 24.04.5, Node 24.18.0, x64 (the file is Linux-only: `it.skipIf(process.platform !== "linux")`); `npm ci` from the locked lockfile. Commands and observed results: - `npx vitest run tests/boundary/firmware/firmwareIntegration.test.ts --project boundary` — pre-fix at `4838f444`: **2 failed | 33 passed (35)** (2 runs); at `2d163c12`: same 2 failures (1 run); at `2b7b698d`: **35 passed** (1 run); with this change: **35 passed** in **6 consecutive runs**. - `npm run test:focused -- tests/boundary/firmware/firmwareIntegration.test.ts` — 35 passed. (`npm run test:local -- <boundary path>` itself executes nothing; it directs built-project paths to `test:focused`.) - `npx vitest run tests/boundary/firmware/ --project boundary` — 41 passed, 2 skipped (`firmwareWorkspaceRecovery.test.ts` skips as uid 0; environmental). - `npx vitest run tests/process-global/firmwareFileLifetime.test.ts --project process-global` — 24 passed. - `npm run check:fast` — typecheck + lint successful (also re-run by the pre-push hook on Windows). - `npx oxfmt --check tests/boundary/firmware/firmwareIntegration.test.ts` — clean. ## Compatibility, safety, and release - Breaking changes or migration steps: none (test-only). - Real Hopper/Ghidra, browser, or OS coverage: not applicable; firmware fixtures are synthetic producers (`tests/fixtures/firmware/`). - Package or release metadata impact: none. - Security, privacy, process, or containment review: not applicable (no production change). ## Author readiness Human owner: <!-- @GitHub-handle --> - [ ] 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 - [x] 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. - [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. - [ ] 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 2026-10-31 01:13:28 +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#6309
No description provided.