Skip to content

Conversation

grypez
Copy link
Contributor

@grypez grypez commented Sep 5, 2025

This PR ensures that the various sources of vat namespace endowments do not provide colliding names, causing the VatSupervisor to throw an error instead in this case.

@grypez grypez changed the base branch from main to grypez/use-kernel-platforms September 5, 2025 19:46
@grypez grypez force-pushed the grypez/use-kernel-platforms branch from 88f8f25 to 5017064 Compare September 8, 2025 16:47
Base automatically changed from grypez/use-kernel-platforms to main September 8, 2025 17:38
@grypez grypez force-pushed the grypez/object-key-disjoint-union branch from d3d2a33 to e84ffcd Compare September 9, 2025 19:43
@grypez grypez force-pushed the grypez/object-key-disjoint-union branch from e84ffcd to 199d7e3 Compare September 9, 2025 19:50
@grypez grypez marked this pull request as ready for review September 10, 2025 12:42
@grypez grypez requested a review from a team as a code owner September 10, 2025 12:42
Comment on lines 14 to 17
for (const obj of objects) {
for (const key of Reflect.ownKeys(obj)) {
if (keys.has(key)) {
const originalIndex = keys.get(key);
Copy link
Contributor

@sirtimid sirtimid Sep 10, 2025

Choose a reason for hiding this comment

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

Suggested change
for (const obj of objects) {
for (const key of Reflect.ownKeys(obj)) {
if (keys.has(key)) {
const originalIndex = keys.get(key);
objects.forEach((obj, originalIndex) => {
for (const key of Reflect.ownKeys(obj)) {
if (keys.has(key)) {

I guess this is simpler?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah no you are using the collidingIndex. But I'm not so sure why we need that and not simply do something like

  const seen = new Map<PropertyKey, number>();
  const out: Record<PropertyKey, unknown> = Object.create(null);

  objects.forEach((obj, idx) => {
    for (const key of Reflect.ownKeys(obj)) {
      if (seen.has(key)) {
        throw new Error(
          `Duplicate key "${String(key)}" found in entries ${seen.get(key)} and ${idx}`
        );
      }
      seen.set(key, idx);
    }
    Object.defineProperties(out, Object.getOwnPropertyDescriptors(obj));
  });

  return out 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case the index is used to differentiate between an internal error and a user error where the VatSupervisor uses the function.

@grypez grypez force-pushed the grypez/object-key-disjoint-union branch from 650c260 to 996a6f3 Compare September 10, 2025 16:28
cursor[bot]

This comment was marked as outdated.

@grypez grypez enabled auto-merge (squash) September 10, 2025 17:16
};
throw new DuplicateEndowmentError(String(key), collidingIndex === 1);
}
// Otherwise, just rethrow the error.
Copy link

Choose a reason for hiding this comment

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

Bug: Endowment Collision Classification Error

The DuplicateEndowmentError.isInternal flag is incorrectly determined. It currently only checks if collidingIndex === 1, which misclassifies internal endowment collisions. Since workerEndowments, platformEndowments, and lsEndowments are all internal sources, any collision between them should be marked as internal. The classification needs to consider both originalIndex and collidingIndex to accurately reflect internal conflicts.

Fix in Cursor Fix in Web

Copy link
Contributor

@sirtimid sirtimid left a comment

Choose a reason for hiding this comment

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

LGTM

@grypez grypez merged commit f423cbc into main Sep 10, 2025
23 checks passed
@grypez grypez deleted the grypez/object-key-disjoint-union branch September 10, 2025 17:32
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