morluto/rea

Fork 0

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

[PR #3001] [MERGED] fix(javascript): keep CommonJS percent bytes literal #5552

New issue

Closed

opened 2026-10-31 01:07:32 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:07:32 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3001
Author: @ShivekRanjan
Created: 10/13/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/commonjs-encoded-specifiers

📝 Commits (2)

c4179f0 fix(javascript): keep CommonJS percent bytes literal
280ed55 Merge origin/main into fix/commonjs-encoded-specifiers

📊 Changes

2 files changed (+86 additions, -6 deletions)

View changed files

📝 src/application/javascript/JavaScriptArtifactPathResolution.ts (+21 -4)
📝 tests/boundary/filesystem/javascriptRequireLiteralPaths.test.ts (+65 -2)

📄 Description

Summary

Refs #2960 (CommonJS part only).

analyze-javascript-application rejected CommonJS requests whose literal file names contain %2e, %2f or %5c, although Node's CommonJS loader never percent-decodes a request and loads those files. For example, require("./lit%2eb") loads lit%2eb.js in Node, but REA reported it as rejected on main (unresolved in 6.4.0).

Problem and expected behavior

Smallest trigger: an application containing lit%2eb.js and require("./lit%2eb").

Violated invariant: docs/javascript-artifact-reconstruction.md says "CommonJS relative paths and legacy package main fields retain literal filename punctuation". The encoded dot and separator rule ran before the resolver knew the loader, so it also applied to CommonJS, where % is not an escape.

Expected: CommonJS requests and legacy package main fields resolve to the literal file Node loads. Root confinement still applies.

Change and scope

rejectDeclaration takes whether % is literal. declaresLiteralPercent is true only for require module specifiers and require package entrypoints, the two places Node's CommonJS loader reads text literally.
NUL and backslash are still rejected in every context.
Root confinement is unchanged: confineCandidate still rejects any joined path that leaves the artifact root. A literal %2e%2e is a directory name, never a parent segment.
Package exports and imports targets keep the strict rule. They are URLs for both loaders, and Node rejects encoded dot segments in them itself.

Not included:

ESM import specifiers are unchanged and still reject encoded dots. #2960 asks whether REA's stricter policy should continue there.
Package-role loads edges that have no loader mode (for example an Electron package.json main) keep the strict rule, because the loader is unknown.

Follow the contribution scope. This is an ordinary fix within existing boundaries; no maintainer scope decision is needed.

Contract and boundary impact

Semantic owner and earliest changed stage: JavaScriptArtifactPathResolution declaration admission, before candidate lookup
Contribution scope fit and linked maintainer decision: not applicable (bug fix within existing boundaries)
CLI and MCP/tool-catalog contract: none; resolution_status values are unchanged, and affected CommonJS requests now report resolved (or not-found when Node also finds nothing)
Provider, bridge, target-format, or platform compatibility: none
Evidence, artifact, provenance, or reconstruction contract: affected CommonJS import edges now carry the resolved path
Target selection, authority, upstream behavior, and cleanup details: not applicable
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

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/filesystem/javascriptRequireLiteralPaths.test.ts adds "agrees with Node for requests whose literal names contain %2e or %2F". Node's own createRequire().resolve computes the expected file for ./lit%2eb, ./x%2Fy, ./sub/%2e%2e/x, a package main of lit%2eb, and a bare dep/lit%2eb subpath. It also checks that ./%2e%2e/escape stays unresolved in both Node and REA.
Base reproduction or other evidence: with only JavaScriptArtifactPathResolution.ts reverted to the base revision, the new test fails with ./lit%2eb: expected null to be 'app/lit%2eb.js'. The original report and repro script are in #2960.

User-visible CLI/MCP output (if applicable): the repro script from #2960, before (rea-agents 6.4.0) and after (this branch):

mode     specifier             Node loads   REA (6.4.0)   REA (this branch)
require  ./lit%2eb             lit%2eb.js   unresolved    resolved lit%2eb.js
require  ./x%2Fy               x%2Fy.js     unresolved    resolved x%2Fy.js
import   ./a%2eb.mjs           a.b.mjs      rejected      rejected (unchanged, see #2960)
import   ./sub/%2e%2e/x.mjs    x.mjs        rejected      rejected (unchanged, see #2960)

Remaining proof gaps: ESM behavior awaits the maintainer answer in #2960; package-role loads edges without a loader mode keep the strict rule; the boundary tests were not run on a Windows source checkout (see Validation).

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

Run on c4179f027:

Ubuntu (WSL2), Node v24.21.0: npm run test:focused -- tests/boundary/filesystem/javascriptRequireLiteralPaths.test.ts tests/boundary/filesystem/javascript-applications/javascriptModuleSpecifierPaths.test.ts tests/boundary/javascript/packageImportAliases.test.ts src/application/javascript/JavaScriptArtifactPathResolution.test.ts: 4 files, 183 tests passed
Same environment, with only JavaScriptArtifactPathResolution.ts reverted to the base revision: npm run test:focused -- tests/boundary/filesystem/javascriptRequireLiteralPaths.test.ts: 1 failed (the new test, message above), 4 passed
Windows x64, Node v24.15.0: npm run check:fast: typecheck and lint passed
Not run on Windows: the boundary tests in javascriptRequireLiteralPaths.test.ts. On a Windows source checkout every test in that file stops before its assertions because the Windows native controls aren't built (see CONTRIBUTING "Windows source checkouts").

Compatibility, safety, and release

Breaking changes or migration steps: none. CommonJS requests containing %2e, %2f or %5c that were reported as rejected now report resolved, or not-found when Node also finds no file.
Real Hopper/Ghidra, browser, or OS coverage: not applicable (static JavaScript analysis, no provider). Tests ran on Linux (WSL2); static checks ran on Windows.
Package or release metadata impact: none.
Security, privacy, process, or containment review: root confinement is unchanged, a literal %2e%2e is never treated as a parent segment (covered by the new test), NUL and backslash are still rejected, and package exports/imports targets keep the strict rule.

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/3001 **Author:** [@ShivekRanjan](https://github.com/ShivekRanjan) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/commonjs-encoded-specifiers` --- ### 📝 Commits (2) - [`c4179f0`](https://github.com/morluto/rea/commit/c4179f0276700c34461f72c90baffb2f7d8f7900) fix(javascript): keep CommonJS percent bytes literal - [`280ed55`](https://github.com/morluto/rea/commit/280ed55ca259cfc5442ee016c8bd1581c5650e3e) Merge origin/main into fix/commonjs-encoded-specifiers ### 📊 Changes **2 files changed** (+86 additions, -6 deletions) <details> <summary>View changed files</summary> 📝 `src/application/javascript/JavaScriptArtifactPathResolution.ts` (+21 -4) 📝 `tests/boundary/filesystem/javascriptRequireLiteralPaths.test.ts` (+65 -2) </details> ### 📄 Description ## Summary Refs #2960 (CommonJS part only). `analyze-javascript-application` rejected CommonJS requests whose literal file names contain `%2e`, `%2f` or `%5c`, although Node's CommonJS loader never percent-decodes a request and loads those files. For example, `require("./lit%2eb")` loads `lit%2eb.js` in Node, but REA reported it as `rejected` on main (unresolved in 6.4.0). ## Problem and expected behavior Smallest trigger: an application containing `lit%2eb.js` and `require("./lit%2eb")`. Violated invariant: `docs/javascript-artifact-reconstruction.md` says "CommonJS relative paths and legacy package main fields retain literal filename punctuation". The encoded dot and separator rule ran before the resolver knew the loader, so it also applied to CommonJS, where `%` is not an escape. Expected: CommonJS requests and legacy package `main` fields resolve to the literal file Node loads. Root confinement still applies. ## Change and scope - `rejectDeclaration` takes whether `%` is literal. `declaresLiteralPercent` is true only for `require` module specifiers and `require` package entrypoints, the two places Node's CommonJS loader reads text literally. - NUL and backslash are still rejected in every context. - Root confinement is unchanged: `confineCandidate` still rejects any joined path that leaves the artifact root. A literal `%2e%2e` is a directory name, never a parent segment. - Package `exports` and `imports` targets keep the strict rule. They are URLs for both loaders, and Node rejects encoded dot segments in them itself. Not included: - ESM `import` specifiers are unchanged and still reject encoded dots. #2960 asks whether REA's stricter policy should continue there. - Package-role `loads` edges that have no loader mode (for example an Electron `package.json` `main`) keep the strict rule, because the loader is unknown. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). This is an ordinary fix within existing boundaries; no maintainer scope decision is needed. ## Contract and boundary impact - Semantic owner and earliest changed stage: `JavaScriptArtifactPathResolution` declaration admission, before candidate lookup - Contribution scope fit and linked maintainer decision: not applicable (bug fix within existing boundaries) - CLI and MCP/tool-catalog contract: none; `resolution_status` values are unchanged, and affected CommonJS requests now report `resolved` (or `not-found` when Node also finds nothing) - Provider, bridge, target-format, or platform compatibility: none - Evidence, artifact, provenance, or reconstruction contract: affected CommonJS import edges now carry the resolved path - Target selection, authority, upstream behavior, and cleanup details: not applicable - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [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/filesystem/javascriptRequireLiteralPaths.test.ts` adds "agrees with Node for requests whose literal names contain %2e or %2F". Node's own `createRequire().resolve` computes the expected file for `./lit%2eb`, `./x%2Fy`, `./sub/%2e%2e/x`, a package `main` of `lit%2eb`, and a bare `dep/lit%2eb` subpath. It also checks that `./%2e%2e/escape` stays unresolved in both Node and REA. - Base reproduction or other evidence: with only `JavaScriptArtifactPathResolution.ts` reverted to the base revision, the new test fails with `./lit%2eb: expected null to be 'app/lit%2eb.js'`. The original report and repro script are in #2960. - User-visible CLI/MCP output (if applicable): the repro script from #2960, before (rea-agents 6.4.0) and after (this branch): ``` mode specifier Node loads REA (6.4.0) REA (this branch) require ./lit%2eb lit%2eb.js unresolved resolved lit%2eb.js require ./x%2Fy x%2Fy.js unresolved resolved x%2Fy.js import ./a%2eb.mjs a.b.mjs rejected rejected (unchanged, see #2960) import ./sub/%2e%2e/x.mjs x.mjs rejected rejected (unchanged, see #2960) ``` - Remaining proof gaps: ESM behavior awaits the maintainer answer in #2960; package-role `loads` edges without a loader mode keep the strict rule; the boundary tests were not run on a Windows source checkout (see Validation). 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 Run on c4179f027: - Ubuntu (WSL2), Node v24.21.0: `npm run test:focused -- tests/boundary/filesystem/javascriptRequireLiteralPaths.test.ts tests/boundary/filesystem/javascript-applications/javascriptModuleSpecifierPaths.test.ts tests/boundary/javascript/packageImportAliases.test.ts src/application/javascript/JavaScriptArtifactPathResolution.test.ts`: 4 files, 183 tests passed - Same environment, with only `JavaScriptArtifactPathResolution.ts` reverted to the base revision: `npm run test:focused -- tests/boundary/filesystem/javascriptRequireLiteralPaths.test.ts`: 1 failed (the new test, message above), 4 passed - Windows x64, Node v24.15.0: `npm run check:fast`: typecheck and lint passed - Not run on Windows: the boundary tests in `javascriptRequireLiteralPaths.test.ts`. On a Windows source checkout every test in that file stops before its assertions because the Windows native controls aren't built (see CONTRIBUTING "Windows source checkouts"). ## Compatibility, safety, and release - Breaking changes or migration steps: none. CommonJS requests containing `%2e`, `%2f` or `%5c` that were reported as `rejected` now report `resolved`, or `not-found` when Node also finds no file. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (static JavaScript analysis, no provider). Tests ran on Linux (WSL2); static checks ran on Windows. - Package or release metadata impact: none. - Security, privacy, process, or containment review: root confinement is unchanged, a literal `%2e%2e` is never treated as a parent segment (covered by the new test), NUL and backslash are still rejected, and package `exports`/`imports` targets keep the strict rule. ## 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 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:07:32 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:44:23 +00:00

[PR #5566] [CLOSED] fix(native): keep per-input demangle outcomes when swift-demangle skips _ #6966

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

No description provided.

Rows
Columns