[PR #847] [MERGED] fix(artifacts): report which path constraint failed #2285

New issue

Closed

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

bsakaguchi commented

2026-10-16 21:16:45 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/847
Author: @kaoru0822-kitauji
Created: 10/7/2026
Status: ✅ Merged
Merged: 10/7/2026
Merged by: @morluto

Base: main ← Head: fix/artifact-failure-detail

📝 Commits (1)

3a9738a fix(artifacts): report which path constraint failed

📊 Changes

7 files changed (+58 additions, -7 deletions)

View changed files

📝 src/application/JavaScriptApplicationService.ts (+6 -1)
📝 src/artifacts/ArtifactPaths.ts (+6 -3)
📝 src/artifacts/ArtifactProvider.ts (+6 -1)
📝 src/domain/analysisErrorProjection.ts (+1 -0)
📝 src/domain/artifactOperationError.ts (+2 -0)
📝 tests/boundary/providers/native/artifactProvider.archiveSafety.test.ts (+2 -2)
➕ tests/boundary/providers/native/artifactProvider.failureDetail.test.ts (+35 -0)

📄 Description

Summary

When an archive fails REA's path-safety checks, the caller-visible error says to inspect a path that it never reports:

{
  "code": "artifact_operation_failed",
  "message": "Artifact contains an unsafe or conflicting internal path. Inspect the reported path and correct the artifact before retrying.",
  "details": { "operation": "analyze_javascript_application", "reason": "path" }
}

The specific failure, for example Artifact path collision: a.js, exists as the ArtifactReaderFailure message. However, ArtifactProvider.translateFailure and JavaScriptApplicationService drop it when they build the ArtifactOperationError. With 87 throw sites, the reason alone (path, format, limit, …) can't tell an analyst which entry or constraint failed. AGENTS.md: "Diagnostics should identify the failed constraint and the target… generic catches must not erase actionable validation details."

Real archives can trigger this. Case-fold collisions such as Main.js + main.js are legal in ZIP/ASAR files built on case-sensitive filesystems, and REA deliberately rejects them (fail-closed). This PR leaves that policy unchanged; it only makes the rejection explain itself.

Fix

ArtifactOperationError gets an optional detail, which both conversion sites fill from ArtifactReaderFailure.message. The error projection emits it as details.detail. export_web_scripts already preserved its message this way.
The path messages now identify the target:
- Case-fold collision: Artifact path collision: main.js differs only in case from Main.js (both spellings).
- Artifact path is absolute or unsafe: "a\\b.js" and Artifact path is not normalized: "../escape". The input is JSON-escaped because such names can contain NUL or control characters.

After the change:

"details": { "operation": "analyze_javascript_application", "reason": "path",
             "detail": "Artifact path collision: a.js differs only in case from A.js" }

Contract / behavior changes

artifact_operation_failed errors gain an optional details.detail string. The published error contract already defines that variant's details as an open record, so docs:check passes with no regeneration.
The integrity-mismatch variant, which has strict details, is unchanged.
Three internal failure messages are more specific; the one existing test asserting them is updated.

Verification

New tests/boundary/providers/native/artifactProvider.failureDetail.test.ts uses real ZIPs (Main.js + main.js, and a\b.js) through the public ArtifactProvider and projectAnalysisError. Both cases fail on main and pass here.
Built CLI: analyze-javascript-application and inspect-artifact on crafted ASARs (case collision, backslash, NUL, and / in a name) now report a detail for each.
All artifact-provider, filesystem, artifact MCP, and CLI-output boundary suites: 66 files / 405 tests passed (1 skipped).
npm run check:changed (33 files / 333 tests), npm run check:fast, and npm run docs:check passed.

No Hopper/Ghidra/IDA involvement.

🤖 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/847 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/7/2026 **Status:** ✅ Merged **Merged:** 10/7/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/artifact-failure-detail` --- ### 📝 Commits (1) - [`3a9738a`](https://github.com/morluto/rea/commit/3a9738a61489731d31e7408fb686952e0fcf1f0e) fix(artifacts): report which path constraint failed ### 📊 Changes **7 files changed** (+58 additions, -7 deletions) <details> <summary>View changed files</summary> 📝 `src/application/JavaScriptApplicationService.ts` (+6 -1) 📝 `src/artifacts/ArtifactPaths.ts` (+6 -3) 📝 `src/artifacts/ArtifactProvider.ts` (+6 -1) 📝 `src/domain/analysisErrorProjection.ts` (+1 -0) 📝 `src/domain/artifactOperationError.ts` (+2 -0) 📝 `tests/boundary/providers/native/artifactProvider.archiveSafety.test.ts` (+2 -2) ➕ `tests/boundary/providers/native/artifactProvider.failureDetail.test.ts` (+35 -0) </details> ### 📄 Description ## Summary When an archive fails REA's path-safety checks, the caller-visible error says to inspect a path that it never reports: ```json { "code": "artifact_operation_failed", "message": "Artifact contains an unsafe or conflicting internal path. Inspect the reported path and correct the artifact before retrying.", "details": { "operation": "analyze_javascript_application", "reason": "path" } } ``` The specific failure, for example `Artifact path collision: a.js`, exists as the `ArtifactReaderFailure` message. However, `ArtifactProvider.translateFailure` and `JavaScriptApplicationService` drop it when they build the `ArtifactOperationError`. With 87 throw sites, the reason alone (`path`, `format`, `limit`, …) can't tell an analyst which entry or constraint failed. AGENTS.md: *"Diagnostics should identify the failed constraint and the target… generic catches must not erase actionable validation details."* Real archives can trigger this. Case-fold collisions such as `Main.js` + `main.js` are legal in ZIP/ASAR files built on case-sensitive filesystems, and REA deliberately rejects them (fail-closed). This PR leaves that policy unchanged; it only makes the rejection explain itself. ## Fix - `ArtifactOperationError` gets an optional `detail`, which both conversion sites fill from `ArtifactReaderFailure.message`. The error projection emits it as `details.detail`. `export_web_scripts` already preserved its message this way. - The path messages now identify the target: - Case-fold collision: `Artifact path collision: main.js differs only in case from Main.js` (both spellings). - `Artifact path is absolute or unsafe: "a\\b.js"` and `Artifact path is not normalized: "../escape"`. The input is JSON-escaped because such names can contain NUL or control characters. After the change: ```json "details": { "operation": "analyze_javascript_application", "reason": "path", "detail": "Artifact path collision: a.js differs only in case from A.js" } ``` ## Contract / behavior changes - `artifact_operation_failed` errors gain an optional `details.detail` string. The published error contract already defines that variant's `details` as an open record, so `docs:check` passes with no regeneration. - The integrity-mismatch variant, which has strict `details`, is unchanged. - Three internal failure messages are more specific; the one existing test asserting them is updated. ## Verification - New `tests/boundary/providers/native/artifactProvider.failureDetail.test.ts` uses real ZIPs (`Main.js` + `main.js`, and `a\b.js`) through the public `ArtifactProvider` and `projectAnalysisError`. **Both cases fail on `main` and pass here.** - Built CLI: `analyze-javascript-application` and `inspect-artifact` on crafted ASARs (case collision, backslash, NUL, and `/` in a name) now report a `detail` for each. - All artifact-provider, filesystem, artifact MCP, and CLI-output boundary suites: 66 files / 405 tests passed (1 skipped). - `npm run check:changed` (33 files / 333 tests), `npm run check:fast`, and `npm run docs:check` passed. No Hopper/Ghidra/IDA involvement. 🤖 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:16:45 +00:00