feat(project-system): recover malformed element files without ever rewriting them - #2159
feat(project-system): recover malformed element files without ever rewriting them#2159yuto-trd wants to merge 35 commits into
Conversation
…writing them Opening a scene whose .belm sidecar no longer parses previously failed the whole project open. Such elements now load as disabled fallback elements that retain the original raw text: open_project reports a warning naming the file and parser error while healthy elements keep loading and rendering. Recovered elements are unpersistable at the CoreSerializer level, so every save path — Scene.Serialize, the editor's Ctrl+S child loop, the auto-save service, and toolkit saves — leaves the un-parseable file byte-identical on disk, and the auto-save delete branch exempts them so removing a recovered element cannot destroy the recoverable sidecar. Their element Id comes from a quote-aware top-level scan of the raw text when present, or a deterministic UUIDv5 of the filename, so repeated opens agree, and the fallback's declarative projection carries a valid $type and Id so edits to unrelated elements reconcile normally.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe project system recovers malformed element sidecars with stable IDs and suppressed writes. Project opening reports deserialization warnings and structured incidents. Save, rehome, autosave, rendering, editing, and deletion preserve recovered source bytes. ChangesMalformed element recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OpenProject
participant Scene
participant ElementSidecar
participant CoreSerializer
OpenProject->>Scene: Open project and restore elements
Scene->>ElementSidecar: Read serialized element
Scene->>CoreSerializer: Deserialize element
CoreSerializer-->>Scene: Return fallback or restored object
CoreSerializer-->>Scene: Record fallback incident
Scene->>Scene: Preserve recovery metadata and raw bytes
Scene-->>OpenProject: Return recovered elements
OpenProject->>OpenProject: Collect warnings and incidents
OpenProject-->>OpenProject: Return project summary
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Code Review BotNo comment/code divergences or documentation drift (partially analyzed) detected. Reviewed 61 file(s); skipped 4. |
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (35): Last reviewed commit: "fix: preserve recovery repair workflows" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs (1)
35-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the corrupt payload into one field.
The same truncated-JSON literal appears on Lines 35, 79, 93, and 111. A single
private static readonly byte[] s_corruptByteskeeps the four tests in sync when the payload shape changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs` around lines 35 - 36, Extract the repeated truncated-JSON byte payload into a single private static readonly field named s_corruptBytes in MalformedElementRecoveryTests, then replace the inline literals at all four test locations with that shared field while preserving the existing test behavior.tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs (2)
674-723: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse this fixture in the two warning tests.
CreateProjectWithMalformedElementrepeats the project, healthy element, and malformed element setup thatOpen_project_warns_about_malformed_element_json_and_keeps_healthy_elementsbuilds inline on Lines 98-135. The two blocks differ only in the malformed payload. Add a payload parameter to the fixture builder and call it from that test. Also extract the repeatedRenderToolsconstruction (Lines 66-78 and 144-156) into a helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs` around lines 674 - 723, Refactor the malformed-element warning tests to reuse CreateProjectWithMalformedElement: add a malformed-payload parameter, use it when writing the malformed element JSON, and update Open_project_warns_about_malformed_element_json_and_keeps_healthy_elements to obtain its setup from the fixture. Extract the duplicated RenderTools construction into a helper and use that helper in both warning tests.
165-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo warning assertions depend on error text that Beutl does not own. The shared root cause is that both tests match substrings produced by the JSON layer rather than the message
CollectDeserializationWarningsformats. A .NET or converter update can reword that text and break both tests without any behavior regression. Assert on the element filename plus the Beutl-owned phrasecould not be deserialized, and treat the parser detail as an optional extra assertion.
tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs#L165-L169: replace the"JsonReaderException"and"invalid start"substring matches with the Beutl-owned warning phrase.tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs#L87-L89: replace the"could not be converted"substring match with the Beutl-owned warning phrase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs` around lines 165 - 169, The warning assertions in SessionToolsTests should rely on Beutl-owned wording rather than parser-specific error text. At tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs:165-169, keep the malformed element filename assertion and replace the JsonReaderException and invalid start checks with “could not be deserialized”; at tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs:87-89, replace the “could not be converted” check with the same phrase. Parser details may remain only as optional assertions.src/Beutl.ProjectSystem/ProjectSystem/Scene.cs (1)
64-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify recovered-element tracking.
_recoveredElementsonly stores entries made whenMarkRecoveredElementsetselement.IsStorageWriteSuppressed = true, andCoreSerializer.StoreToUrialready returns for that state.RecoveredElementSource.RawTextis not read anywhere in the codebase, and entries are never removed when children are detached. If no future code needs the retained raw text, remove the dictionary and rely onElement.IsStorageWriteSuppressed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs` around lines 64 - 65, Remove the unused _recoveredElements tracking and any associated RecoveredElementSource/RawText bookkeeping in Scene and recovery-related methods. Preserve MarkRecoveredElement’s behavior by setting Element.IsStorageWriteSuppressed directly, and update callers to rely on that flag instead of dictionary lookups or retained raw text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Beutl.AgentToolkit/Tools/SessionTools.cs`:
- Around line 80-83: Update the session response construction so
CollectDeserializationWarnings(result.Project) executes through
result.Session.ReadOnSession, matching the dispatch used by CreateSummary. Keep
the warning collection within the session-dispatch callback to ensure traversal
of the live project graph occurs on the owning editor thread.
In `@src/Beutl.Core/Serialization/CoreSerializer.cs`:
- Around line 230-234: Update the CoreSerializer flow around StoreToUri so
IsStorageWriteSuppressed does not make skipped writes indistinguishable from
successful writes: when the target URI changes, allow the write and Uri update,
and for new locations copy the original recovered sidecar bytes. Preserve
suppression only when no URI relocation or sidecar copy is required.
In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs`:
- Around line 765-784: Remove the matches[0] fallback branch from
ResolveRecoveredElementId; only accept an ID returned by FindTopLevelIdMatch,
and otherwise fall through to CreateVersion5Guid using the relative path.
- Around line 694-733: The catch in RestoreElementOrFallback currently handles
only JsonException; broaden it to include InvalidOperationException and the
serializer’s unsupported-deserialization exception type so valid sidecars with
unresolvable types enter the existing fallback construction path. Preserve the
current fallback metadata, projection, recovery marking, and return behavior.
In `@tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs`:
- Around line 141-142: Add a success assertion immediately after each
OpenProject call in tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs at
lines 141-142, 183-183, and 219-219, using opened.IsSuccess and
opened.Error?.Message before accessing opened.Value, opened.Value!.Session, or
serializing the result.
In `@tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs`:
- Around line 51-54: Update the recovery test around the first and second
restored IDs to assert that the recovered ID is not Guid.Empty, while retaining
the existing equality assertion between both restores.
---
Nitpick comments:
In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs`:
- Around line 64-65: Remove the unused _recoveredElements tracking and any
associated RecoveredElementSource/RawText bookkeeping in Scene and
recovery-related methods. Preserve MarkRecoveredElement’s behavior by setting
Element.IsStorageWriteSuppressed directly, and update callers to rely on that
flag instead of dictionary lookups or retained raw text.
In `@tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs`:
- Around line 674-723: Refactor the malformed-element warning tests to reuse
CreateProjectWithMalformedElement: add a malformed-payload parameter, use it
when writing the malformed element JSON, and update
Open_project_warns_about_malformed_element_json_and_keeps_healthy_elements to
obtain its setup from the fixture. Extract the duplicated RenderTools
construction into a helper and use that helper in both warning tests.
- Around line 165-169: The warning assertions in SessionToolsTests should rely
on Beutl-owned wording rather than parser-specific error text. At
tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs:165-169, keep the
malformed element filename assertion and replace the JsonReaderException and
invalid start checks with “could not be deserialized”; at
tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs:87-89, replace the
“could not be converted” check with the same phrase. Parser details may remain
only as optional assertions.
In `@tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs`:
- Around line 35-36: Extract the repeated truncated-JSON byte payload into a
single private static readonly field named s_corruptBytes in
MalformedElementRecoveryTests, then replace the inline literals at all four test
locations with that shared field while preserving the existing test behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ef5de1e-6292-495a-bc04-9373741552d7
📒 Files selected for processing (7)
src/Beutl.AgentToolkit/Tools/SessionTools.cssrc/Beutl.Core/CoreObject.cssrc/Beutl.Core/Serialization/CoreSerializer.cssrc/Beutl.Editor/AutoSaveService.cssrc/Beutl.ProjectSystem/ProjectSystem/Scene.cstests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cstests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b10445d024
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Save-as now copies a recovered element's retained raw bytes to the new location instead of silently skipping the write, so a saved-as project keeps the element while the original file stays untouched. Recovery catches the full deserialization-domain failure set (a resolvable but non-Element $type surfaced as InvalidCastException and aborted the whole open), reads sidecar text only on the recovery path instead of doubling every healthy element's I/O, and never adopts a nested or quoted Id when no top-level Id exists. open_project collects deserialization warnings on the session thread, and the tests assert open success before use, a non-empty recovered Id, save-as rehoming, and non-Element-discriminator recovery.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1688213481
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…discriminators before deserializing - SuppressedStorageSource now retains raw bytes, so rehoming keeps a BOM, foreign encodings, and undecodable bytes verbatim. - The rehome path no longer mutates the suppression record; the source location stays skip-protected even if a failed multi-file save rolls Uri back. - TypeFormat.ToType returns null for unparsable names instead of leaking parser exceptions, and the legacy discriminator fill-in keys on string presence so garbage $type recovers instead of loading as the default. - RestoreFromUri rejects a discriminator type incompatible with the expected type before instantiating it, preventing wrong-type load side effects (e.g. a Scene declared in a .belm globbing element files).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8acc3245f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- The legacy discriminator fill-in keys on the $type/@type property key alone, so a non-string or blank discriminator recovers instead of loading as the legacy default. - The recovery filter inverts to catch every non-filesystem failure (value converters throw freely, e.g. FormatException from Color.Parse); IOException/UnauthorizedAccessException still propagate. - Fallback creation records a thread-local incident so elements whose only fallback lives outside the hierarchy (e.g. a plain property or keyframe value) are still byte-frozen. - A recovered element that surfaces an Id another element owns yields it and falls back to its deterministic path-derived identity. - The rehome branch is create-only: it never overwrites an existing file, preserving manual repairs at the destination. - The toolkit's Save As keeps element sidecar file names so path-derived recovery identities stay stable across rehoming.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Beutl.Core/Serialization/CoreSerializer.cs`:
- Around line 243-271: Replace the File.Exists check and subsequent
File.WriteAllBytes call in the suppressedObj rehoming branch with an atomic
FileMode.CreateNew write, preserving the existing-file behavior by catching the
already-exists condition, updating suppressedObj.Uri, and returning without
overwriting the file.
In `@src/Beutl.Core/TypeFormat.cs`:
- Around line 13-25: Update ParseNestedType to guard the resolved type before
calling MakeGenericType, returning null when parent?.GetNestedType or
_assembly?.GetType yields null. Also catch ArgumentException from
Type.MakeGenericType, while preserving the existing null result behavior for
malformed type names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8db0d964-a15f-463c-8910-0be1fe9be983
📒 Files selected for processing (15)
src/Beutl.AgentToolkit/Sessions/FileEditingSession.cssrc/Beutl.AgentToolkit/Tools/SessionTools.cssrc/Beutl.Core/CoreObject.cssrc/Beutl.Core/Serialization/CoreSerializer.cssrc/Beutl.Core/Serialization/DeserializationIncidents.cssrc/Beutl.Core/Serialization/FallbackDeserializationHelper.cssrc/Beutl.Core/Serialization/JsonSerializationContext.Deserialize.cssrc/Beutl.Core/Serialization/SuppressedStorageSource.cssrc/Beutl.Core/TypeFormat.cssrc/Beutl.Editor/AutoSaveService.cssrc/Beutl.ProjectSystem/ProjectSystem/Scene.cstests/Beutl.AgentToolkit.Tests/Sessions/FileEditingSessionTests.cstests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cstests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cstests/Beutl.UnitTests/Serialization/DeserializationIncidentsTests.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Beutl.Editor/AutoSaveService.cs
- tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs
- src/Beutl.ProjectSystem/ProjectSystem/Scene.cs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e663648f57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…riminator parsing - The create-only rehome now opens the destination with FileMode.CreateNew, closing the exists-check/write race; an already-existing file is treated as the protected skip path and other IO failures still propagate. - TypeNameParser.ParseNestedType returns null when the assembly or nested type cannot be resolved instead of calling MakeGenericType on null, and ToType's filter also absorbs ArgumentException so malformed generic discriminators read as unknown types.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13d9a6212a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…command, and nested rehome paths - Nested deserialization (TryDeserializeCoreSerializable and DeserializeFromJsonObject) rejects a discriminator type that is not assignable to the expected base before instantiating it, so a Scene declared inside Objects becomes a fallback instead of recursively reopening its own sidecar. - open_project warning collection traverses each property's keyframe animation values, surfacing fallbacks that live only in keyframes. - Scene's DeleteCommand skips File.Delete for elements carrying a suppressed storage source, so deleting a recovered element keeps the retained sidecar bytes. - Save As preserves each sidecar's scene-relative subpath (falling back to the basename for rooted/escaping paths), keeping path-derived recovery identities stable for nested layouts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29f89260e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Beutl.ProjectSystem/ProjectSystem/Scene.cs (3)
712-715: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReject generated IDs that are already claimed.
The
claimedIds.Add(child.Id)result is ignored after assigning the path-derived ID. If that ID matches a healthy element or an earlier recovered element, duplicateElement.Idvalues remain. Check each generated candidate and derive another deterministic candidate when the candidate is already claimed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs` around lines 712 - 715, Update the recovered-element ID assignment in the surrounding recovery method to check the result of claimedIds.Add for the path-derived candidate. When the candidate is already claimed, deterministically derive another candidate and retry until it can be added, then assign that unique ID to child.Id.
741-745: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftLimit recovery to content-related exceptions.
This catch block converts every non-I/O
Exceptioninto a disabled fallback element, including conversion failures as intended, but also programming errors and fatal runtime conditions. Project loading can then succeed while hiding the failure and preserving only raw bytes. Catch the known deserialization and conversion exceptions, and rethrow cancellation plus fatal runtime exceptions such asOutOfMemoryException.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs` around lines 741 - 745, Update the recovery catch filter in Scene loading to catch only the known deserialization and value-conversion exceptions needed for fallback handling. Explicitly exclude cancellation and fatal runtime exceptions such as OutOfMemoryException, while preserving propagation of filesystem failures and unexpected programming errors.
799-813: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire a root object and a non-empty ID before using the recovered ID.
Guid.TryParse("00000000-0000-0000-0000-000000000000", out var id)returnstruewhile reassigningGuid.Empty;FindTopLevelIdMatchalso accepts an inner object of a root array. Require the root JSON object andtopLevelId != Guid.Emptybefore callingreturn topLevelId.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs` around lines 799 - 813, Update ResolveRecoveredElementId to require the recovered JSON root to be an object, ensure FindTopLevelIdMatch does not accept an inner object from a root array, and only return topLevelId when the parsed value is non-empty (topLevelId != Guid.Empty). Otherwise preserve the existing deterministic filename GUID fallback.src/Beutl.Core/Serialization/CoreSerializer.cs (1)
266-280: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate write failures after a successful
CreateNew.The
tryblock also coversstream.Write; dispose-related failures can follow the same path. If creation succeeds but writing or disposal fails, the destination still exists, so the catch treats the partial file as an existing destination and returns successfully. Catch the existing-file error only aroundFileStreamconstruction. Let write and disposal failures propagate, and remove any partial destination.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Beutl.Core/Serialization/CoreSerializer.cs` around lines 266 - 280, Restrict the existing-file IOException handling in the recovery flow to the FileStream construction in CoreSerializer, so stream.Write and disposal failures propagate instead of being treated as successful recovery. If writing or disposal fails after creation, remove the partial rehomedPath destination before rethrowing, while preserving the existing-file behavior for CreateNew collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Beutl.Core/Serialization/CoreSerializer.cs`:
- Around line 70-75: Update both CoreSerializer entry points and
TryDeserializeCoreSerializable to resolve an existing $type/@type discriminator
before checking baseType.IsSealed. Validate that the resolved actualType is
assignable to baseType before instantiation; use baseType only when neither
discriminator key is present and fallback is required.
---
Outside diff comments:
In `@src/Beutl.Core/Serialization/CoreSerializer.cs`:
- Around line 266-280: Restrict the existing-file IOException handling in the
recovery flow to the FileStream construction in CoreSerializer, so stream.Write
and disposal failures propagate instead of being treated as successful recovery.
If writing or disposal fails after creation, remove the partial rehomedPath
destination before rethrowing, while preserving the existing-file behavior for
CreateNew collisions.
In `@src/Beutl.ProjectSystem/ProjectSystem/Scene.cs`:
- Around line 712-715: Update the recovered-element ID assignment in the
surrounding recovery method to check the result of claimedIds.Add for the
path-derived candidate. When the candidate is already claimed, deterministically
derive another candidate and retry until it can be added, then assign that
unique ID to child.Id.
- Around line 741-745: Update the recovery catch filter in Scene loading to
catch only the known deserialization and value-conversion exceptions needed for
fallback handling. Explicitly exclude cancellation and fatal runtime exceptions
such as OutOfMemoryException, while preserving propagation of filesystem
failures and unexpected programming errors.
- Around line 799-813: Update ResolveRecoveredElementId to require the recovered
JSON root to be an object, ensure FindTopLevelIdMatch does not accept an inner
object from a root array, and only return topLevelId when the parsed value is
non-empty (topLevelId != Guid.Empty). Otherwise preserve the existing
deterministic filename GUID fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bcaef780-8c81-434f-9965-efa26f1768ba
📒 Files selected for processing (8)
src/Beutl.AgentToolkit/Sessions/FileEditingSession.cssrc/Beutl.AgentToolkit/Tools/SessionTools.cssrc/Beutl.Core/Serialization/CoreSerializer.cssrc/Beutl.Core/Serialization/JsonSerializationContext.Deserialize.cssrc/Beutl.ProjectSystem/ProjectSystem/Scene.cstests/Beutl.AgentToolkit.Tests/Sessions/FileEditingSessionTests.cstests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cstests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Beutl.Core/Serialization/JsonSerializationContext.Deserialize.cs
- src/Beutl.AgentToolkit/Sessions/FileEditingSession.cs
- tests/Beutl.AgentToolkit.Tests/Sessions/FileEditingSessionTests.cs
- src/Beutl.AgentToolkit/Tools/SessionTools.cs
- tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs
- tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs
…m, and recovery scanning - A rehome write failure after FileMode.CreateNew succeeded deletes the partial file and rethrows instead of being misread as a pre-existing repair; only a create-phase failure with the file present skips. - Recovered-id dedupe processes children in stable sidecar-path order (the parallel load is unordered) and derives further deterministic UUIDv5 candidates when the path-derived replacement is itself taken. - Fallback projections keep the original discriminator; the runtime fallback type is written only when the projection has none. - Save As containment resolves the full path instead of a '..' prefix test, so directories like '..assets' keep their subpath. - The top-level Id scan tracks array nesting, so a root-array sidecar's inner Id is never adopted.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be2f05d0a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…k edits with non-hierarchical fallbacks - Recovered-id UUIDv5 inputs and the dedupe ordering key normalize the scene-relative path to forward slashes, so identities match across Windows and Unix. - Collision remaps persist in the scene file (RecoveredElementIds, path -> Guid): a remap applies on load regardless of the current claimant set, entries are pruned when their sidecar heals or leaves, and a persisted id a healthy element now owns is dropped in favor of a fresh deterministic derivation. - Recovery warnings name the scene-relative sidecar path, so same-named files in different subdirectories are distinguishable. - Reconciler baseline collection uses the same full serialized-graph traversal as its sandbox (property values and keyframe animation values), so a pre-existing non-hierarchical fallback no longer rejects every apply_edit. - The sealed-baseType discriminator short-circuit is kept and documented: sealed wrappers such as Optional<T> carry the wrapped payload's $type on their own node and interpret it themselves.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59bd61dd4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 714efed907
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccf5e4dab8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4402ffbab2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c532e3786d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a0b98df3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b01b8b973a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 400f7a96a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0db5276f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbb2493af7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Make recovered reference migration cycle-safe and explicit across property, keyframe, expression, and wrapper contracts. Preserve long-lived recovery state, retain sidecar identity safely, and share serialized-graph and path-boundary traversal. BREAKING CHANGE: Custom IProperty/IProperty<T> implementations must implement ReplaceCurrentValue; custom IKeyFrame implementations must implement ReplaceValue; custom IReferenceExpression implementations must implement Rebind; IReferenceRewritable implementations must use CreateReferenceRewriteTarget plus void RewriteReferences. Beutl.AgentToolkit.Common.PathBoundary and PathComparison were replaced by an internal Beutl.Core path helper. Affected projects: Beutl.Core, Beutl.Engine, Beutl.ProjectSystem, and Beutl.AgentToolkit.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Addressed the additional recovery review summary in 81cfb0c.
Validation:
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
CI regression follow-up pushed in
Local validation:
|
|
No TODO comments were found. |
Minimum allowed line rate is |
Description
.belmsidecar no longer parses previously failed the whole project open. Such elements now load as disabled fallback elements that retain the original raw text, andopen_projectreports a warning naming the file and the parser error while healthy elements keep loading and rendering.CoreSerializerlevel, so every save path —Scene.Serialize, the editor's Ctrl+S child loop, the auto-save service, and toolkit saves — leaves the un-parseable file byte-identical on disk. The auto-save delete branch exempts them, so removing a recovered element cannot destroy the recoverable sidecar.$typeandIdso edits to unrelated elements reconcile normally.Affected areas
Beutl.Engine(rendering / scene / track)Beutl.ProjectSystem(project / document persistence)Beutl.Editor,Beutl.Editor.Components,Beutl.Controls)Beutl.Extensibility(plugin abstractions)Beutl.NodeGraph(node editor)Beutl.FFmpegIpc/Beutl.FFmpegWorker(media IPC boundary)Beutl.Api(server API client)Breaking changes
None. Opening a project with a corrupt element previously failed outright; it now succeeds with a warning and never rewrites the corrupt file.
Test plan
tests/Beutl.UnitTests/ProjectSystem/MalformedElementRecoveryTests.cs— byte-identity across open/edit/save for truncated and wrong-$typesidecars, stable Ids across repeated opens (including nested-Id-first inputs), auto-save delete exemption, direct childStoreToUriandAutoSaveService.SaveObjectsdesktop paths.tests/Beutl.AgentToolkit.Tests/Tools/SessionToolsTests.cs—open_projectwarnings naming the file and parser error; no false positives on healthy projects; edits to unrelated elements succeed while a recovered element exists.Fixed issues / References
Extracted from the feature-004 review hardening so it can land independently of
speckit/004-gpu-pass-fusion.Summary by CodeRabbit
New Features
Bug Fixes