morluto/rea

Fork 0

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

[PR #5321] [CLOSED] fix(artifacts): note legacy code page readings of unflagged ZIP names #6878

New issue

Closed

opened 2026-11-01 21:43:45 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:43:45 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5321
Author: @darshjoshi
Created: 10/20/2026
Status: ❌ Closed

Base: main ← Head: fix/5320-legacy-zip-name-limitation

📝 Commits (2)

8ff02eb fix(artifacts): note legacy code page readings of unflagged ZIP names
3437d36 test(artifacts): cover legacy code page limitations on ZIP names

📊 Changes

2 files changed (+57 additions, -0 deletions)

View changed files

📝 src/artifacts/ZipArtifactReader.ts (+19 -0)
📝 tests/boundary/filesystem/artifactZipIntegrity.test.ts (+38 -0)

📄 Description

Summary

Fixes #5320.

inspect-artifact reported a ZIP member whose name was written in a legacy code page other than CP437 only as its CP437 reading, with no limitation. 한글.txt stored as CP949 bytes (c7 d1 b1 db 2e 74 78 74) appeared as ╟╤▒█.txt, and nothing in the Evidence told a caller the name might be a misreading.

Problem and expected behavior

Decoding unflagged names as CP437 follows APPNOTE and the decision in #3237, and this PR keeps it. The gap is that a name with high bytes, which other archivers may have written in their own code page, was presented as a confident reading. The expected outcome from the issue: same logical_path, same complete coverage, plus a limitation on the affected occurrence. ASCII-only and UTF-8 names stay unchanged.

Change and scope

projectZipEntry in src/artifacts/ZipArtifactReader.ts now adds one occurrence limitation when all of these are true:

the UTF-8 flag (bit 11) is clear;
there is no valid Unicode Path extra field (which already overrides the name);
the raw name has at least one byte at or above 0x80.

The limitation records the raw name bytes as hex, so a caller can decode them in another code page without re-reading the archive:

ZIP member name bytes c7 d1 b1 db 2e 74 78 74 are not flagged as UTF-8 and were read as CP437; the archiver may have written them in another legacy code page.

This follows the existing pattern for the omitted ./ prefix. ASCII names, including the ASCII control bytes handled in #2365, get no limitation.

Not included: an alternative CP949 (or Shift_JIS, etc.) reading. As the issue notes, a valid CP949 decoding is not proof (naïve.txt also decodes as CP949), so which code pages to try and how to label a candidate seems like a maintainer decision. The raw bytes make that possible later without changing this limitation.

Follow the contribution scope. Unresolved scope, authority, side effects, or required verification block integration. Link the required maintainer scope decision for new providers or capabilities involving target execution, injection, mutation, or protection recovery; ordinary fixes within existing boundaries may state "not applicable".

Contract and boundary impact

Semantic owner and earliest changed stage: ZIP artifact reader (src/artifacts/ZipArtifactReader.ts), entry projection
Contribution scope fit and linked maintainer decision: not applicable, an ordinary fix within existing artifact boundaries
CLI and MCP/tool-catalog contract: no schema change. Affected occurrences gain one string in their existing limitations array.
Provider, bridge, target-format, or platform compatibility: none
Evidence, artifact, provenance, or reconstruction contract: logical_path, member identity, hashes and coverage are unchanged; only the limitation is added
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/filesystem/artifactZipIntegrity.test.ts, new it.each "limits legacy code page readings of %s ZIP member names". It covers unflagged CP949 bytes (limitation expected) and three controls with no limitation: unflagged ASCII, unflagged ASCII control (Icon\r) and flagged UTF-8 한글.txt. Every case also checks complete coverage and a verified hash.
Base reproduction or other evidence: executed. With only ZipArtifactReader.ts restored to 83313650, the CP949 case fails and the three controls pass. With the fix, all 24 tests in the file pass. The probe script from #5320 printed "limitations": [] for ╟╤▒█.txt before the fix and the limitation above after it; plain.txt stays [] and coverage stays complete in both.
User-visible CLI/MCP output (if applicable):

"occurrences": [
  { "logical_path": "plain.txt", "limitations": [] },
  { "logical_path": "╟╤▒█.txt", "limitations": ["ZIP member name bytes c7 d1 b1 db 2e 74 78 74 are not flagged as UTF-8 and were read as CP437; the archiver may have written them in another legacy code page."] }
],
"coverage": { "scope": "container-entries", "status": "complete" }

Remaining proof gaps: only the self-authored fixture from the issue; archives from real Korean or Japanese Windows archivers were not tested. A name that is genuinely CP437 with high bytes (for example é.txt) also gets the limitation, since the bytes alone cannot tell the two apart.

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

Node 24.18.0, npm 11.16.0, macOS arm64:

npm run test:focused -- tests/boundary/filesystem/artifactZipIntegrity.test.ts tests/boundary/filesystem/artifactZipRelativePaths.test.ts tests/acceptance/artifactZipCurrentDirectory.test.ts src/artifacts/ZipArtifactReader.test.ts: 37 passed before the new test was added
node scripts/run-development-tests.mjs changed --base upstream/main (this is test:changed with origin pointing at my fork): 5 files, 45 tests passed
npm run check: passed, including typecheck, lint, format, knip, metadata and architecture guards

Compatibility, safety, and release

Breaking changes or migration steps: none. Callers that compare exact limitations arrays for unflagged high-byte names will see one more entry.
Real Hopper/Ghidra, browser, or OS coverage: not applicable (no provider involved); macOS only
Package or release metadata impact: none
Security, privacy, process, or containment review: no new effects

Author readiness

Human owner:

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/5321 **Author:** [@darshjoshi](https://github.com/darshjoshi) **Created:** 10/20/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/5320-legacy-zip-name-limitation` --- ### 📝 Commits (2) - [`8ff02eb`](https://github.com/morluto/rea/commit/8ff02eb56080a11753549496c1a0c03afc2f827c) fix(artifacts): note legacy code page readings of unflagged ZIP names - [`3437d36`](https://github.com/morluto/rea/commit/3437d36f119fa928fc1ddd8c1464c42fa96b0bfc) test(artifacts): cover legacy code page limitations on ZIP names ### 📊 Changes **2 files changed** (+57 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `src/artifacts/ZipArtifactReader.ts` (+19 -0) 📝 `tests/boundary/filesystem/artifactZipIntegrity.test.ts` (+38 -0) </details> ### 📄 Description ## Summary Fixes #5320. `inspect-artifact` reported a ZIP member whose name was written in a legacy code page other than CP437 only as its CP437 reading, with no limitation. `한글.txt` stored as CP949 bytes (`c7 d1 b1 db 2e 74 78 74`) appeared as `╟╤▒█.txt`, and nothing in the Evidence told a caller the name might be a misreading. ## Problem and expected behavior Decoding unflagged names as CP437 follows APPNOTE and the decision in #3237, and this PR keeps it. The gap is that a name with high bytes, which other archivers may have written in their own code page, was presented as a confident reading. The expected outcome from the issue: same `logical_path`, same `complete` coverage, plus a limitation on the affected occurrence. ASCII-only and UTF-8 names stay unchanged. ## Change and scope `projectZipEntry` in `src/artifacts/ZipArtifactReader.ts` now adds one occurrence limitation when all of these are true: - the UTF-8 flag (bit 11) is clear; - there is no valid Unicode Path extra field (which already overrides the name); - the raw name has at least one byte at or above `0x80`. The limitation records the raw name bytes as hex, so a caller can decode them in another code page without re-reading the archive: ``` ZIP member name bytes c7 d1 b1 db 2e 74 78 74 are not flagged as UTF-8 and were read as CP437; the archiver may have written them in another legacy code page. ``` This follows the existing pattern for the omitted `./` prefix. ASCII names, including the ASCII control bytes handled in #2365, get no limitation. Not included: an alternative CP949 (or Shift_JIS, etc.) reading. As the issue notes, a valid CP949 decoding is not proof (`naïve.txt` also decodes as CP949), so which code pages to try and how to label a candidate seems like a maintainer decision. The raw bytes make that possible later without changing this limitation. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Unresolved scope, authority, side effects, or required verification block integration. Link the required maintainer scope decision for new providers or capabilities involving target execution, injection, mutation, or protection recovery; ordinary fixes within existing boundaries may state "not applicable". ## Contract and boundary impact - Semantic owner and earliest changed stage: ZIP artifact reader (`src/artifacts/ZipArtifactReader.ts`), entry projection - Contribution scope fit and linked maintainer decision: not applicable, an ordinary fix within existing artifact boundaries - CLI and MCP/tool-catalog contract: no schema change. Affected occurrences gain one string in their existing `limitations` array. - Provider, bridge, target-format, or platform compatibility: none - Evidence, artifact, provenance, or reconstruction contract: `logical_path`, member identity, hashes and coverage are unchanged; only the limitation is added - 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/filesystem/artifactZipIntegrity.test.ts`, new `it.each` "limits legacy code page readings of %s ZIP member names". It covers unflagged CP949 bytes (limitation expected) and three controls with no limitation: unflagged ASCII, unflagged ASCII control (`Icon\r`) and flagged UTF-8 `한글.txt`. Every case also checks `complete` coverage and a verified hash. - Base reproduction or other evidence: executed. With only `ZipArtifactReader.ts` restored to `83313650`, the CP949 case fails and the three controls pass. With the fix, all 24 tests in the file pass. The probe script from #5320 printed `"limitations": []` for `╟╤▒█.txt` before the fix and the limitation above after it; `plain.txt` stays `[]` and coverage stays `complete` in both. - User-visible CLI/MCP output (if applicable): ```json "occurrences": [ { "logical_path": "plain.txt", "limitations": [] }, { "logical_path": "╟╤▒█.txt", "limitations": ["ZIP member name bytes c7 d1 b1 db 2e 74 78 74 are not flagged as UTF-8 and were read as CP437; the archiver may have written them in another legacy code page."] } ], "coverage": { "scope": "container-entries", "status": "complete" } ``` - Remaining proof gaps: only the self-authored fixture from the issue; archives from real Korean or Japanese Windows archivers were not tested. A name that is genuinely CP437 with high bytes (for example `é.txt`) also gets the limitation, since the bytes alone cannot tell the two apart. 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 Node 24.18.0, npm 11.16.0, macOS arm64: - `npm run test:focused -- tests/boundary/filesystem/artifactZipIntegrity.test.ts tests/boundary/filesystem/artifactZipRelativePaths.test.ts tests/acceptance/artifactZipCurrentDirectory.test.ts src/artifacts/ZipArtifactReader.test.ts`: 37 passed before the new test was added - `node scripts/run-development-tests.mjs changed --base upstream/main` (this is `test:changed` with `origin` pointing at my fork): 5 files, 45 tests passed - `npm run check`: passed, including typecheck, lint, format, knip, metadata and architecture guards ## Compatibility, safety, and release - Breaking changes or migration steps: none. Callers that compare exact `limitations` arrays for unflagged high-byte names will see one more entry. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (no provider involved); macOS only - Package or release metadata impact: none - Security, privacy, process, or containment review: no new effects ## Author readiness  Human owner:  - [ ] 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-01 21:43:45 +00:00

closed this issue
added the
pull-request
label

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

No description provided.

Rows
Columns