morluto/rea

Fork 0

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

[PR #6362] [MERGED] fix(scripts): keep the first proxy error in the package-update verifier #7308

New issue

Closed

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

bsakaguchi commented

2026-11-01 21:46:55 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/6038-verify-update-proxy-errors

📝 Commits (1)

33cb2c6 fix(scripts): keep the first proxy error in the package-update verifier

📊 Changes

2 files changed (+109 additions, -1 deletions)

View changed files

📝 scripts/verify-package-update.mjs (+11 -1)
➕ tests/boundary/process/packageUpdateRegistryFailure.test.ts (+98 -0)

📄 Description

TL;DR: The package-update test fixture hid the real error behind a second, confusing one. Now the fixture keeps the first error and reports it. Thank you, matbrgz, for the clear report.

Summary

Refs #6038. Title: fix(scripts): keep the first proxy error in the package-update verifier.
The update check runs a small fake registry. That fake sent headers before it read the real body. A body failure then caused a second header write and hid the real error. Thank you, matbrgz, for the report and the exact stack.

Problem and expected behavior

The fixture called response.writeHead(upstream.status, ...) and only then awaited upstream.arrayBuffer(). Its error callback always called writeHead(500).
If the body failed, that second call threw ERR_HTTP_HEADERS_SENT. The first error was lost and the verifier process died.
Expected: the first failure is the one reported, and no header is written twice.

Change and scope

The fixture reads the body first, then writes headers. On error it writes a 500 only if headers are not yet sent; otherwise it destroys the response. It records the first error, and verifyPackageUpdate rethrows that error from its own catch.
Not included: killing the update child early. Also not included: the checks for leftover child processes and a rerun of the real install. That is why this PR says Refs and not Fixes.

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".
Draft PRs may leave the scope decision pending and the readiness and merge checklists
unchecked. Maintainers can help resolve open questions.

Contract and boundary impact

Semantic owner and earliest changed stage:
scripts/verify-package-update.mjs, the registry fixture request handler.
Contribution scope fit and linked maintainer decision:
Not applicable. A fix inside an existing verification script.
CLI and MCP/tool-catalog contract:
None.
Provider, bridge, target-format, or platform compatibility:
None. Node-only change, tested on Node 22, 24 and 26.
Evidence, artifact, provenance, or reconstruction contract:
None.
Target selection, authority, upstream behavior, and cleanup details (if applicable):
Not applicable. No target is run. The fixture still proxies unknown paths to the public npm registry, as before.
Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs):
None. The script is a verifier and not part of the published package behavior.

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/process/packageUpdateRegistryFailure.test.ts (new). It runs verifyPackageUpdate in a child process with a stub package and a stubbed upstream fetch whose arrayBuffer() rejects. It needs no public registry.

Base reproduction or other evidence:
Executed on main (fd5cd1b05).

	Base (main)	With this change
Result	Test fails; harness crashes	Test passes
Error seen	`ERR_HTTP_HEADERS_SENT` at `scripts/verify-package-update.mjs:29:16`	`injected upstream body failure`
Original error kept	No	Yes

User-visible CLI/MCP output (if applicable):
Not applicable.
Remaining proof gaps:
The headersSent then destroy() branch has no test, because the new order means a stub cannot reach it. No assertion yet that no owned child survives. The real npm run verify:package was not run, since it installs from the network.

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

npm run test:focused -- tests/boundary/process/packageUpdateRegistryFailure.test.ts on main's script — 1 failed, ERR_HTTP_HEADERS_SENT at line 29
same command with this change on Node 22.23.1 — 1 passed
same command with this change on Node 24.18.0 — 1 passed
same command with this change on Node 26.11.1 — 1 passed
npm run typecheck — exit 0
npm run lint — exit 0 (warnings only; none new in kind)
npm run format:check — exit 0
npm run verify:package — not run: it installs packages from the network

Compatibility, safety, and release

Breaking changes or migration steps:
None.
Real Hopper/Ghidra, browser, or OS coverage:
Not applicable. Linux x64 only for the test run.
Package or release metadata impact:
None.
Security, privacy, process, or containment review:
The fixture binds 127.0.0.1 only, as before. No new process or network use.

Author readiness

Human owner: @CassieFox23

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.

🤖 Written and tested with Claude Code (Sonnet 5.5, verified by Opus 5.5), posted with my OK 🦊

_{🔄 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/6362 **Author:** [@CassieFox23](https://github.com/CassieFox23) **Created:** 10/22/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/6038-verify-update-proxy-errors` --- ### 📝 Commits (1) - [`33cb2c6`](https://github.com/morluto/rea/commit/33cb2c6386647eea8fa791b68f66c76049a6a5b3) fix(scripts): keep the first proxy error in the package-update verifier ### 📊 Changes **2 files changed** (+109 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `scripts/verify-package-update.mjs` (+11 -1) ➕ `tests/boundary/process/packageUpdateRegistryFailure.test.ts` (+98 -0) </details> ### 📄 Description **TL;DR:** The package-update test fixture hid the real error behind a second, confusing one. Now the fixture keeps the first error and reports it. Thank you, matbrgz, for the clear report.  ## Summary  Refs #6038. Title: `fix(scripts): keep the first proxy error in the package-update verifier`. The update check runs a small fake registry. That fake sent headers before it read the real body. A body failure then caused a second header write and hid the real error. Thank you, matbrgz, for the report and the exact stack. ## Problem and expected behavior  The fixture called `response.writeHead(upstream.status, ...)` and only then awaited `upstream.arrayBuffer()`. Its error callback always called `writeHead(500)`. If the body failed, that second call threw `ERR_HTTP_HEADERS_SENT`. The first error was lost and the verifier process died. Expected: the first failure is the one reported, and no header is written twice. ## Change and scope  The fixture reads the body first, then writes headers. On error it writes a 500 only if headers are not yet sent; otherwise it destroys the response. It records the first error, and `verifyPackageUpdate` rethrows that error from its own catch. Not included: killing the update child early. Also not included: the checks for leftover child processes and a rerun of the real install. That is why this PR says Refs and not Fixes. 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". Draft PRs may leave the scope decision pending and the readiness and merge checklists unchecked. Maintainers can help resolve open questions. ## Contract and boundary impact  - Semantic owner and earliest changed stage: `scripts/verify-package-update.mjs`, the registry fixture request handler. - Contribution scope fit and linked maintainer decision: Not applicable. A fix inside an existing verification script. - CLI and MCP/tool-catalog contract: None. - Provider, bridge, target-format, or platform compatibility: None. Node-only change, tested on Node 22, 24 and 26. - Evidence, artifact, provenance, or reconstruction contract: None. - Target selection, authority, upstream behavior, and cleanup details (if applicable): Not applicable. No target is run. The fixture still proxies unknown paths to the public npm registry, as before. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): None. The script is a verifier and not part of the published package behavior. 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/process/packageUpdateRegistryFailure.test.ts` (new). It runs `verifyPackageUpdate` in a child process with a stub package and a stubbed upstream `fetch` whose `arrayBuffer()` rejects. It needs no public registry. - Base reproduction or other evidence: Executed on main (fd5cd1b05). | | Base (main) | With this change | |---|---|---| | Result | Test fails; harness crashes | Test passes | | Error seen | `ERR_HTTP_HEADERS_SENT` at `scripts/verify-package-update.mjs:29:16` | `injected upstream body failure` | | Original error kept | No | Yes | - User-visible CLI/MCP output (if applicable): Not applicable. - Remaining proof gaps: The `headersSent` then `destroy()` branch has no test, because the new order means a stub cannot reach it. No assertion yet that no owned child survives. The real `npm run verify:package` was not run, since it installs from the network. 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  - `npm run test:focused -- tests/boundary/process/packageUpdateRegistryFailure.test.ts` on main's script — 1 failed, `ERR_HTTP_HEADERS_SENT` at line 29 - same command with this change on Node 22.23.1 — 1 passed - same command with this change on Node 24.18.0 — 1 passed - same command with this change on Node 26.11.1 — 1 passed - `npm run typecheck` — exit 0 - `npm run lint` — exit 0 (warnings only; none new in kind) - `npm run format:check` — exit 0 - `npm run verify:package` — not run: it installs packages from the network ## Compatibility, safety, and release  - Breaking changes or migration steps: None.  - Real Hopper/Ghidra, browser, or OS coverage: Not applicable. Linux x64 only for the test run. - Package or release metadata impact: None. - Security, privacy, process, or containment review: The fixture binds 127.0.0.1 only, as before. No new process or network use. ## Author readiness  Human owner: @CassieFox23 - [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  - [ ] 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. 🤖 Written and tested with Claude Code (Sonnet 5.5, verified by Opus 5.5), posted with my OK 🦊 --- <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:55 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-05 01:47:52 +00:00

[PR #7308] fix(reconstruction): reject inconsistent artifact comparisons #7812

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

No description provided.

Rows
Columns