Repository navigation
SDK transaction guards and extension runtime parity - #857
Conversation
Add capability-gated beforeMutation hooks with host-bound read-only SDK context and fail-closed stable diagnostics across create, update, annotations, close, delete, restore, and structured transaction paths. Bring extension command tests to production SDK parity, unify active custom item-type lifecycle lookup, expand public contracts and 100% coverage, regenerate performance budgets and changelog, and record pm-wx2lr5, pm-hx23u5, and pm-scga6k delivery evidence.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
@coderabbitai full review @greptileai please review this exact head, including the transactional boundaries, extension SDK contracts, and denial side-effect guarantees. |
|
Warning Review limit reached
Next review available in: 24 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 Plus Run ID: 📒 Files selected for processing (35)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds transactional ChangesExtension mutation platform
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ExtensionCommand
participant MutationLifecycle
participant BeforeMutationRuntime
participant GuardSdk
participant ItemHistoryStore
ExtensionCommand->>MutationLifecycle: create, update, comment, delete, or restore
MutationLifecycle->>GuardSdk: provide host-bound read-only access
MutationLifecycle->>BeforeMutationRuntime: runActiveBeforeMutationHooks(context)
BeforeMutationRuntime-->>MutationLifecycle: allow or structured denial
MutationLifecycle->>ItemHistoryStore: persist item and history after approval
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
Reviewer's GuideIntroduce capability-gated, fail-closed transactional mutation guards into the extension runtime, wire them into all item lifecycle mutations with a host-bound read-only SDK, and bring the SDK testing harness and extension item-type resolution into parity with production, with supporting docs, error catalog, and performance budget updates. Sequence diagram for fail-closed beforeMutation guards in item lifecyclesequenceDiagram
actor ExtensionPackage
participant Lifecycle as mutateItem_create_delete_restore
participant ExtRuntime as runActiveBeforeMutationHooks
participant HookRuntime as runBeforeMutationHooks
participant Guard as BeforeMutationHook
ExtensionPackage->>Lifecycle: invoke mutation (create/update/delete/restore)
Lifecycle->>Lifecycle: canonicalDocument(before/after)
Lifecycle->>Lifecycle: createMutationGuardSdk(params)
Lifecycle->>ExtRuntime: runActiveBeforeMutationHooks(context)
ExtRuntime->>HookRuntime: runBeforeMutationHooks(hooks, context)
loop each RegisteredExtensionHook<BeforeMutationHook>
HookRuntime->>Guard: entry.run(context)
HookRuntime->>HookRuntime: enforceMutationGuardDecision(decision, entry, operation)
end
alt all guards allow or return void
HookRuntime-->>Lifecycle: success
Lifecycle->>Lifecycle: persist item and history
else any guard denial or failure
HookRuntime-->>Lifecycle: throw PmCliError (extension_mutation_guard_*)
Lifecycle-->>ExtensionPackage: mutation blocked, no item/history side effects
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 59 minutes. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sdk/testing.ts (1)
465-488: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove
before_mutationfromrunRegisteredHookForTest.
before_mutationhas dedicated helpers (runRegisteredMutationGuardForTestandrunBeforeMutationHooks), butrunRegisteredHookForTestlacks a switch branch for it. A registered mutation guard passes the registration check and then returnsundefined; useassertRegisteredHookfor registrations and call the mutation guard helper for invocation.🤖 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/testing.ts` around lines 465 - 488, Remove before_mutation from the RegisteredHookKind/HOOK_KIND_TO_REGISTRY_FIELD path used by runRegisteredHookForTest, and update that method to reject it through the existing assertRegisteredHook validation rather than silently returning undefined. Keep mutation-hook invocation routed through runRegisteredMutationGuardForTest and runBeforeMutationHooks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/extensions/extension-hook-runtime.ts`:
- Around line 132-163: Update runBeforeMutationHooks so each entry.run
invocation is bounded by the established guard-timeout mechanism or a defined
timeout race. When the timeout wins, throw a PmCliError with conflict semantics
and structured code extension_mutation_guard_timed_out, including the guard
identity and actionable next steps; preserve existing PmCliError propagation and
extension_mutation_guard_failed handling for non-timeout failures.
In `@src/core/store/item-store.ts`:
- Around line 874-888: The delete path in deleteItem currently canonicalizes
beforeDocument without active extension field names. Update the
canonicalDocument call to use the existing helper or options that collect and
pass extensionFieldNames, while preserving the schema, so extension-registered
fields remain available to mutation hooks, history, and service-override
payloads.
In `@src/sdk/testing.ts`:
- Around line 210-214: Update the documentation for sdkFactory and its pmRoot
context property to explicitly state that pmRoot is the tracker storage root,
not the workspace root, and should be used for tracker data under .agents/pm.
---
Outside diff comments:
In `@src/sdk/testing.ts`:
- Around line 465-488: Remove before_mutation from the
RegisteredHookKind/HOOK_KIND_TO_REGISTRY_FIELD path used by
runRegisteredHookForTest, and update that method to reject it through the
existing assertRegisteredHook validation rather than silently returning
undefined. Keep mutation-hook invocation routed through
runRegisteredMutationGuardForTest and runBeforeMutationHooks.
🪄 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 Plus
Run ID: 3c99b966-19ce-48da-a4eb-47d544a1b4fb
📒 Files selected for processing (35)
.agents/pm/extensions/.managed-extensions.json.agents/pm/history/pm-hx23u5.jsonl.agents/pm/history/pm-scga6k.jsonl.agents/pm/history/pm-wx2lr5.jsonl.agents/pm/issues/pm-hx23u5.toon.agents/pm/issues/pm-scga6k.toon.agents/pm/issues/pm-wx2lr5.toonCHANGELOG.mddocs/EXTENSIONS.mddocs/performance/sdk-entrypoint-import-costs.mdscripts/bench/sdk-entrypoint-budgets.jsonsdk/public-surface.jsonsrc/core/extensions/activation-summary.tssrc/core/extensions/activation-transaction.tssrc/core/extensions/capability-usage.tssrc/core/extensions/extension-hook-runtime.tssrc/core/extensions/extension-registries.tssrc/core/extensions/extension-types.tssrc/core/extensions/index.tssrc/core/extensions/loader.tssrc/core/store/item-store.tssrc/sdk/authoring.tssrc/sdk/compose.tssrc/sdk/define.tssrc/sdk/generated-error-code-catalog.tssrc/sdk/lifecycle/close.tssrc/sdk/lifecycle/create.tssrc/sdk/lifecycle/restore.tssrc/sdk/runtime-primitives.tssrc/sdk/testing.tstests/fixtures/contracts/full.jsontests/unit/extensions/extension-loader.spec.tstests/unit/sdk/sdk-compose.spec.tstests/unit/sdk/sdk-define.spec.tstests/unit/sdk/sdk-extension-mutation-platform.spec.ts
Fail closed stalled mutation guards after five seconds, preserve active extension metadata during delete canonicalization, and clarify tracker-root semantics in the command test harness. Add regression coverage, regenerate public error/contracts surfaces, and append exact-head review evidence to pm-hx23u5 and pm-wx2lr5.
Keep fail-closed beforeMutation invocation on the dedicated mutation-guard helper and reject unsupported runtime input instead of silently returning undefined. Cover registered and legacy empty registries, retain 100/100/100/100 coverage, and record the outside-diff review evidence on pm-wx2lr5.
|
@codspeed-hq Acknowledged the no-regression report at #857 (comment). The reviewed SDK/testing cold-import cost is separately budgeted, while the tracked runtime benchmarks remain unchanged and passing. |
|
@sourcery-ai The reviewer guide at #857 (comment) accurately maps the change and was useful for cross-checking scope. The separate rate-limit review at #857 (review) contains no actionable code feedback; all repository gates remain green. |
|
@gemini-code-assist Acknowledged the sunset notice in #857 (comment). It contains no code finding to apply; the unavailable reviewer is therefore recorded as non-actionable. |
|
@codecov Acknowledged the patch report at #857 (comment). After both review-fix commits, the full local all-source gate still reports 6,666 tests and 100% statements, branches, functions, and lines. |
|
@coderabbitai The walkthrough and command acknowledgement at #857 (comment) and #857 (comment) were useful. All three inline findings and the review-level outside-diff finding in #857 (review) are implemented on exact head @coderabbitai full review @greptileai please perform a fresh review of exact head |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 24 minutes. |
|
@gemini-code-assist The repeated sunset notice at #857 (comment) is acknowledged as unavailable and non-actionable. @coderabbitai The final-head request response at #857 (comment) confirms the full review was requested but unavailable under the included rate limit. We waited through the full CI/review window; all earlier findings are fixed, every finding thread is resolved, and all exact-head checks pass. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Final bot-artifact acknowledgement: the repeated Gemini sunset notice at #857 (comment) remains unavailable/non-actionable, and the CodeRabbit response at #857 (comment) confirms the exact-head request was accepted but rate-limited. Both were reacted to by usefulness. No unresolved review threads remain, and exact head |
Summary
beforeMutationextension hooks with canonical before/after documents, changed fields, stable diagnostics, and a host-bound read-only SDKProject management
Fixes #853
Fixes #854
Fixes #855
Verification
context.sdk.client, enforced a mutation guard, and resolved the packed executable through local,npx --no-install, andbunx --no-installpathspm-changelog2026.8.1 output is stableCompatibility note
The production-shaped test harness intentionally loads the native
PmClientservice graph. Its measured cold-import cost is isolated to@unbrained/pm-cli/sdk/testingand recorded in the regenerated reviewed budget; runtime entrypoints remain unchanged.Summary by Sourcery
Introduce transactional before-mutation extension guards with a host-bound read-only SDK, and align extension runtime, item lifecycle, and test harness behavior with production semantics.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
Summary by cubic
Adds capability-gated
beforeMutationguards to enforce extension invariants across item transactions with fail‑closed, stable diagnostics and a 5s timeout. Also injects a real host-bound SDK into extension command tests, unifies extension item-type resolution, and routes mutation guard testing through a dedicated helper.New Features
beforeMutationguard with canonical before/after docs, changed fields, and operation; exposes a host-bound read‑only SDK; denial aborts with stable error codes (extension_mutation_guard_*); guard execution is bounded to 5s and fails closed.defineBeforeMutationHook, blueprintbeforeMutationsupport,ExtensionMutationGuardSdk, and clarifies tracker‑root semantics.Bug Fixes
beforeMutation; tests invoke guards via the dedicated mutation-guard helper, preserving fail‑closed behavior for both registered and legacy empty registries.Written for commit 36f55d6. Summary will update on new commits.