morluto/rea

Fork 0

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

[PR #5230] fix(artifacts): keep readable entries of a directory with unreadable entries #6825

New issue

Open

opened 2026-11-01 21:43:19 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:43:19 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5230
Author: @rodricksz4h5
Created: 10/19/2026
Status: 🔄 Open

Base: main ← Head: fix/directory-unreadable-entries

📝 Commits (1)

6cbe91b fix(artifacts): keep readable entries of a directory with unreadable entries

📊 Changes

6 files changed (+206 additions, -9 deletions)

View changed files

📝 src/application/javascript/JavaScriptArtifactGraphBuilder.ts (+5 -0)
📝 src/artifacts/ArtifactReader.ts (+4 -1)
📝 src/artifacts/DirectoryArtifactReader.ts (+32 -5)
📝 src/artifacts/inventory/scanReader.ts (+24 -2)
📝 src/artifacts/javascript/JavaScriptArtifactFiles.ts (+5 -1)
➕ tests/boundary/filesystem/artifactDirectoryUnreadable.test.ts (+136 -0)

📄 Description

Summary

Fixes #5229

rea analyze <directory> failed the whole analysis when one file or subdirectory was unreadable by the current user. Inventory now keeps what it could read, as the filesystem snapshot (#2545) and reference imports (#4013) do, and the application graph reports the inventory's gaps instead of claiming complete.

Problem and expected behavior

DirectoryArtifactReader throws for any opendir/open failure, and scanReader tolerates an open failure only for an unavailable unpacked ASAR entry, so one EACCES ends the inventory. graphCoverage never reads snapshot.coverage, so entries inventory already omits (FIFOs, sockets) leave the graph complete. Expected: readable files are analyzed, an unreadable file is hash_status: "unavailable", an unreadable subdirectory is a traversal omission, coverage is not complete, and an unreadable root still fails.

Change and scope

src/artifacts/ArtifactReader.ts: traversal option omitUnreadableDirectories and omission kind unreadable-directory.
src/artifacts/DirectoryArtifactReader.ts: with the option set, a child directory whose opendir fails with EACCES/EPERM is reported through onOmission instead of failing. Off by default, so callers that fail closed on purpose (Apktool's decoded-workspace inspection) are unchanged. The root and every other error stay terminal.
src/artifacts/inventory/scanReader.ts: inventory opts in, and a directory file whose open is denied keeps its occurrence with hash_status: "unavailable" and a limitation, as an unavailable unpacked ASAR entry does.
src/artifacts/javascript/JavaScriptArtifactFiles.ts: the source read re-walks the directory with the same option. A relevant inventoried file that became unreadable since still fails the existing "disappeared after inventory" check.
src/application/javascript/JavaScriptArtifactGraphBuilder.ts: an inventory with coverage unknown makes the graph partial and carries the inventory limitations, as integrity_contradictions already do. This also corrects the existing complete report for inventory-omitted FIFOs, sockets and devices.

Design choice, flagged: skipping unreadable directories is opt-in per consumer rather than reader-wide, following the existing split (Apktool fails closed; snapshots and reference imports keep partial results).

Follow the contribution scope.
Scope decision: not applicable (ordinary fix within existing boundaries).

Contract and boundary impact

Semantic owner and earliest changed stage: directory traversal and artifact inventory.
Contribution scope fit and linked maintainer decision: not applicable.
CLI and MCP/tool-catalog contract: no schema change; one new ArtifactTraversalOmission kind value inside existing limitation text; graph coverage becomes partial where inventory coverage is unknown.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: unreadable entries become explicit unavailable occurrences / omissions instead of a failure.
Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable.
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Expected effects:

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: tests/boundary/filesystem/artifactDirectoryUnreadable.test.ts (skipped on Windows and as root, where permission bits do not deny):
- inventory keeps readable entries, an unreadable file is unavailable, an unreadable subdirectory is omitted, coverage unknown;
- an unreadable root still fails;
- DirectoryArtifactReader still fails by default and reports the omission when opted in;
- application analysis succeeds with graph coverage partial and the omission limitation.
  With the five source files reverted to main, tests 1, 3 and 4 fail; test 2 passes.
Base reproduction (executed), rea analyze <dir> --format json, REA built from source on Linux as a non-root user:

fixture	main `39275420`	this PR
`chmod 000 lib/key.pem secret`	`artifact_operation_failed`, `io`, `Could not open entry at .../lib/key.pem (EACCES)`	succeeds; graph `partial`; `Artifact traversal omitted unreadable-directory at "secret".`
`mkfifo lib/pipe`	graph `complete`, pipe not mentioned	graph `partial`; `Artifact traversal omitted fifo at "lib/pipe".`
only a relevant `lib/b.js` unreadable	fails	graph `partial` (existing unavailable-relevant-file path)

User-visible CLI/MCP output (if applicable): above.
Remaining proof gaps: MCP not run separately (same service); Windows ACL denial not exercised (tests skip on Windows); ASAR roots unaffected (option is directory-only).

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

Linux, node:24.18.0-bookworm, non-root user, npm ci and npm run build:cached:

npx vitest run tests/boundary/filesystem/artifactDirectoryUnreadable.test.ts — 4 passed
npx vitest run src/artifacts src/application tests/boundary/filesystem src/apktool tests/boundary/android tests/boundary/mcp — 5682 passed, 9 failed in 9 files, all failing the same way on main in this container: ciStaticArtifacts (3), directAnalysisSnapshotReplay, documentationFacts, productCatalog, setupSkill, zipDetection, plus contractPresentation and toolSchemaValidity (fail to load); processSnapshotCleanupMcp passed on rerun
npx vitest run src/application/javascript src/artifacts/javascript src/domain/javascript tests/acceptance/applications tests/boundary/filesystem/javascript-applications — 3284 passed, 1 failed: hopperPublisherRejectionCli (Hopper launcher in a Linux container)
npm run check:fast — pass; oxfmt --check on the changed files — pass; npm run verify:test-discovery — all 1638 test files once

Compatibility, safety, and release

Breaking changes or migration steps: none; analyses that previously failed now return partial results, and graphs over inventory-omitted special files now report partial.
Real Hopper/Ghidra, browser, or OS coverage: Linux only.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new reads; denied entries are only named, never opened again.

Author readiness

Human owner: @rodricksz4h5

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 output 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/5230 **Author:** [@rodricksz4h5](https://github.com/rodricksz4h5) **Created:** 10/19/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/directory-unreadable-entries` --- ### 📝 Commits (1) - [`6cbe91b`](https://github.com/morluto/rea/commit/6cbe91b907cec910e2b064a327367a0f085af748) fix(artifacts): keep readable entries of a directory with unreadable entries ### 📊 Changes **6 files changed** (+206 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `src/application/javascript/JavaScriptArtifactGraphBuilder.ts` (+5 -0) 📝 `src/artifacts/ArtifactReader.ts` (+4 -1) 📝 `src/artifacts/DirectoryArtifactReader.ts` (+32 -5) 📝 `src/artifacts/inventory/scanReader.ts` (+24 -2) 📝 `src/artifacts/javascript/JavaScriptArtifactFiles.ts` (+5 -1) ➕ `tests/boundary/filesystem/artifactDirectoryUnreadable.test.ts` (+136 -0) </details> ### 📄 Description ## Summary Fixes #5229 `rea analyze <directory>` failed the whole analysis when one file or subdirectory was unreadable by the current user. Inventory now keeps what it could read, as the filesystem snapshot (#2545) and reference imports (#4013) do, and the application graph reports the inventory's gaps instead of claiming `complete`. ## Problem and expected behavior `DirectoryArtifactReader` throws for any `opendir`/`open` failure, and `scanReader` tolerates an `open` failure only for an unavailable unpacked ASAR entry, so one `EACCES` ends the inventory. `graphCoverage` never reads `snapshot.coverage`, so entries inventory already omits (FIFOs, sockets) leave the graph `complete`. Expected: readable files are analyzed, an unreadable file is `hash_status: "unavailable"`, an unreadable subdirectory is a traversal omission, coverage is not `complete`, and an unreadable root still fails. ## Change and scope - `src/artifacts/ArtifactReader.ts`: traversal option `omitUnreadableDirectories` and omission kind `unreadable-directory`. - `src/artifacts/DirectoryArtifactReader.ts`: with the option set, a child directory whose `opendir` fails with `EACCES`/`EPERM` is reported through `onOmission` instead of failing. Off by default, so callers that fail closed on purpose (Apktool's decoded-workspace inspection) are unchanged. The root and every other error stay terminal. - `src/artifacts/inventory/scanReader.ts`: inventory opts in, and a directory file whose `open` is denied keeps its occurrence with `hash_status: "unavailable"` and a limitation, as an unavailable unpacked ASAR entry does. - `src/artifacts/javascript/JavaScriptArtifactFiles.ts`: the source read re-walks the directory with the same option. A relevant inventoried file that became unreadable since still fails the existing "disappeared after inventory" check. - `src/application/javascript/JavaScriptArtifactGraphBuilder.ts`: an inventory with coverage `unknown` makes the graph `partial` and carries the inventory limitations, as `integrity_contradictions` already do. This also corrects the existing `complete` report for inventory-omitted FIFOs, sockets and devices. Design choice, flagged: skipping unreadable directories is opt-in per consumer rather than reader-wide, following the existing split (Apktool fails closed; snapshots and reference imports keep partial results). Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Scope decision: not applicable (ordinary fix within existing boundaries). ## Contract and boundary impact - Semantic owner and earliest changed stage: directory traversal and artifact inventory. - Contribution scope fit and linked maintainer decision: not applicable. - CLI and MCP/tool-catalog contract: no schema change; one new `ArtifactTraversalOmission` kind value inside existing limitation text; graph coverage becomes `partial` where inventory coverage is `unknown`. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: unreadable entries become explicit `unavailable` occurrences / omissions instead of a failure. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Expected effects: - [x] 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: `tests/boundary/filesystem/artifactDirectoryUnreadable.test.ts` (skipped on Windows and as root, where permission bits do not deny): - inventory keeps readable entries, an unreadable file is `unavailable`, an unreadable subdirectory is omitted, coverage `unknown`; - an unreadable root still fails; - `DirectoryArtifactReader` still fails by default and reports the omission when opted in; - application analysis succeeds with graph coverage `partial` and the omission limitation. With the five source files reverted to main, tests 1, 3 and 4 fail; test 2 passes. - Base reproduction (executed), `rea analyze <dir> --format json`, REA built from source on Linux as a non-root user: | fixture | main 39275420 | this PR | | --- | --- | --- | | `chmod 000 lib/key.pem secret` | `artifact_operation_failed`, `io`, `Could not open entry at .../lib/key.pem (EACCES)` | succeeds; graph `partial`; `Artifact traversal omitted unreadable-directory at "secret".` | | `mkfifo lib/pipe` | graph `complete`, pipe not mentioned | graph `partial`; `Artifact traversal omitted fifo at "lib/pipe".` | | only a relevant `lib/b.js` unreadable | fails | graph `partial` (existing unavailable-relevant-file path) | - User-visible CLI/MCP output (if applicable): above. - Remaining proof gaps: MCP not run separately (same service); Windows ACL denial not exercised (tests skip on Windows); ASAR roots unaffected (option is directory-only). 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 Linux, `node:24.18.0-bookworm`, non-root user, `npm ci` and `npm run build:cached`: - `npx vitest run tests/boundary/filesystem/artifactDirectoryUnreadable.test.ts` — 4 passed - `npx vitest run src/artifacts src/application tests/boundary/filesystem src/apktool tests/boundary/android tests/boundary/mcp` — 5682 passed, 9 failed in 9 files, all failing the same way on main in this container: `ciStaticArtifacts` (3), `directAnalysisSnapshotReplay`, `documentationFacts`, `productCatalog`, `setupSkill`, `zipDetection`, plus `contractPresentation` and `toolSchemaValidity` (fail to load); `processSnapshotCleanupMcp` passed on rerun - `npx vitest run src/application/javascript src/artifacts/javascript src/domain/javascript tests/acceptance/applications tests/boundary/filesystem/javascript-applications` — 3284 passed, 1 failed: `hopperPublisherRejectionCli` (Hopper launcher in a Linux container) - `npm run check:fast` — pass; `oxfmt --check` on the changed files — pass; `npm run verify:test-discovery` — all 1638 test files once ## Compatibility, safety, and release - Breaking changes or migration steps: none; analyses that previously failed now return partial results, and graphs over inventory-omitted special files now report `partial`. - Real Hopper/Ghidra, browser, or OS coverage: Linux only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new reads; denied entries are only named, never opened again. ## Author readiness Human owner: @rodricksz4h5 - [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 output 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:43:19 +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#6825

No description provided.

Rows
Columns