Skip to content

Isolate property arrays in vocabulary clones - #1208

Merged
dahlia merged 1 commit into
fedify-dev:2.0-maintenancefrom
dahlia:bugfix/vocab/clones-share-property-arrays
Oct 3, 2026
Merged

dahlia merged 1 commit into
fedify-dev:2.0-maintenancefrom
dahlia:bugfix/vocab/clones-share-property-arrays

Conversation

@dahlia

@dahlia dahlia commented Oct 2, 2026

Copy link
Copy Markdown
Member

Shared property arrays let a lookup on a clone rewrite its source without updating the source's trust state or JSON-LD cache.

Use .slice() to copy arrays in generated clone() methods and plural constructor/clone inputs. This preserves sparse inputs and nested object/URL identity.

Regression tests check lookup/serialization isolation, caller and frozen arrays, and trust behavior. Generator snapshots are updated for Deno/Node.js/Bun.

Fixes #1207.

Clones shared property arrays with their sources, so dereferencing a
clone replaced values on the original without updating its trust state
or cached JSON-LD. Plural constructor and clone inputs also retained
caller-owned arrays.

Copy arrays at these three ownership boundaries. Keep nested objects
and URLs shared, preserve sparse inputs, and retain the public API and
trust policy. Cover lookup and serialization isolation, caller and
frozen arrays, cache reuse, and embedded object identity in regressions.
Regenerate all runtime snapshots and add changelog fragments.

Fixes fedify-dev#1207

Assisted-by: Codex:gpt-6-astra
Assisted-by: Claude Code:claude-fable-5-1
@dahlia dahlia self-assigned this Oct 2, 2026
@dahlia dahlia added component/vocab Activity Vocabulary related component/vocab-tools Vocabulary code generation (@fedify/vocab-tools) labels Oct 2, 2026
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
CONTRIBUTING.md — configured

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c4604494-3b21-4c7f-8b25-e97e513b1e08

📥 Commits

Reviewing files that changed from the base of the PR and between fd218a8 and cea807f.

⛔ Files ignored due to path filters (3)
  • packages/vocab-tools/src/__snapshots__/class.test.ts.deno.snap is excluded by !**/*.snap
  • packages/vocab-tools/src/__snapshots__/class.test.ts.node.snap is excluded by !**/*.snap
  • packages/vocab-tools/src/__snapshots__/class.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (5)
  • CHANGES.md
  • changes.d/vocab-tools/clone-array-ownership.md
  • changes.d/vocab/clone-array-ownership.md
  • packages/vocab-tools/src/constructor.ts
  • packages/vocab/src/vocab.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

Generated vocabulary constructors and clone() methods copy property arrays. Regression tests cover caller-array isolation, independent clone dereferencing, and preservation of nested-object and URL identity.

Changes

Vocabulary array ownership

Layer / File(s) Summary
Copy property arrays in constructors and clones
packages/vocab-tools/src/constructor.ts, CHANGES.md, changes.d/vocab-tools/clone-array-ownership.md, changes.d/vocab/clone-array-ownership.md
Generated constructors and clones copy property arrays instead of retaining caller or source array references. Changelog entries describe the behavior and its effect on dereferencing.
Verify array ownership and clone isolation
packages/vocab/src/vocab.test.ts
Tests cover independent dereferencing and fetch caches, caller-array preservation, sparse arrays, trusted embedded-object identity, and cross-origin fetch isolation.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: palcimer

Merge Risk: ⚪ Minimal · up to cea80

The array-ownership change has no identified issue requiring resolution before merge.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the main change: isolating property arrays in vocabulary clones.
Description check ✅ Passed The description directly explains the shared-array bug, the .slice() fix, and the regression tests. It is related to the changeset.
Linked Issues check ✅ Passed The PR addresses #1207. Generated constructors and clone() methods copy plural-property arrays with .slice(). This prevents clone cache writes from changing the source and prevents cache writes fr…
Out of Scope Changes check ✅ Passed The source changes, regression tests, generator snapshots, and changelog entries all support the array-ownership and clone-isolation requirements in #1207. No unrelated change is identified.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@dahlia
dahlia requested review from 2chanhaeng and sij411 October 2, 2026 08:36
@codecov

codecov Bot commented Oct 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Files with missing lines Coverage Δ
packages/vocab-tools/src/constructor.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dahlia
dahlia merged commit bf0ebff into fedify-dev:2.0-maintenance Oct 3, 2026
17 checks passed
@dahlia
dahlia deleted the bugfix/vocab/clones-share-property-arrays branch October 3, 2026 07:01

@2chanhaeng 2chanhaeng left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use the title to summarize the intent of the change, and note which issue the PR addresses in the body. If the title only includes the issue number, GitHub cannot link the issue to the PR.
I think this PR needs a changelog entry because the number of remote lookups and the timing of key updates will change. These changes are visible to users.
I also think users should have an option to disable the cache. However, it doesn’t need to be implemented right away, so you can create a separate issue.
The PR description says AI was used, but the only commit that changed the code, 3ff6fa0, doesn’t have Assisted-by trailers in its message. Please add them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/vocab Activity Vocabulary related component/vocab-tools Vocabulary code generation (@fedify/vocab-tools)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

clone() shares property arrays, so dereferencing on a clone rewrites the original

2 participants