[PR #6325] [MERGED] fix(pyinstaller): report changed admission as integrity #7285

Closed
opened 2026-11-01 21:46:46 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6325
Author: @Bilalmesk
Created: 10/22/2026
Status: ✅ Merged
Merged: 10/22/2026
Merged by: @morluto

Base: main ← Head: fix/pyinstaller-admission-integrity


📝 Commits (1)

  • 8741b7f fix(pyinstaller): report changed admission as integrity

📊 Changes

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

View changed files

➕ src/artifacts/pyinstaller/PyInstallerFileAdmission.test.ts (+33 -0)
📝 src/artifacts/pyinstaller/PyInstallerFileAdmission.ts (+7 -0)

📄 Description

Summary

Fixes #6067.

extract_artifact previously classified a source replacement observed during PyInstaller reader admission (RegularFileChangedError) as a generic io failure: artifact_operation_failed / execution_failure with "PyInstaller archive could not be opened" and file-access advice. The descriptor had opened successfully; the selected name no longer matched it.

The reader admission mapper now preserves that changed-file condition as integrity, with PyInstaller source changed during admission: <path>. The existing stable-artifact guidance applies. No new error code or retryability change.

Problem and expected behavior

Trigger: during extract_artifact, the selected PyInstaller source is replaced with a byte-identical regular file after the descriptor opens and before admission finishes its identity check. openRegularFile raises RegularFileChangedError; pyInstallerFileAdmissionFailure had no branch for that type and fell through to io.

Expected: preserve the changed-source condition as an integrity outcome rather than an unreadable archive. Cancellation, cleanup retention, non-regular input, and unknown I/O classifications must remain unchanged. The reader's existing post-read source check already uses integrity.

Change and scope

  • Added the missing RegularFileChangedError branch to pyInstallerFileAdmissionFailure in src/artifacts/pyinstaller/PyInstallerFileAdmission.ts.
  • Added a reader-level regression test in src/artifacts/pyinstaller/PyInstallerFileAdmission.test.ts.
  • The shared helper is also reached by private PYZ-image admission, so that path receives the same classification.
  • Intentionally not included: no extraction-path changes, no new error code, no retryability change, and no changes to the shared ArtifactOperationFailure.ts projector.

This is a routine fix within the existing read-only artifact boundary. It adds no execution, injection, mutation, network, or protection-recovery authority, so a separate maintainer scope decision is not applicable.

Contract and boundary impact

  • Semantic owner and earliest changed stage: PyInstaller reader admission mapping (pyInstallerFileAdmissionFailure), reached from PyInstallerArtifactReader.#admit and private PYZ-image admission.
  • Contribution scope fit and linked maintainer decision: existing read-only artifact handling; no new authority or capability; not applicable.
  • CLI and MCP/tool-catalog contract: no schema, argument, or catalog change. For this specific changed-source condition, the failure reason moves from io to the existing integrity classification and its existing recovery guidance.
  • Provider, bridge, target-format, or platform compatibility: PyInstaller only. The regression test is platform-independent and mocks the admission boundary.
  • Evidence, artifact, provenance, or reconstruction contract: no artifact identity or evidence format change. The original cause is retained on the reader failure, and changed input is still refused rather than extracted.
  • Target selection, authority, upstream behavior, and cleanup details: no new target authority. Cancellation and cleanup observation handling remain unchanged.
  • 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: src/artifacts/pyinstaller/PyInstallerFileAdmission.test.ts makes openRegularFile throw RegularFileChangedError and asserts that reader admission reports integrity with the changed-source message while still requesting symlink-rejecting admission.
  • Base reproduction or other evidence: issue #6067 reports the pre-fix production MCP output: artifact_operation_failed / execution_failure, reason: io, detail PyInstaller archive could not be opened. Source-derived: on the base revision the same cause reaches the generic fallback because the branch added here is absent. I did not rerun the end-to-end MCP reproduction after the fix on this host.
  • User-visible CLI/MCP output (if applicable): the pre-fix output is quoted above from issue #6067. Post-fix end-to-end output was not captured locally; the reader-level regression covers the changed reason, and existing presentation code maps integrity to the stable-artifact guidance.
  • Remaining proof gaps: the regression isolates reader admission with a mocked openRegularFile instead of reproducing a live rename race. Full-repository typecheck did not complete locally; see the validation gap below. CI source workflows are still awaiting maintainer approval.

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 node_modules/vitest/vitest.mjs run src/artifacts/pyinstaller --maxWorkers=1 — 7 test files, 121 tests passed after rebasing onto the latest origin/main.
  • npm run lint — 0 errors; 2,233 existing warnings reported, none introduced by this change.
  • npm run verify:test-discovery — all 2,042 test files discovered exactly once.
  • npm run format:check — all 5,051 files formatted.
  • Scoped tsc --noEmit over the changed files and their dependency graph — passed.
  • Full npm run typecheck — did not complete; the local run timed out after 20 minutes. Treat full-repository typecheck as unverified locally.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. The only observable change is the reason/recovery for the specific changed-source admission condition; callers that matched the old io result for that case will now see the intended integrity result.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable; this touches PyInstaller admission classification only.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no new authority, execution, network access, or persistent writes. The selected file is still refused when its identity changes, and the original error cause remains available.

Author readiness

Human owner: @Bilalmesk

The human owner marked the PR ready for review. The declarations below remain unchecked pending the owner's explicit confirmation, per the repository's AI-contribution policy.

  • 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/6325 **Author:** [@Bilalmesk](https://github.com/Bilalmesk) **Created:** 10/22/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/pyinstaller-admission-integrity` --- ### 📝 Commits (1) - [`8741b7f`](https://github.com/morluto/rea/commit/8741b7fbc8bc7dd15bab5fe137780f143ebc8fe4) fix(pyinstaller): report changed admission as integrity ### 📊 Changes **2 files changed** (+40 additions, -0 deletions) <details> <summary>View changed files</summary> ➕ `src/artifacts/pyinstaller/PyInstallerFileAdmission.test.ts` (+33 -0) 📝 `src/artifacts/pyinstaller/PyInstallerFileAdmission.ts` (+7 -0) </details> ### 📄 Description ## Summary Fixes #6067. `extract_artifact` previously classified a source replacement observed during PyInstaller reader admission (`RegularFileChangedError`) as a generic `io` failure: `artifact_operation_failed` / `execution_failure` with "PyInstaller archive could not be opened" and file-access advice. The descriptor had opened successfully; the selected name no longer matched it. The reader admission mapper now preserves that changed-file condition as `integrity`, with `PyInstaller source changed during admission: <path>`. The existing stable-artifact guidance applies. No new error code or retryability change. ## Problem and expected behavior **Trigger:** during `extract_artifact`, the selected PyInstaller source is replaced with a byte-identical regular file after the descriptor opens and before admission finishes its identity check. `openRegularFile` raises `RegularFileChangedError`; `pyInstallerFileAdmissionFailure` had no branch for that type and fell through to `io`. **Expected:** preserve the changed-source condition as an `integrity` outcome rather than an unreadable archive. Cancellation, cleanup retention, non-regular input, and unknown I/O classifications must remain unchanged. The reader's existing post-read source check already uses `integrity`. ## Change and scope - Added the missing `RegularFileChangedError` branch to `pyInstallerFileAdmissionFailure` in `src/artifacts/pyinstaller/PyInstallerFileAdmission.ts`. - Added a reader-level regression test in `src/artifacts/pyinstaller/PyInstallerFileAdmission.test.ts`. - The shared helper is also reached by private PYZ-image admission, so that path receives the same classification. - Intentionally not included: no extraction-path changes, no new error code, no retryability change, and no changes to the shared `ArtifactOperationFailure.ts` projector. This is a routine fix within the existing read-only artifact boundary. It adds no execution, injection, mutation, network, or protection-recovery authority, so a separate maintainer scope decision is not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: PyInstaller reader admission mapping (`pyInstallerFileAdmissionFailure`), reached from `PyInstallerArtifactReader.#admit` and private PYZ-image admission. - Contribution scope fit and linked maintainer decision: existing read-only artifact handling; no new authority or capability; not applicable. - CLI and MCP/tool-catalog contract: no schema, argument, or catalog change. For this specific changed-source condition, the failure reason moves from `io` to the existing `integrity` classification and its existing recovery guidance. - Provider, bridge, target-format, or platform compatibility: PyInstaller only. The regression test is platform-independent and mocks the admission boundary. - Evidence, artifact, provenance, or reconstruction contract: no artifact identity or evidence format change. The original cause is retained on the reader failure, and changed input is still refused rather than extracted. - Target selection, authority, upstream behavior, and cleanup details: no new target authority. Cancellation and cleanup observation handling remain unchanged. - 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:** `src/artifacts/pyinstaller/PyInstallerFileAdmission.test.ts` makes `openRegularFile` throw `RegularFileChangedError` and asserts that reader admission reports `integrity` with the changed-source message while still requesting symlink-rejecting admission. - **Base reproduction or other evidence:** issue #6067 reports the pre-fix production MCP output: `artifact_operation_failed` / `execution_failure`, `reason: io`, detail `PyInstaller archive could not be opened`. Source-derived: on the base revision the same cause reaches the generic fallback because the branch added here is absent. I did not rerun the end-to-end MCP reproduction after the fix on this host. - **User-visible CLI/MCP output (if applicable):** the pre-fix output is quoted above from issue #6067. Post-fix end-to-end output was not captured locally; the reader-level regression covers the changed reason, and existing presentation code maps `integrity` to the stable-artifact guidance. - **Remaining proof gaps:** the regression isolates reader admission with a mocked `openRegularFile` instead of reproducing a live rename race. Full-repository typecheck did not complete locally; see the validation gap below. CI source workflows are still awaiting maintainer approval. 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 - `node node_modules/vitest/vitest.mjs run src/artifacts/pyinstaller --maxWorkers=1` — 7 test files, 121 tests passed after rebasing onto the latest `origin/main`. - `npm run lint` — 0 errors; 2,233 existing warnings reported, none introduced by this change. - `npm run verify:test-discovery` — all 2,042 test files discovered exactly once. - `npm run format:check` — all 5,051 files formatted. - Scoped `tsc --noEmit` over the changed files and their dependency graph — passed. - Full `npm run typecheck` — did not complete; the local run timed out after 20 minutes. Treat full-repository typecheck as unverified locally. ## Compatibility, safety, and release - Breaking changes or migration steps: none. The only observable change is the reason/recovery for the specific changed-source admission condition; callers that matched the old `io` result for that case will now see the intended `integrity` result. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; this touches PyInstaller admission classification only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new authority, execution, network access, or persistent writes. The selected file is still refused when its identity changes, and the original error cause remains available. ## Author readiness Human owner: @Bilalmesk The human owner marked the PR ready for review. The declarations below remain unchecked pending the owner's explicit confirmation, per the repository's AI-contribution policy. - [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. - [ ] 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-11-01 21:46:46 +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#7285
No description provided.