feat: add native source maps support #31268
Conversation
fraidev
changed the title
fraidev
force-pushed
the
native_sourcemaps_core
branch
4 times, most recently
from
c53725d to
5a94d7c
Compare
fraidev
changed the title
fraidev
changed the title
fraidev
force-pushed
the
native_sourcemaps_core
branch
from
5a94d7c to
2ddd584
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for loading external source maps by implementing the load_external_source_map method in the ModuleLoader trait. The changes enable proper source mapping for JavaScript files with external .map files and files with inline base64-encoded source maps, including those with trailing content after the sourceMappingURL comment.
Key changes:
- Implements
load_external_source_mapin bothCliModuleLoaderInnerandEmbeddedModuleLoader - Enhances
get_source_mapped_source_lineto read original source files from the file system when not in the module graph - Adds comprehensive test coverage for various sourcemap scenarios
Reviewed Changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/module_loader.rs | Implements load_external_source_map method to fetch external source maps via file_fetcher, refactors get_source_mapped_source_line to support files outside the module graph, extracts helper function extract_source_line |
| cli/rt/run.rs | Implements load_external_source_map for EmbeddedModuleLoader to load source maps from the embedded module store |
| Cargo.toml | Updates deno_core dependency to use a git branch with native sourcemap support |
| Cargo.lock | Updates dependency lockfile with git-based deno_core, deno_ops, and serde_v8 versions |
| tests/specs/run/sourcemap_external/* | Adds test for external source map files (.js.map) |
| tests/specs/run/sourcemap_inline_base64/* | Adds test for inline base64-encoded source maps |
| tests/specs/run/sourcemap_none/* | Adds test for files without source maps |
| tests/specs/run/sourcemap_relative_paths/* | Adds tests for source maps with relative paths both within and outside CWD |
| tests/specs/run/sourcemap_trailing_content/* | Adds test for source maps with trailing content after sourceMappingURL |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fraidev
force-pushed
the
native_sourcemaps_core
branch
4 times, most recently
from
c7e15bf to
e28defa
Compare
|
|
||
| fn load_external_source_map( | ||
| &self, | ||
| source_map_url: &str, |
There was a problem hiding this comment.
It feels like we should respect permission checks here - though I'm not sure how to determine if we should use "static" or "dynamic" permissions here. @dsherret any ideas?
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
This looks really good to me, I'm just worried about the missing permission check. Let's discuss during the meeting
fraidev
force-pushed
the
native_sourcemaps_core
branch
from
e28defa to
7355fdd
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdated workspace dependencies in Cargo.toml (bumped deno_core and switched deno_graph to a git/patch source). Added support for loading external source maps via new helpers in the CLI module loader and a corresponding method on EmbeddedModuleLoader. Refactored inline source-map handling to derive maps from prepared modules and introduced a single extract_source_line helper. get_source_mapped_source_line now resolves specifiers, skips non-code entry kinds, and falls back to filesystem reads when graph entries are missing. Added tests covering external, remote, inline (base64), relative-path, no-sourcemap, and trailing-content sourcemap scenarios. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runtime as Runtime
participant Loader as CliModuleLoader
participant Graph as ModuleGraph/PreparedModule
participant Embedded as EmbeddedModuleLoader
participant Fetcher as FileFetcher/Cache
participant FS as Filesystem
Runtime->>Loader: request source-mapped line for file_name
Loader->>Loader: resolve file_name -> Specifier
Loader->>Graph: load prepared module / graph entry
Graph-->>Loader: prepared module bytes or graph entry/none
alt prepared module contains inline source map
note right of Loader `#E8F0FF`: parse inline map from prepared bytes
Loader->>Loader: extract_source_line(text, line)
Loader-->>Runtime: return mapped source line
else source map references external URL
note right of Loader `#FFF4E6`: external map -> ask embedded loader
Loader->>Embedded: load_external_source_map(url)
Embedded->>Fetcher: read module bytes from shared store
Fetcher-->>Embedded: bytes? / None
alt bytes found
Embedded-->>Loader: source map bytes
Loader->>Loader: parse external map & extract_source_line
Loader-->>Runtime: return mapped source line
else not found
Embedded-->>Loader: None
Loader->>FS: read original file bytes
FS-->>Loader: file bytes
Loader->>Loader: extract_source_line
Loader-->>Runtime: return mapped source line or None
end
else no map found in graph/prepared module
Loader->>FS: read file from filesystem as fallback
FS-->>Loader: file bytes
Loader->>Loader: extract_source_line
Loader-->>Runtime: return original source line or None
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
fraidev
force-pushed
the
native_sourcemaps_core
branch
from
7355fdd to
b3128a5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/rt/run.rs (1)
586-593: Standaloneload_external_source_mapwiring looks correct; confirm data layout and consider a brief doc commentFor the embedded/standalone case this implementation is straightforward:
- Parses
source_map_urlinto aUrl.- Reads from
self.shared.modules, which is already the canonical store for snapshot contents.- Returns
Cow::Owned(data.data.to_vec()), assuming the corresponding entry’sdatafield contains the raw sourcemap bytes for the external map.That matches the model where external sourcemaps are embedded as separate modules/assets in the snapshot and avoids any extra I/O at runtime.
Two small follow‑ups:
- Please double‑check that the snapshot writer indeed stores external sourcemaps as dedicated entries whose
datafield is the sourcemap payload (and not, for example, a JS wrapper), so consumers interpret this correctly.- A one‑line doc comment on this method (mirroring the trait’s intent) would make the contract very clear for future maintainers.
🧹 Nitpick comments (4)
tests/specs/run/sourcemap_external/external.js (1)
1-8: Fixture correctly exercises external sourcemap stack tracesThe fixture reliably throws and points at
external.js.map, which is exactly what the external sourcemap tests need. The"Original TypeScript file"comment is slightly misleading for a.jsfile, but harmless—consider clarifying it or pointing atoriginal.tsif you touch this again.Cargo.toml (1)
64-65: Ensure git override ofdeno_coreis temporary and reverted before mergePointing
deno_coreat a personal fork/branch is reasonable for a draft while iterating, but this should not ship onmain. Please plan to:
- Switch back to the official
deno_coresource (versioned crate or official git repo/branch) once the native sourcemaps work lands upstream.- Drop or update the commented version line accordingly.
cli/module_loader.rs (2)
1213-1249: Inline and external sourcemap loading logic looks structurally sound
get_source_map:
- Resolves
file_nameto aModuleSpecifierand filters to the same external schemes we previously supported.- Uses
load_prepared_module_for_source_map_syncagainst the current graph, thensource_map_from_codeover the prepared source bytes. That keeps sourcemap extraction tied to the same pipeline that produced the code, which is good for correctness and avoids direct disk layout assumptions.
load_external_source_map:
- Resolves
source_map_urland usesfile_fetcher.get_cached_source_or_localto retrieve the bytes, returning an ownedCow<[u8]>.- This deliberately avoids hitting the network on sourcemap lookup and relies on caches/local file system, which seems like a reasonable trade‑off for stack traces.
One thing to clarify: both code paths bypass explicit
CheckSpecifierKind/permission routing and rely on the fact that these files have already been fetched or are local sources the runtime is allowed to see. If there are expectations around treating sourcemap reads as “static” vs “dynamic” file accesses, it may be worth documenting that here or threading the appropriate permission context intoget_cached_source_or_localin the future.
1251-1304: Source-mapped line lookup andextract_source_linehelper are correct; consider clarifying permission expectations
get_source_mapped_source_linenow:
- Resolves
file_nameto a specifier.- For
Js/Jsonmodules present in the graph, uses the in‑graphmodule.source.text, which is the right source of truth.- Early returns
NoneforWasm,Npm,Node, andExternalmodules, avoiding misleading line lookups where there is no meaningful source.- When the specifier is not in the graph, falls back to
file_fetcher.get_cached_source_or_localand delegates toextract_source_lineon the decoded contents. This nicely handles original TS/JS files that only exist on disk/cache but are referenced by sourcemaps.
extract_source_line:
- Uses
split('\n')instead of.lines()to preserve a terminating empty line, matching the comment.- Avoids allocating an intermediate
Vec, which is an improvement over the previous pattern.- Provides a consistent warning message when
line_numberis out of bounds.As with
load_external_source_map, this fallback introduces a direct read viaget_cached_source_or_localwithout an explicit permission token. If sourcemap‑driven reads should obey any particularPermissionsContainerpolicy (for example, mirroring the static vs dynamic distinction you already use inload_asset), it would be good to either:
- Document that sourcemap lookups are intentionally “best‑effort debugging helpers” that may read from cache/local disk, or
- Route this through a checked fetch helper similar to
load_asset.Functionally the logic looks right; the main open question is just how strict these reads should be.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
Cargo.lockis excluded by!**/*.locktests/specs/npm/cjs_this_in_exports/cjs_this_in_exports/main.outis excluded by!**/*.outtests/specs/run/sourcemap_external/external.js.mapis excluded by!**/*.maptests/specs/run/sourcemap_external/external.js.outis excluded by!**/*.outtests/specs/run/sourcemap_inline_base64/inline_base64.js.outis excluded by!**/*.outtests/specs/run/sourcemap_none/no_sourcemap.js.outis excluded by!**/*.outtests/specs/run/sourcemap_relative_paths/outside_cwd.js.outis excluded by!**/*.outtests/specs/run/sourcemap_trailing_content/trailing_content.js.outis excluded by!**/*.out
📒 Files selected for processing (15)
Cargo.toml(1 hunks)cli/module_loader.rs(2 hunks)cli/rt/run.rs(1 hunks)tests/specs/run/sourcemap_external/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_external/external.js(1 hunks)tests/specs/run/sourcemap_external/original.ts(1 hunks)tests/specs/run/sourcemap_inline_base64/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_inline_base64/inline_base64.js(1 hunks)tests/specs/run/sourcemap_none/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_none/no_sourcemap.js(1 hunks)tests/specs/run/sourcemap_relative_paths/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_relative_paths/outside_cwd.js(1 hunks)tests/specs/run/sourcemap_relative_paths/src/original.ts(1 hunks)tests/specs/run/sourcemap_trailing_content/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_trailing_content/trailing_content.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cli/rt/run.rs (1)
cli/module_loader.rs (1)
load_external_source_map(1233-1249)
cli/module_loader.rs (3)
cli/rt/run.rs (2)
load_external_source_map(586-593)get_source_mapped_source_line(595-616)runtime/worker.rs (1)
get_source_mapped_source_line(1304-1315)cli/graph_container.rs (2)
graph(26-26)graph(162-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug linux-x86_64
🔇 Additional comments (12)
tests/specs/run/sourcemap_external/original.ts (1)
1-6: External sourcemap TS fixture looks goodStraightforward error-throwing fixture with a clear, scenario-specific message; suitable for exercising external sourcemap handling.
tests/specs/run/sourcemap_inline_base64/__test__.jsonc (1)
1-5: Inline base64 sourcemap test spec is consistentThe
args,output, andexitCodealign with theinline_base64.jsfixture and the fact it throws; structure matches other run specs.tests/specs/run/sourcemap_none/no_sourcemap.js (1)
1-6: No-sourcemap JS fixture is appropriateSimple, self-explanatory error-throwing script with no
sourceMappingURL, which is exactly what this scenario needs.tests/specs/run/sourcemap_relative_paths/src/original.ts (1)
1-6: Relative-path original TS fixture matches scenarioThe location under
src/and the error message clearly target the “within cwd / relative path” sourcemap case; implementation is minimal and correct.tests/specs/run/sourcemap_none/__test__.jsonc (1)
1-5: No-sourcemap test spec is consistentArguments, output file name, and exit code all line up with
no_sourcemap.jsthrowing an error and match the structure of the other sourcemap run specs.tests/specs/run/sourcemap_inline_base64/inline_base64.js (1)
1-8: Inline base64 JS + embedded sourcemap look correctThe script throws with a message that matches the embedded TS in the base64 data URL, and the inline
sourceMappingURLformat is appropriate for exercising inline sourcemap handling.tests/specs/run/sourcemap_trailing_content/trailing_content.js (1)
1-13: Trailing-content sourcemap fixture is well-structuredThis neatly captures the “inline sourcemap plus trailing comments/blank lines” scenario, with a clear issue reference and matching error message for debugging expectations.
tests/specs/run/sourcemap_trailing_content/__test__.jsonc (1)
1-5: Trailing-content test spec matches the fixtureThe command, expected output file, and non-zero exit code are all aligned with
trailing_content.jsthrowing as intended and are consistent with the other sourcemap specs.tests/specs/run/sourcemap_external/__test__.jsonc (1)
1-5: Test spec matches fixture behaviorCommand, output file, and exit code all line up with
external.jsthrowing an error once. Looks consistent with otherrunspecs.tests/specs/run/sourcemap_relative_paths/__test__.jsonc (1)
1-9: LGTM! Test configuration is well-structured.The test specification correctly configures the test to run
outside_cwd.js, expects exit code 1 (appropriate since the script throws an error), and references an output file for verification.tests/specs/run/sourcemap_relative_paths/outside_cwd.js (2)
1-6: LGTM! Test case is well-structured.The test correctly sets up a scenario to verify source map handling for files outside the current working directory. The function throws an error that will test whether the stack trace correctly references the mapped source location.
8-8: Test correctly validates absolute path handling in source maps.The expected output file uses
[WILDCARD]pattern matching (part of Deno's test framework) to validate stack traces. The test is working as designed:
- The inline source map contains
file:///absolute/path/outside/original.ts(absolute path intentionally outside CWD)- The expected output pattern
file:///[WILDCARD]original.tscorrectly matches resolved stack traces- Stack trace line numbers (3:1, 6:1) properly resolve through the source map to the original source content
- The implementation correctly handles absolute paths in source maps without security issues
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
Cargo.lockis excluded by!**/*.locktests/specs/npm/cjs_this_in_exports/cjs_this_in_exports/main.outis excluded by!**/*.outtests/specs/run/sourcemap_external/external.js.mapis excluded by!**/*.maptests/specs/run/sourcemap_external/external.js.outis excluded by!**/*.outtests/specs/run/sourcemap_inline_base64/inline_base64.js.outis excluded by!**/*.outtests/specs/run/sourcemap_none/no_sourcemap.js.outis excluded by!**/*.outtests/specs/run/sourcemap_relative_paths/outside_cwd.js.outis excluded by!**/*.outtests/specs/run/sourcemap_trailing_content/trailing_content.js.outis excluded by!**/*.out
📒 Files selected for processing (15)
Cargo.toml(1 hunks)cli/module_loader.rs(2 hunks)cli/rt/run.rs(1 hunks)tests/specs/run/sourcemap_external/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_external/external.js(1 hunks)tests/specs/run/sourcemap_external/original.ts(1 hunks)tests/specs/run/sourcemap_inline_base64/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_inline_base64/inline_base64.js(1 hunks)tests/specs/run/sourcemap_none/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_none/no_sourcemap.js(1 hunks)tests/specs/run/sourcemap_relative_paths/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_relative_paths/outside_cwd.js(1 hunks)tests/specs/run/sourcemap_relative_paths/src/original.ts(1 hunks)tests/specs/run/sourcemap_trailing_content/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_trailing_content/trailing_content.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/specs/run/sourcemap_inline_base64/test.jsonc
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/specs/run/sourcemap_external/original.ts
- tests/specs/run/sourcemap_trailing_content/test.jsonc
- tests/specs/run/sourcemap_inline_base64/inline_base64.js
- tests/specs/run/sourcemap_none/test.jsonc
- tests/specs/run/sourcemap_relative_paths/outside_cwd.js
- Cargo.toml
- cli/rt/run.rs
🧰 Additional context used
🧬 Code graph analysis (1)
cli/module_loader.rs (3)
cli/rt/run.rs (2)
load_external_source_map(586-593)get_source_mapped_source_line(595-616)runtime/worker.rs (1)
get_source_mapped_source_line(1292-1303)cli/graph_container.rs (2)
graph(26-26)graph(162-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug linux-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug macos-x86_64
🔇 Additional comments (9)
tests/specs/run/sourcemap_relative_paths/__test__.jsonc (1)
1-9: LGTM!The test specification is well-formed and correctly configured to test source map handling with relative paths outside the current working directory.
tests/specs/run/sourcemap_none/no_sourcemap.js (1)
1-6: LGTM!This baseline test correctly verifies that JavaScript execution works normally without a source map.
tests/specs/run/sourcemap_relative_paths/src/original.ts (1)
1-6: LGTM!This original TypeScript source file is correctly structured for source map testing.
tests/specs/run/sourcemap_trailing_content/trailing_content.js (1)
1-13: LGTM!This test correctly validates that inline source maps remain valid when followed by trailing content, addressing issue #21988.
tests/specs/run/sourcemap_external/__test__.jsonc (1)
1-5: LGTM!The test specification is correctly configured to test external source map loading functionality.
tests/specs/run/sourcemap_external/external.js (1)
1-8: LGTM!This test correctly exercises external source map loading by referencing an external
.mapfile.cli/module_loader.rs (3)
1213-1231: LGTM!The refactored
get_source_mapmethod correctly loads prepared modules and extracts inline source maps. The scheme filtering appropriately restricts lookups to external module types.
1292-1304: LGTM!The
extract_source_linehelper function is well-implemented with an efficient approach usingsplit('\n').nth()that avoids allocating all lines. The comment explaining why.lines()is not used is helpful, and the out-of-bounds handling provides a clear warning message.
1251-1289: The review comment incorrectly conflates error formatting with code loading.The original concern about permission handling in
get_source_mapped_source_line(lines 1272-1277) is based on a misunderstanding of the method's purpose and design. The method is designed to retrieve source files where cached files are returned, and for local files, they are read from disk.This method is used exclusively for error formatting and stack trace generation in diagnostic contexts (as seen in worker.rs), not for code execution or loading. The design of
get_cached_source_or_localexplicitly restricts access to cached or local files, which is appropriate for this use case. The use of&Default::default()for permissions is intentional and correct for a diagnostics context—it differs from code loading paths for good reason.The method does not require explicit permission checks at the call site because it operates in a different security model than code loading: error formatting and stack trace generation have fundamentally different security requirements than code execution.
Likely an incorrect or invalid review comment.
| fn load_external_source_map( | ||
| &self, | ||
| source_map_url: &str, | ||
| ) -> Option<Cow<'_, [u8]>> { | ||
| let specifier = resolve_url(source_map_url).ok()?; | ||
|
|
||
| if let Ok(Some(file)) = self | ||
| .0 | ||
| .shared | ||
| .file_fetcher | ||
| .get_cached_source_or_local(&specifier) | ||
| { | ||
| return Some(Cow::Owned(file.source.to_vec())); | ||
| } | ||
|
|
||
| None | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Add permission checks and documentation.
This method has two concerns:
-
Missing permission checks: As previously noted by @bartlomieju, the method accesses source map files via
get_cached_source_or_localwithout explicit permission validation. This could allow unauthorized access to source map files on the filesystem. -
Missing documentation: The method lacks doc comments explaining its purpose, parameters, and return value.
Please verify whether get_cached_source_or_local performs its own permission checks internally. If not, explicit permission validation should be added here. Run the following to check the implementation:
Additionally, consider adding doc comments:
+ /// Loads an external source map from the given URL.
+ /// Returns the source map bytes if available in cache or on the local filesystem.
+ ///
+ /// # Arguments
+ /// * `source_map_url` - The URL of the external source map to load
+ ///
+ /// # Returns
+ /// An optional `Cow<[u8]>` containing the source map bytes
fn load_external_source_map(
&self,
source_map_url: &str,
) -> Option<Cow<'_, [u8]>> {Based on past review comments.
🏁 Script executed:
#!/bin/bash
# Search for get_cached_source_or_local implementation to verify permission handling
ast-grep --pattern $'fn get_cached_source_or_local($$$) {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for get_cached_source_or_local function definition using ripgrep
rg -n "get_cached_source_or_local" --type rust -A 10 -B 2Length of output: 2948
🏁 Script executed:
#!/bin/bash
# Search for permission checks or deno_web related permission handling
rg -n "permission" --type rust cli/module_loader.rs -C 3Length of output: 8306
🏁 Script executed:
#!/bin/bash
# Find where file_fetcher is defined to understand the type
rg -n "file_fetcher\s*:" --type rust -B 5 -A 5Length of output: 71976
Add permission checks and documentation to load_external_source_map.
The method bypasses permission checks when accessing source map files. Remote fetches perform permission checks via permissions.check_specifier(), but local file access through fetch_local does not. Since load_external_source_map loads user-facing debug information that should respect file access permissions, explicit permission validation is required. The method should follow the pattern used by load_asset (lines 732-753), passing FetchPermissionsOptionRef::Restricted with appropriate permissions to enforce consistent security boundaries.
Additionally, add doc comments explaining the method's purpose, parameters, and return value.
🤖 Prompt for AI Agents
In cli/module_loader.rs around lines 1233 to 1249, load_external_source_map
currently skips permission checks for local file access; update it to perform
explicit permission validation like load_asset (lines 732-753) by calling the
file fetcher with FetchPermissionsOptionRef::Restricted and the same permissions
object (e.g., self.0.shared.permissions) so both remote and local source map
loads honor permissions.check_specifier(), and return None on denied access;
also add a short doc comment above the function describing its purpose,
parameters (source_map_url) and return value (Option<Cow<'_, [u8]>>) indicating
it returns the source map bytes if allowed and found.
d95b86d to
3e987b1
Compare
fraidev
changed the title
fraidev
force-pushed
the
native_sourcemaps_core
branch
from
3e987b1 to
c27ca35
Compare
fraidev
changed the title
fraidev
force-pushed
the
native_sourcemaps_core
branch
2 times, most recently
from
dba8e95 to
af7a82e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/module_loader.rs (1)
1233-1249: External sourcemap loading should honor read permissions
load_external_source_mapgoes straight tofile_fetcher.get_cached_source_or_local(&specifier)without any explicitFetchPermissionsOptionRef/CheckSpecifierKindhandling, so it can access files (or cached entries) even when read permissions would otherwise block a direct fetch. That effectively bypasses the permission model for external sourcemaps and mirrors the unresolved concern noted in earlier reviews.Consider routing this through a permission-aware path similar to
load_asset(usingfetch_with_options+FetchPermissionsOptionRef::Restrictedand an appropriateCheckSpecifierKind), or otherwise ensuring that external sourcemap loads cannot read data the main module graph wasn’t allowed to access.
🧹 Nitpick comments (1)
cli/module_loader.rs (1)
1251-1289: Graph-first source-line lookup is solid; align FS fallback with permission modelPreferring
graph.get(&specifier)for JS/JSON modules and bailing out for Wasm/Npm/Node/External keepsget_source_mapped_source_linefocused on real source text and avoids bogus lookups. The FS fallback viafile_fetcher.get_cached_source_or_localis useful for original, source-mapped files, but it repeats the same permission-blind access pattern asload_external_source_map.Once you decide how
load_external_source_mapshould enforce permissions, consider factoring out a small, permission-aware helper for “read file or cache for diagnostics” and reusing it here instead of callingget_cached_source_or_localdirectly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
Cargo.lockis excluded by!**/*.locktests/specs/npm/cjs_this_in_exports/cjs_this_in_exports/main.outis excluded by!**/*.outtests/specs/run/sourcemap_external/external.js.mapis excluded by!**/*.maptests/specs/run/sourcemap_external/external.js.outis excluded by!**/*.outtests/specs/run/sourcemap_inline_base64/inline_base64.js.outis excluded by!**/*.outtests/specs/run/sourcemap_none/no_sourcemap.js.outis excluded by!**/*.outtests/specs/run/sourcemap_relative_paths/outside_cwd.js.outis excluded by!**/*.outtests/specs/run/sourcemap_remote/external.js.outis excluded by!**/*.outtests/specs/run/sourcemap_trailing_content/trailing_content.js.outis excluded by!**/*.out
📒 Files selected for processing (18)
Cargo.toml(1 hunks)cli/module_loader.rs(2 hunks)cli/rt/run.rs(1 hunks)tests/specs/run/sourcemap_external/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_external/external.js(1 hunks)tests/specs/run/sourcemap_external/original.ts(1 hunks)tests/specs/run/sourcemap_inline_base64/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_inline_base64/inline_base64.js(1 hunks)tests/specs/run/sourcemap_none/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_none/no_sourcemap.js(1 hunks)tests/specs/run/sourcemap_relative_paths/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_relative_paths/outside_cwd.js(1 hunks)tests/specs/run/sourcemap_relative_paths/src/original.ts(1 hunks)tests/specs/run/sourcemap_remote/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_remote/external.js(1 hunks)tests/specs/run/sourcemap_remote/original.ts(1 hunks)tests/specs/run/sourcemap_trailing_content/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_trailing_content/trailing_content.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/specs/run/sourcemap_external/external.js
- tests/specs/run/sourcemap_relative_paths/src/original.ts
- tests/specs/run/sourcemap_external/original.ts
- tests/specs/run/sourcemap_trailing_content/trailing_content.js
- tests/specs/run/sourcemap_trailing_content/test.jsonc
- Cargo.toml
- tests/specs/run/sourcemap_remote/test.jsonc
- tests/specs/run/sourcemap_relative_paths/outside_cwd.js
- tests/specs/run/sourcemap_remote/external.js
- tests/specs/run/sourcemap_inline_base64/test.jsonc
- tests/specs/run/sourcemap_none/test.jsonc
- tests/specs/run/sourcemap_none/no_sourcemap.js
- tests/specs/run/sourcemap_remote/original.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
cli/module_loader.rscli/rt/run.rs
cli/module_loader.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Module loading and resolution is handled in
cli/module_loader.rs
Files:
cli/module_loader.rs
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/specs/run/sourcemap_inline_base64/inline_base64.js
tests/specs/**/__test__.jsonc
📄 CodeRabbit inference engine (CLAUDE.md)
Spec tests should be created in
tests/specs/with a__test__.jsoncfile describing test steps and input files
Files:
tests/specs/run/sourcemap_relative_paths/__test__.jsonctests/specs/run/sourcemap_external/__test__.jsonc
tests/specs/**/{__test__.jsonc,*.out}
📄 CodeRabbit inference engine (CLAUDE.md)
Output assertions in spec tests should use
__test__.jsoncinline fields or.outfiles with special matching syntax:[WILDCARD],[WILDLINE],[WILDCHAR],[WILDCHARS(n)],[UNORDERED_START]/[UNORDERED_END], and[# comment]
Files:
tests/specs/run/sourcemap_relative_paths/__test__.jsonctests/specs/run/sourcemap_external/__test__.jsonc
🧠 Learnings (6)
📚 Learning: 2025-11-24T16:19:37.798Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.798Z
Learning: Applies to cli/module_loader.rs : Module loading and resolution is handled in `cli/module_loader.rs`
Applied to files:
cli/module_loader.rscli/rt/run.rs
📚 Learning: 2025-11-24T16:19:37.798Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.798Z
Learning: Applies to tests/specs/**/{__test__.jsonc,*.out} : Output assertions in spec tests should use `__test__.jsonc` inline fields or `.out` files with special matching syntax: `[WILDCARD]`, `[WILDLINE]`, `[WILDCHAR]`, `[WILDCHARS(n)]`, `[UNORDERED_START]`/`[UNORDERED_END]`, and `[# comment]`
Applied to files:
tests/specs/run/sourcemap_inline_base64/inline_base64.jstests/specs/run/sourcemap_relative_paths/__test__.jsonctests/specs/run/sourcemap_external/__test__.jsonc
📚 Learning: 2025-11-24T16:19:37.798Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.798Z
Learning: Applies to tests/specs/**/__test__.jsonc : Spec tests should be created in `tests/specs/` with a `__test__.jsonc` file describing test steps and input files
Applied to files:
tests/specs/run/sourcemap_relative_paths/__test__.jsonctests/specs/run/sourcemap_external/__test__.jsonc
📚 Learning: 2025-11-24T16:19:37.798Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.798Z
Learning: Applies to tests/specs/**/*.out : Use `[UNORDERED_START]` and `[UNORDERED_END]` in spec test output files to match lines in any order for non-deterministic output
Applied to files:
tests/specs/run/sourcemap_relative_paths/__test__.jsonctests/specs/run/sourcemap_external/__test__.jsonc
📚 Learning: 2025-11-24T16:19:37.798Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.798Z
Learning: Applies to tests/specs/**/*.out : Use `[WILDCHAR]` in spec test output files to match the next single character
Applied to files:
tests/specs/run/sourcemap_relative_paths/__test__.jsonc
📚 Learning: 2025-11-24T16:19:37.798Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.798Z
Learning: Applies to tests/specs/**/*.out : Use line comments in spec test output files with format `[# example]`
Applied to files:
tests/specs/run/sourcemap_external/__test__.jsonc
🧬 Code graph analysis (2)
cli/module_loader.rs (1)
cli/rt/run.rs (2)
load_external_source_map(586-593)get_source_mapped_source_line(595-616)
cli/rt/run.rs (3)
cli/module_loader.rs (1)
load_external_source_map(1233-1249)ext/fetch/lib.rs (1)
url(457-459)cli/args/mod.rs (1)
parse(1792-1794)
🔇 Additional comments (7)
tests/specs/run/sourcemap_inline_base64/inline_base64.js (2)
1-6: LGTM!The test setup is correct. The function throws an error that will trigger source map resolution, allowing verification of inline base64 source map support.
8-8: No issues found — source map is valid.The base64-encoded source map decodes correctly and contains a valid source map v3 structure with all required fields (version, sources, sourcesContent, names, mappings) properly formatted.
tests/specs/run/sourcemap_relative_paths/__test__.jsonc (1)
1-9: Spec wiring foroutside_cwdlooks correctThe
tests.outside_cwdentry (args/output/exitCode) matches the spec-runner format and should correctly driveoutside_cwd.jsandoutside_cwd.js.out. No changes needed.Based on learnings, this aligns with the
tests/specs/**/__test__.jsoncconventions.tests/specs/run/sourcemap_external/__test__.jsonc (1)
1-5: Single-test spec for external sourcemap is fineThe top-level spec (args/output/exitCode) cleanly wires
external.jstoexternal.js.outwith exit code 1 and matches the expected spec-test schema.Based on learnings, this follows the accepted
tests/specs/**/__test__.jsoncpattern.cli/rt/run.rs (1)
586-593: Embedded external sourcemap loading looks correctParsing
source_map_url, reading fromshared.modules, and returning the raw bytes (orNoneon failure) is consistent with how standalone modules are stored and avoids unnecessary complexity.cli/module_loader.rs (2)
1213-1231: Prepared-module-based inline sourcemap extraction looks goodResolving the specifier, restricting to supported schemes, then using
load_prepared_module_for_source_map_syncplussource_map_from_codekeepsget_source_mapaligned with the actual prepared module contents and avoids duplicating graph logic.As per coding guidelines, this keeps source-map handling inside
cli/module_loader.rs’s module-loading flow.
1292-1304:extract_source_linehelper is efficient and reusableThe helper avoids allocating a full
Vecof lines by usingsplit('\n').nth(line_number)and centralizes the out-of-bounds warning formatting, which simplifies both call sites.
fraidev
force-pushed
the
native_sourcemaps_core
branch
from
af7a82e to
1a388bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/module_loader.rs (1)
1233-1249: External sourcemap loading should respect permissions (matches earlier concern)
load_external_source_mapgoes straight tofile_fetcher.get_cached_source_or_local(&specifier)without going through the permission-awarefetch_with_options/FetchPermissionsOptionRefflow you use inload_asset. That means externalsourceMappingURLentries can cause local or cached files to be consulted outside the usual static/dynamic permission checks.Given earlier discussion on this function, it would be safer and more consistent to:
- Route external sourcemap loads through a permission-checked path (mirroring
load_asset’s use ofCheckSpecifierKindandFetchPermissionsOptionRef::Restricted), or- Clearly document and intentionally justify why sourcemap resolution is exempt from those checks.
Right now the earlier permission concern still applies to this implementation.
🧹 Nitpick comments (1)
cli/module_loader.rs (1)
1251-1290: Refactored source-line extraction is clearer and avoids extra allocationThe updated
get_source_mapped_source_linelogic is an improvement:
- It explicitly skips non-text-like modules (Wasm/Npm/Node/External) instead of trying to render nonsense.
- It falls back to the cached/local file only when the specifier isn’t in the graph, which matches the intended “original source” behavior.
extract_source_linecentralizes the line splitting logic, avoids allocating aVecof all lines, and preserves the existing warning message for out-of-bounds lines.This keeps behavior consistent while simplifying the implementation.
Also applies to: 1292-1304
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
Cargo.lockis excluded by!**/*.locktests/specs/npm/cjs_this_in_exports/cjs_this_in_exports/main.outis excluded by!**/*.outtests/specs/run/sourcemap_external/external.js.mapis excluded by!**/*.maptests/specs/run/sourcemap_external/external.js.outis excluded by!**/*.outtests/specs/run/sourcemap_inline_base64/inline_base64.js.outis excluded by!**/*.outtests/specs/run/sourcemap_none/no_sourcemap.js.outis excluded by!**/*.outtests/specs/run/sourcemap_relative_paths/outside_cwd.js.outis excluded by!**/*.outtests/specs/run/sourcemap_remote/external.js.outis excluded by!**/*.outtests/specs/run/sourcemap_trailing_content/trailing_content.js.outis excluded by!**/*.out
📒 Files selected for processing (18)
Cargo.toml(2 hunks)cli/module_loader.rs(2 hunks)cli/rt/run.rs(1 hunks)tests/specs/run/sourcemap_external/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_external/external.js(1 hunks)tests/specs/run/sourcemap_external/original.ts(1 hunks)tests/specs/run/sourcemap_inline_base64/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_inline_base64/inline_base64.js(1 hunks)tests/specs/run/sourcemap_none/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_none/no_sourcemap.js(1 hunks)tests/specs/run/sourcemap_relative_paths/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_relative_paths/outside_cwd.js(1 hunks)tests/specs/run/sourcemap_relative_paths/src/original.ts(1 hunks)tests/specs/run/sourcemap_remote/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_remote/external.js(1 hunks)tests/specs/run/sourcemap_remote/original.ts(1 hunks)tests/specs/run/sourcemap_trailing_content/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_trailing_content/trailing_content.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/specs/run/sourcemap_remote/external.js
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/specs/run/sourcemap_external/external.js
- Cargo.toml
- tests/specs/run/sourcemap_remote/test.jsonc
- tests/specs/run/sourcemap_relative_paths/src/original.ts
- tests/specs/run/sourcemap_external/test.jsonc
- tests/specs/run/sourcemap_none/test.jsonc
- tests/specs/run/sourcemap_trailing_content/test.jsonc
- tests/specs/run/sourcemap_relative_paths/outside_cwd.js
- tests/specs/run/sourcemap_relative_paths/test.jsonc
- tests/specs/run/sourcemap_inline_base64/inline_base64.js
- tests/specs/run/sourcemap_remote/original.ts
- tests/specs/run/sourcemap_none/no_sourcemap.js
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
cli/rt/run.rscli/module_loader.rs
tests/specs/**/__test__.jsonc
📄 CodeRabbit inference engine (CLAUDE.md)
Spec tests should be created in
tests/specs/with a__test__.jsoncfile describing test steps and input files
Files:
tests/specs/run/sourcemap_inline_base64/__test__.jsonc
tests/specs/**/{__test__.jsonc,*.out}
📄 CodeRabbit inference engine (CLAUDE.md)
Output assertions in spec tests should use
__test__.jsoncinline fields or.outfiles with special matching syntax:[WILDCARD],[WILDLINE],[WILDCHAR],[WILDCHARS(n)],[UNORDERED_START]/[UNORDERED_END], and[# comment]
Files:
tests/specs/run/sourcemap_inline_base64/__test__.jsonc
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/specs/run/sourcemap_trailing_content/trailing_content.jstests/specs/run/sourcemap_external/original.ts
cli/module_loader.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Module loading and resolution is handled in
cli/module_loader.rs
Files:
cli/module_loader.rs
🧠 Learnings (4)
📚 Learning: 2025-11-24T16:19:37.798Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.798Z
Learning: Applies to cli/module_loader.rs : Module loading and resolution is handled in `cli/module_loader.rs`
Applied to files:
cli/rt/run.rscli/module_loader.rs
📚 Learning: 2025-11-24T16:19:37.798Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.798Z
Learning: Applies to tests/specs/**/{__test__.jsonc,*.out} : Output assertions in spec tests should use `__test__.jsonc` inline fields or `.out` files with special matching syntax: `[WILDCARD]`, `[WILDLINE]`, `[WILDCHAR]`, `[WILDCHARS(n)]`, `[UNORDERED_START]`/`[UNORDERED_END]`, and `[# comment]`
Applied to files:
tests/specs/run/sourcemap_inline_base64/__test__.jsonctests/specs/run/sourcemap_trailing_content/trailing_content.js
📚 Learning: 2025-11-24T16:19:37.798Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.798Z
Learning: Applies to tests/specs/**/__test__.jsonc : Spec tests should be created in `tests/specs/` with a `__test__.jsonc` file describing test steps and input files
Applied to files:
tests/specs/run/sourcemap_inline_base64/__test__.jsonc
📚 Learning: 2025-11-24T16:19:37.798Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.798Z
Learning: Applies to tests/specs/**/*.out : Use line comments in spec test output files with format `[# example]`
Applied to files:
tests/specs/run/sourcemap_trailing_content/trailing_content.js
🧬 Code graph analysis (2)
cli/rt/run.rs (1)
cli/module_loader.rs (1)
load_external_source_map(1233-1249)
cli/module_loader.rs (1)
cli/rt/run.rs (2)
load_external_source_map(586-593)get_source_mapped_source_line(595-616)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: test debug linux-aarch64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: build libs
🔇 Additional comments (5)
tests/specs/run/sourcemap_external/original.ts (1)
1-6: TS fixture is fineSimple throw helper is clear and sufficient for the external sourcemap test; no changes needed.
tests/specs/run/sourcemap_inline_base64/__test__.jsonc (1)
1-5: Spec descriptor matches test harness conventionsArgs, output, and exitCode are wired as expected for this run spec; aligns with the JSONC spec-test format.
tests/specs/run/sourcemap_trailing_content/trailing_content.js (1)
1-13: Trailing-content sourcemap fixture looks correctThe inline
sourceMappingURLplus extra comments/blank lines is a good reproduction of the trailing-content scenario; nothing problematic here.cli/rt/run.rs (1)
586-593: Embedded loader external sourcemap support is sound
load_external_source_maponly consults the in-memoryStandaloneModulesstore and returns cloned bytes, so it doesn’t introduce new IO paths or permission concerns in the standalone runtime. Behavior is consistent withget_source_map.cli/module_loader.rs (1)
1213-1231: Switchingget_source_mapto use prepared modules is appropriateUsing
load_prepared_module_for_source_map_syncand deriving the sourcemap viasource_map_from_codeties sourcemap lookup directly to the emitted code, which is exactly what the native sourcemap path needs. The scheme filter guards non-external schemes appropriately.
fraidev
force-pushed
the
native_sourcemaps_core
branch
from
1a388bf to
913ee62
Compare
2 participants
Needs:
Related:
#4499
#21988