Repository navigation
SDK crash-recoverable workspace transactions and atomic VCS merges - #605
Conversation
Introduce a public durable transaction coordinator for ordered SDK mutations, expose it through extension command contexts, and migrate pm-vcs merges to atomic item-plus-relationship commits with compensation and replay. Document the public recovery contract, close the VCS transaction waiver, add exhaustive failure and concurrency coverage, and record GitHub intake plus tracker evidence for pm-4e12, pm-ju83, pm-8ngt, and pm-ds3b.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
@greptileai please review the complete current head, including the public SDK transaction/recovery contract, VCS compensation/replay integration, and tests. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Sourcery review 4728832958 acknowledged: the bot reported its weekly diff-character quota rather than a code finding. No action is possible in this PR; exact-head Greptile and the full repository quality/release gates provide the independent review and verification surface. |
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (25)
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:
📝 WalkthroughWalkthroughAdds a public durable workspace transaction coordinator with crash recovery, replay, and compensation. Exposes it through the SDK and extension context, integrates it into VCS merge reconciliation, adds comprehensive test coverage and documentation, and updates related project records with implementation evidence and verification metadata. ChangesWorkspace transaction boundary
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant ExtensionCommand
participant WorkspaceTransaction
participant ChangesetStore
participant RelationshipLedger
ExtensionCommand->>WorkspaceTransaction: commit transaction with ordered steps
WorkspaceTransaction->>ChangesetStore: inspect changeset and vcs_ref
WorkspaceTransaction->>RelationshipLedger: inspect latest merge event
WorkspaceTransaction->>ChangesetStore: apply changeset update to merged
WorkspaceTransaction->>RelationshipLedger: append merge relationship edge
WorkspaceTransaction->>ChangesetStore: compensate restore changeset if needed
WorkspaceTransaction->>RelationshipLedger: append relationship rollback
WorkspaceTransaction-->>ExtensionCommand: committed result with recovered flag
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Merging this PR will regress 4 benchmarks
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | encodeQueryCursor |
101 µs | 101.1 µs | -0.09% |
| ❌ | stableStringify (nested contract) |
161.2 µs | 161.3 µs | -0.02% |
| ❌ | decodeQueryCursor |
115.9 µs | 115.9 µs | -0.02% |
| ❌ | normalizeLowercaseWhitespace |
135.8 µs | 135.8 µs | 0% |
| ⚡ | build index (2000 docs x 80 words) |
161.4 ms | 161.3 ms | +0.06% |
| ⚡ | build index (200 docs x 40 words) |
8.4 ms | 8.4 ms | +0.04% |
| ⚡ | jaccardSimilarity |
153.3 µs | 153.3 µs | +0.02% |
| ⚡ | createQueryFingerprint |
514.3 µs | 514.2 µs | +0.02% |
| ⚡ | tokenizeAlphaNumeric |
367.3 µs | 367.3 µs | +0.01% |
| ⚡ | tokenize + score query over 2000 docs |
7.5 ms | 7.5 ms | +0.01% |
| ⚡ | score query over 2000 docs |
5.6 ms | 5.6 ms | 0% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing feat/sdk-atomic-workspace-transactions (f96b40e) with main (22417d2)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a public SDK transaction boundary via commitWorkspaceTransaction to coordinate ordered, idempotent item and relationship mutations under a workspace writer lock and a durable replay journal, resolving feature pm-4e12. It integrates this coordinator into the extension command SDK context and migrates the pm-vcs merge command to use it, closing the last explicit SDK waiver. Comprehensive unit and integration tests are added to cover transaction recovery, compensation races, and VCS merge transactions, alongside updated documentation. The review feedback correctly points out that the shared string normalization helpers requiredIdentifier and requiredText should accept unknown and validate the type before calling .trim() to prevent potential runtime crashes from non-string inputs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR adds a crash-recoverable workspace transaction API and moves VCS merges onto it. The main changes are:
Confidence Score: 5/5This PR appears safe to merge with low residual risk. The transaction coordinator validates journal identity and lifecycle, snapshots options before locking, persists started-step provenance before mutation, clones durable JSON values before journaling, and compensates in reverse order. The VCS consumer uses transaction-owned compensation and post-commit validation, and the tests cover the reported crash and JSON fidelity scenarios. No files require special attention.
 T-Rex LogsWhat T-Rex did
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Consumer as SDK / extension consumer
participant Tx as commitWorkspaceTransaction
participant Journal as Durable journal
participant Lock as Workspace writer lock
participant Domain as Item + relationship stores
Consumer->>Tx: transactionId, author, ordered steps
Tx->>Lock: acquire workspace transaction lock
Tx->>Journal: load or create applying journal
loop each step
Tx->>Domain: inspect durable state
alt pending
Tx->>Domain: prepareCompensation()
Tx->>Journal: record started step + compensation data
Tx->>Domain: apply()
end
Tx->>Journal: record completed step + JSON result
end
Tx->>Journal: mark committed
Tx->>Lock: release lock
alt failure before commit
Tx->>Journal: mark compensating
loop reverse started/applied steps
Tx->>Domain: inspect and compensate owned mutation
Tx->>Journal: remove step provenance/result
end
Tx->>Journal: mark compensated
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Consumer as SDK / extension consumer
participant Tx as commitWorkspaceTransaction
participant Journal as Durable journal
participant Lock as Workspace writer lock
participant Domain as Item + relationship stores
Consumer->>Tx: transactionId, author, ordered steps
Tx->>Lock: acquire workspace transaction lock
Tx->>Journal: load or create applying journal
loop each step
Tx->>Domain: inspect durable state
alt pending
Tx->>Domain: prepareCompensation()
Tx->>Journal: record started step + compensation data
Tx->>Domain: apply()
end
Tx->>Journal: record completed step + JSON result
end
Tx->>Journal: mark committed
Tx->>Lock: release lock
alt failure before commit
Tx->>Journal: mark compensating
loop reverse started/applied steps
Tx->>Domain: inspect and compensate owned mutation
Tx->>Journal: remove step provenance/result
end
Tx->>Journal: mark compensated
end
Reviews (15): Last reviewed commit: "fix(sdk): harden atomic transaction runt..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR adds crash-recoverable workspace transactions to the public SDK and moves VCS merge onto that path. The main changes are:
Confidence Score: 5/5Safe to merge with minimal risk. The reviewed transaction and VCS merge paths cover replay, compensation, duplicate handling, and conflict cases, with no accepted correctness or security issues. No files require special attention.
T-Rex LogsWhat T-Rex did
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Extension as VCS extension command
participant SDK as ExtensionCommandSdk
participant Tx as commitWorkspaceTransaction
participant Journal as Durable transaction journal
participant Item as PmClient item store
participant Rel as RelationshipEventStore
Extension->>SDK: "commitWorkspaceTransaction({ transactionId, steps })"
SDK->>Tx: bind pmRoot and invoke coordinator
Tx->>Journal: load or prepare journal
Tx->>Item: inspect/apply merge-item step
Tx->>Journal: record merge-item result
Tx->>Rel: inspect/apply merge-relationship step
Tx->>Journal: record relationship result
Tx->>Journal: mark committed
Tx-->>Extension: committed result
alt ordinary failure before commit
Tx->>Journal: mark compensating
Tx->>Rel: compensate relationship step
Tx->>Item: compensate item step
Tx->>Journal: mark compensated for replay
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Extension as VCS extension command
participant SDK as ExtensionCommandSdk
participant Tx as commitWorkspaceTransaction
participant Journal as Durable transaction journal
participant Item as PmClient item store
participant Rel as RelationshipEventStore
Extension->>SDK: "commitWorkspaceTransaction({ transactionId, steps })"
SDK->>Tx: bind pmRoot and invoke coordinator
Tx->>Journal: load or prepare journal
Tx->>Item: inspect/apply merge-item step
Tx->>Journal: record merge-item result
Tx->>Rel: inspect/apply merge-relationship step
Tx->>Journal: record relationship result
Tx->>Journal: mark committed
Tx-->>Extension: committed result
alt ordinary failure before commit
Tx->>Journal: mark compensating
Tx->>Rel: compensate relationship step
Tx->>Item: compensate item step
Tx->>Journal: mark compensated for replay
end
Reviews (2): Last reviewed commit: "feat(sdk): add crash-recoverable workspa..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@packages/pm-vcs/extensions/vcs/index.ts`:
- Around line 431-436: The item compensation at
packages/pm-vcs/extensions/vcs/index.ts lines 431-436 must restore the
transaction’s original vcs_ref, or conditionally compensate only when the merge
target still matches that original value; update the compensation callback
around client.update to preserve prior state. The relationship compensation at
packages/pm-vcs/extensions/vcs/index.ts lines 478-491 must verify that the
latest relationship add belongs to this merge before appending its removal,
leaving newer unrelated relationship state intact.
- Around line 450-453: Update the return value in the recovered relationship
event result to use the durable event’s actual identifier, `event.eventId`,
instead of the stale `eventId` variable; keep the existing relationshipId and
sequence fields unchanged.
- Around line 500-509: Update the merge-success validation after reloading
changeset via getVcsItem to also require the item status to be merged and its
resolution to match the expected resolution, in addition to the existing
findLatestMergeRelationshipEvent and matchesRequestedMerge checks; throw the
same failure error when either side is invalid.
In `@packages/pm-vcs/GAP_REPORT.md`:
- Around line 24-31: Update the documentation change in GAP_REPORT.md using the
pm docs workflow so it is linked to the active pm tracker item. Preserve the
existing conclusion content and ensure the generated or added pm docs linkage is
visible before merging.
In `@src/sdk/workspace-transaction.ts`:
- Around line 263-264: Update the result-map initialization and corresponding
result storage in the transaction flow around completedStepIds and results to
safely preserve a valid __proto__ step ID. Use a null-prototype map or explicit
own-property definitions for both the journal and returned results, while
keeping normal step-ID behavior unchanged.
- Around line 214-235: Strengthen validation in the journal-checking flow around
hasValidJournalMetadata and statusMatches: require completedStepIds to contain
only valid, unique plan step IDs, require results to be a non-array object, and
enforce lifecycle completion consistency so committed journals contain all plan
steps while compensated journals contain none. Keep the existing rejection path
and return only journals satisfying these invariants.
- Around line 347-362: Update discoverAppliedSteps to rebuild
journal.completedStepIds exclusively from steps whose current inspection.state
is "applied", rather than seeding from journal.completedStepIds. Also remove
stale journal.results entries for steps no longer reported as applied, while
preserving inspected results for currently applied steps so
applyPreparedTransaction cannot skip pending or compensated steps.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: e10bcb67-22b2-4b1f-a6e0-b3d82307c10a
📒 Files selected for processing (25)
.agents/pm/features/pm-4e12.toon.agents/pm/features/pm-cfed.toon.agents/pm/features/pm-ju83.toon.agents/pm/history/pm-4e12.jsonl.agents/pm/history/pm-8ngt.jsonl.agents/pm/history/pm-cfed.jsonl.agents/pm/history/pm-d9g9.jsonl.agents/pm/history/pm-ds3b.jsonl.agents/pm/history/pm-ju83.jsonl.agents/pm/history/pm-ukml.jsonl.agents/pm/issues/pm-d9g9.toon.agents/pm/issues/pm-ukml.toon.agents/pm/stories/pm-8ngt.toon.agents/pm/tasks/pm-ds3b.toonCHANGELOG.mddocs/SDK.mdpackages/pm-vcs/GAP_REPORT.mdpackages/pm-vcs/README.mdpackages/pm-vcs/extensions/vcs/index.tssrc/core/extensions/extension-types.tssrc/sdk/extension-command-context.tssrc/sdk/index.tssrc/sdk/workspace-transaction.tstests/unit/packages/vcs-extension.spec.tstests/unit/sdk/workspace-transaction.spec.ts
|
CodeRabbit action comment 5011949946 acknowledged. The full-review command completed and its seven inline findings were individually verified, reacted to, answered in-thread, implemented where valid, and resolved. |
|
Codecov report 5011958283 acknowledged. After the hosted-review fixes, the full local all-source run passes 311 files and 5,916 tests at exact 100% statements, branches, functions, and lines (39,078 / 27,492 / 7,955 / 38,042). |
|
CodSpeed report 5011955154 reviewed. I down-voted the regression classification because the report explicitly compares different runtime environments and labels 0% to 0.06% deltas on untouched pagination, BM25, and normalization benchmarks while also showing six improvements. The transaction/VCS changes do not touch those hot paths; the repository token/performance quality gates remain clean. |
|
Greptile review 4728839642 acknowledged. The original vcs_ref compensation finding was accepted and verified by a two-ref failure/retry scenario in the VCS acceptance suite. |
|
Greptile summary 5011959342 acknowledged. Its transaction-owned VCS compensation finding was accepted: original refs are restored, newer item state and unrelated relationship winners are preserved, and concurrency regressions cover both sides. |
|
CodeRabbit review 4728847226 acknowledged. Six code findings were accepted and implemented; the documentation-link finding was declined with live pm docs evidence. Every inline thread has a reaction, contextual reply, and resolved state. |
|
Gemini review 4728838613 acknowledged. The non-string runtime validation finding was accepted and covered for transaction IDs, authors, and step IDs before any trim call. |
|
Greptile summary 5011961381 acknowledged. The exact-head review evidence was reconciled with every inline thread; the accepted recovery and compensation hardening is now covered by full 100% all-source tests and packed-artifact acceptance. |
|
CodeRabbit walkthrough 5011950655 acknowledged. Its architectural summary is accurate. The reported 18.75% docstring warning is a scanner mismatch: the repository's strict source/exported/member docstring gate currently reports 100% / 100% / 100%, and the full static gate passes. The actionable inline findings were handled separately. |
Validate durable journal lifecycle state, preserve adversarial result keys, and rebuild recovery completion from live inspections. Make VCS merge compensation transaction-owned, restore original refs, preserve concurrent winners, and validate both item and relationship state before success. Record hosted review evidence and exact full-coverage acceptance in the canonical pm items.
Persist compensating state before domain inspection and reconcile every step through its idempotent transaction-owned compensation when inspection fails. Add a regression proving a conflicted journal reaches compensated and a subsequent attempt commits without manual journal deletion, then record the Greptile finding and exact coverage evidence in pm-4e12.
|
@greptileai please re-review the complete current head f5115de, including the hosted-review hardening and the inspection-conflict compensation recovery fix. |
|
@gemini-code-assist /review |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 59 minutes. |
|
@coderabbitai full review |
|
@gemini-code-assist /review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 4 minutes. |
There was a problem hiding this comment.
Code Review
This pull request introduces a public crash-recoverable transaction coordinator, commitWorkspaceTransaction, which allows SDK consumers to execute atomic multi-item and relationship-event mutations with reverse append-only compensation. The pm-vcs package has been migrated to use this new primitive for its merge command, successfully closing its last explicit SDK waiver. The implementation includes robust runtime JSON cloning to prevent serialization failures from poisoning recovery, along with comprehensive test coverage. Feedback is provided on an optimization opportunity in compensateAppliedSteps to skip unstarted or uncompleted steps, avoiding redundant disk I/O and transition emissions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Reject nested non-finite numbers instead of silently journaling null, and skip compensation writes for steps absent from both durable provenance sets. Extend the runtime-boundary regression through compensation and retry at exact coverage.
|
@greptileai Please perform the final exact-head review of non-finite JSON rejection and compensation provenance skipping. |
|
@coderabbitai full review |
|
@gemini-code-assist /review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Code Review
This pull request introduces a public SDK transaction boundary (commitWorkspaceTransaction) that coordinates ordered, idempotent multi-item and relationship-event mutations under a workspace writer lock and a durable replay journal. It migrates the pm-vcs merge command onto this new primitive, replacing its bespoke crash-consistency protocol, and adds comprehensive tests and documentation. The review feedback suggests enhancing error handling to prevent compensation failures from overwriting original transaction errors, and adding runtime validation for the pmRoot parameter.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/pm-vcs/extensions/vcs/index.ts`:
- Around line 437-476: Update the merge transaction’s prepareCompensation flow
to reload the changeset after the coordinator’s writer lock is acquired,
validate that it is a proposed changeset with a non-empty vcs_ref, and capture
the trimmed current reference for rollback. Stop reusing the pre-lock
originalRef value; keep compensateChangesetMerge using the state returned by
prepareCompensation.
In `@src/sdk/workspace-transaction.ts`:
- Around line 68-69: Update the documentation for the transition property in
WorkspaceTransactionTransition so it states that step_applied is emitted after
apply() completes and before the journal records the step, accurately describing
the crash boundary.
- Around line 573-616: Snapshot options.steps and the associated transaction
options before the first await, then use the resulting stableOptions
consistently throughout the transaction flow. Update step ID validation,
acquireLock, loadJournal, prepareJournalForApply, and applyPreparedTransaction
calls to derive from that snapshot so mutations during locking or onTransition
cannot diverge from the journaled execution plan.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 777e695f-7f83-4f76-9a8d-54c5d05b7d88
📒 Files selected for processing (25)
.agents/pm/features/pm-4e12.toon.agents/pm/features/pm-cfed.toon.agents/pm/features/pm-ju83.toon.agents/pm/history/pm-4e12.jsonl.agents/pm/history/pm-8ngt.jsonl.agents/pm/history/pm-cfed.jsonl.agents/pm/history/pm-d9g9.jsonl.agents/pm/history/pm-ds3b.jsonl.agents/pm/history/pm-ju83.jsonl.agents/pm/history/pm-ukml.jsonl.agents/pm/issues/pm-d9g9.toon.agents/pm/issues/pm-ukml.toon.agents/pm/stories/pm-8ngt.toon.agents/pm/tasks/pm-ds3b.toonCHANGELOG.mddocs/SDK.mdpackages/pm-vcs/GAP_REPORT.mdpackages/pm-vcs/README.mdpackages/pm-vcs/extensions/vcs/index.tssrc/core/extensions/extension-types.tssrc/sdk/extension-command-context.tssrc/sdk/index.tssrc/sdk/workspace-transaction.tstests/unit/packages/vcs-extension.spec.tstests/unit/sdk/workspace-transaction.spec.ts
Reject every lossy runtime JSON value, preserve forward and compensation failures, validate and snapshot transaction inputs, and capture VCS rollback state under the transaction lock. Add exact regressions for mutable plans, dual failures, invalid roots, non-JSON values, and post-preflight VCS state races; record the complete pm-4e12 verification evidence.
|
@greptileai please review the exact current head f96b40e; all prior findings are fixed with full 100/100/100/100 coverage. |
|
@coderabbitai full review |
|
@gemini-code-assist /review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 59 minutes. |
There was a problem hiding this comment.
Code Review
This pull request introduces commitWorkspaceTransaction, a public crash-recoverable workspace transaction coordinator that coordinates ordered, idempotent item and relationship mutations under a workspace writer lock and a durable replay journal. The pm-vcs merge command has been migrated to use this new primitive, closing the last explicit SDK waiver in GAP_REPORT.md. Comprehensive unit tests and updated documentation have been added to support this feature. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
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 (2)
src/sdk/workspace-transaction.ts (2)
586-607: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSnapshot each step, not only the containing array.
steps = [...options.steps]retains caller-owned step objects. Mutation ofstep.id,inspect, orapplywhile awaiting the lock or fromonTransitioncan still make execution diverge from the validatedstepIdsand durable plan.Proposed fix
- const steps = [...options.steps]; + const steps = options.steps.map((step) => ({ + id: step.id, + inspect: step.inspect, + apply: step.apply, + prepareCompensation: step.prepareCompensation, + compensate: step.compensate, + }));🤖 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/sdk/workspace-transaction.ts` around lines 586 - 607, Snapshot each transaction step when building steps in the workspace transaction setup, rather than only cloning the containing array. Create independent step objects before validation and storing them in stableOptions, preserving the validated stepIds while preventing later mutation of id, inspect, or apply by callers or onTransition handlers.
207-223: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject object shapes that stringify lossily.
The replacer catches invalid property values, but
Date,Map, symbol-keyed properties, sparse arrays, and customtoJSON()values can still be silently transformed or dropped. Recursively validate and clone the JSON value tree instead of relying onJSON.stringifyas validation.🤖 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/sdk/workspace-transaction.ts` around lines 207 - 223, Replace JSON.stringify/parse validation in cloneJournalValue with recursive traversal and cloning of the JSON value tree. Explicitly reject lossy shapes including Date, Map, symbol-keyed properties, sparse arrays, and custom toJSON() values, while preserving rejection of non-finite numbers and other non-JSON values and returning a WorkspaceTransactionJsonValue clone.
🤖 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 `@packages/pm-vcs/extensions/vcs/index.ts`:
- Around line 468-473: Update the transaction compensation flow around
prepareCompensation so an item discovered as already merged, with no preparation
data, is treated as transaction-unowned and skipped during compensation. Ensure
relationship-step failures do not invoke item compensation with undefined data,
while preserving compensation for items whose preparation actually ran.
---
Outside diff comments:
In `@src/sdk/workspace-transaction.ts`:
- Around line 586-607: Snapshot each transaction step when building steps in the
workspace transaction setup, rather than only cloning the containing array.
Create independent step objects before validation and storing them in
stableOptions, preserving the validated stepIds while preventing later mutation
of id, inspect, or apply by callers or onTransition handlers.
- Around line 207-223: Replace JSON.stringify/parse validation in
cloneJournalValue with recursive traversal and cloning of the JSON value tree.
Explicitly reject lossy shapes including Date, Map, symbol-keyed properties,
sparse arrays, and custom toJSON() values, while preserving rejection of
non-finite numbers and other non-JSON values and returning a
WorkspaceTransactionJsonValue clone.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07c9d37e-2db0-4a88-96ec-d4e03cb81938
📒 Files selected for processing (6)
.agents/pm/features/pm-4e12.toon.agents/pm/history/pm-4e12.jsonlpackages/pm-vcs/extensions/vcs/index.tssrc/sdk/workspace-transaction.tstests/unit/packages/vcs-extension.spec.tstests/unit/sdk/workspace-transaction.spec.ts
|
Final exact-head review inventory for f96b40e: every GitHub Actions, security, coverage, performance, and static gate is green; Greptile completed clean; Gemini completed with no findings; CodeRabbit’s sole new comment was traced against startedStepIds ownership, declined with regression evidence, downvoted, and resolved. Every bot comment/review artifact has been reacted to, every actionable thread has contextual inline disposition, and zero review threads remain unresolved. |
…reconciled clean (4e12 transaction waiver retired, 8ngt held open over kr3t, resolution 276 zero-leak); maintainer dogfood merge-safety wave filed as 14 items GH-595/598-604/606-609/611/612 (wc1r/9q2t/m3nl/wwfd/xdn6/gpo7/ol3p under g5sx, cxyv under 33cw, pibw under dj98, 4uqm/wrss/xh82 under mpbb, x29o/n1xx under usfg) with family map + execution order on g5sx; GH-610 mapped to i6pi as dup; GH-596/597 pm-links backfilled; census 1886n/3171e/242a zero actionable Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XYPB6bCGL5t41DLywtcKW
Summary
pm items
Verification
Summary by cubic
Adds a public
commitWorkspaceTransactionSDK primitive for crash‑recoverable, atomic item+relationship commits, and migrates@unbrained/pm-vcsmerges to use it. Latest changes persist compensation provenance, enforce exact step identity, and harden runtime boundaries with strict JSON fidelity, input snapshots, and locked rollback for safe recovery.New Features
commitWorkspaceTransaction(...): durable journal with writer lock, ordered idempotent steps, reverse append‑only compensation.@unbrained/pm-vcsmerge now commits items and relationships atomically via the new coordinator; closes the GAP waiver.Bug Fixes
Written for commit f96b40e. Summary will update on new commits.