perf(sessions): find matching checkpoints without sorting#96964
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 26, 2026, 6:49 PM ET / 22:49 UTC. Summary PR surface: Source +7, Tests +86. Total +93 across 2 files. Reproducibility: yes. Source inspection shows sessions.compaction.branch and sessions.compaction.restore call the shared getSessionCompactionCheckpoint helper, which still sorts the full checkpoint list before the optimized accessor mutation runs. Review metrics: none identified. Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Apply the single-pass newest-match lookup to the shared gateway checkpoint getter used before branch/restore, keep ordered list behavior where callers need lists, and add real after-fix proof before merge. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows sessions.compaction.branch and sessions.compaction.restore call the shared getSessionCompactionCheckpoint helper, which still sorts the full checkpoint list before the optimized accessor mutation runs. Is this the best way to solve the issue? No. The accessor scan is a good local step, but the best fix also updates the shared gateway lookup so the normal user-facing branch/restore path avoids the O(n log n) sort. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ab966c214be0. Label changesLabel justifications:
Evidence reviewedPR surface: Source +7, Tests +86. Total +93 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
57aa5e0 to
fc1315f
Compare
|
Maintainer review/proof for synced head
Proceeding to merge through |
|
Merged via squash.
|
…6964) Co-authored-by: ly-wang19 <[email protected]>
…6964) Co-authored-by: ly-wang19 <[email protected]>
…6964) Co-authored-by: ly-wang19 <[email protected]>
…6964) Co-authored-by: ly-wang19 <[email protected]>
What Problem This Solves
Branching or restoring from a compaction checkpoint only needs the newest checkpoint with the requested id, but the accessor copied and sorted the entire checkpoint array before finding that id. Sessions with many retained compaction checkpoints paid avoidable allocation and O(n log n) work.
Why This Change Was Made
This replaces the full copy/sort/find path with a single scan that tracks the newest checkpoint matching the trimmed requested id. It keeps the previous newest-match behavior while ignoring newer checkpoints with other ids.
User Impact
Checkpoint branch/restore operations do less work for sessions with long compaction histories. The selected checkpoint, persistence behavior, and missing-checkpoint handling remain unchanged.
Evidence
node scripts/run-vitest.mjs src/config/sessions/session-accessor.test.ts.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main --parallel-tests "node scripts/run-vitest.mjs src/config/sessions/session-accessor.test.ts"git diff --checkResubmitted from #96956 after freeing an active PR slot; #96956 was auto-closed only because the author had more than 20 active PRs.