[PR #2873] [MERGED] fix(source-maps): keep sources when sourcesContent length differs #5456

Closed
opened 2026-10-31 01:06:47 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

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

Base: main ← Head: fix/source-map-short-contents


📝 Commits (1)

  • 739df91 fix(source-maps): keep sources when sourcesContent length differs

📊 Changes

6 files changed (+114 additions, -29 deletions)

View changed files

📝 src/application/javascript/JavaScriptArtifactAnalysis.ts (+2 -2)
📝 src/domain/sourceMapContents.test.ts (+14 -6)
📝 src/domain/sourceMapContents.ts (+9 -6)
📝 src/javascript/sourceMaps/SourceMapFormat.ts (+0 -13)
📝 tests/boundary/browser/webSourceMapFetcher.test.ts (+44 -1)
📝 tests/boundary/filesystem/javascriptArtifactReconstruction.test.ts (+45 -1)

📄 Description

Summary

Fixes #2821.

When a source map's sourcesContent array is shorter or longer than sources, keep every source declaration and whatever embedded text is present. This applies to local application analysis and to browser source-map collection.

Problem and expected behavior

With sources: ["a.ts", "b.ts"] and sourcesContent: ["export const answer = 42;"]:

  • Local analysis (analyze-javascript-application) marked the map invalid. It dropped both originals and reported the graph as partial.
  • Browser collection rejected the whole map.

Padding the array with one null made both work.

ECMA-426 DecodeSourceMapSources, step 6.5, reads a content entry only when its index is within sourcesContent, and starts each source with null content. A shorter array leaves trailing sources without content. Extra entries add no sources, because the loop runs over sources.

Change and scope

  • hasValidSourceMapContents drops the source-count parameter and the equality check. It now checks only that the field is absent, null, or an array of strings and nulls.
  • The local error message names the remaining rule.
  • Browser collection drops its separate content check. The leaf schema (z.array(z.string().nullable()).nullable().optional()) already rejects bad entry types, and raw content records are still counted against the budget before parsing.
  • Both collectors already read content by index with a null fallback. The trace path (TraceSourceMap) already accepted short arrays (TraceSourceMap.test.ts maps two sources to one content entry), so all three paths now agree.

Not included: a non-string, non-null entry still makes the map invalid. The spec allows treating such an entry as null with an optional error. That is a separate policy change.

This is an ordinary fix within existing boundaries, so no maintainer scope decision is needed.

Contract and boundary impact

  • Semantic owner and earliest changed stage: src/domain/sourceMapContents.ts (content admission), used by local analysis; SourceMapFormat leaf structure for browser collection.

  • Contribution scope fit and linked maintainer decision: not applicable (bug fix in read-only source-map interpretation).

  • CLI and MCP/tool-catalog contract: no schema change. Maps that were rejected are now admitted. The limitation text for invalid content changes to "must be an array of strings or null entries".

  • Provider, bridge, target-format, or platform compatibility: none.

  • Evidence, artifact, provenance, or reconstruction contract: source_index and declaration_index stay aligned with sources. Sources past the end of sourcesContent report no content.

  • Target selection, authority, upstream behavior, and cleanup details: not applicable.

  • Generated metadata, package, or installation impact: none.

  • Read existing files or captured data.

Evidence and regression coverage

  • Tests added or updated:

    • tests/boundary/filesystem/javascriptArtifactReconstruction.test.ts: shorter and longer arrays keep both originals (a.ts with content, b.ts without), and coverage stays complete. Removed the case that expected a length mismatch to be invalid.
    • tests/boundary/browser/webSourceMapFetcher.test.ts: the same two cases for browser collection (included, two originals, artifact only for a.ts). Removed the wrong-length invalid case; the scalar and non-string entry cases still fail.
    • src/domain/sourceMapContents.test.ts: updated for the new signature.
  • Base reproduction: all four new boundary cases fail on origin/main and pass with this change. The script from #2821 now prints { name: 'short', coverage: 'complete', originals: 2 } (previously partial, 0).

  • Remaining proof gaps: browser collection was verified through the boundary test with a stubbed fetch, not against a live browser.

  • 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

  • npm run test:focused -- <every *.test.ts that mentions sourcesContent> (14 files): 276 passed.
  • npm run check:fast: typecheck and lint passed.
  • npm run verify:test-discovery: all 1088 test files discovered exactly once.
  • Reproduction script from #2821 against npm run build:cached: short, padded, and longer maps all report complete with 2 originals.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. Previously rejected maps are now accepted.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable. Linux x64, Node 24.18.0.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: the change only widens input acceptance. The content-record budget still applies before parsing.

Related: #2829 (the reporter's closed PR for this issue) and #2831 (merged; it touched the neighbouring names check in SourceMapFormat.ts). This change is rebased on top of #2831.

#2869 also targets this issue. It allows only shorter arrays (length <= sourceCount), so a longer sourcesContent is still rejected, and it leaves the boundary tests that expect sourcesContent: [] to be invalid. This PR accepts both directions, as step 6 of the spec does, and updates those boundary tests.

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/2873 **Author:** [@rileygramlich](https://github.com/rileygramlich) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/source-map-short-contents` --- ### 📝 Commits (1) - [`739df91`](https://github.com/morluto/rea/commit/739df91ab7a50bf319635ac1cd60fe6d97ddf23a) fix(source-maps): keep sources when sourcesContent length differs ### 📊 Changes **6 files changed** (+114 additions, -29 deletions) <details> <summary>View changed files</summary> 📝 `src/application/javascript/JavaScriptArtifactAnalysis.ts` (+2 -2) 📝 `src/domain/sourceMapContents.test.ts` (+14 -6) 📝 `src/domain/sourceMapContents.ts` (+9 -6) 📝 `src/javascript/sourceMaps/SourceMapFormat.ts` (+0 -13) 📝 `tests/boundary/browser/webSourceMapFetcher.test.ts` (+44 -1) 📝 `tests/boundary/filesystem/javascriptArtifactReconstruction.test.ts` (+45 -1) </details> ### 📄 Description ## Summary Fixes #2821. When a source map's `sourcesContent` array is shorter or longer than `sources`, keep every source declaration and whatever embedded text is present. This applies to local application analysis and to browser source-map collection. ## Problem and expected behavior With `sources: ["a.ts", "b.ts"]` and `sourcesContent: ["export const answer = 42;"]`: - Local analysis (`analyze-javascript-application`) marked the map invalid. It dropped both originals and reported the graph as `partial`. - Browser collection rejected the whole map. Padding the array with one `null` made both work. [ECMA-426 DecodeSourceMapSources](https://tc39.es/ecma426/#sec-decodesourcemapsources), step 6.5, reads a content entry only when its index is within `sourcesContent`, and starts each source with null content. A shorter array leaves trailing sources without content. Extra entries add no sources, because the loop runs over `sources`. ## Change and scope - `hasValidSourceMapContents` drops the source-count parameter and the equality check. It now checks only that the field is absent, null, or an array of strings and nulls. - The local error message names the remaining rule. - Browser collection drops its separate content check. The leaf schema (`z.array(z.string().nullable()).nullable().optional()`) already rejects bad entry types, and raw content records are still counted against the budget before parsing. - Both collectors already read content by index with a null fallback. The trace path (`TraceSourceMap`) already accepted short arrays (`TraceSourceMap.test.ts` maps two sources to one content entry), so all three paths now agree. Not included: a non-string, non-null entry still makes the map invalid. The spec allows treating such an entry as null with an optional error. That is a separate policy change. This is an ordinary fix within existing boundaries, so no maintainer scope decision is needed. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/domain/sourceMapContents.ts` (content admission), used by local analysis; `SourceMapFormat` leaf structure for browser collection. - Contribution scope fit and linked maintainer decision: not applicable (bug fix in read-only source-map interpretation). - CLI and MCP/tool-catalog contract: no schema change. Maps that were rejected are now admitted. The limitation text for invalid content changes to "must be an array of strings or null entries". - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: `source_index` and `declaration_index` stay aligned with `sources`. Sources past the end of `sourcesContent` report no content. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. ## Evidence and regression coverage - Tests added or updated: - `tests/boundary/filesystem/javascriptArtifactReconstruction.test.ts`: shorter and longer arrays keep both originals (`a.ts` with content, `b.ts` without), and coverage stays `complete`. Removed the case that expected a length mismatch to be invalid. - `tests/boundary/browser/webSourceMapFetcher.test.ts`: the same two cases for browser collection (`included`, two originals, artifact only for `a.ts`). Removed the wrong-length invalid case; the scalar and non-string entry cases still fail. - `src/domain/sourceMapContents.test.ts`: updated for the new signature. - Base reproduction: all four new boundary cases fail on `origin/main` and pass with this change. The script from #2821 now prints `{ name: 'short', coverage: 'complete', originals: 2 }` (previously `partial`, `0`). - Remaining proof gaps: browser collection was verified through the boundary test with a stubbed fetch, not against a live browser. - [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 - `npm run test:focused -- <every *.test.ts that mentions sourcesContent>` (14 files): 276 passed. - `npm run check:fast`: typecheck and lint passed. - `npm run verify:test-discovery`: all 1088 test files discovered exactly once. - Reproduction script from #2821 against `npm run build:cached`: short, padded, and longer maps all report `complete` with 2 originals. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Previously rejected maps are now accepted. - Real Hopper/Ghidra, browser, or OS coverage: not applicable. Linux x64, Node 24.18.0. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the change only widens input acceptance. The content-record budget still applies before parsing. Related: #2829 (the reporter's closed PR for this issue) and #2831 (merged; it touched the neighbouring `names` check in `SourceMapFormat.ts`). This change is rebased on top of #2831. #2869 also targets this issue. It allows only shorter arrays (`length <= sourceCount`), so a longer `sourcesContent` is still rejected, and it leaves the boundary tests that expect `sourcesContent: []` to be invalid. This PR accepts both directions, as step 6 of the spec does, and updates those boundary tests. ## 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:06:47 +00:00
Sign in to join this conversation.
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#5456
No description provided.