Wrap mock in GetterSetter for indexed-accessor spyOn

[robobun]

What

spyOn(target, index) where the target has no callable value at that numeric
index (e.g. spyOn(obj, 1002) on a plain object, or a missing array index)
stored the mock directly via putDirectIndex with the
PropertyAttribute::Accessor bit set, instead of wrapping it in a
JSC::GetterSetter:

} else if (auto index = parseIndex(propertyKey)) {
    // For indexed properties, set the mock directly instead of wrapping in GetterSetter
    object->putDirectIndex(globalObject, *index, mock, attributes, PutDirectIndexLikePutDirect);
}

This violates a JSC invariant: an indexed property entry carrying the
Accessor attribute must hold a GetterSetter cell. SparseArrayEntry::get
and SparseArrayEntry::put uncheckedDowncast<GetterSetter> the stored value
whenever the Accessor bit is set. So any later read or write of the spied
index downcasts the non-GetterSetter mock, tripping the asObject()
cell->isObjectSlow() assertion (JSObject.h:1346) — or, on the read path,
the PropertySlot::setValue assertion.

The non-indexed branch immediately below already does the right thing:
putDirectAccessor(..., GetterSetter::create(vm, globalObject, mock, mock), attributes).

Fix

Wrap the mock in a GetterSetter on the indexed-accessor path too, so the
stored value matches the Accessor attribute. The value/function indexed
branch (non-accessor, plain data property) is unchanged.

Repro (before fix, debug build)

const obj = {};
Bun.jest().spyOn(obj, 1002);
obj[1002];            // ASSERTION FAILED: PropertySlot::setValue
Reflect.set(obj, 1002, 1); // ASSERTION FAILED: asObject / isObjectSlow

Found by the fuzzer.

Test

Added a regression test in test/js/bun/test/mock-fn.test.js covering read,
write (sloppy + Reflect.set), and mockRestore of a spied missing indexed
property. Verified it SIGABRTs on the pre-fix debug build and passes after.
Full mock-fn.test.js suite (73 tests) passes.

[robobun]

^{Updated 4:51 PM PT - May 30th, 2026}

❌ @robobun, your commit b1d114b has 1 failures in Build #59398 (All Failures):

• test/js/third_party/duckdb/duckdb-basic-usage.test.ts - code 1 on 🪟 2019 x64

---

🧪   To try this PR locally:

bunx bun-pr 31619

That installs a local version of the PR into your bun-31619 executable, so you can run:

bun-31619 --bun

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 64013830-5422-4856-a87f-b24112a230da

📥 Commits

Reviewing files that changed from the base of the PR and between 6a98cbc and b1d114b.

📒 Files selected for processing (1)

• src/jsc/bindings/JSMockFunction.cpp

---

Walkthrough

Modified spyOn to wrap indexed properties with JSC::GetterSetter accessors instead of storing mock values directly. Added a regression test confirming that spying on missing numeric-keyed properties installs the accessor without crashes, invokes the spy on read/write, and supports mockRestore().

Changes

Indexed Property Spy Installation

Layer / File(s)	Summary
GetterSetter accessor wrapper for indexed properties src/jsc/bindings/JSMockFunction.cpp, test/js/bun/test/mock-fn.test.js	In JSMock__jsSpyOn, indexed property handling now wraps the mock with a JSC::GetterSetter accessor via putDirectIndex instead of storing it directly. A regression test validates the behavior on missing indexed properties, confirming no crashes on property access, spy invocation on initial reads, and successful mockRestore() cleanup.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and specifically describes the main code change: wrapping a mock in GetterSetter for indexed-accessor spyOn, directly matching the core fix applied.
Description check	✅ Passed	The description is comprehensive and well-structured, covering the bug explanation, root cause, fix, reproduction case, and test verification. It fully addresses both template sections.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

This PR may be a duplicate of:

1. Fix assertion failure when spyOn is used on non-callable indexed property #29481 - Fixes the same assertion failure when spyOn is used on a non-callable indexed property, using a different fix strategy (stripping the Accessor attribute instead of wrapping in GetterSetter)

🤖 Generated with Claude Code

[claude]

LGTM — brings the indexed-accessor branch into parity with the non-indexed branch right below it.

Extended reasoning...

Overview

This PR changes one line in src/jsc/bindings/JSMockFunction.cpp (the spyOn implementation) and adds a regression test to test/js/bun/test/mock-fn.test.js. When spyOn(obj, <numeric index>) hits the non-callable / missing-value branch, the code sets PropertyAttribute::Accessor and stores the property via putDirectIndex. Previously it stored the raw mock function; now it stores JSC::GetterSetter::create(vm, globalObject, mock, mock) — exactly what the adjacent non-indexed else branch already does via putDirectAccessor.

The fix is mechanically obvious: JSC requires that any sparse-array entry carrying the Accessor attribute hold a GetterSetter cell (since SparseArrayEntry::get/put unchecked-downcast it). The old code violated that invariant and tripped debug assertions on any subsequent read/write of the spied index. The new code matches the established pattern one branch down.

Security risks

None. This is test-runner mocking infrastructure (jest.spyOn / bun:test spyOn), not a production runtime path. There's no auth, crypto, permissions, or untrusted-input handling involved. The change strictly tightens an invariant that was previously violated.

Level of scrutiny

Low. The diff is a single-line substitution that mirrors the existing, correct code path immediately below it (putDirectAccessor(..., GetterSetter::create(...), attributes)). The function/data-property indexed branch above is intentionally left unchanged (it doesn't set Accessor, so storing the mock directly is correct there). The blast radius is limited to spyOn on numeric indices where the existing value is missing or non-callable — an edge case that previously crashed in debug builds.

Other factors

• Found by the fuzzer; the PR description includes a clear repro and explanation of the JSC invariant.
• A regression test is added covering read, sloppy write, Reflect.set, and mockRestore on the spied index.
• The multi-agent bug hunting system found no issues.
• No CODEOWNERS entries cover this path.
• No outstanding reviewer comments.

[claude]

LGTM — one-line fix that makes the indexed-accessor branch match the non-indexed branch directly below it, with a regression test.

Extended reasoning...

Overview

This PR changes a single line in src/jsc/bindings/JSMockFunction.cpp (the spyOn implementation) and adds a 15-line regression test to test/js/bun/test/mock-fn.test.js. When spyOn(obj, <numeric index>) is called on a property that has no callable value, the code installs the spy as an accessor property. Previously, the indexed-property branch passed the mock function directly to putDirectIndex while setting PropertyAttribute::Accessor, violating JSC's invariant that an accessor-flagged slot must hold a GetterSetter cell. The fix wraps the mock in JSC::GetterSetter::create(vm, globalObject, mock, mock) — exactly what the adjacent non-indexed else branch already does via putDirectAccessor.

Security risks

None. This is test-runner mock/spy infrastructure (bun:test's spyOn), not auth, crypto, network, or filesystem code. The change replaces an invalid downcast/assertion failure with the correct cell type; it cannot expose data or grant capabilities.

Level of scrutiny

Low-to-moderate. While the file is C++ JSC bindings, the change is mechanical: it makes one branch consistent with the branch immediately following it. The PR description clearly explains the JSC invariant being violated (SparseArrayEntry::get/put do uncheckedDowncast<GetterSetter> when the Accessor bit is set), and the fix is the obvious one. I also verified that mockRestore()/clearSpy() is unaffected: for missing properties spyAttributes is saved as 0 (no Accessor bit), so restoration writes a plain data property and doesn't need a GetterSetter.

Other factors

• No CODEOWNERS apply to these paths.
• No bugs were found by the bug hunting system.
• A regression test covers read, sloppy write, Reflect.set, and mockRestore.
• The duplicate-PR bot flagged #29481 which fixes the same assertion via a different strategy (stripping the Accessor attribute). This PR's approach is preferable because it preserves the existing spy-on-non-callable-property semantics (spy fires on read), matching the established behavior for non-indexed properties tested elsewhere in the suite. Maintainers may want to close one of the two, but that's a coordination matter, not a correctness concern with this change.