Repository navigation
Conversation
Copy nested vocabulary values without hydration leaking back to the source. Preserve graph aliases, defaults and historical portable referrers, while dropping cached JSON-LD and signed representations. Add regression coverage, a copy benchmark and manual guidance. Design was reviewed with Claude Code (claude-opus-5-5) and Codex (gpt-6-astra); prose was reviewed with Claude Code (claude-fable-5-1). Fixes fedify-dev#1243 fedify-dev#1242 fedify-dev#1246 fedify-dev/hollo#643 Assisted-by: Codex:gpt-6.1-sol Assisted-by: Claude Code:claude-fable-5-1
dahlia
added
component/vocab
✅ Deploy Preview for fedify-json-schema canceled.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Portable provenance retargeting packages/vocab-runtime/src/internal/portable-dereference.ts, packages/vocab-runtime/src/internal/portable-dereference.test.ts, packages/vocab/src/gateway.test.ts |
Adds a helper that replaces an existing provenance referrer in a copy when a matching object is resolved. Tests cover missing or unresolved referrers and historical collection ancestry. |
Generated deep-clone methods and graph copiers packages/vocab-tools/src/constructor.ts, packages/vocab-tools/src/deep-clone.ts, packages/vocab-tools/src/class.ts |
Generates deepClone() methods and per-type descriptors. The graph copier memoizes object copies, copies URL and Uint8Array values, and retargets portable references. |
Graph-copy tests and benchmark packages/vocab/src/deep-clone.test.ts, packages/vocab/src/gateway.test.ts, packages/vocab/src/signed-representation.test.ts, packages/vocab/src/vocab.bench.ts |
Adds coverage for graph isolation, aliases, cycles, options, trust, replacement values, subclasses, and signed representations. Adds activity-copy benchmarks for shallow clone, deep clone, and a JSON-LD round trip. |
API and behavior documentation docs/manual/vocab.md, claude-plugin/skills/fedify/SKILL.md, CHANGES.md, changes.d/vocab/deep-clone.md |
Documents the API, copy semantics, options, provenance, and representation handling. Adds the changelog entry and issue and PR references. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Feature · Severity of issue fixed: Medium
Sequence Diagram(s)
sequenceDiagram
participant Caller
participant DeepClone as Generated deepClone()
participant Clone as this.clone()
participant Graph as deepCloneGraph
participant Descriptor as Per-type clone descriptor
participant Provenance as retargetPortableReferrer
Caller->>DeepClone: Provide replacements and optional root settings
DeepClone->>Clone: Create root copy
DeepClone->>Graph: Copy source graph into root
Graph->>Descriptor: Create copies of typed objects
Graph->>Provenance: Retarget copied portable referrers
DeepClone-->>Caller: Return copied vocabulary object
Possibly related PRs
- fedify-dev/fedify#1247: Adds CI validation for changelog fragments, which applies to this PR’s changelog entries.
- fedify-dev/fedify#1331: Changes vocabulary code generation; this PR adds generators and generated vocabulary methods.
Merge Risk: 
1761a
-
No actionable issue remains from the reviewed changes; the PR is ready for normal merge checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title "Add deep cloning for vocabulary objects" matches the main change. The changeset adds a deepClone()method to vocabulary objects, and the PR objectives state the same goal. The title is co…Description check The description explains the motivation and design of the change. It states that deepClone()gives an independent graph whileclone()stays unchanged. It covers identity maps, replacement copying,…Linked Issues check Directly linked issue [ #1243] has coding requirements only. The PR meets them, based on the PR description and the change summary. No repository inspection was available, so this rests on the summary …Out of Scope Changes check Each change in the summary relates to the deepClone()objective in [#1243] or to its design.
-vocab-tools(class.ts,constructor.ts,deep-clone.ts): generates thedeepClone()method and …Full details: Title check
-
Explanation
The title "Add deep cloning for vocabulary objects" matches the main change. The changeset adds a
deepClone()method to vocabulary objects, and the PR objectives state the same goal. The title is concise and specific, and it does not describe any unrelated change.
Full details: Description check
-
Explanation
The description explains the motivation and design of the change. It states that
deepClone()gives an independent graph whileclone()stays unchanged. It covers identity maps, replacement copying, portable referrers, and discarded cached JSON-LD and signed representations, all of which appear in the changeset. It also links the related issues.
Full details: Linked Issues check
-
Explanation
Directly linked issue [
#1243] has coding requirements only. The PR meets them, based on the PR description and the change summary. No repository inspection was available, so this rests on the summary and the named tests.- Add
deepClone()generated by@fedify/vocab-tools:constructor.tsaddsgenerateDeepCloner, which emits a publicdeepClone(values, options)method.class.tsanddeep-clone.tsemit the supporting code into generated classes. - Same parameters as
clone(): the method takesvaluesand anoptionsobject with loader and verifier fields and callsthis.clone(values, ...)for each node. Exact option parity withclone()is not confirmed. The summary listsdocumentLoader,contextLoader, andverifyPortableObjectbut does not list the fullclone()option set. - Leave
clone()unchanged: the summary shows only documentation changes toclone(). The shallow behavior is kept. - Copy nested objects recursively, so hydration on either copy does not change the other:
deep-clone.tswalks the graph with an identity map.deep-clone.test.tscovers embedded quote hydration isolation. - Shared subobjects: the identity map preserves repeated references, which answers the first open question in [
#1243]. - Cached JSON-LD and trust/provenance: the PR discards cached JSON-LD and signed representations on every copied node. It keeps embedded-object trust and portable restrictions and does not inherit positive verification evidence. This answers the open questions in [
#1243]. Discarding the cache is a documented design choice, not a conflict with the issue. The Hollo echo case is covered by keeping the original object, as the PR description states. - Values passed in: replacement values are copied, which answers the fourth open question.
- Cost:
vocab.bench.tsadds shallow, deep, and JSON-LD round-trip benchmarks. - The 2.5.0 timing is a release note, not a coding requirement.
The excluded snapshot files (
class.test.ts*.snap) are test artifacts. No requirement depends on their contents. - Add
Full details: Out of Scope Changes check
-
Explanation
Each change in the summary relates to the
deepClone()objective in [#1243] or to its design.vocab-tools(class.ts,constructor.ts,deep-clone.ts): generates thedeepClone()method and its helpers. Direct implementation.vocab-runtime(portable-dereference.tsand its test): addsretargetPortableReferrer. The graph copy uses it to keep portable referrers pointing at historical copies. This supports the stated design and is not an unrelated change.vocabtests (deep-clone.test.ts,gateway.test.ts,signed-representation.test.ts): test the copy behavior, collection ancestry, and dropped signed representations.vocab.bench.ts: the cost benchmark named as an open question in [#1243].docs/manual/vocab.md,claude-plugin/skills/fedify/SKILL.md, andCHANGES.md/changes.d/vocab/deep-clone.md: document the new public method and its behavior, and follow the project changelog convention. Documentation for a new public API supports the objective.
No change in the summary lacks a connection to the objective.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 9 files. (4 skipped: 4 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
clone()shares nested objects, so resolving an embeddedNotecan change theQuoteRequestHollo later echoes in anAcceptorReject.deepClone()gives the resolving code an independent graph while leavingclone()cheap and unchanged.Fixes #1243. Related: #1242, #1246, fedify-dev/hollo#643.
Design
clone(), reusing its validation, subclass construction and trust rules. The graph copy then retains that node's loader, verifier, tracer and base URL defaults, with caller options applied only to the root so nested objects keep their own policies.x.deepClone({ object: x })does not manufacture a self-cycle in the updated root.