Skip to content

fix(csharp): copy-on-read frozen binding arrays in namespace-siblings#1083

Closed
mann1x wants to merge 1 commit into
abhigyanpatwari:mainfrom
mann1x:fix/csharp-frozen-bindings-array
Closed

fix(csharp): copy-on-read frozen binding arrays in namespace-siblings#1083
mann1x wants to merge 1 commit into
abhigyanpatwari:mainfrom
mann1x:fix/csharp-frozen-bindings-array

Conversation

@mann1x

@mann1x mann1x commented Apr 26, 2026

Copy link
Copy Markdown
Contributor

Summary

scope-extractor.ts freezes per-name binding arrays via frozenBindings.set(name, Object.freeze(refs.slice())) (line 180). When populateCsharpNamespaceSiblings later reads back an existing entry via scopeBindings.get(name) ?? [] and tries to existing.push(...) on it, the frozen reference throws

TypeError: Cannot add property N, object is not extensible
    at Array.push (<anonymous>)
    at Object.populateCsharpNamespaceSiblings [as populateNamespaceSiblings]
      (.../csharp/namespace-siblings.js:321:26)
    at runScopeResolution (.../scope-resolution/pipeline/run.js:113:18)
    ...
  Phase 'scopeResolution' failed

…which abandons scope resolution for the entire repo (the index is never produced). The populator's leading comment says "indexes.bindings is typed ReadonlyMap<...> but is a plain Map at runtime; mutating here is the established pattern" — that's true for the outer Map, but the inner per-name arrays are nonetheless frozen by the extractor, so the established pattern is silently broken at three sites.

Reproduction

Triggered when a file locally declares a class with the same simple name as a class in a sibling namespace imported via using. Hit live on the PersistentWindows real-world WinForms repo (large mix of .Designer.cs + namespaced classes); also hit on smaller mixed-namespace C# codebases.

Minimal fixture added under test/fixtures/cross-file-binding/csharp-frozen-binding-collision/:

  • Models/User.cs declares Collision.Models.User
  • App/Program.cs imports using Collision.Models; AND locally declares class User in Collision.App — the local declaration causes scope-extractor to populate (and freeze) User in the Module scope's bindings; the populator's namespace-import path then attempts to push Collision.Models.User into the same frozen array.

Fix

Spread to a fresh mutable copy on read at all three sites in populateCsharpNamespaceSiblings:

- const existing = scopeBindings.get(name) ?? [];
+ const existing: BindingRef[] = [...(scopeBindings.get(name) ?? [])];

When the populator hits a name pre-populated by the extractor, it now appends to a fresh array and writes that back, replacing the frozen entry in the per-scope Map. Origin precedence (local shadows namespace) is preserved by the existing csharpMergeBindings ordering (see csharp-hooks.test.ts).

Three sites fixed:

  • populateCsharpNamespaceSiblings using-static loop (origin: 'import')
  • populateCsharpNamespaceSiblings namespace-import loop (origin: 'namespace')
  • populateCsharpNamespaceSiblings cross-namespace bucket loop (third namespace-injection site)

Verification

  • npx tsc --noEmit clean
  • npx vitest run test/integration/resolvers/csharp.test.ts test/unit/scope-resolution/csharp/ test/unit/named-bindings/csharp.test.ts → 5 test files / 282 tests / 0 fail (3 of those are the new regression test + 279 existing).
  • Pre-fix beforeAll of the new test rejects with Phase 'scopeResolution' failed: Cannot add property N, object is not extensible. Post-fix all three test cases pass: scope resolution completes, both User declarations are detected, and new User() inside Program.Run() resolves to the local Collision.App.User (origin:local shadows origin:namespace).

Risk / rollback

Minimal — purely a copy-on-read tweak in a populator that already documented its intent to mutate the per-name arrays. Behavior is unchanged for callers that don't pre-populate the same name. The Object.freeze upstream in scope-extractor.ts is left in place; if a future change wants to enforce true immutability throughout, this is the right starting point because it makes the freeze contract honest at the populator boundary instead of silently broken.

Rollback: revert the commit; the upstream behavior degrades back to crashing on collision.

Test plan

  • npx tsc --noEmit in gitnexus/
  • Full C# test suite green (integration + unit + named-bindings)
  • New regression test fails before fix, passes after
  • Optional: maintainer to run on a wider corpus

`scope-extractor.ts` freezes per-name binding arrays via
`frozenBindings.set(name, Object.freeze(refs.slice()))`. When
`populateCsharpNamespaceSiblings` later reads back an existing entry
via `scopeBindings.get(name) ?? []` and tries to `existing.push(...)`
on it, the frozen reference throws

    TypeError: Cannot add property N, object is not extensible

at three sites (using-static `origin: 'import'`, namespace-import
`origin: 'namespace'`, and the cross-namespace bucket loop). The
populator's leading comment even says "indexes.bindings is typed
ReadonlyMap<...> but is a plain Map at runtime; mutating here is the
established pattern" — which is true for the Map itself, but the
inner per-name arrays were nonetheless frozen by the extractor.

Reproduced live on the PersistentWindows real-world repo (any C#
codebase where a file locally declares a class with the same simple
name as a class in a `using`-imported sibling namespace). Verified
the crash via the runner.js cause-stack and pinpointed line 321 in
the compiled file (now line ~314 source).

Fix: spread to a fresh mutable copy on read at all three sites:

    const existing: BindingRef[] = [...(scopeBindings.get(name) ?? [])];

Behavior: when the populator hits a name pre-populated by the
extractor, it now appends to a fresh array and writes that back,
replacing the frozen entry in the per-scope Map. Origin precedence
(`local` shadows `namespace`) is preserved by the existing
`csharpMergeBindings` ordering — see csharp-hooks.test.ts.

Test: integration regression in csharp.test.ts under "C# regression:
namespace-siblings injection vs frozen bindings" + minimal fixture
at csharp-frozen-binding-collision/. Pre-fix the new `beforeAll`
rejects with "Cannot add property N..."; post-fix the suite passes.

Verified:
  - npx tsc --noEmit clean
  - test/integration/resolvers/csharp.test.ts:  3 new + 200 existing
  - test/unit/scope-resolution/csharp/, test/unit/named-bindings/csharp.test.ts:
    full suite green
@vercel

vercel Bot commented Apr 26, 2026

Copy link
Copy Markdown

Someone is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

mann1x pushed a commit to mann1x/claude-hooks that referenced this pull request Apr 26, 2026
PR opened: abhigyanpatwari/GitNexus#1083 —
"fix(csharp): copy-on-read frozen binding arrays in namespace-siblings".

Root cause: scope-extractor.ts froze per-name binding arrays via
Object.freeze(refs.slice()), then populateCsharpNamespaceSiblings
tried to .push() onto them at three sites — TypeError: Cannot add
property N, object is not extensible. Crashed scopeResolution for the
entire repo on any C# codebase that locally declared a class with the
same simple name as a class in a `using`-imported sibling namespace.
Reproduced live on PersistentWindows; minimal fixture committed to
the upstream test suite.

Fix: spread to a fresh mutable copy on read at all three sites.
Verified by 5 test files / 282 tests / 0 fail under the C# vitest
suites + a new regression test that reproduces the bug pre-fix.

This commit:

- COMPANION_TOOLS.md: replace the "known limitation" wording with a
  pointer to PR #1083 + a one-shot install recipe from a forked
  build until the fix ships in an npm release.

- docker/gitnexus/Dockerfile: optional GITNEXUS_TARBALL build-arg so
  hosts using the Docker wrapper can bake in the fork build directly.
  Default behavior (npm i -g gitnexus@latest) unchanged.

Real-world verification on pandorum:

  $ gitnexus analyze ./PersistentWindows
  Repository indexed successfully (5.6s)
  1025 nodes | 1715 edges | 28 clusters | 25 flows

  $ gitnexus list
  Indexed Repositories (3)
    foo_dsd_trellis / osync / PersistentWindows

(PersistentWindows previously hard-crashed scopeResolution; now
indexes cleanly.)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@magyargergo

Copy link
Copy Markdown
Collaborator

I just merged, i think, a fix, for it. Could you please check #1082 ?

@magyargergo

Copy link
Copy Markdown
Collaborator

We could use your test fixture to test it against.

@mann1x

mann1x commented Apr 26, 2026

Copy link
Copy Markdown
Contributor Author

Wow almost in parallel!
Let me check it out

@mann1x

mann1x commented Apr 26, 2026

Copy link
Copy Markdown
Contributor Author

Confirmed, it works and it's great, real major improvement!

  ┌─────────────────────────────────────────────┬────────────────────────┬────────────────────────────────┬──────────────┐
  │                                             │ Before (PR #1083 fork) │ After (PR #1082 upstream/main) │      Δ       │
  ├─────────────────────────────────────────────┼────────────────────────┼────────────────────────────────┼──────────────┤
  │ Cannot add property … not extensible errors │ 0 (also fixed)         │ 0                              │ —            │
  ├─────────────────────────────────────────────┼────────────────────────┼────────────────────────────────┼──────────────┤
  │ Other freeze-related crashes                │ 0                      │ 0                              │ —            │
  ├─────────────────────────────────────────────┼────────────────────────┼────────────────────────────────┼──────────────┤
  │ Nodes                                       │ 1025                   │ 1113                           │ +88 (+8.6%)  │
  ├─────────────────────────────────────────────┼────────────────────────┼────────────────────────────────┼──────────────┤
  │ Edges                                       │ 1715                   │ 2987                           │ +1272 (+74%) │
  ├─────────────────────────────────────────────┼────────────────────────┼────────────────────────────────┼──────────────┤
  │ Clusters                                    │ 28                     │ 39                             │ +11          │
  ├─────────────────────────────────────────────┼────────────────────────┼────────────────────────────────┼──────────────┤
  │ Flows                                       │ 25                     │ 97                             │ +72 (+288%)  │
  ├─────────────────────────────────────────────┼────────────────────────┼────────────────────────────────┼──────────────┤
  │ Wall time                                   │ 5.6 s                  │ 8.1 s                          │ +2.5 s       │
  └─────────────────────────────────────────────┴────────────────────────┴────────────────────────────────┴──────────────┘

Closing this and opening a small PR for the fixture-only.

@mann1x mann1x closed this Apr 26, 2026
magyargergo pushed a commit that referenced this pull request Apr 27, 2026
…1085)

The csharp-large-cache-miss-resolution fixture added in #1082 reproduces
the freeze contract failure via tree-sitter cache-miss reparse on >32 KB
files. This adds a complementary trigger for the same root cause that
does not depend on file size: a small-file pair where the importer
locally declares a class with the same simple name as a sibling reached
through `using`.

Pre-#1082 path: scope-extractor pre-populates (and freezes) `User` in
the importer's Module bindings, then populateCsharpNamespaceSiblings'
namespace-import loop calls push() on the frozen array and throws
"Cannot add property N, object is not extensible", aborting the whole
scopeResolution phase.

Post-#1082 the augmentation channel keeps both bindings visible; the
local `Collision.App.User` shadows the namespace-imported one per
origin precedence, so `Program.Run -> new User()` resolves to the
local class.

Three assertions:
- scopeResolution completes (no throw on the colliding bucket).
- both `User` declarations are detected across the two namespaces.
- `Program.Run -> User` constructor edge points at App/Program.cs
  (not Models/User.cs), verifying origin:local shadows origin:namespace.

Verified: full csharp.test.ts suite green (207/207). tsc --noEmit clean.

Refs: #1066, #1082, #1083 (closed as superseded).
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