morluto/rea

Fork 0

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

[PR #1392] [CLOSED] fix(javascript): root application graphs at the application manifest #2849

New issue

Closed

opened 2026-10-17 21:10:59 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-17 21:10:59 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1392
Author: @marnixpluim
Created: 10/10/2026
Status: ❌ Closed

Base: main ← Head: fix/application-package-root

📝 Commits (3)

b4e558c fix(javascript): root application graphs at the application manifest
f75a40c test(javascript): cover renderer entries from dependency manifests
11f7afb Merge upstream main into fix/application-package-root

📊 Changes

2 files changed (+63 additions, -1 deletions)

View changed files

📝 src/application/javascript/JavaScriptArtifactGraphStructure.ts (+22 -1)
📝 tests/composition/javascript-applications/electronApplicationAnalysis.test.ts (+41 -0)

📄 Description

Requested by Marnix · project thread

Summary

The application graph takes the first package.json in path order as its root, and gives every manifest's entries Electron roles: main becomes electron-main, and renderer, browser or module becomes electron-renderer. When an app ships unpacked dependencies beside its bundle, a dependency's manifest can sort first: in Hermes Desktop v0.21.6, dist/node_modules/get-windows/package.json sorts before package.json. REA then roots the whole graph at that dependency and labels dependency entry points as Electron main and renderer entries. This change roots the graph at the shallowest manifest outside node_modules, and stops assigning Electron roles from dependency manifests.

No issue is linked. This was found while analyzing the packaged Hermes Desktop v0.21.6 (NousResearch/hermes-agent, MIT).

Problem and expected behavior

Smallest trigger: a directory with package.json ({"name": "app", "main": "main.js"}) and node_modules/dep/package.json ({"name": "dep", "main": "index.js", "browser": "browser.js"}).

Expected: the graph root is app, main.js is the only electron-main entry, and there is no electron-renderer entry. On 7aa4d768 the root is dep, because node_modules/dep/package.json sorts before package.json; index.js is labeled electron-main too, and browser.js electron-renderer.

Invariant: the application graph is rooted at the application, and only the application's manifest declares Electron entry points. Electron never loads a dependency's main as an application entry.

Change and scope

applicationManifestPath picks the manifest outside any node_modules directory with the fewest path segments, breaking ties by code-point order, so the choice stays deterministic.
If every manifest is inside node_modules, the shallowest of them is still chosen, so an archive that had a root before still has one.
Dependency manifests still become package nodes with their inventory evidence; they just no longer receive Electron roles, neither electron-main from main nor electron-renderer from renderer, browser or module.

Not included: archives with several top-level application manifests still get one root, now the shallowest.

Contract and boundary impact

Semantic owner and earliest changed stage: application JavaScript artifact graph structure (src/application/javascript/JavaScriptArtifactGraphStructure.ts).
CLI and MCP/tool-catalog contract: no shape change. root_node_ids change only for archives whose first manifest by path was a dependency's. Electron role nodes change for any archive that ships manifests inside node_modules: their entries no longer become electron-main or electron-renderer roles.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: dependency package nodes and their evidence are kept; only the root edge and role assignment move.
Process execution, authorization, cleanup, or containment impact: none.
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Evidence and regression coverage

Tests added or updated: tests/composition/javascript-applications/electronApplicationAnalysis.test.ts, "roots the graph at the application manifest, not a dependency", using the trigger above. It expects root label app, electron-main entries ["main.js"] and no electron-renderer entry.
Base reproduction or other evidence: on 7aa4d768 the test gets root labels ["dep"], electron-main entries ["main.js", "index.js"] and electron-renderer entries ["browser.js"]. On the Hermes Desktop v0.21.6 Linux app.asar, with the member-mutation and alias fixes applied in both runs:

before after

root package get-windows Hermes

electron-main entries ./lib/index.js (node-pty), ./index.js (get-windows), dist/electron-main.mjs dist/electron-main.mjs
User-visible CLI/MCP output (if applicable): the root package and Electron roles in analyze-javascript-application output, as in the table.
Remaining proof gaps: HTML pages inside node_modules can still produce renderer nodes through their script tags; this change covers only manifest entries.

	before	after
root package	`get-windows`	`Hermes`
`electron-main` entries	`./lib/index.js` (node-pty), `./index.js` (get-windows), `dist/electron-main.mjs`	`dist/electron-main.mjs`

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 oxfmt --check on the changed files: all formatted
npx oxlint on the changed files: no warnings
node scripts/verify-module-boundaries.mjs: passed
npx tsc --noEmit: exit 0
npx vitest run tests/composition/javascript-applications src/application/javascript: 24 files, 317 tests passed
Full suite on the four branches from this Hermes Desktop run, merged (npm run test:prepare, then npx vitest run; root, Linux x64, Node 22.22): 6,246 tests passed, 114 skipped, 1 failed. The failure is analysisSelectorCli.test.ts "rejects missing and conflicting selectors before opening a target": it starts the CLI 12 times, hit its 30 s timeout under the parallel run, and passes alone in 27.6 s. It covers CLI argument parsing, which these changes don't touch.

Compatibility, safety, and release

Breaking changes or migration steps: none. Consumers that relied on the first manifest by path being the root now see the application manifest instead.
Real Hopper/Ghidra, browser, or OS coverage: not applicable; static analysis only. Run on Linux x64 with Node 22.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new process execution, file access or network access.

Review checklist

This PR addresses a concrete problem or an agreed enhancement.
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.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XPY2GmufMNGQKoEgRUx1pd

_{🔄 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/1392 **Author:** [@marnixpluim](https://github.com/marnixpluim) **Created:** 10/10/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/application-package-root` --- ### 📝 Commits (3) - [`b4e558c`](https://github.com/morluto/rea/commit/b4e558c9e20020bfef197eb01b82ba074c3f6ff8) fix(javascript): root application graphs at the application manifest - [`f75a40c`](https://github.com/morluto/rea/commit/f75a40c34377efe56e78ee189368eede138988cb) test(javascript): cover renderer entries from dependency manifests - [`11f7afb`](https://github.com/morluto/rea/commit/11f7afb81417ed878f8293246cfdd991135099a5) Merge upstream main into fix/application-package-root ### 📊 Changes **2 files changed** (+63 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/application/javascript/JavaScriptArtifactGraphStructure.ts` (+22 -1) 📝 `tests/composition/javascript-applications/electronApplicationAnalysis.test.ts` (+41 -0) </details> ### 📄 Description  _Requested by **Marnix** · [project thread](https://claude.ai/code/project/chan_015mc63Gczn836s2xXzmeLLa?thread=cmsg_015mc63Gczn836s2xXzmeLLa375aS21D2UMwQouWTw67kh)_ ## Summary The application graph takes the first `package.json` in path order as its root, and gives every manifest's entries Electron roles: `main` becomes `electron-main`, and `renderer`, `browser` or `module` becomes `electron-renderer`. When an app ships unpacked dependencies beside its bundle, a dependency's manifest can sort first: in Hermes Desktop v0.21.6, `dist/node_modules/get-windows/package.json` sorts before `package.json`. REA then roots the whole graph at that dependency and labels dependency entry points as Electron main and renderer entries. This change roots the graph at the shallowest manifest outside `node_modules`, and stops assigning Electron roles from dependency manifests. No issue is linked. This was found while analyzing the packaged Hermes Desktop v0.21.6 ([NousResearch/hermes-agent](https://github.com/NousResearch/hermes-agent), MIT). ## Problem and expected behavior Smallest trigger: a directory with `package.json` (`{"name": "app", "main": "main.js"}`) and `node_modules/dep/package.json` (`{"name": "dep", "main": "index.js", "browser": "browser.js"}`). Expected: the graph root is `app`, `main.js` is the only `electron-main` entry, and there is no `electron-renderer` entry. On `7aa4d768` the root is `dep`, because `node_modules/dep/package.json` sorts before `package.json`; `index.js` is labeled `electron-main` too, and `browser.js` `electron-renderer`. Invariant: the application graph is rooted at the application, and only the application's manifest declares Electron entry points. Electron never loads a dependency's `main` as an application entry. ## Change and scope - `applicationManifestPath` picks the manifest outside any `node_modules` directory with the fewest path segments, breaking ties by code-point order, so the choice stays deterministic. - If every manifest is inside `node_modules`, the shallowest of them is still chosen, so an archive that had a root before still has one. - Dependency manifests still become package nodes with their inventory evidence; they just no longer receive Electron roles, neither `electron-main` from `main` nor `electron-renderer` from `renderer`, `browser` or `module`. Not included: archives with several top-level application manifests still get one root, now the shallowest. ## Contract and boundary impact - Semantic owner and earliest changed stage: application JavaScript artifact graph structure (`src/application/javascript/JavaScriptArtifactGraphStructure.ts`). - CLI and MCP/tool-catalog contract: no shape change. `root_node_ids` change only for archives whose first manifest by path was a dependency's. Electron role nodes change for any archive that ships manifests inside `node_modules`: their entries no longer become `electron-main` or `electron-renderer` roles. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: dependency package nodes and their evidence are kept; only the root edge and role assignment move. - Process execution, authorization, cleanup, or containment impact: none. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. ## Evidence and regression coverage - Tests added or updated: `tests/composition/javascript-applications/electronApplicationAnalysis.test.ts`, "roots the graph at the application manifest, not a dependency", using the trigger above. It expects root label `app`, `electron-main` entries `["main.js"]` and no `electron-renderer` entry. - Base reproduction or other evidence: on `7aa4d768` the test gets root labels `["dep"]`, `electron-main` entries `["main.js", "index.js"]` and `electron-renderer` entries `["browser.js"]`. On the Hermes Desktop v0.21.6 Linux `app.asar`, with the member-mutation and alias fixes applied in both runs: | | before | after | | --- | --- | --- | | root package | `get-windows` | `Hermes` | | `electron-main` entries | `./lib/index.js` (node-pty), `./index.js` (get-windows), `dist/electron-main.mjs` | `dist/electron-main.mjs` | - User-visible CLI/MCP output (if applicable): the root package and Electron roles in `analyze-javascript-application` output, as in the table. - Remaining proof gaps: HTML pages inside `node_modules` can still produce renderer nodes through their script tags; this change covers only manifest entries. 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 oxfmt --check` on the changed files: all formatted - `npx oxlint` on the changed files: no warnings - `node scripts/verify-module-boundaries.mjs`: passed - `npx tsc --noEmit`: exit 0 - `npx vitest run tests/composition/javascript-applications src/application/javascript`: 24 files, 317 tests passed - Full suite on the four branches from this Hermes Desktop run, merged (`npm run test:prepare`, then `npx vitest run`; root, Linux x64, Node 22.22): 6,246 tests passed, 114 skipped, 1 failed. The failure is `analysisSelectorCli.test.ts` "rejects missing and conflicting selectors before opening a target": it starts the CLI 12 times, hit its 30 s timeout under the parallel run, and passes alone in 27.6 s. It covers CLI argument parsing, which these changes don't touch. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Consumers that relied on the first manifest by path being the root now see the application manifest instead. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; static analysis only. Run on Linux x64 with Node 22. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new process execution, file access or network access. ## Review checklist - [x] This PR addresses a concrete problem or an agreed enhancement. - [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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01XPY2GmufMNGQKoEgRUx1pd --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-17 21:10:59 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-10-31 01:06:38 +00:00

[PR #2849] [CLOSED] fix(test): reject excluded symlink aliases in exact selections #5438

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

No description provided.

Rows
Columns