[PR #3604] [CLOSED] refactor(go): parse module records once at the build-info owner #5853

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3604
Author: @LucaBienz
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: refactor/go-module-records-owner


📝 Commits (1)

  • 7158746 refactor(go): parse module records once at the build-info owner

📊 Changes

3 files changed (+36 additions, -24 deletions)

View changed files

📝 src/go/GoBinaryImage.ts (+11 -3)
📝 src/go/GoBinaryProvider.ts (+1 -21)
📝 tests/boundary/go/goBinaryProvider.test.ts (+24 -0)

📄 Description

Summary

GoBinaryImage validates the Go module framing and decodes the module body, but GoBinaryProvider re-derived that body itself: it decoded module_bytes_base64 back into bytes and re-applied the 16-byte framing offsets (subarray(16, -16)) before parsing module records. Two files encoded the same framing rule. This PR makes the image reader the single owner: it returns the parsed records as build_info.module, and the provider uses them directly. No issue is linked; this is a small ownership cleanup within the existing Go reader.

Problem and expected behavior

No user-visible failure today. The brittleness is that a change to module framing in GoBinaryImage would leave the provider silently slicing the wrong bytes on the non-UTF-8 path, and that path had no test. Expected behavior is unchanged: the same inspect_go_binary report for every input.

Change and scope

  • src/go/GoBinaryImage.ts: parse module records once from the already-validated body (parseGoModuleText for UTF-8 text, parseGoModuleBytes otherwise, same selection as before) and return them on GoBuildInfo.module.
  • src/go/GoBinaryProvider.ts: use info.module; remove the base64 round trip, the framing re-slice, and the duplicated parse branch. Also remove the two pre-parse output-size gates: build-info strings are capped at 1 MiB in aggregate (MAX_BUILD_INFO_BYTES), so they could not exceed the 8 MiB output budget, and the serialized report check that enforces that budget is kept.
  • Not included: any change to the module text parser, output schema, or limits.

Contribution scope: ordinary refactor within an existing in-process reader; maintainer scope decision not applicable.

Contract and boundary impact

  • Semantic owner and earliest changed stage: GoBinaryImage (go adapter layer) now owns module record parsing in addition to framing validation.
  • Contribution scope fit and linked maintainer decision: not applicable (no new capability).
  • CLI and MCP/tool-catalog contract: none; goBinarySchema and the report shape are unchanged.
  • Provider, bridge, target-format, or platform compatibility: none.
  • Evidence, artifact, provenance, or reconstruction contract: none; report output is byte-identical on the real binaries checked below.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): 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/go/goBinaryProvider.test.ts adds provider coverage for a module body with one non-UTF-8 line (records around it are parsed, the exact line bytes are kept in unparsed_line_bytes_base64, complete is false, and the non-UTF-8 limitation is reported). This path previously had no test.
  • Base reproduction or other evidence: behavior-preserving refactor, so there is no failure to reproduce. The new test was run against the base revision before the source change and passed (5 passed, 1 skipped), confirming it records existing behavior. Executed comparison on real Go-compiled PE binaries: built main and this branch, ran rea inspect-go-binary <path> --json, and compared normalized_result byte for byte. Both were identical:
    • gh.exe (GitHub CLI): pe/amd64, inline, go1.26.5, 166 dependencies, module body 16014 bytes, complete: true.
    • oh-my-posh.exe: pe/amd64, inline, go1.26.0-X:jsonv2, 68 dependencies, module body 6823 bytes, complete: true.
  • User-visible CLI/MCP output (if applicable): unchanged (see comparison above).
  • Remaining proof gaps: no real binary with a non-UTF-8 module body was available, so that path is covered only by the authored fixture. Real ELF and Mach-O binaries were not checked; those formats are covered by the existing authored fixtures in the focused tests. CPU and memory were not measured; the change removes one base64 decode and buffer copy on the non-UTF-8 path, and no speedup is claimed.

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

Environment: Windows 11, Node 24.14.1, npm 11.11.0 (.nvmrc pins 24.18.0).

  • npm run test:focused -- tests/boundary/go/goBinaryProvider.test.ts on the base revision with only the new test: 5 passed, 1 skipped (Windows symlink guard).
  • npm run check:fast: typecheck and lint passed (lint includes module-boundary verification).
  • npm run test:focused -- tests/boundary/go/goBinaryProvider.test.ts tests/boundary/go/goBinaryImage.test.ts tests/boundary/go/goBinaryMappings.test.ts tests/boundary/go/goModuleText.test.ts tests/boundary/mcp/goBinaryMcp.test.ts tests/acceptance/goBinaryCli.test.ts src/application/go/GoBinaryService.test.ts: 7 files passed; 160 tests passed, 3 skipped (Windows symlink/ELOOP guards).
  • Pre-commit hook (oxfmt, oxlint --fix): no changes.
  • Real-binary before/after comparison described above: identical normalized_result for both binaries.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable (in-process reader); validated on Windows only.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no new reads, processes, or network access; resource limits unchanged.

Author readiness

Human owner: @LucaBienz

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

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/3604 **Author:** [@LucaBienz](https://github.com/LucaBienz) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `refactor/go-module-records-owner` --- ### 📝 Commits (1) - [`7158746`](https://github.com/morluto/rea/commit/715874630c0b7d6aa350e7c83570c919ed705ba3) refactor(go): parse module records once at the build-info owner ### 📊 Changes **3 files changed** (+36 additions, -24 deletions) <details> <summary>View changed files</summary> 📝 `src/go/GoBinaryImage.ts` (+11 -3) 📝 `src/go/GoBinaryProvider.ts` (+1 -21) 📝 `tests/boundary/go/goBinaryProvider.test.ts` (+24 -0) </details> ### 📄 Description ## Summary `GoBinaryImage` validates the Go module framing and decodes the module body, but `GoBinaryProvider` re-derived that body itself: it decoded `module_bytes_base64` back into bytes and re-applied the 16-byte framing offsets (`subarray(16, -16)`) before parsing module records. Two files encoded the same framing rule. This PR makes the image reader the single owner: it returns the parsed records as `build_info.module`, and the provider uses them directly. No issue is linked; this is a small ownership cleanup within the existing Go reader. ## Problem and expected behavior No user-visible failure today. The brittleness is that a change to module framing in `GoBinaryImage` would leave the provider silently slicing the wrong bytes on the non-UTF-8 path, and that path had no test. Expected behavior is unchanged: the same `inspect_go_binary` report for every input. ## Change and scope - `src/go/GoBinaryImage.ts`: parse module records once from the already-validated body (`parseGoModuleText` for UTF-8 text, `parseGoModuleBytes` otherwise, same selection as before) and return them on `GoBuildInfo.module`. - `src/go/GoBinaryProvider.ts`: use `info.module`; remove the base64 round trip, the framing re-slice, and the duplicated parse branch. Also remove the two pre-parse output-size gates: build-info strings are capped at 1 MiB in aggregate (`MAX_BUILD_INFO_BYTES`), so they could not exceed the 8 MiB output budget, and the serialized report check that enforces that budget is kept. - Not included: any change to the module text parser, output schema, or limits. Contribution scope: ordinary refactor within an existing in-process reader; maintainer scope decision not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: `GoBinaryImage` (go adapter layer) now owns module record parsing in addition to framing validation. - Contribution scope fit and linked maintainer decision: not applicable (no new capability). - CLI and MCP/tool-catalog contract: none; `goBinarySchema` and the report shape are unchanged. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none; report output is byte-identical on the real binaries checked below. - Target selection, authority, upstream behavior, and cleanup details (if applicable): 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/go/goBinaryProvider.test.ts` adds provider coverage for a module body with one non-UTF-8 line (records around it are parsed, the exact line bytes are kept in `unparsed_line_bytes_base64`, `complete` is false, and the non-UTF-8 limitation is reported). This path previously had no test. - Base reproduction or other evidence: behavior-preserving refactor, so there is no failure to reproduce. The new test was run against the base revision before the source change and passed (5 passed, 1 skipped), confirming it records existing behavior. Executed comparison on real Go-compiled PE binaries: built `main` and this branch, ran `rea inspect-go-binary <path> --json`, and compared `normalized_result` byte for byte. Both were identical: - `gh.exe` (GitHub CLI): pe/amd64, inline, go1.26.5, 166 dependencies, module body 16014 bytes, `complete: true`. - `oh-my-posh.exe`: pe/amd64, inline, go1.26.0-X:jsonv2, 68 dependencies, module body 6823 bytes, `complete: true`. - User-visible CLI/MCP output (if applicable): unchanged (see comparison above). - Remaining proof gaps: no real binary with a non-UTF-8 module body was available, so that path is covered only by the authored fixture. Real ELF and Mach-O binaries were not checked; those formats are covered by the existing authored fixtures in the focused tests. CPU and memory were not measured; the change removes one base64 decode and buffer copy on the non-UTF-8 path, and no speedup is claimed. 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 Environment: Windows 11, Node 24.14.1, npm 11.11.0 (`.nvmrc` pins 24.18.0). - `npm run test:focused -- tests/boundary/go/goBinaryProvider.test.ts` on the base revision with only the new test: 5 passed, 1 skipped (Windows symlink guard). - `npm run check:fast`: typecheck and lint passed (lint includes module-boundary verification). - `npm run test:focused -- tests/boundary/go/goBinaryProvider.test.ts tests/boundary/go/goBinaryImage.test.ts tests/boundary/go/goBinaryMappings.test.ts tests/boundary/go/goModuleText.test.ts tests/boundary/mcp/goBinaryMcp.test.ts tests/acceptance/goBinaryCli.test.ts src/application/go/GoBinaryService.test.ts`: 7 files passed; 160 tests passed, 3 skipped (Windows symlink/ELOOP guards). - Pre-commit hook (oxfmt, oxlint --fix): no changes. - Real-binary before/after comparison described above: identical `normalized_result` for both binaries. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (in-process reader); validated on Windows only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new reads, processes, or network access; resource limits unchanged. ## Author readiness Human owner: @LucaBienz - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## 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:09:55 +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#5853
No description provided.