fix: abort hung OpenClaw target version fetches - #69
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review in progressClawSweeper is reviewing this revision. This supersedes any previous blocked status. |
|
Codex review: blocked before merge. Reviewed September 8, 2026, 12:37 PM ET / 16:37 UTC. ClawSweeper reviewWhat this changesAdds configurable deadlines and size limits to OpenClaw npm downloads, classified errors, regression tests, and a changelog entry. Regression provenancePossible regression — suspected (reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 5 items remain This fix remains necessary, but the new metadata limit rejects the healthy npm registry response used by latest and beta resolution. The supplied timeout proof is useful; the patch needs correction before merge. Priority: P2 Review scores
Verification
How this fits togetherPlugin Inspector downloads an explicitly selected OpenClaw release to compare its public plugin contracts with a plugin. Registry metadata selects the version, then an integrity-checked archive supplies a cached compatibility target. flowchart TD
A[Requested OpenClaw version] --> B[Resolve npm metadata]
B --> C[Apply download limits]
C --> D{Prepared target cached?}
D -->|Yes| F[Read public plugin contracts]
D -->|No| E[Download and verify archive]
E --> F
F --> G[Plugin compatibility report]
Decision needed
Why: The timeout demonstration proves cancellation but does not establish that the chosen defaults preserve healthy slow downloads; selecting that upgrade tradeoff requires owner intent. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Use bounded tag-specific metadata lookup, preserve integrity and cache semantics, and ship documented, compatibility-tested download budgets approved by the area owner. Do we have a high-confidence way to reproduce the issue? Yes: the measured healthy registry response exceeds the introduced cap, and source tracing proves latest/beta resolution rejects it before cache lookup. The patched CLI was not executed during this read-only review. Is this the best way to solve the issue? No, as written: the full metadata request is incompatible with its new cap; tag-specific registry endpoints provide the needed version metadata in substantially smaller responses. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 84ede904fd6e. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Pass AbortSignal.timeout on npm metadata and tarball downloads, reject oversized archives before buffering, and surface target-download-timeout when a registry or tarball never completes. Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
000214a to
4f1a06c
Compare
What Problem This Solves
Fixes an issue where users selecting an npm OpenClaw target could wait indefinitely for stalled registry or archive responses, or buffer an unbounded download. A metadata limit alone also breaks healthy
latestandbetaresolution when the full package history exceeds that limit.Why This Change Was Made
Each metadata or archive request uses a 30-second default deadline covering its headers and body reads, not a single deadline for the entire resolution and preparation operation. Invalid, nonpositive, fractional, nonfinite, or overflowing timeout values fall back to that default. Operators can override the budgets with
PLUGIN_INSPECTOR_TARGET_FETCH_TIMEOUT_MS,PLUGIN_INSPECTOR_TARGET_METADATA_MAX_BYTES, andPLUGIN_INSPECTOR_TARGET_ARCHIVE_MAX_BYTES.Tag resolution reads the official flat
/-/package/openclaw/dist-tagsresponse, then fetches metadata for the resolved exact version. The metadata and archive limits remain 16 MiB and 256 MiB respectively; the full packument is not downloaded.Declared and streamed oversized bodies are rejected, HTTP-error bodies are cancelled, and acquired readers are released on success and failure. Integrity verification, failed-extraction cleanup, cache behavior, and the requirement to prepare the original resolved target object are preserved.
This follow-up builds directly on SebTardif's original commit
4f1a06ca249c6d49d71d7f259571a11bb1eb879c; contributor ancestry and authorship are retained. Capture, registration, and synthetic-probe timeout paths are outside this PR.User Impact
Healthy tag-based target selection stays within the metadata budget. Stalled or oversized downloads fail with
target-download-timeoutortarget-download-too-large, without retaining response readers or preparing a failed target.Evidence
11b0e89f8d0a33888009b01bc90f2d0700f35f87: https://github.com/openclaw/plugin-inspector/actions/runs/34367312677. Node 22 CI recorded 12 expected failures covering the packument limit, response cleanup, and timeout overflow; 259 tests passed.55bff71c0b5fee15b2e1405253ed7f04f0391433: https://github.com/openclaw/plugin-inspector/actions/runs/34367826440. Node 22npm run checkpassed all 272 tests with no skips or cancellations, followed bypackage contents: pass.84ede904fd6e766a9fc4de002f39af87d90c1916was P2-clean, with no accepted or actionable P0-P2 findings./-/package/openclaw/dist-tagsreturned HTTP 200, 69 bytes, and a flat object withlatest: "2026.9.3"andbeta: "2026.9.1"./openclaw/2026.9.3returned HTTP 200, 108,241 bytes, the matching version, a string tarball URL, and SHA-512 integrity metadata. No full packument or live archive was downloaded.action_requiredrun was superseded by approved runs for the updated PR head.Source-owner review has accepted candidate
55bff71c0b5fee15b2e1405253ed7f04f0391433and its per-request bounded defaults for merge. No release is included. Downstream Crabpot smoke remains deferred to its separately authorized stage. A real registry outage and a slow-WAN full archive transfer were not tested.