fix exact StartPos restores after restarting Celeste - #121
Conversation
|
@greptileai review |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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: ace9e69016
ℹ️ 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".
ace9e69 to
77e91eb
Compare
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (37)
This PR persists complete v6 StartPos reconstruction graphs in
WalkthroughThis change replaces embedded StartPos room-state data with persistent v6 reconstruction snapshots. StartPos capture and restoration use stable engine-update scheduling and restore runtime, audio, random, visual, and gameplay-buffer state. Setup packs store validated snapshot attachments in the v3 archive format with slot binding and rollback. Community downloads stream with size and checksum validation. QA commands capture reference frames and load diagnostics. Tests and documentation cover persistence, reconstruction, archive validation, and compatibility rules. Sequence Diagram(s)sequenceDiagram
participant QACommand
participant AutomationService
participant StartPosActions
participant AkronSaveLoad
participant LevelRender
QACommand->>AutomationService: defer run completion
QACommand->>StartPosActions: capture or load StartPos
StartPosActions->>AkronSaveLoad: persist or restore runtime snapshot
AkronSaveLoad->>LevelRender: restore gameplay buffer and frame generation
LevelRender-->>AutomationService: complete deferred run
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/module-settings-tests.cs (1)
4161-4176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test now has only negative cases.
All three assertions expect
false. An implementation ofIsStartPosInAreathat always returnsfalsepasses this test, so the rename toStartPosEntriesRequireLiveRuntimeSnapshotsis not actually pinned. Add one positive case with a matchingAreaSidand aStateSlotNamethatAkronSaveLoadService.HasRuntimeStateaccepts, so the test distinguishes "requires a live snapshot" from "always rejects".🤖 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/module-settings-tests.cs` around lines 4161 - 4176, Add a positive assertion to StartPosEntriesRequireLiveRuntimeSnapshots using an AkronStartPos with matching AreaSid and a StateSlotName accepted by AkronSaveLoadService.HasRuntimeState, while preserving the existing negative cases. Ensure InvokeStartPosInArea returns true for the valid live-runtime snapshot so the test rejects an always-false implementation.Source/Setups/akron-setup-packs.cs (1)
49-60: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemoving
RoomStateSnapshotwithout changingSetupPackFormatbreaks import of existing v2 packs.
SetupPackFormatstays"akron-setup-v2",JsonOptionssetsUnmappedMemberHandling = JsonUnmappedMemberHandling.Disallow, andValidateJsonContractValuecallsRequireExactJsonPropertieson every StartPos entry. A pack exported by an earlier build still containsroomStateSnapshotin its StartPos entries.Readnow rejects it with "Setup pack StartPos entries fields do not match the akron-setup-v2 contract", andImportreports the generic "Unsupported setup pack." message.Either bump the format string and add an explicit migration, or keep tolerating the removed property and discard its value on read.
🤖 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 `@Source/Setups/akron-setup-packs.cs` around lines 49 - 60, Preserve compatibility with existing "akron-setup-v2" packs by keeping the removed RoomStateSnapshot/roomStateSnapshot property accepted during StartPos deserialization while discarding its value. Update the StartPos contract validation used by ValidateJsonContractValue and RequireExactJsonProperties, along with the relevant AkronStartPosPackEntry mapping, so strict validation no longer rejects this legacy field; do not change the format identifier.Source/SaveLoad/akron-native-savestate-support.cs (1)
318-318: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore the force tracker rebuild for in-memory Savestates.
Load,RestoreRuntimeState, andRestorePersistentRuntimeStateall run sharedloadedValuescallbacks after copying or reloading entities. Persistent restores callTracker.Refresh(level, force: true)before those callbacks; native runtime saves only call it during Save. Restore a matchingTracker.Refresh(level, force: true)inRestoreNativeSlotor the caller so helper hooks do not crash on tracked types registered after the captured room.🤖 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 `@Source/SaveLoad/akron-native-savestate-support.cs` at line 318, Restore the forceful tracker rebuild in the native in-memory savestate restore flow by adding Tracker.Refresh(level, force: true) to RestoreNativeSlot or its immediate caller after entities are copied/reloaded and before loadedValues callbacks execute. Match the ordering used by Load, RestoreRuntimeState, and RestorePersistentRuntimeState so tracked types registered after the captured room are rebuilt before helper hooks run.
🤖 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 `@Source/Actions/akron-startpos-actions.cs`:
- Around line 139-152: The deferred callbacks in
Source/Actions/akron-startpos-actions.cs at lines 139-152 and 226-231 must
revalidate the active scene before mutating runtime state: in the inner callback
around the persist/restore flow, return immediately when Engine.Scene != level
and call AkronSaveLoadService.DiscardRuntimeStateMemory(stateSlotName) first;
around RestoreStartPos, wrap the call in a lambda that returns when Engine.Scene
!= level. Use the existing scene-check pattern from the outer capture callback
and RestoreStartPosAfterDeath.
- Around line 551-555: The StartPos restore logic currently rejects
position-only entries whose StateSlotName is empty. Update
HasRestorableStartPosState and the related BuildRuntimeStartPositions import
flow to preserve and expose these entries, either by allowing empty
StateSlotName values as valid position-only states or by retaining the imported
slot value before persistence, while keeping runtime-state validation for
entries that specify a slot.
In `@Source/Automation/akron-automation-service.cs`:
- Around line 263-266: Add a frame-based deadline when runCompletionDeferred is
set, and update both runCompletionDeferred checks in ProcessPendingCommands to
finalize the active run with a failure status when that deadline expires;
otherwise preserve the pending behavior. Ensure the timeout path clears the
deferred/active state and allows later commands, including the secondary check
around CompleteDeferredRun, to proceed without repeatedly writing pending
results.
In `@Source/Commands/akron-qa-commands.cs`:
- Around line 459-461: Move the AkronAutomationService.DeferRunCompletion call
out of the setup before AkronActions.LoadStartPos and place it only after
ScheduleAfterStableEngineUpdate has successfully registered the completion
callback. Preserve the existing start-position setup, and ensure any failure
before scheduling cannot leave the automation run deferred.
In `@Source/Module/AkronModule.cs`:
- Around line 684-692: Update RunAfterEngineUpdateActions to isolate each queued
action invocation: catch exceptions from Dequeue().Invoke(), log the failure
using the module’s existing logging mechanism, and continue processing the
remaining actions in the captured boundary. Preserve the current count-based
draining behavior so newly scheduled actions wait for the next engine update.
- Around line 736-747: Update the render-target search in the surrounding
hook-installation method to match only the null-to-SetRenderTarget call whose
target operand is GameplayBuffers.Level, rather than the first matching sequence
by method name. Preserve the existing warning and early return when the anchored
hook cannot be found, then emit the PresentArmedLevelBuffer delegate only after
that exact match.
In `@Source/SaveLoad/akron-native-savestate-support.cs`:
- Around line 45-66: Update AkronRandomState.Restore to preserve the existing
random-stack depth when randomStackTopFirst is empty, instead of replacing it
with an empty stack. Build the replacement stack with cloned entries matching
the current stack depth, while retaining the saved-stack reconstruction and RNG
restoration behavior for non-empty randomStackTopFirst.
In `@Source/SaveLoad/akron-reconstruction-graph.cs`:
- Around line 138-152: Update Restore and the surrounding
AkronReconstructionGraph restore flow to track every VirtualRenderTarget created
by VirtualContent.CreateRenderTarget during restoration, and dispose tracked
targets when restoration fails or a node is replaced on a subsequent restore.
Preserve existing-resource ownership and successful restore behavior, and ensure
cleanup covers targets created before any later restore step throws.
- Around line 261-289: Add an explicit reset method for the armed presentation
state that clears both armedLevelPresentation and armedPresentationLevel, then
invoke it from AkronModule.Unload and the level-unload path. Update
PresentArmedLevelBuffer and ArmLevelPresentation only as needed to reuse this
reset, ensuring no static Level reference or cloned buffer remains after scene
or module teardown.
In `@Source/SaveLoad/AkronSaveLoad.cs`:
- Around line 84-88: The sequential identity returned by
GetNextRegisteredActionId is unstable when conditional or third-party
registrations change. Replace it with a deterministic ID derived from each
action’s stable metadata, such as the declaring type’s assembly-qualified name
and registering mod metadata name, and update RegisterSaveLoadAction
callers/data flow to supply that metadata. In RestorePersistentRuntimeState,
validate that the snapshot’s recorded action-ID set exactly matches the current
registrations and reject mismatches before restoring state.
- Around line 623-651: Update AkronCumulativeStats.Capture and
RestoreWithoutRewinding to safely access Areas_Safe and Modes using the same
bounds-checking logic as AkronModule.TryGetAreaModeStats. Treat out-of-range or
missing AreaModeStats as zero in Capture, and skip area-specific restoration
when no valid stats entry exists while preserving the existing session and
save-data restoration.
- Around line 464-489: Update RestorePersistentRuntimeState to bracket
TryLoadFreshRoom with AkronIgnoreSaveStateComponent.RemoveAll(level) before
loading and ReAddAll(level) afterward, matching the pattern used by
RestoreRuntimeState. Ensure ignored entities are temporarily removed from
level.Entities during room reconstruction and restored afterward.
In `@tests/packages.lock.json`:
- Around line 15-27: Regenerate tests/packages.lock.json with a trusted restore
using RestorePackagesWithLockFile, starting from the MonoMod.RuntimeDetour
dependency entry and tests/akron-tests.csproj request. Ensure the resulting lock
graph reflects the published dependency ranges, particularly MonoMod.Backports,
MonoMod.ILHelpers, and MonoMod.Utils, rather than retaining incompatible pinned
versions.
In `@tests/setup-pack-tests.cs`:
- Around line 626-629: Update the snapshot assertion near
SerializePackPayloadForArchive to probe the serialized camelCase key for
RoomStateSnapshot and use a case-insensitive containment comparison. Preserve
the existing StartPositions assertion and ensure the check remains effective if
serialized casing changes.
In `@tests/startpos-persistence-tests.cs`:
- Around line 183-191: Update the verification lookup in the start-position
persistence test to search for the stable call symbol
“AkronStartPosReconstruction.Verify” without newline or indentation assumptions.
Preserve the ordering assertion against trackerRefresh, and bound the
verification-context substring length to the available source length before
passing it to Assert.Contains.
In `@tests/startpos-reconstruction-tests.cs`:
- Around line 51-60: Move the capture.Success assertion immediately after every
graph.Capture call, before any access to capture.Document or subsequent
reconstruction operations. Apply this consistently at the anchor test and the
additional affected tests, including
ReconstructionUsesTheFreshResourceAtTheSavedStructuralPath,
RestoreSeparatesOrdinaryObjectsThatTheFreshRoomAliases,
MissingFreshResourceFailsAtItsExactPathBeforeChangingTheRoom,
StructuralOwnerPathFindsAFreshResourceWhenItsRuntimeNameAndListIndexChange, and
UniqueRoomObjectTypeMatchesAfterEntityListOrderChanges, while preserving
capture.Error in the assertion.
---
Outside diff comments:
In `@Source/SaveLoad/akron-native-savestate-support.cs`:
- Line 318: Restore the forceful tracker rebuild in the native in-memory
savestate restore flow by adding Tracker.Refresh(level, force: true) to
RestoreNativeSlot or its immediate caller after entities are copied/reloaded and
before loadedValues callbacks execute. Match the ordering used by Load,
RestoreRuntimeState, and RestorePersistentRuntimeState so tracked types
registered after the captured room are rebuilt before helper hooks run.
In `@Source/Setups/akron-setup-packs.cs`:
- Around line 49-60: Preserve compatibility with existing "akron-setup-v2" packs
by keeping the removed RoomStateSnapshot/roomStateSnapshot property accepted
during StartPos deserialization while discarding its value. Update the StartPos
contract validation used by ValidateJsonContractValue and
RequireExactJsonProperties, along with the relevant AkronStartPosPackEntry
mapping, so strict validation no longer rejects this legacy field; do not change
the format identifier.
In `@tests/module-settings-tests.cs`:
- Around line 4161-4176: Add a positive assertion to
StartPosEntriesRequireLiveRuntimeSnapshots using an AkronStartPos with matching
AreaSid and a StateSlotName accepted by AkronSaveLoadService.HasRuntimeState,
while preserving the existing negative cases. Ensure InvokeStartPosInArea
returns true for the valid live-runtime snapshot so the test rejects an
always-false implementation.
🪄 Autofix
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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c8a1a70b-b783-48ae-b3c6-4b54e21c444b
📒 Files selected for processing (29)
CHANGELOG.mdSource/Actions/akron-startpos-actions.csSource/Automation/akron-automation-service.csSource/Commands/akron-qa-commands.csSource/Commands/akron-startpos-commands.csSource/Community/akron-community-pack-uploads.csSource/Core/AkronIgnoreSaveStateComponent.csSource/Core/AkronModuleSaveData.csSource/Core/AkronModuleSession.csSource/Core/akron-event-instance-utils.csSource/Module/AkronModule.csSource/SaveLoad/AkronSaveLoad.csSource/SaveLoad/akron-native-savestate-support.csSource/SaveLoad/akron-persistent-startpos-snapshots.csSource/SaveLoad/akron-reconstruction-graph.csSource/SaveLoad/akron-save-load-models.csSource/Setups/akron-setup-packs.csSource/Tools/akron-capture.csdocs/feature-guide/startpos.mdxdocs/player-guide/community-packs.mdxdocs/project/startpos-exact-state-feasibility.mdxdocs/troubleshooting/startpos-recovery.mdxtests/akron-tests.csprojtests/module-settings-tests.cstests/packages.lock.jsontests/screenshot-scanner-tests.cstests/setup-pack-tests.cstests/startpos-persistence-tests.cstests/startpos-reconstruction-tests.cs
💤 Files with no reviewable changes (5)
- Source/SaveLoad/akron-persistent-startpos-snapshots.cs
- Source/Commands/akron-startpos-commands.cs
- Source/Community/akron-community-pack-uploads.cs
- Source/Core/AkronModuleSession.cs
- Source/Core/AkronModuleSaveData.cs
|
@greptileai review |
77e91eb to
02dca99
Compare
02dca99 to
44d08b2
Compare
44d08b2 to
7230952
Compare
|
@greptileai review |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7230952a15
ℹ️ 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: 19
🤖 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 `@CHANGELOG.md`:
- Around line 9-17: Update the Unreleased CHANGELOG entry to explicitly document
the breaking setup-pack format change: AkronSetupPacks.SetupPackFormat now uses
akron-setup-v3, and existing akron-setup-v2 local and community packs are
rejected during import.
In `@docs/reference/akr-archives.mdx`:
- Around line 69-79: Update the JSON example’s section value from “Audio” to
“Whole” so it matches the included buttonBindings, menuActionBindings, and
startPositions properties validated by ValidatePortablePackJson; keep the
combined payload fields unchanged.
In `@Source/Actions/akron-startpos-actions.cs`:
- Around line 159-169: Update the failure message in the persist/restore
validation block to report the failed operation’s result: use persistResult when
persistence fails and restoreResult when persistence succeeds but restoration
fails. Preserve the existing detail selection, warning log, toast, and early
return behavior.
In `@Source/Automation/akron-automation-service.cs`:
- Around line 320-332: Update DeferRunCompletion to call WriteResult(status:
"pending") once when deferral begins, then remove the repeated WriteResult call
from the deferred countdown branch in HandleDeferredRun. Keep the frame
countdown, completion failure handling, and return values unchanged.
In `@Source/Core/akron-event-instance-utils.cs`:
- Around line 183-185: Update the persistent event state handling around
ListenerMask to track a HasListenerMask flag, setting it only when
getListenerMask returns RESULT.OK. In RestorePersistentState, call
setListenerMask only when the flag is true, and update
PersistentEventStatesMatch to compare HasListenerMask alongside ListenerMask.
In `@Source/SaveLoad/akron-native-savestate-support.cs`:
- Around line 223-227: Extract the shared "Akron StartPos " prefix into an
internal constant, preferably alongside the existing slot-name logic in
AkronActions.GetStartPosStateSlotName, and replace every matching StartsWith
literal in the current save-state code and AkronSaveLoad with that constant.
Ensure the builder and all branch checks use the same shared symbol.
In `@Source/SaveLoad/akron-reconstruction-graph.cs`:
- Around line 1132-1155: Replace the per-call LINQ filtering in
FindFreshResource and FindUniqueFreshRoomObject with incrementally maintained
unpaired candidate queues or indexes keyed by resource key and exact room-object
Type. When pairedFreshObjects is assigned during CaptureValue, remove or lazily
skip that candidate in both indexes; ensure each lookup advances past paired
entries without rescanning the full matches list. Preserve FindFreshResource’s
unavailable and ambiguity exceptions, including the unpaired count, and keep
FindUniqueFreshRoomObject returning a result only when exactly one unpaired
candidate remains.
- Around line 2319-2345: The PersistentEventStatesMatch method uses exact
equality for FMOD-derived float fields, causing valid restored states to fail
verification. Update comparisons for Volume, Pitch, all
position/velocity/forward/up components, TimelinePosition, and parameter values
to use a small consistent tolerance, while preserving exact comparisons for
non-float fields and the existing dictionary key/count checks.
- Around line 750-781: Update TryResolveDetourNextMethod to explicitly validate
and document the required MonoMod reflection members—DetourInfo.detour,
NextTrampoline, and TrampolineMethod—using the supported MonoMod version or
startup/build metadata. When any expected member is missing or incompatible,
surface an explicit unsupported-state failure before CreateDelegate or
VerifyDelegate reaches the generic “saved hook position is unavailable” path,
rather than silently returning false.
In `@Source/SaveLoad/AkronSaveLoad.cs`:
- Around line 772-780: Update AkronStartPosReconstruction.DeleteSnapshot to
catch IOException and UnauthorizedAccessException from File.Delete, log the
failure, and return without propagating the exception. Keep ClearRuntimeState’s
subsequent metadata cleanup and RunClearStateActions flow unchanged so callers
such as ClearStartPos and ReplacePersistentStartPositionsForMap can complete
their updates.
- Around line 453-458: Update both gameplay-buffer restore paths in
RestoreNativeSlot and the persistent restore flow around Verify so
AkronGameplayBufferState.Restore failures only record or log the buffer error,
skip ArmLevelPresentation, and continue returning the successful restore result.
Remove the early Failed returns and any associated cleanup/reload behavior,
preserving simulation-state restoration even when presentation pixels mismatch.
- Around line 112-117: Update AkronNativeSavestateSupport.Reset() to clear the
registered actions before the next Initialize() re-registers core runtime
support, preventing AddRegisteredAction from encountering duplicate identities
after module reload. Preserve unrelated runtime reset behavior and avoid
changing AddRegisteredAction unless necessary.
In `@Source/Setups/akron-setup-packs.cs`:
- Around line 533-558: The fail-closed InvalidDataException checks in
WriteArchive must be handled by both export paths: in
Source/Setups/akron-setup-packs.cs lines 533-558, update ExportCurrent to catch
InvalidDataException and report the failing slot through a toast; in
Source/Community/akron-community-pack-uploads.cs lines 192-203, add
InvalidDataException to the catch filter at line 946 so FailUpload executes and
releases the upload slot.
- Around line 1596-1633: Update Install to back up each existing destinationPath
in the staging area before File.Move overwrites it, tracking the original
destination alongside installed paths. In the catch rollback, restore each
backed-up snapshot to its original destination and delete newly created
destinations without backups, preserving the pre-install state for all
previously processed slots.
- Around line 420-440: Reorder the StartPos flow around
MergeScopedStartPositions and prepared.Install: compute importedSlotMap before
installing snapshots, then call prepared.Install(importedSlotMap) once
regardless of AkronModule.Instance. Remove the earlier parameterless install and
the conditional post-merge install logic, while preserving
RefreshStartPositionsAfterSnapshotImport ordering and the existing
activeImported selection.
In `@Source/Tools/akron-capture.cs`:
- Around line 43-77: Update CaptureGameplayBufferQaFrame so every captured,
failed, and gameplay-buffer-unavailable result is also written through the
module logger in addition to AkronAutomationService.RecordOutput. Reuse the
exact result messages for both outputs, including the success hash/path and
failure reasons, so manual akron_qa_pixel_checkpoint runs remain visible without
an active automation run.
In `@tests/module-settings-tests.cs`:
- Around line 4681-4684: The shared default snapshot key collides between
SetupPackArchiveRoundTripsListedSetupSystems and
WholeArchiveRoundTripPreservesPortableMenuBindingsAndCurrentMapStartPositions.
In tests/module-settings-tests.cs lines 4681-4684, change the areaSid to a value
unique to SetupPackArchiveRoundTripsListedSetupSystems; in
tests/setup-pack-tests.cs lines 33-34, use a different unique AreaSid for
WholeArchiveRoundTripPreservesPortableMenuBindingsAndCurrentMapStartPositions
and update its matching assertion at line 55 and cleanup at line 69, without
changing other test behavior.
In `@tests/setup-pack-tests.cs`:
- Around line 714-717: In the test setup around SavePackSnapshot and the
try/finally block, create the temporary directory before writing the snapshot,
then move SavePackSnapshot inside the try so any setup failure is covered by
cleanup. Keep stateSlotName resolvable for the finally cleanup, using the
deterministic slot-name lookup if needed, and preserve
AkronStartPosReconstruction.DeleteSnapshot execution on every failure path.
In `@tests/startpos-reconstruction-tests.cs`:
- Around line 288-312: Replace the raw JSON substring assertion in
NullAndReferenceValuesOmitEmptyScalarMetadata with an assertion over the
deserialized capture document’s field-node structure. Inspect the relevant
document nodes and verify no node contains both empty TypeName and empty Scalar
metadata, using the actual member names of the field-node type; keep the
existing serialization round-trip and restore assertions unchanged.
🪄 Autofix
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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9306712e-b793-43fa-9b55-8ba2bbc0bfd7
📒 Files selected for processing (35)
CHANGELOG.mdSource/Actions/akron-startpos-actions.csSource/Automation/akron-automation-service.csSource/Commands/akron-qa-commands.csSource/Commands/akron-startpos-commands.csSource/Community/akron-community-pack-uploads.csSource/Community/akron-community-packs.csSource/Core/AkronIgnoreSaveStateComponent.csSource/Core/AkronModuleSaveData.csSource/Core/AkronModuleSession.csSource/Core/akron-event-instance-utils.csSource/Module/AkronModule.csSource/Packs/akron-archive.csSource/SaveLoad/AkronSaveLoad.csSource/SaveLoad/akron-native-savestate-support.csSource/SaveLoad/akron-persistent-startpos-snapshots.csSource/SaveLoad/akron-reconstruction-graph.csSource/SaveLoad/akron-save-load-models.csSource/Setups/akron-setup-packs.csSource/Tools/akron-capture.csdocs/concepts/akr-files-and-setup-state.mdxdocs/feature-guide/startpos.mdxdocs/player-guide/community-packs.mdxdocs/reference/akr-archives.mdxdocs/reference/community-pack-catalog.mdxdocs/troubleshooting/startpos-recovery.mdxtests/akron-tests.csprojtests/archive-tests.cstests/community-pack-tests.cstests/module-settings-tests.cstests/packages.lock.jsontests/screenshot-scanner-tests.cstests/setup-pack-tests.cstests/startpos-persistence-tests.cstests/startpos-reconstruction-tests.cs
💤 Files with no reviewable changes (4)
- Source/Commands/akron-startpos-commands.cs
- Source/Core/AkronModuleSession.cs
- Source/SaveLoad/akron-persistent-startpos-snapshots.cs
- Source/Core/AkronModuleSaveData.cs
7230952 to
cbc1423
Compare
cbc1423 to
6fca418
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f1e11eb16
ℹ️ 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".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@Source/Actions/akron-startpos-actions.cs`:
- Around line 159-168: Wrap the second-phase
AkronModule.ScheduleAfterStableEngineUpdate call in the StartPos flow with error
handling that runs when scheduling itself throws. In that failure path, clear
startPosCaptureInProgress and discard the staged runtime state via
AkronSaveLoadService.DiscardRuntimeStateMemory(stateSlotName), while preserving
the inner callback’s existing finally cleanup for successfully registered
callbacks.
In `@Source/Commands/akron-qa-commands.cs`:
- Around line 434-445: Defer the success diagnostics in the StartPos capture
flow until the stable-update work scheduled by AkronActions.SetStartPos has
completed. Move the captured log and calls to LogControlledPlayerProbe,
LogQaStartPosBackdropState, and LogStartPosStatus into the
AkronModule.ScheduleAfterStableEngineUpdate callback, ensuring no captured
message is emitted before the StartPositions[slot] commit finishes; keep the
existing failure path immediate.
In `@Source/Packs/akron-archive.cs`:
- Around line 97-126: Refactor ReadSinglePayloadArchive to avoid opening the
archive and validating entries twice: remove its preliminary ZipArchive block
and redundant attachmentNames check, then add an optional exact-entry
requirement to ReadPayloadArchive and enforce the existing “exactly one manifest
and one payload” error inside its single using ZipArchive path when enabled.
Pass that option from ReadSinglePayloadArchive while preserving the current
validation behavior and error message.
In `@Source/Tools/akron-capture.cs`:
- Around line 19-32: Update RequestGameplayBufferQaCapture to handle an already
populated pendingGameplayBufferQaTag before assigning the new request: report
the displaced tag using the existing reporting mechanism and invoke
pendingGameplayBufferQaCompletion when present. Then replace the pending tag and
completion with the new request while preserving normalization and invalid-tag
behavior.
In `@tests/archive-tests.cs`:
- Around line 47-73: Add a negative test alongside
PayloadArchiveRoundTripsDeclaredBinaryAttachment that writes an archive with one
attachment, then asserts ReadPayloadArchive throws InvalidDataException when
maxAttachmentCount is 0 and when maxTotalAttachmentBytes is below the attachment
size. Verify both read-side limit rejection branches independently.
In `@tests/screenshot-scanner-tests.cs`:
- Around line 312-319: Strengthen
GameplayBufferQaHashCoversEveryRgbaChannelInOrder with a differential assertion
that independently verifies RGBA ordering, such as asserting ComputePixelHash
returns different digests for the original byte sequence and a sequence with
channel order changed. Retain the existing golden digest assertion and use
inputs that preserve the same channel values while altering their order.
- Around line 321-333: Update
PendingGameplayBufferQaCaptureAlwaysRunsItsCompletion to drain
CapturePendingGameplayBufferQaFrame a second time after asserting the first
completion, then assert completions remains 1 to prove the static pending
completion was cleared. Add cleanup that reliably drains or clears the pending
capture even when an assertion fails, preventing leaked AkronCapture state
across tests.
In `@tests/startpos-persistence-tests.cs`:
- Around line 14-33: Define the shared non-parallel xUnit collection from the
review and annotate StartPosPersistenceTests in
tests/startpos-persistence-tests.cs#L14-L33, ModuleSettingsTests in
tests/module-settings-tests.cs#L4180-L4194, SetupPackTests in
tests/setup-pack-tests.cs#L806-L806, and ScreenshotScannerTests in
tests/screenshot-scanner-tests.cs#L321-L333 with it. This serializes all classes
that mutate or observe process-wide runtime state; no additional per-test
cleanup changes are needed.
- Around line 205-212: Guard every IndexOf result before the corresponding
Substring in the test helpers, including the anchor extraction around
persistStart, persistEnd, restoreStart, restoreEnd, and persistentModelStart and
the repeated sites identified in this file. Assert each symbol is found with a
clear missing-symbol/file message before slicing, reusing the existing assertion
pattern from nearby guarded tests.
In `@tests/startpos-reconstruction-tests.cs`:
- Around line 1519-1525: Move the Success assertions for levelCapture and
actionCapture immediately after their graph.Capture calls, before accessing
either Document in the ActionStateDocument assignment. Preserve the existing
Assert.True(...Success, ...Error) diagnostics, and remove the now-redundant
later assertions.
- Around line 855-863: Add an explicit assertion in the test around
emptyMetadataValues to verify the filter finds the expected null/reference
metadata objects before Assert.All runs. Preserve the existing TypeName and
Scalar null-property checks for every matched value.
- Around line 296-349: Update
DeserializeRebuildsNonemptyPathsBeforeCheckingParentCycles to use built-in
captured parent references in a non-cyclic reconstruction document, then assert
the restored paths after deserialization. Do not assign forged Path values,
since AkronReconstructionNode.Path is ignored during JSON serialization;
alternatively remove those assignments if retaining this test solely for
parent-cycle detection.
🪄 Autofix
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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 627cf128-7dac-4ab1-a19c-7ae6959a286f
📒 Files selected for processing (36)
CHANGELOG.mdSource/Actions/akron-startpos-actions.csSource/Automation/akron-automation-service.csSource/Commands/akron-qa-commands.csSource/Commands/akron-startpos-commands.csSource/Community/akron-community-pack-uploads.csSource/Community/akron-community-packs.csSource/Core/AkronIgnoreSaveStateComponent.csSource/Core/AkronModuleSaveData.csSource/Core/AkronModuleSession.csSource/Core/akron-event-instance-utils.csSource/Module/AkronModule.csSource/Packs/akron-archive.csSource/SaveLoad/AkronSaveLoad.csSource/SaveLoad/akron-native-savestate-support.csSource/SaveLoad/akron-persistent-startpos-snapshots.csSource/SaveLoad/akron-reconstruction-graph.csSource/SaveLoad/akron-save-load-exports.csSource/SaveLoad/akron-save-load-models.csSource/Setups/akron-setup-packs.csSource/Tools/akron-capture.csdocs/concepts/akr-files-and-setup-state.mdxdocs/feature-guide/startpos.mdxdocs/player-guide/community-packs.mdxdocs/reference/akr-archives.mdxdocs/reference/community-pack-catalog.mdxdocs/troubleshooting/startpos-recovery.mdxtests/akron-tests.csprojtests/archive-tests.cstests/community-pack-tests.cstests/module-settings-tests.cstests/packages.lock.jsontests/screenshot-scanner-tests.cstests/setup-pack-tests.cstests/startpos-persistence-tests.cstests/startpos-reconstruction-tests.cs
💤 Files with no reviewable changes (4)
- Source/Commands/akron-startpos-commands.cs
- Source/Core/AkronModuleSession.cs
- Source/SaveLoad/akron-persistent-startpos-snapshots.cs
- Source/Core/AkronModuleSaveData.cs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 025f424304
ℹ️ 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".
|
@greptileai review |
1 similar comment
|
@greptileai review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 025f424304
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e02a5788c2
ℹ️ 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".
|
@greptileai review |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff82f6148d
ℹ️ 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/SaveLoad/akron-save-load-exports.cs (1)
11-39: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument or recover at the save/load interop boundary.
RegisterSaveLoadActioncan throwInvalidOperationExceptionwhen callback owners resolve to the same identity, whileRegisterNamedSaveLoadActionthrowsArgumentExceptionfor a blank or whitespace name. Both failures pass throughMegahack.SaveLoadexports; at least document these failure modes so caller mods can handle repeat registrations or invalid names.🤖 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 `@Source/SaveLoad/akron-save-load-exports.cs` around lines 11 - 39, Document the exception behavior at the save/load interop boundary for RegisterSaveLoadAction and RegisterNamedSaveLoadAction: identify InvalidOperationException for duplicate callback-owner identities and ArgumentException for blank or whitespace registrationName values. Ensure caller-facing documentation explains these repeat-registration and invalid-name cases without changing the existing registration behavior.
🤖 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 `@Source/Actions/akron-startpos-actions.cs`:
- Around line 69-74: Reset start-position capture state during module unload so
pending callbacks cannot leave it permanently active. Add an internal reset
method for startPosCaptureInProgress near the existing capture logic, then
invoke it from AkronModule.Unload alongside clearing queued after-engine-update
actions; preserve normal callback-based state transitions during runtime.
- Around line 208-225: Update finishCapture to catch unexpected exceptions
around its full execution, invoke completion with failure when it has not
already been reported, then rethrow the exception. Add and maintain a
reportedCompletion guard by setting it immediately beside each existing
completion invocation, including the new failure path, so callers receive
exactly one result while normal cleanup and propagation remain unchanged.
In `@Source/Commands/akron-qa-commands.cs`:
- Around line 575-593: Update the backdrop state construction in the renderer
fade diagnostic loop to avoid appending backdrop.Position via its
culture-sensitive ToString(). Format the position components explicitly with
CultureInfo.InvariantCulture, following the existing approach used by
FormatQaExactMember, while preserving the current field structure and
separators.
- Around line 506-516: Guard the pixel-capture arming logic in recordProbe so it
runs only when Engine.Scene is a Level, matching the consumption condition in
AkronModule.EngineOnRenderCore. Preserve the existing tag validation and
rejection behavior, and do not arm or retain a pending capture when the current
scene is not a Level.
- Around line 456-475: Move the successful reference-frame capture out of the
initial command path and into the success callback passed to
AkronActions.SetStartPos, after the StartPos commit has completed. Arm the
capture using the same deferred/render-boundary mechanism as
QaStartPosLoadProbe, and only log or continue the reference-capture flow once
that capture completes, so both hashes represent post-restore frames.
In `@Source/Community/akron-community-packs.cs`:
- Line 82: Update the pack-download flow using MaxPackBytes so the 8-second
client timeout does not cancel large response-body reads after headers arrive.
Either use a dedicated HttpClient with a sufficiently larger timeout for pack
downloads, or preserve the catalog client and apply a bounded cancellation token
consistently to ReadAsStreamAsync and subsequent ReadAsync operations before
WriteVerifiedPack handles the stream.
In `@Source/Module/AkronModule.cs`:
- Around line 406-413: Update EngineOnRenderCore so
renderedStartPosFrameGeneration is recorded on every render frame, regardless of
whether the current scene is a Level; keep any Level-specific rendering logic
inside its existing scene check. Preserve LevelOnUpdate’s generation comparison
while ensuring scene changes cannot leave it permanently skipping updates.
In `@Source/Packs/akron-archive.cs`:
- Around line 186-195: Replace the attachment size calculation in the archive
validation block with a single loop over the selected entries, reusing each
retrieved entry instead of calling archive.GetEntry repeatedly. Accumulate sizes
with an explicit overflow-safe check, and throw InvalidDataException("Archive
attachments exceed their size limit.") when any entry size would exceed
maxTotalAttachmentBytes or the accumulated total reaches it; preserve the
existing count-limit behavior.
In `@tests/screenshot-scanner-tests.cs`:
- Around line 337-339: Both cleanup finally blocks must suppress exceptions from
CapturePendingGameplayBufferQaFrame so cleanup cannot mask test results. In
tests/screenshot-scanner-tests.cs lines 337-339 within
PendingGameplayBufferQaCaptureAlwaysRunsItsCompletion and lines 361-363 within
ReplacingPendingGameplayBufferQaCaptureCompletesTheDisplacedRequest, wrap each
drain call with Record.Exception and discard the returned exception.
---
Outside diff comments:
In `@Source/SaveLoad/akron-save-load-exports.cs`:
- Around line 11-39: Document the exception behavior at the save/load interop
boundary for RegisterSaveLoadAction and RegisterNamedSaveLoadAction: identify
InvalidOperationException for duplicate callback-owner identities and
ArgumentException for blank or whitespace registrationName values. Ensure
caller-facing documentation explains these repeat-registration and invalid-name
cases without changing the existing registration behavior.
🪄 Autofix
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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 18b09969-19ca-4e61-9b3a-59e4bb5a33c2
📒 Files selected for processing (37)
CHANGELOG.mdSource/Actions/akron-startpos-actions.csSource/Automation/akron-automation-service.csSource/Commands/akron-qa-commands.csSource/Commands/akron-startpos-commands.csSource/Community/akron-community-pack-uploads.csSource/Community/akron-community-packs.csSource/Core/AkronIgnoreSaveStateComponent.csSource/Core/AkronModuleSaveData.csSource/Core/AkronModuleSession.csSource/Core/AkronToast.csSource/Core/akron-event-instance-utils.csSource/Module/AkronModule.csSource/Packs/akron-archive.csSource/SaveLoad/AkronSaveLoad.csSource/SaveLoad/akron-native-savestate-support.csSource/SaveLoad/akron-persistent-startpos-snapshots.csSource/SaveLoad/akron-reconstruction-graph.csSource/SaveLoad/akron-save-load-exports.csSource/SaveLoad/akron-save-load-models.csSource/Setups/akron-setup-packs.csSource/Tools/akron-capture.csdocs/concepts/akr-files-and-setup-state.mdxdocs/feature-guide/startpos.mdxdocs/player-guide/community-packs.mdxdocs/reference/akr-archives.mdxdocs/reference/community-pack-catalog.mdxdocs/troubleshooting/startpos-recovery.mdxtests/akron-tests.csprojtests/archive-tests.cstests/community-pack-tests.cstests/module-settings-tests.cstests/packages.lock.jsontests/screenshot-scanner-tests.cstests/setup-pack-tests.cstests/startpos-persistence-tests.cstests/startpos-reconstruction-tests.cs
💤 Files with no reviewable changes (4)
- Source/Commands/akron-startpos-commands.cs
- Source/Core/AkronModuleSaveData.cs
- Source/Core/AkronModuleSession.cs
- Source/SaveLoad/akron-persistent-startpos-snapshots.cs
| if (startPosCaptureInProgress) { | ||
| Engine.Scene?.Add(new AkronToast("StartPos capture is still finishing.")); | ||
| completion?.Invoke(false); | ||
| return; | ||
| } | ||
| startPosCaptureInProgress = true; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
startPosCaptureInProgress is never reset on module unload.
The flag is cleared only by the deferred callbacks scheduled in SchedulePersistentStartPosCapture. AkronModule.Unload clears afterEngineUpdateActions (Source/Module/AkronModule.cs line 339) without invoking the queued callbacks. ScheduleAfterStableEngineUpdate can also keep a capture callback queued across several updates while a random scope is active. If an unload happens while a capture is pending, the flag stays true for the process lifetime. After a hot reload, SetStartPos, LoadStartPos, ClearStartPos, and RestoreStartPosAfterDeath all reject every call, and the only feedback is the "StartPos capture is still finishing." toast.
Add an internal reset and call it from AkronModule.Unload.
🛡️ Proposed reset hook
private static bool startPosCaptureInProgress;
+
+ internal static void ResetStartPosCaptureState() {
+ startPosCaptureInProgress = false;
+ }In Source/Module/AkronModule.cs Unload:
afterEngineUpdateActions.Clear();
+ AkronActions.ResetStartPosCaptureState();🤖 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 `@Source/Actions/akron-startpos-actions.cs` around lines 69 - 74, Reset
start-position capture state during module unload so pending callbacks cannot
leave it permanently active. Add an internal reset method for
startPosCaptureInProgress near the existing capture logic, then invoke it from
AkronModule.Unload alongside clearing queued after-engine-update actions;
preserve normal callback-based state transitions during runtime.
| try { | ||
| try { | ||
| Directory.CreateDirectory(stagingDirectory); | ||
| persistResult = AkronSaveLoadService.PersistRuntimeStateSnapshot( | ||
| level, | ||
| stateSlotName, | ||
| stagingDirectory); | ||
| persistError = AkronSaveLoadService.LastPersistentSnapshotError; | ||
| } catch (Exception exception) when (exception is IOException || exception is UnauthorizedAccessException) { | ||
| persistError = exception.GetType().Name + ": " + exception.Message; | ||
| } | ||
| } finally { | ||
| restoreResult = AkronSaveLoadService.LoadRuntimeState(level, stateSlotName, allowDeadPlayer: true); | ||
| if (playerSnapshot != null && level.Tracker.GetEntity<Player>() is Player restoredPlayer) { | ||
| playerSnapshot.Restore(restoredPlayer); | ||
| } | ||
| level.Session.RespawnPoint = originalRespawnPoint; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
An unexpected persist exception skips completion and leaves callers waiting.
The inner catch at line 216 covers only IOException and UnauthorizedAccessException. PersistRuntimeStateSnapshot reaches the reconstruction graph, deep clone, and FMOD code, which can throw InvalidOperationException, TargetInvocationException, or MissingMemberException. Such an exception escapes finishCapture, so completion?.Invoke(false) at lines 250 and 259 is never reached. RunAfterEngineUpdateActions logs it and continues, and the caller that passed a completion callback never receives a result. AkronActions.SetStartPos(level, completion) is used by automation, so an automated run stalls with no failure signal.
Wrap the body of finishCapture so any exception reports failure through completion before it propagates.
🛡️ Proposed fix
Action finishCapture = () => {
+ bool reportedCompletion = false;
try {
if (Engine.Scene != level) {
AkronSaveLoadService.DiscardRuntimeStateMemory(stateSlotName);
+ reportedCompletion = true;
completion?.Invoke(false);
return;
}
@@
} finally {
startPosCaptureInProgress = false;
+ if (!reportedCompletion) {
+ completion?.Invoke(false);
+ }
}
};Set reportedCompletion = true next to each existing completion?.Invoke(...) call in this method.
🤖 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 `@Source/Actions/akron-startpos-actions.cs` around lines 208 - 225, Update
finishCapture to catch unexpected exceptions around its full execution, invoke
completion with failure when it has not already been reported, then rethrow the
exception. Add and maintain a reportedCompletion guard by setting it immediately
beside each existing completion invocation, including the new failure path, so
callers receive exactly one result while normal cleanup and propagation remain
unchanged.
| if (!AkronCapture.CaptureGameplayBufferQaFrameNow(tag, out string normalizedTag)) { | ||
| Log("qa-startpos-reference-capture: failed;tag=" + normalizedTag); | ||
| return; | ||
| } | ||
| AkronAutomationService.DeferRunCompletion(); | ||
| try { | ||
| AkronActions.SetStartPos(level, captured => { | ||
| try { | ||
| Log("qa-startpos-reference-capture: " + (captured ? "captured" : "failed") + | ||
| ";slot=" + slot.ToString(CultureInfo.InvariantCulture) + ";tag=" + normalizedTag); | ||
| if (!captured) { | ||
| return; | ||
| } | ||
| LogControlledPlayerProbe(level, "qa-startpos-reference-capture"); | ||
| LogQaStartPosBackdropState(level, slot, "qa-startpos-reference-capture"); | ||
| LogStartPosStatus(level); | ||
| } finally { | ||
| AkronAutomationService.CompleteDeferredRun(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The reference hash is taken from a different frame than the StartPos it claims to describe.
CaptureGameplayBufferQaFrameNow reads GameplayBuffers.Level.Target synchronously during the command, which runs inside Engine.Update. At that point the target still holds the previously completed frame. AkronActions.SetStartPos commits its state later, across stable engine updates, and the callback at line 462 runs after that commit.
The load-probe path uses the opposite timing. QaStartPosLoadProbe arms RequestGameplayBufferQaCapture, which AkronModule.EngineOnRenderCore runs at the render boundary after orig(self). Comparing the two hashes therefore compares a pre-capture frame against a post-restore frame, so a matching restore can still report different hashes.
Arm the capture from inside the success callback so the reference frame is produced at the same render boundary as the probe frame.
🛠️ Proposed timing fix
AkronActions.SetStartPosSlot(slot);
- if (!AkronCapture.CaptureGameplayBufferQaFrameNow(tag, out string normalizedTag)) {
- Log("qa-startpos-reference-capture: failed;tag=" + normalizedTag);
- return;
- }
AkronAutomationService.DeferRunCompletion();
try {
AkronActions.SetStartPos(level, captured => {
+ bool waitForPixelCapture = false;
try {
Log("qa-startpos-reference-capture: " + (captured ? "captured" : "failed") +
- ";slot=" + slot.ToString(CultureInfo.InvariantCulture) + ";tag=" + normalizedTag);
+ ";slot=" + slot.ToString(CultureInfo.InvariantCulture) + ";tag=" + tag);
if (!captured) {
return;
}
LogControlledPlayerProbe(level, "qa-startpos-reference-capture");
LogQaStartPosBackdropState(level, slot, "qa-startpos-reference-capture");
LogStartPosStatus(level);
+ waitForPixelCapture = AkronCapture.RequestGameplayBufferQaCapture(
+ tag,
+ out string _,
+ AkronAutomationService.CompleteDeferredRun);
} finally {
- AkronAutomationService.CompleteDeferredRun();
+ if (!waitForPixelCapture) {
+ AkronAutomationService.CompleteDeferredRun();
+ }
}
});🤖 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 `@Source/Commands/akron-qa-commands.cs` around lines 456 - 475, Move the
successful reference-frame capture out of the initial command path and into the
success callback passed to AkronActions.SetStartPos, after the StartPos commit
has completed. Arm the capture using the same deferred/render-boundary mechanism
as QaStartPosLoadProbe, and only log or continue the reference-capture flow once
that capture completes, so both hashes represent post-restore frames.
| if (!string.IsNullOrWhiteSpace(pixelTag)) { | ||
| if (AkronCapture.RequestGameplayBufferQaCapture( | ||
| pixelTag, | ||
| out string normalizedTag, | ||
| AkronAutomationService.CompleteDeferredRun)) { | ||
| waitForPixelCapture = true; | ||
| AkronAutomationService.RecordOutput("qa-startpos-load-probe-pixel: armed;tag=" + normalizedTag); | ||
| } else { | ||
| AkronAutomationService.RecordOutput("qa-startpos-load-probe-pixel: rejected"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Arm the pixel capture only when the scene is a Level.
AkronModule.EngineOnRenderCore calls AkronCapture.CapturePendingGameplayBufferQaFrame() inside the scene is Level branch. recordProbe falls back to the captured level when Engine.Scene is not a Level, so it can arm a request that the render boundary never consumes. CompleteDeferredRun then never runs, the run waits the full 600-frame deadline, and the stale pending tag displaces the next capture request.
🛠️ Proposed guard
- if (!string.IsNullOrWhiteSpace(pixelTag)) {
+ if (!string.IsNullOrWhiteSpace(pixelTag) && Engine.Scene is Level) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!string.IsNullOrWhiteSpace(pixelTag)) { | |
| if (AkronCapture.RequestGameplayBufferQaCapture( | |
| pixelTag, | |
| out string normalizedTag, | |
| AkronAutomationService.CompleteDeferredRun)) { | |
| waitForPixelCapture = true; | |
| AkronAutomationService.RecordOutput("qa-startpos-load-probe-pixel: armed;tag=" + normalizedTag); | |
| } else { | |
| AkronAutomationService.RecordOutput("qa-startpos-load-probe-pixel: rejected"); | |
| } | |
| } | |
| if (!string.IsNullOrWhiteSpace(pixelTag) && Engine.Scene is Level) { | |
| if (AkronCapture.RequestGameplayBufferQaCapture( | |
| pixelTag, | |
| out string normalizedTag, | |
| AkronAutomationService.CompleteDeferredRun)) { | |
| waitForPixelCapture = true; | |
| AkronAutomationService.RecordOutput("qa-startpos-load-probe-pixel: armed;tag=" + normalizedTag); | |
| } else { | |
| AkronAutomationService.RecordOutput("qa-startpos-load-probe-pixel: rejected"); | |
| } | |
| } |
🤖 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 `@Source/Commands/akron-qa-commands.cs` around lines 506 - 516, Guard the
pixel-capture arming logic in recordProbe so it runs only when Engine.Scene is a
Level, matching the consumption condition in AkronModule.EngineOnRenderCore.
Preserve the existing tag validation and rejection behavior, and do not arm or
retain a pending capture when the current scene is not a Level.
| foreach (Backdrop backdrop in renderer.Backdrops.Where(candidate => candidate != null)) { | ||
| StringBuilder state = new StringBuilder(); | ||
| state.Append(backdrop.GetType().Name) | ||
| .Append('#').Append(ObjectIdentity(backdrop)) | ||
| .Append(",position=").Append(backdrop.Position) | ||
| .Append(",visible=").Append(backdrop.Visible) | ||
| .Append(",fade=").Append(backdrop.FadeAlphaMultiplier.ToString("R", CultureInfo.InvariantCulture)); | ||
| if (backdrop is Snow) { | ||
| state.Append(",visibleFade=").Append(FormatQaExactMember(backdrop, "visibleFade")) | ||
| .Append(",linearFade=").Append(FormatQaExactMember(backdrop, "linearFade")) | ||
| .Append(",particles=").Append(DescribeQaSnowParticles(backdrop)); | ||
| } else if (backdrop is Parallax) { | ||
| state.Append(",fadeIn=").Append(FormatQaExactMember(backdrop, "fadeIn")); | ||
| } | ||
| states.Add(state.ToString()); | ||
| } | ||
| return "rendererFade=" + renderer.Fade.ToString("R", CultureInfo.InvariantCulture) + | ||
| ",items=" + (states.Count == 0 ? "none" : string.Join("|", states)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
backdrop.Position is formatted with the current culture and breaks the field separator.
Line 579 appends Vector2.ToString(), which formats its floats with the current culture. Every other value in this diagnostic uses CultureInfo.InvariantCulture. On a locale that uses a comma as the decimal separator, the emitted position contains , characters, which are the same separator used between fields on line 579. The line becomes unparseable and the saved/current comparison differs across machines.
Format the components explicitly, as FormatQaExactMember already does.
🛠️ Proposed fix
- .Append(",position=").Append(backdrop.Position)
+ .Append(",position=").Append(backdrop.Position.X.ToString("R", CultureInfo.InvariantCulture))
+ .Append('/').Append(backdrop.Position.Y.ToString("R", CultureInfo.InvariantCulture))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| foreach (Backdrop backdrop in renderer.Backdrops.Where(candidate => candidate != null)) { | |
| StringBuilder state = new StringBuilder(); | |
| state.Append(backdrop.GetType().Name) | |
| .Append('#').Append(ObjectIdentity(backdrop)) | |
| .Append(",position=").Append(backdrop.Position) | |
| .Append(",visible=").Append(backdrop.Visible) | |
| .Append(",fade=").Append(backdrop.FadeAlphaMultiplier.ToString("R", CultureInfo.InvariantCulture)); | |
| if (backdrop is Snow) { | |
| state.Append(",visibleFade=").Append(FormatQaExactMember(backdrop, "visibleFade")) | |
| .Append(",linearFade=").Append(FormatQaExactMember(backdrop, "linearFade")) | |
| .Append(",particles=").Append(DescribeQaSnowParticles(backdrop)); | |
| } else if (backdrop is Parallax) { | |
| state.Append(",fadeIn=").Append(FormatQaExactMember(backdrop, "fadeIn")); | |
| } | |
| states.Add(state.ToString()); | |
| } | |
| return "rendererFade=" + renderer.Fade.ToString("R", CultureInfo.InvariantCulture) + | |
| ",items=" + (states.Count == 0 ? "none" : string.Join("|", states)); | |
| } | |
| foreach (Backdrop backdrop in renderer.Backdrops.Where(candidate => candidate != null)) { | |
| StringBuilder state = new StringBuilder(); | |
| state.Append(backdrop.GetType().Name) | |
| .Append('#').Append(ObjectIdentity(backdrop)) | |
| .Append(",position=").Append(backdrop.Position.X.ToString("R", CultureInfo.InvariantCulture)) | |
| .Append('/').Append(backdrop.Position.Y.ToString("R", CultureInfo.InvariantCulture)) | |
| .Append(",visible=").Append(backdrop.Visible) | |
| .Append(",fade=").Append(backdrop.FadeAlphaMultiplier.ToString("R", CultureInfo.InvariantCulture)); | |
| if (backdrop is Snow) { | |
| state.Append(",visibleFade=").Append(FormatQaExactMember(backdrop, "visibleFade")) | |
| .Append(",linearFade=").Append(FormatQaExactMember(backdrop, "linearFade")) | |
| .Append(",particles=").Append(DescribeQaSnowParticles(backdrop)); | |
| } else if (backdrop is Parallax) { | |
| state.Append(",fadeIn=").Append(FormatQaExactMember(backdrop, "fadeIn")); | |
| } | |
| states.Add(state.ToString()); | |
| } | |
| return "rendererFade=" + renderer.Fade.ToString("R", CultureInfo.InvariantCulture) + | |
| ",items=" + (states.Count == 0 ? "none" : string.Join("|", states)); |
🤖 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 `@Source/Commands/akron-qa-commands.cs` around lines 575 - 593, Update the
backdrop state construction in the renderer fade diagnostic loop to avoid
appending backdrop.Position via its culture-sensitive ToString(). Format the
position components explicitly with CultureInfo.InvariantCulture, following the
existing approach used by FormatQaExactMember, while preserving the current
field structure and separators.
|
|
||
| private const int MaxIndexBytes = 1024 * 1024; | ||
| private const int MaxPackBytes = 4 * 1024 * 1024; | ||
| private const int MaxPackBytes = 512 * 1024 * 1024; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does HttpClient.Timeout apply while reading the response stream with HttpCompletionOption.ResponseHeadersRead in .NET 8?
💡 Result:
No, HttpClient.Timeout does not apply while reading the response stream when using HttpCompletionOption.ResponseHeadersRead in.NET 8 [1][2]. When you use HttpCompletionOption.ResponseHeadersRead, the HttpClient operation is considered complete as soon as the response headers are received [1][3]. Consequently, the HttpClient.Timeout timer stops at that point [4]. It does not cover the duration spent reading the actual content from the response stream [1][2][5]. If you need to enforce a timeout during the content-reading phase, you must implement it separately, such as by passing a CancellationToken (created from a CancellationTokenSource with a specific timeout) to the methods used to read the content, like ReadAsStreamAsync or CopyToAsync [6][2][5].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpcompletionoption?view=net-10.0
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpcompletionoption?view=netframework-4.8.1
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.sendasync?view=net-9.0
- 4: HttpClient Timeout Clarification dotnet/runtime#83530
- 5: https://stackoverflow.com/questions/68878399/timeout-when-using-httpcompletionoption-responseheadersread
- 6: https://stackoverflow.com/questions/62994860/httpclient-timeout-using-httpcompletionoption-responseheadersread
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant source without executing repository code.
if [ -f "Source/Community/akron-community-packs.cs" ]; then
echo "== file exists =="
wc -l "Source/Community/akron-community-packs.cs"
echo
echo "== outline =="
ast-grep outline "Source/Community/akron-community-packs.cs" --view compact || true
echo
echo "== relevant sections =="
sed -n '1,130p' "Source/Community/akron-community-packs.cs"
echo
echo "== relevant download/write sections =="
rg -n "GetAsync|ReadAsStreamAsync|CopyToAsync|WriteVerifiedPack|TaskCanceledException|MaxPackBytes|CreateSafeHttpClient|HttpClient" "Source/Community/akron-community-packs.cs"
echo
sed -n '400,470p' "Source/Community/akron-community-packs.cs"
else
echo "missing Source/Community/akron-community-packs.cs"
fd -a "akron-community-packs.cs" .
fi
echo
echo "== changed diff summary/stat =="
git diff --stat HEAD^..HEAD 2>/dev/null || true
echo
git diff --unified=3 HEAD^..HEAD -- "Source/Community/akron-community-packs.cs" 2>/dev/null | sed -n '1,220p' || trueRepository: Microck/akron
Length of output: 18505
🌐 Web query:
Site:learn.microsoft.com .NET System.Net.Http.HttpCompletionOption ResponseHeadersRead HttpClient Timeout response headers read
💡 Result:
When using HttpClient with HttpCompletionOption.ResponseHeadersRead, the HttpClient.Timeout property applies only until the response headers are received [1][2]. Once the headers are read and the request method (e.g., GetAsync, SendAsync) returns, the HttpClient.Timeout no longer applies to subsequent operations, such as reading the response content from the stream [1][2]. If you need to enforce a timeout for the entire duration of the request—including reading the content—you must manage that timeout separately, typically by using a CancellationTokenSource when reading the content [2]. In summary: 1. HttpClient.Timeout scope: With ResponseHeadersRead, the timeout covers only the time required to receive the response headers from the server [1][2]. 2. Content reading: If the server sends headers quickly but streams the content slowly, the HttpClient.Timeout will not trigger. You must implement your own timeout mechanism (e.g., passing a CancellationToken to ReadAsStreamAsync or ReadAsync) to ensure the content reading process does not exceed your desired limits [2]. 3. Exceptions: In the event of a timeout, HttpClient typically throws an OperationCanceledException (on.NET Core and.NET 5+) or an HttpRequestException (on.NET Framework) [3][4]. On.NET 5 and later, the OperationCanceledException will contain an inner TimeoutException [3][4].
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpcompletionoption?view=net-10.0
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpcompletionoption?view=netframework-4.8.1
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.sendasync?view=net-9.0
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.net.http.httpclient.sendasync?view=net-10.0
Content-read timeout does not apply to pack downloads.
ResponseHeadersRead only bounds header delivery. Streams the 8 s client timeout to large packs after headers arrive:
ResponseHeadersRead.GetAsync(...)returns beforeReadAsStreamAsync(...)starts.- At
MaxPackBytes, 512 MiB content can exceed the 8 s client timeout during the copy loop. WriteVerifiedPackdeletes the temporary file on failure, which also happens when the stream read cancels.
Use a separate HttpClient with a larger timeout for pack downloads, or keep the catalog timeout and pass a bounded CancellationToken to ReadAsStreamAsync/ReadAsync.
🤖 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 `@Source/Community/akron-community-packs.cs` at line 82, Update the
pack-download flow using MaxPackBytes so the 8-second client timeout does not
cancel large response-body reads after headers arrive. Either use a dedicated
HttpClient with a sufficiently larger timeout for pack downloads, or preserve
the catalog client and apply a bounded cancellation token consistently to
ReadAsStreamAsync and subsequent ReadAsync operations before WriteVerifiedPack
handles the stream.
| } finally { | ||
| AkronCapture.CapturePendingGameplayBufferQaFrame(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Both cleanup drains call CapturePendingGameplayBufferQaFrame without Record.Exception. The in-test drains at lines 332-333 and 359 already treat this call as throwing in a test process without a graphics device. The two finally blocks do not, so a cleanup throw replaces or hides the real assertion failure and can fail an otherwise passing test.
tests/screenshot-scanner-tests.cs#L337-L339: wrap the drain inPendingGameplayBufferQaCaptureAlwaysRunsItsCompletionas_ = Record.Exception(AkronCapture.CapturePendingGameplayBufferQaFrame);.tests/screenshot-scanner-tests.cs#L361-L363: apply the same wrapping inReplacingPendingGameplayBufferQaCaptureCompletesTheDisplacedRequest.
📍 Affects 1 file
tests/screenshot-scanner-tests.cs#L337-L339(this comment)tests/screenshot-scanner-tests.cs#L361-L363
🤖 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/screenshot-scanner-tests.cs` around lines 337 - 339, Both cleanup
finally blocks must suppress exceptions from CapturePendingGameplayBufferQaFrame
so cleanup cannot mask test results. In tests/screenshot-scanner-tests.cs lines
337-339 within PendingGameplayBufferQaCaptureAlwaysRunsItsCompletion and lines
361-363 within
ReplacingPendingGameplayBufferQaCaptureCompletesTheDisplacedRequest, wrap each
drain call with Record.Exception and discard the returned exception.
|
@greptileai review |
|
@codex review |
|
@coderabbitai review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acfc63d302
ℹ️ 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".
| replacing player progress. Akron keeps the greater current or captured value | ||
| for session time, session deaths, room deaths, save-file time and deaths, area | ||
| time and deaths. These statistics do not rewind. Room simulation clocks restore |
There was a problem hiding this comment.
When an imported snapshot contains larger author counters, this required-state contract says Akron keeps the greater current or captured values, but AkronCumulativeStats.RestoreWithoutRewinding now assigns the recipient's pre-load values exactly. The documentation therefore still promises the counter inflation behavior that the implementation was changed to prevent; update this paragraph to state that recipient time and death counters are preserved exactly.
Useful? React with 👍 / 👎.
1 participant
what changed
.akrfile instead of exporting coordinates alone.akron-setup-v3. old partial room snapshots are not kept as a fallback.why
the previous persistent format did not contain enough state to recreate the exact Set frame after closing Celeste. coordinates and selected fields also cannot preserve a custom map's object graph.
StartPos packs had the same problem. carrying the position without the exact snapshot meant the imported StartPos was not the same StartPos. v3 puts the snapshot in the pack.
verification
follow-up
large custom room cold reconstruction remains expensive and is tracked in #120.
Greptile Summary
The PR replaces coordinate-based StartPos persistence with validated v6 reconstruction snapshots that support exact restoration after restarting Celeste.
akron-setup-v3setup and community pack format.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant Actions as AkronActions participant Engine as Stable engine boundary participant SaveLoad as Save/load service participant Disk as Snapshot storage User->>Actions: Set StartPos Actions->>SaveLoad: Capture runtime state Actions->>Engine: Schedule persistent capture Engine->>Actions: Reload fresh room Actions->>Engine: Wait one stable update Engine->>SaveLoad: Build and validate reconstruction graph SaveLoad->>Disk: Stage v6 snapshot SaveLoad->>Actions: Restore exact Set frame Actions->>Disk: Commit snapshot and metadata User->>Actions: Load StartPos after restart Actions->>SaveLoad: Load persistent snapshot SaveLoad->>Engine: Reconstruct fresh room graph SaveLoad-->>Actions: Exact frame restoredReviews (4): Last reviewed commit: "fix: preserve exact StartPos state acros..." | Re-trigger Greptile