morluto/rea

Fork 0

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

[PR #753] [MERGED] fix(artifacts): make extraction output tree work on Windows #2216

New issue

Closed

opened 2026-10-16 21:15:48 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-16 21:15:48 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/753
Author: @DeryFerd
Created: 10/6/2026
Status: ✅ Merged
Merged: 10/6/2026
Merged by: @N0zoM1z0

Base: main ← Head: fix/safe-output-tree-windows

📝 Commits (1)

1ad743e fix(artifacts): make extraction output tree work on Windows

📊 Changes

3 files changed (+47 additions, -12 deletions)

View changed files

📝 .github/workflows/ci.yml (+2 -0)
📝 src/artifacts/SafeOutputTree.ts (+31 -12)
📝 tests/boundary/filesystem/safeOutputTree.test.ts (+14 -0)

📄 Description

Summary

extract_artifact fails on every Windows host. SafeOutputTree.create() opens the freshly created output directory and calls chmod(0o700) on the directory handle, and commit() calls sync() on a directory handle. Both are POSIX-only operations, and Windows rejects each with EPERM: operation not permitted. The provider catches the failure and reports it as a generic io reason, so the caller sees artifact_operation_failed with no output directory and no hint about the cause. This change skips the two directory-only operations on Windows, where they have no effect to begin with, and adds a regression test plus Windows CI coverage so the behavior cannot silently break again.

Problem and expected behavior

The smallest trigger is a single extraction on Windows. Given any extractable archive target (zip, ipa, apk, msix, appx, or a macho slice set), ArtifactProvider.execute("extract_artifact", { output_root }) calls extractArtifact, which calls SafeOutputTree.create(output_root). On Windows that call throws EPERM: operation not permitted, fchmod before any file is written, extractArtifact rolls back the unsealed tree, and the provider returns { ok: false, reason: "io" }. The tool is advertised as available on Windows, and the operation is a first-class workflow, so the effect is that a supported capability is entirely broken rather than partially degraded.

The violated invariant is narrower than it looks. The directory chmod was meant to force 0o700 on the new output root, but mkdir(canonicalOutput, { mode: 0o700 }) on the line above already applies the requested mode, so the handle chmod only re-asserts a mode that is already set. The directory sync in commit() was meant to make the tree durable, but each file's bytes are already flushed and read back inside write(), and Windows has no directory fsync at all. Neither operation contributes anything on Windows, and both are the reason the whole operation aborts there.

The expected behavior is that extraction produces the same files with the same digests on Windows as it does on Linux and macOS, and that the POSIX-only durability steps remain in place on the hosts that support them. That is what this change does.

Change and scope

SafeOutputTree now takes the platform as a constructor value with a process.platform default, matching the seam already used elsewhere in the artifact layer (for example ArtifactProvider(platform = process.platform)). create() only opens the directory handle and applies 0o700 when the platform is not win32; on Windows it relies on the mkdir mode, which is what actually sets the mode on that host. commit() marks the tree published and returns immediately on Windows, because there is no directory fsync there and the file-level durability guarantee is already established in write(). The POSIX path is byte-for-byte unchanged, including the descriptor close on the failure path and the parent/output sync ordering.

Two regression surfaces are added. tests/boundary/filesystem/safeOutputTree.test.ts gains a case that builds and commits a tree through the win32 seam and asserts the written file is present with the expected content, which is exactly the path that used to throw. The curated Windows CI lane in .github/workflows/ci.yml now runs safeOutputTree.test.ts and artifactProvider.archiveSafety.test.ts, so the Windows-only behavior is exercised on a Windows runner instead of only on developer machines.

Intentionally not included: no change to extraction containment, path normalization, digest verification, rollback, or the public tool contract; no new capability flag or approval step; and no attempt to give Windows a directory-level durability guarantee it does not have. The Windows behavior is now "the same result, minus the two no-op POSIX steps", not "a reduced guarantee that used to exist".

Contract and boundary impact

Semantic owner and earliest changed stage: artifact extraction (src/artifacts/SafeOutputTree.ts), reached from src/application/ArtifactExtraction.ts and src/firmware/FirmwarePublication.ts.
CLI and MCP/tool-catalog contract: none. extract_artifact input, output, and result schemas are unchanged.
Provider, bridge, target-format, or platform compatibility: Windows x64 now produces output for extraction instead of failing; Linux and macOS behavior is unchanged.
Evidence, artifact, provenance, or reconstruction contract: none. Extraction results, digests, and cleanup reports keep their existing shape.
Process execution, authorization, cleanup, or containment impact: none. Rollback, O_EXCL | O_NOFOLLOW, symlink-resistant parent creation, and readback verification are untouched.
Generated metadata (docs/product-catalog.json), package, or installation impact: none.

Evidence and regression coverage

Tests added or updated: a new Windows-path case in tests/boundary/filesystem/safeOutputTree.test.ts, and the Windows CI lane now includes safeOutputTree.test.ts and artifactProvider.archiveSafety.test.ts.
Base reproduction or other evidence: on unchanged main, safeOutputTree.test.ts fails three cases with EPERM: operation not permitted, fchmod, and artifactProvider.archiveSafety.test.ts fails the two msix/appxbundle extraction cases with expected false to be true. The new case fails on base with the same fchmod error and passes after the fix, which is the red/green check that the test actually covers the change.
User-visible CLI/MCP output (if applicable): not applicable; the fix removes a failure rather than changing a result shape.
Remaining proof gaps: the full deterministic suite could not be completed in the authoring environment because the test runner exhausts the sandbox heap, so verification here is limited to the focused files listed below; hosted CI covers the complete gate. No real Hopper, Ghidra, or browser workflow is involved in this path.

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

npx tsc --noEmit — passed, exit 0.
npm run build:cached — passed, 1 task successful.
npx vitest run tests/boundary/filesystem/safeOutputTree.test.ts tests/boundary/providers/native/artifactProvider.archiveSafety.test.ts tests/boundary/providers/native/artifactProvider.extraction.test.ts tests/process-global/curatedWindowsLane.test.ts — 10 passed, 1 skipped (4 files).
npx oxlint src/artifacts/SafeOutputTree.ts tests/boundary/filesystem/safeOutputTree.test.ts — 0 warnings, 0 errors.
npx oxfmt --check on the changed files — clean (LF, committed form).

The focused runs above were executed on Windows x64 with Node.js 24, which is the affected host. The Windows CI lane will re-run the two boundary files on a hosted Windows runner.

Compatibility, safety, and release

Breaking changes or migration steps: none. The change is additive and scoped to a host where the previous code path always failed.
Real Hopper/Ghidra, browser, or OS coverage: no real-provider claim is made. The change is in a filesystem helper shared by artifact extraction and firmware publication; firmware publication remains Linux-only.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no containment or authorization behavior changes. The Windows path still creates the output root exclusively with mkdir and still writes each file with O_CREAT | O_EXCL | O_WRONLY | O_NOFOLLOW under a symlink-checked parent. The only thing skipped on Windows is a redundant mode re-assertion and a directory sync that Windows does not implement.

Review checklist

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. There is no linked issue: this is a reproduced platform gap with a deterministic reproduction and a regression test, so it does not need separate triage.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed. No generated metadata changes are required.
User-visible CLI/MCP changes include representative output. Not applicable; this fixes a failure rather than changing 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/753 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/6/2026 **Status:** ✅ Merged **Merged:** 10/6/2026 **Merged by:** [@N0zoM1z0](https://github.com/N0zoM1z0) **Base:** `main` ← **Head:** `fix/safe-output-tree-windows` --- ### 📝 Commits (1) - [`1ad743e`](https://github.com/morluto/rea/commit/1ad743e741765b0701a771ccc89f047535773678) fix(artifacts): make extraction output tree work on Windows ### 📊 Changes **3 files changed** (+47 additions, -12 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/ci.yml` (+2 -0) 📝 `src/artifacts/SafeOutputTree.ts` (+31 -12) 📝 `tests/boundary/filesystem/safeOutputTree.test.ts` (+14 -0) </details> ### 📄 Description ## Summary `extract_artifact` fails on every Windows host. `SafeOutputTree.create()` opens the freshly created output directory and calls `chmod(0o700)` on the directory handle, and `commit()` calls `sync()` on a directory handle. Both are POSIX-only operations, and Windows rejects each with `EPERM: operation not permitted`. The provider catches the failure and reports it as a generic `io` reason, so the caller sees `artifact_operation_failed` with no output directory and no hint about the cause. This change skips the two directory-only operations on Windows, where they have no effect to begin with, and adds a regression test plus Windows CI coverage so the behavior cannot silently break again. ## Problem and expected behavior The smallest trigger is a single extraction on Windows. Given any extractable archive target (`zip`, `ipa`, `apk`, `msix`, `appx`, or a `macho` slice set), `ArtifactProvider.execute("extract_artifact", { output_root })` calls `extractArtifact`, which calls `SafeOutputTree.create(output_root)`. On Windows that call throws `EPERM: operation not permitted, fchmod` before any file is written, `extractArtifact` rolls back the unsealed tree, and the provider returns `{ ok: false, reason: "io" }`. The tool is advertised as available on Windows, and the operation is a first-class workflow, so the effect is that a supported capability is entirely broken rather than partially degraded. The violated invariant is narrower than it looks. The directory `chmod` was meant to force `0o700` on the new output root, but `mkdir(canonicalOutput, { mode: 0o700 })` on the line above already applies the requested mode, so the handle `chmod` only re-asserts a mode that is already set. The directory `sync` in `commit()` was meant to make the tree durable, but each file's bytes are already flushed and read back inside `write()`, and Windows has no directory `fsync` at all. Neither operation contributes anything on Windows, and both are the reason the whole operation aborts there. The expected behavior is that extraction produces the same files with the same digests on Windows as it does on Linux and macOS, and that the POSIX-only durability steps remain in place on the hosts that support them. That is what this change does. ## Change and scope `SafeOutputTree` now takes the platform as a constructor value with a `process.platform` default, matching the seam already used elsewhere in the artifact layer (for example `ArtifactProvider(platform = process.platform)`). `create()` only opens the directory handle and applies `0o700` when the platform is not `win32`; on Windows it relies on the `mkdir` mode, which is what actually sets the mode on that host. `commit()` marks the tree published and returns immediately on Windows, because there is no directory `fsync` there and the file-level durability guarantee is already established in `write()`. The POSIX path is byte-for-byte unchanged, including the descriptor close on the failure path and the parent/output sync ordering. Two regression surfaces are added. `tests/boundary/filesystem/safeOutputTree.test.ts` gains a case that builds and commits a tree through the `win32` seam and asserts the written file is present with the expected content, which is exactly the path that used to throw. The curated Windows CI lane in `.github/workflows/ci.yml` now runs `safeOutputTree.test.ts` and `artifactProvider.archiveSafety.test.ts`, so the Windows-only behavior is exercised on a Windows runner instead of only on developer machines. Intentionally not included: no change to extraction containment, path normalization, digest verification, rollback, or the public tool contract; no new capability flag or approval step; and no attempt to give Windows a directory-level durability guarantee it does not have. The Windows behavior is now "the same result, minus the two no-op POSIX steps", not "a reduced guarantee that used to exist". ## Contract and boundary impact - Semantic owner and earliest changed stage: artifact extraction (`src/artifacts/SafeOutputTree.ts`), reached from `src/application/ArtifactExtraction.ts` and `src/firmware/FirmwarePublication.ts`. - CLI and MCP/tool-catalog contract: none. `extract_artifact` input, output, and result schemas are unchanged. - Provider, bridge, target-format, or platform compatibility: Windows x64 now produces output for extraction instead of failing; Linux and macOS behavior is unchanged. - Evidence, artifact, provenance, or reconstruction contract: none. Extraction results, digests, and cleanup reports keep their existing shape. - Process execution, authorization, cleanup, or containment impact: none. Rollback, `O_EXCL | O_NOFOLLOW`, symlink-resistant parent creation, and readback verification are untouched. - Generated metadata (`docs/product-catalog.json`), package, or installation impact: none. ## Evidence and regression coverage - Tests added or updated: a new Windows-path case in `tests/boundary/filesystem/safeOutputTree.test.ts`, and the Windows CI lane now includes `safeOutputTree.test.ts` and `artifactProvider.archiveSafety.test.ts`. - Base reproduction or other evidence: on unchanged `main`, `safeOutputTree.test.ts` fails three cases with `EPERM: operation not permitted, fchmod`, and `artifactProvider.archiveSafety.test.ts` fails the two `msix`/`appxbundle` extraction cases with `expected false to be true`. The new case fails on base with the same `fchmod` error and passes after the fix, which is the red/green check that the test actually covers the change. - User-visible CLI/MCP output (if applicable): not applicable; the fix removes a failure rather than changing a result shape. - Remaining proof gaps: the full deterministic suite could not be completed in the authoring environment because the test runner exhausts the sandbox heap, so verification here is limited to the focused files listed below; hosted CI covers the complete gate. No real Hopper, Ghidra, or browser workflow is involved in this path. 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 - `npx tsc --noEmit` — passed, exit 0. - `npm run build:cached` — passed, 1 task successful. - `npx vitest run tests/boundary/filesystem/safeOutputTree.test.ts tests/boundary/providers/native/artifactProvider.archiveSafety.test.ts tests/boundary/providers/native/artifactProvider.extraction.test.ts tests/process-global/curatedWindowsLane.test.ts` — 10 passed, 1 skipped (4 files). - `npx oxlint src/artifacts/SafeOutputTree.ts tests/boundary/filesystem/safeOutputTree.test.ts` — 0 warnings, 0 errors. - `npx oxfmt --check` on the changed files — clean (LF, committed form). The focused runs above were executed on Windows x64 with Node.js 24, which is the affected host. The Windows CI lane will re-run the two boundary files on a hosted Windows runner. ## Compatibility, safety, and release - Breaking changes or migration steps: none. The change is additive and scoped to a host where the previous code path always failed. - Real Hopper/Ghidra, browser, or OS coverage: no real-provider claim is made. The change is in a filesystem helper shared by artifact extraction and firmware publication; firmware publication remains Linux-only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no containment or authorization behavior changes. The Windows path still creates the output root exclusively with `mkdir` and still writes each file with `O_CREAT | O_EXCL | O_WRONLY | O_NOFOLLOW` under a symlink-checked parent. The only thing skipped on Windows is a redundant mode re-assertion and a directory sync that Windows does not implement. ## Review checklist - [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. There is no linked issue: this is a reproduced platform gap with a deterministic reproduction and a regression test, so it does not need separate triage. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. No generated metadata changes are required. - [x] User-visible CLI/MCP changes include representative output. Not applicable; this fixes a failure rather than changing 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-16 21:15:48 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-10-31 01:03:47 +00:00

[PR #2216] [MERGED] test(native): verify real PE and DOS entry views #5090

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

No description provided.

Rows
Columns