[PR #6313] [MERGED] fix(web): refuse an existing script-export destination as invalid input #7274

New issue

Closed

opened 2026-11-01 21:46:39 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:46:39 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6313
Author: @mhamzasakhi
Created: 10/22/2026
Status: ✅ Merged
Merged: 10/23/2026
Merged by: @baron

Base: main ← Head: fix/web-script-export-existing-destination

📝 Commits (3)

235b483 fix(web): refuse an existing script-export destination as invalid input
5004e94 Merge main into fix/web-script-export-existing-destination
9523af4 fix(artifacts): type SafeOutputTree destination refusals

📊 Changes

8 files changed (+277 additions, -35 deletions)

View changed files

📝 src/application/environment/environmentBundleWrite.ts (+7 -24)
📝 src/artifacts/SafeOutputTree.ts (+22 -8)
➕ src/artifacts/SafeOutputTreeAdmission.test.ts (+83 -0)
➕ src/artifacts/SafeOutputTreeAdmission.ts (+43 -0)
📝 src/browser/assets/PublishWebScripts.ts (+8 -1)
📝 tests/boundary/cli/webScriptSelectionCli.test.ts (+52 -1)
📝 tests/boundary/filesystem/webScriptExport.test.ts (+5 -0)
📝 tests/boundary/mcp/webScriptSelectionMcp.test.ts (+57 -1)

📄 Description

Summary

Fixes #6029. export_web_scripts correctly refuses an existing output directory, or a link to one, but reported the refusal as artifact_operation_failed / execution_failure. It returned no output_directory field issue and no input-contract recovery, and remediation.action only repeated the message.

Problem and expected behavior

SafeOutputTree.create raises "Extraction output root already exists" as ArtifactReaderFailure("path") before any output tree exists. publishWebScripts rethrew it, and mapExportFailure wrapped it as a WebScriptExportError.

Expected, matching the export guide's absent-destination precondition: invalid_request / invalid_input, an issue on ["output_directory"], and recovery: {kind: "inspect_input_contract", operation: "export_web_scripts"}. The destination must stay untouched. Failures after the destination is admitted keep their artifact semantics.

Change and scope

SafeOutputTree.create now raises SafeOutputTreeDestinationRefusal (new, in src/artifacts/SafeOutputTreeAdmission.ts) only for refusals of the selection itself:
- a relative or reserved root
- an existing root
- an accepted existing root that is non-empty or is not a directory
The last applies only to a root this call did not create; a created root that changes identity remains a host failure. Setup failures after a tree is acquired, such as being unable to restrict a created root to 0700, keep their artifact semantics. Greptile identified that distinction in review.
outputDirectoryAdmissionRefusal(operation, field, cause) converts only that type, directly or wrapped by SafeOutputTreeCreationFailure, into an AnalysisInputError on the caller's directory field.
- Environment export used to apply a broader private rule (directoryRefusal plus an admitted flag). It now uses the shared helper, and both exporters drop their admitted flags because the error type carries the origin.
mapExportFailure on current main passes every AnalysisError through (from #5956's cancellation fix). A cancellation observed in the same failure is rethrown before the conversion.
Not included:
- Other SafeOutputTree.create callers (ArtifactExtraction, WakaruProvider, firmware publication) now receive the typed refusal, but they do not convert it. Their classification is unchanged. Firmware is tracked in #5746, #5899 and #5900.
- Cancellation and failed rollback after output acquisition, tracked in #5956.

Contribution scope: an ordinary error-classification fix within existing boundaries; a maintainer scope decision is not applicable.

Contract and boundary impact

Semantic owner and earliest changed stage: output-destination admission, at SafeOutputTree.create and its callers' admission tracking.
Contribution scope fit and linked maintainer decision: in scope; not applicable.
CLI and MCP/tool-catalog contract: no schema change. An existing output_directory now returns invalid_request with a field issue and input-contract recovery.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: none. The refusal happens before any output exists, and the destination is preserved.
Target selection, authority, upstream behavior, and cleanup details: no change to rollback or ownership. Conversion happens only after the existing cleanup path.
Generated metadata, package, or installation impact: none.
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:
- tests/boundary/cli/webScriptSelectionCli.test.ts (public CLI): an existing directory and a directory link each exit 1 with the expected code, category, recovery and output_directory issue. The retained directory still holds only its marker.
- tests/boundary/mcp/webScriptSelectionMcp.test.ts: the registered export_web_scripts MCP tool returns the same typed error and preserves the directory.
- tests/boundary/filesystem/webScriptExport.test.ts: the existing symlink-destination case now also asserts AnalysisInputError on ["output_directory"].
- src/artifacts/SafeOutputTreeAdmission.test.ts (new): destination refusals are converted, both directly and when wrapped by an acquired tree. An acquired-tree setup failure, a non-refusal path failure and a refusal with unresolved cleanup are not converted. A workflow cannot reliably force the setup failure, so this module test owns that distinction. Its setup-failure case fails under the previous rule.
Base reproduction (executed): with the publisher's admission conversion disabled, all three cases fail. Rechecked after merging current main.

User-visible CLI output (executed, Linux x64). rea export-web-scripts <capture> <existing-dir> --json:

{"error":"Script export failed","code":"invalid_request","category":"invalid_input","message":"Extraction output root already exists","retryable":false,"remediation":{"action":"Read the declared input schema or CLI usage for export_web_scripts; if using MCP and the catalog may be stale, call tools/list. Correct the listed fields, then retry. Do not repeat the unchanged call."},"recovery":{"kind":"inspect_input_contract","operation":"export_web_scripts"},"details":{"operation":"export_web_scripts","issues":[{"path":["output_directory"],"reason":"invalid_value","message":"Extraction output root already exists"}]}}

The existing directory still contained only marker.txt afterwards.

Remaining proof gaps: macOS and Windows were not run. On Windows the CLI test uses a junction for the link case.
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

Run on Ubuntu 24.04 x64 with Node 24.18.0 and npm 11.16.0:

npm run test:focused -- with the 3 changed test files: 39 passed. With the fix reverted: 3 failed.
npm run test:focused -- with every test file that references SafeOutputTree, script export, environment export, artifact extraction or firmware extraction: 39 files, 690 passed, 0 failed. These include main's script-export cancellation cases and environment export's non-empty-directory and file-path refusals. These include environmentBundleWrite, environmentExport and both environment CLI files, which confirm the shared helper preserves environment-export behavior.
npm run check:fast: passed. knip reports nothing for the new module.
rea export-web-scripts on an existing directory: output shown above.

Compatibility, safety, and release

Breaking changes or migration steps: callers that matched artifact_operation_failed for an existing output_directory will now see invalid_request. Genuine write and publication failures are unchanged.
Real Hopper/Ghidra, browser, or OS coverage: not applicable (saved capture; no browser launched). Linux only.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new reads or writes. The destination is still refused before any output is created.

Author readiness

Human owner: @mhamzasakhi

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/6313 **Author:** [@mhamzasakhi](https://github.com/mhamzasakhi) **Created:** 10/22/2026 **Status:** ✅ Merged **Merged:** 10/23/2026 **Merged by:** [@baron](https://github.com/baron) **Base:** `main` ← **Head:** `fix/web-script-export-existing-destination` --- ### 📝 Commits (3) - [`235b483`](https://github.com/morluto/rea/commit/235b4830af1432ea9bff9db1ad0a567b5860670a) fix(web): refuse an existing script-export destination as invalid input - [`5004e94`](https://github.com/morluto/rea/commit/5004e9428bc03f03e80bc037cbc38b24386be37f) Merge main into fix/web-script-export-existing-destination - [`9523af4`](https://github.com/morluto/rea/commit/9523af46bfc52a79b6be68e6846e40ea4553a6ee) fix(artifacts): type SafeOutputTree destination refusals ### 📊 Changes **8 files changed** (+277 additions, -35 deletions) <details> <summary>View changed files</summary> 📝 `src/application/environment/environmentBundleWrite.ts` (+7 -24) 📝 `src/artifacts/SafeOutputTree.ts` (+22 -8) ➕ `src/artifacts/SafeOutputTreeAdmission.test.ts` (+83 -0) ➕ `src/artifacts/SafeOutputTreeAdmission.ts` (+43 -0) 📝 `src/browser/assets/PublishWebScripts.ts` (+8 -1) 📝 `tests/boundary/cli/webScriptSelectionCli.test.ts` (+52 -1) 📝 `tests/boundary/filesystem/webScriptExport.test.ts` (+5 -0) 📝 `tests/boundary/mcp/webScriptSelectionMcp.test.ts` (+57 -1) </details> ### 📄 Description ## Summary Fixes #6029. `export_web_scripts` correctly refuses an existing output directory, or a link to one, but reported the refusal as `artifact_operation_failed` / `execution_failure`. It returned no `output_directory` field issue and no input-contract recovery, and `remediation.action` only repeated the message. ## Problem and expected behavior `SafeOutputTree.create` raises "Extraction output root already exists" as `ArtifactReaderFailure("path")` before any output tree exists. `publishWebScripts` rethrew it, and `mapExportFailure` wrapped it as a `WebScriptExportError`. Expected, matching the export guide's absent-destination precondition: `invalid_request` / `invalid_input`, an issue on `["output_directory"]`, and `recovery: {kind: "inspect_input_contract", operation: "export_web_scripts"}`. The destination must stay untouched. Failures after the destination is admitted keep their artifact semantics. ## Change and scope - `SafeOutputTree.create` now raises `SafeOutputTreeDestinationRefusal` (new, in `src/artifacts/SafeOutputTreeAdmission.ts`) only for refusals of the selection itself: - a relative or reserved root - an existing root - an accepted existing root that is non-empty or is not a directory The last applies only to a root this call did not create; a created root that changes identity remains a host failure. Setup failures after a tree is acquired, such as being unable to restrict a created root to `0700`, keep their artifact semantics. Greptile identified that distinction in review. - `outputDirectoryAdmissionRefusal(operation, field, cause)` converts only that type, directly or wrapped by `SafeOutputTreeCreationFailure`, into an `AnalysisInputError` on the caller's directory field. - Environment export used to apply a broader private rule (`directoryRefusal` plus an `admitted` flag). It now uses the shared helper, and both exporters drop their `admitted` flags because the error type carries the origin. - `mapExportFailure` on current `main` passes every `AnalysisError` through (from #5956's cancellation fix). A cancellation observed in the same failure is rethrown before the conversion. - Not included: - Other `SafeOutputTree.create` callers (`ArtifactExtraction`, `WakaruProvider`, firmware publication) now receive the typed refusal, but they do not convert it. Their classification is unchanged. Firmware is tracked in #5746, #5899 and #5900. - Cancellation and failed rollback after output acquisition, tracked in #5956. Contribution scope: an ordinary error-classification fix within existing boundaries; a maintainer scope decision is not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: output-destination admission, at `SafeOutputTree.create` and its callers' admission tracking. - Contribution scope fit and linked maintainer decision: in scope; not applicable. - CLI and MCP/tool-catalog contract: no schema change. An existing `output_directory` now returns `invalid_request` with a field issue and input-contract recovery. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. The refusal happens before any output exists, and the destination is preserved. - Target selection, authority, upstream behavior, and cleanup details: no change to rollback or ownership. Conversion happens only after the existing cleanup path. - Generated metadata, package, or installation impact: none. - [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 or updated: - `tests/boundary/cli/webScriptSelectionCli.test.ts` (public CLI): an existing directory and a directory link each exit 1 with the expected code, category, recovery and `output_directory` issue. The retained directory still holds only its marker. - `tests/boundary/mcp/webScriptSelectionMcp.test.ts`: the registered `export_web_scripts` MCP tool returns the same typed error and preserves the directory. - `tests/boundary/filesystem/webScriptExport.test.ts`: the existing symlink-destination case now also asserts `AnalysisInputError` on `["output_directory"]`. - `src/artifacts/SafeOutputTreeAdmission.test.ts` (new): destination refusals are converted, both directly and when wrapped by an acquired tree. An acquired-tree setup failure, a non-refusal path failure and a refusal with unresolved cleanup are not converted. A workflow cannot reliably force the setup failure, so this module test owns that distinction. Its setup-failure case fails under the previous rule. - Base reproduction (executed): with the publisher's admission conversion disabled, all three cases fail. Rechecked after merging current `main`. - User-visible CLI output (executed, Linux x64). `rea export-web-scripts <capture> <existing-dir> --json`: ```json {"error":"Script export failed","code":"invalid_request","category":"invalid_input","message":"Extraction output root already exists","retryable":false,"remediation":{"action":"Read the declared input schema or CLI usage for export_web_scripts; if using MCP and the catalog may be stale, call tools/list. Correct the listed fields, then retry. Do not repeat the unchanged call."},"recovery":{"kind":"inspect_input_contract","operation":"export_web_scripts"},"details":{"operation":"export_web_scripts","issues":[{"path":["output_directory"],"reason":"invalid_value","message":"Extraction output root already exists"}]}} ``` The existing directory still contained only `marker.txt` afterwards. - Remaining proof gaps: macOS and Windows were not run. On Windows the CLI test uses a junction for the link case. - [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 Run on Ubuntu 24.04 x64 with Node 24.18.0 and npm 11.16.0: - `npm run test:focused --` with the 3 changed test files: 39 passed. With the fix reverted: 3 failed. - `npm run test:focused --` with every test file that references `SafeOutputTree`, script export, environment export, artifact extraction or firmware extraction: 39 files, 690 passed, 0 failed. These include main's script-export cancellation cases and environment export's non-empty-directory and file-path refusals. These include `environmentBundleWrite`, `environmentExport` and both environment CLI files, which confirm the shared helper preserves environment-export behavior. - `npm run check:fast`: passed. `knip` reports nothing for the new module. - `rea export-web-scripts` on an existing directory: output shown above. ## Compatibility, safety, and release - Breaking changes or migration steps: callers that matched `artifact_operation_failed` for an existing `output_directory` will now see `invalid_request`. Genuine write and publication failures are unchanged. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (saved capture; no browser launched). Linux only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new reads or writes. The destination is still refused before any output is created. ## Author readiness Human owner: @mhamzasakhi - [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. - [ ] 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-11-01 21:46:39 +00:00