[calvin-archastro]

Review on ArchCode

Epic: Aster bug bash fixes
Task: JS --related keeps a dynamic import() edge when another test imports the symbol statically

Problem and author intent

--related dropped a JavaScript/TypeScript test that reaches a changed export only through import() (or require()) whenever another file already named that export statically.

Reported layout (aster 0.30.1, ArchAstro/aster#239): src/str.js exports up; src/dyn.js defines lazy = () => import('./str.js'); test/dyn.test.js calls (await lazy()).up(...); test/static.test.js imports up statically. Editing toUpperCase() to toLowerCase() planned only test/static.test.js. Deleting the static importer made test/dyn.test.js appear (lazy <- up). Both tests should be selected, and removing the static importer should still select only the dynamic test. A module nobody loads dynamically should keep name-based narrowing.

Before this change, the JS walker records an import()/require() specifier on the enclosing definition’s loaded set. In the definition loop, unit users are marked only for an unnamed definition (!referenced && !is_test), plus callbacks/macros. A static importer sets referenced, so Run::users never runs. The dynamic loader uses the file but does not name the symbol, and the dynamic test names the symbol but does not use the source file, so Run::symbol skips it too.

Intent: mark definitions that load the changed file through import()/require() even when something already names the symbol, without widening selection for ordinary static imports.

What changed

Index gains dynamic_users: Vec<Vec<DefId>>, one list per unit. While the link-time resolve closure is in scope, each definition with a non-empty loaded set has those specifiers resolved and re-export-expanded; the file’s own unit is removed; each remaining unit records that definition as a dynamic user. Comments state that static imports are not this index: they name what they use, and file-top loaded also holds static import specifiers and top-level mocks, which must not be treated as dynamic loaders or every importer would be selected.

On each changed definition, Run::dynamic_users marks those loaders with Why::Via even if another file already named the symbol. The loader is skipped when it is the definition itself, and a test definition still does not mark a non-test loader. open_loads and the unnamed users shortcut are unchanged (this diff only adds lines).

tests/related.rs adds js_dynamic_import_edge_survives_a_static_importer, which builds the issue workspace for .js and .ts, with and without the static importer, plus an unrelated plain module used only by static shout/whisper tests.

Scope

• src/related/engine.rs
• tests/related.rs

Diff stats match the projected revision: 7 hunks, +163/−0, no omitted hunks or paths. Six engine hunks and one test hunk account for the line count. No other paths are in the revision.

Out of scope, and not present in the diff: open_loads full-run behavior, and Python/Go import handling.

Risk assessment

Medium. The mark site is on the shared related-engine path, but the new edges come only from existing loaded facts that the JS walker fills for dynamic import()/require(). Selection can widen for JS/TS projects that dynamically load a changed module; it should not drop tests that were already selected. A mistaken index of file-top static imports or top-level mocks would select every importer and defeat name-based narrowing. The new test is written to catch that for one static-only module, but the indexer loop shown does not itself skip a file-top definition.

User impact

Affected-test plans with --related should include tests that reach a changed member through a dynamic loader, including when a static importer already names that member. Plan notes and graph --source are expected to cite the loader (for example lazy and src/dyn.*). Workspaces with only the dynamic importer should still select that test. Modules with no dynamic loader should keep per-name selection (editing shout should not pull in whisper).

Testing

The regression is tests/related.rs::js_dynamic_import_edge_survives_a_static_importer. For js and ts it edits src/str.*, plans test, and asserts the npm test command contains both test/dyn.test.* and test/static.test.*, excludes shout.test and whisper.test, and that notes and a successful graph --source --commit HEAD mention lazy and the dynamic test. A second workspace without the static importer expects only npm test -- test/dyn.test.{ext} --coverage.enabled=false. A further edit of src/plain.js expects only test/shout.test.js.

That function is evidence of intended coverage, not proof it passed. Patch excerpts do not include test results.

Upstream checks all report succeeded with exit code 0: format, lint, test, fix, reformat, verify-lint, verify-test, and commit (“No remediation files to commit”; fix reports no files edited). Those outcomes are advisory. The payload is truncated: check excerpts are cut off and do not name js_dynamic_import_edge_survives_a_static_importer, tests/related.rs, or cargo test --test related. Visible test lines are from other suites. Do not treat this as proof the canonical test ran in CI.

Author-reported verification (not shown in the patch or in untruncated CI logs): cargo test --test related js_dynamic_import_edge_survives_a_static_importer; cargo test --test related (64 passed); cargo test --lib related:: (57 passed); clippy with -D warnings; cargo fmt --all --check. The upstream lint/test commands cited in the fix note are workspace-wide locked runs, but their excerpts do not confirm those related filters.

Follow-ups and known issues

• The new test covers import() only, not require(), even though the index comments treat both as dynamic loads.
• Comments and the author plan say file-top loaded (static imports and top-level mocks) is not indexed. The shown loop records every file.defs entry with non-empty loaded and has no explicit file-top guard. Confirm file-top is not in file.defs, or add a guard, before relying on name-based narrowing.
• CI excerpts here do not show the canonical test. A follow-up run of cargo test --test related js_dynamic_import_edge_survives_a_static_importer is the direct check.
• open_loads and non-JS import handling were left unchanged on purpose.

[calvin-archastro]

ArchDev is watching this pull request. State: watching (cycle 2 of 2 for head c3c5703).

when	trigger	decision	result
2026-11-01T16:42:43.404Z	merge conflict	enqueue job_337be90955b024f3ee3d94da	failed Pipeline step finalize-conflict failed: command exited with code 1
2026-11-01T17:03:01.067Z	verify head	enqueue job_0c5a5e590dbd395e9a5200c4	no change

Paused by the no-autofix label or a draft; never resolves reviewer threads. Jobs: job_337be90955b024f3ee3d94da, job_0c5a5e590dbd395e9a5200c4.