Skip to content

runtime: forward caller arguments through the revokeObjectURL wrap - #799

Merged
colinhacks merged 2 commits into
mainfrom
revokeobjecturl-arity
Aug 28, 2026
Merged

colinhacks merged 2 commits into
mainfrom
revokeobjecturl-arity

Conversation

@colinhacks

Copy link
Copy Markdown
Contributor

URL.revokeObjectURL() with no arguments returned silently under nub; Node throws ERR_MISSING_ARGS. The blob-URL wrap called the saved native function with a fixed arity of one, so the zero-arg call passed undefined through. It now spreads the caller's arguments.

The createObjectURL wrap has the same fixed arity but no divergence: Node throws ERR_INVALID_ARG_TYPE for both zero args and explicit undefined, and ignores extras. Left as is.

New fixture plus integration test, verified RED before the fix. Upstream test/parallel/test-url-revokeobjecturl.js (v26.7.0) goes from exit 1 to exit 0.

The blob-URL wrap called the saved native URL.revokeObjectURL with a fixed
arity of one, so a zero-argument call passed undefined through and Node's own
ERR_MISSING_ARGS check never fired. Spread the caller's arguments instead.
Copilot AI lite review requested due to automatic review settings August 27, 2026 09:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@vercel

vercel Bot commented Aug 27, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Ready Ready Preview Aug 27, 2026 9:51am

Request Review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The runtime fix is right, but the new test hard-asserts ERR_MISSING_ARGS on a Node range where plain Node doesn't throw. CI runs the integration suite on 18.19 and 20.11, so this goes red on two legs.

Reviewed changes — the full diff at a292cac: the revokeObjectURL arity fix, its new fixture, and the integration test that guards it.

  • Forward real caller arity — runtime/worker-blob-url.cjs:162 swaps nativeRevoke(url) for nativeRevoke(...arguments) so a zero-arg call reaches host Node's own arity check instead of a synthesized undefined. Correct on every supported Node: forwarding real arity reproduces host behavior whatever that behavior is, and .length stays 1.
  • New fixture — tests/fixtures/worker/blob-url-revoke-arity.ts prints revoke-no-args:<code> (or no-throw) plus revoke-one-arg:ok.
  • New integration test — worker_blob_url_revoke_forwards_caller_arity at crates/nub-cli/tests/integration.rs:4421.

I verified the PR body's claim that createObjectURL needs no equivalent change, and it holds — no action needed there. createObjectURL has never carried an arguments.length guard (read lib/internal/url.js at v18.19.0, v18.20.8, v20.11.1, v20.13.0, v22.15.0, v24.18.0), zero args and explicit undefined both yield ERR_INVALID_ARG_TYPE, and its body reads only the obj binding so extra arguments are unobservable. Leaving it as is was the right call.

ℹ️ The wiki still lists this as an open regression

wiki/research/nub-v0.5-augmentation-regressions.md:110-113 documents Cause 9a as outstanding — "the wrapper always forwards one argument, defeating Node's arity check … Fix: preserve arity" — which this PR resolves. The entry also carries a stale runtime/worker-blob-url.cjs:109 line reference, and the doc's Changelog has no entry for the fix. AGENTS.md asks for the graph to move in the same commit as the behavior.

Technical details
# Close out Cause 9a in the regressions research doc

## Affected sites
- `wiki/research/nub-v0.5-augmentation-regressions.md:110` — heading `### Cause 9 — URL.revokeObjectURL() arity + domain-sweep DEP0097` now covers one fixed item and one open one.
- `wiki/research/nub-v0.5-augmentation-regressions.md:112` — bullet **9a** states the bug as present tense and prescribes the fix this PR just made. Its `runtime/worker-blob-url.cjs:109` reference is stale; the wrap now lives at `runtime/worker-blob-url.cjs:158-163`.
- `wiki/research/nub-v0.5-augmentation-regressions.md:129-133` — the `## Changelog` section records every revision to the document and has no entry for this one.

## Required outcome
- 9a reads as resolved, pointing at this PR, rather than as an open recommendation.
- 9b (the domain-sweep DEP0097 item) stays open — it is untouched by this PR, so the Cause 9 heading should still make sense with only 9b live.
- A Changelog line dated to the fix, matching the existing entry style.

## Open questions for the human
- Does this doc's convention prefer striking a resolved cause in place or leaving it with a "fixed in #799" annotation? Existing entries (see Cause 8's `VERDICT` / `Correction:` lines) suggest annotate-in-place rather than delete.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using Claude Opus | 𝕏

Comment thread crates/nub-cli/tests/integration.rs

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — the delta since the a292cac review, which is the single commit ded6dc3 touching crates/nub-cli/tests/integration.rs.

  • Version-gated the zero-arg assertion — integration.rs:4435-4439 now selects one exact expected value per band via node_at_least((20, 12, 0)): revoke-no-args:ERR_MISSING_ARGS at or above 20.12, revoke-no-args:no-throw below. This resolves the red-on-two-legs concern from the prior review.

The shape is the right one. It picks a single expected value per version band rather than the contains(A) || contains(B) alternative, so the assertion still fails if the wrap regresses on a modern Node. I also confirmed the gate reads the same Node the fixture runs on: target_node_path (integration.rs:1314) resolves through nub node which executed from the fixtures dir, i.e. the same pin-free project boundary run_nub spawns under — so node_at_least and the fixture can't disagree. .github/workflows/ci.yml:167 still pins the 20.11 and 18.19 legs, so the gate is load-bearing.

One consequence worth knowing rather than changing: on those two floor legs the zero-arg assertion is a no-op guard, since the old fixed-arity wrap and the new one are indistinguishable where Node itself doesn't throw. The 22.13 and 22.15 legs carry the regression signal, and the in-test comment already explains the split.

ℹ️ The wiki entry for this regression is still open

Carried forward from the prior review, unchanged at ded6dc3 and still the only outstanding item: wiki/research/nub-v0.5-augmentation-regressions.md:112 states Cause 9a in the present tense and prescribes the fix this PR makes, with a stale runtime/worker-blob-url.cjs:109 reference, and the doc's Changelog has no entry. AGENTS.md asks for the graph to move in the same commit as the behavior. Cause 9b stays open. Full detail is in the a292cac review — not repeating it here.

Pullfrog  | View workflow run | Using Claude Opus | 𝕏

@colinhacks
colinhacks merged commit 27cfce2 into main Aug 28, 2026
74 checks passed
@colinhacks
colinhacks deleted the revokeobjecturl-arity branch August 28, 2026 21:37
@colinhacks

Copy link
Copy Markdown
Contributor Author

This branch was successfully deployed

1 active deployment
Preview — ded6dc38 Deployed Aug 27, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants