Enforce package source safety policy - #525
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThis PR introduces destructive-overwrite safety gating for package sources. It creates a source-safety-policy module, marks package mutation capabilities as destructive, adds explicit confirmation inputs, enforces snapshot-restorability and overwrite-authorization checks before non-fast-forward publishes, extends RPC and Durable Object contracts to thread confirmation through publish flows, and validates all changes with comprehensive test coverage. ChangesDestructive Overwrite Safety Policy for Package Sources
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
🔎 Preview deployed: https://kody-pr-525.kentcdodds.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/worker/src/repo/repo-session-do.ts`:
- Around line 1566-1573: The call to assertPackageSourceOverwriteAllowed is
being applied unconditionally on forced publishSession flows, causing
non-package sources (e.g., syncArtifactSourceSnapshot) to hit a package-only
safety gate; modify the logic around the publish flow so that
assertPackageSourceOverwriteAllowed(...) is only invoked when the source is a
package (check source.type or the appropriate package-identifying field on
source) and skip it for non-package sources, preserving use of input.force and
input.destructiveOverwriteConfirmed for package publishes only; update code
paths in publishSession (where input.force is checked) to branch on source being
a package before calling assertPackageSourceOverwriteAllowed.
In `@packages/worker/src/repo/source-safety-policy.ts`:
- Around line 87-99: The code assumes snapshot.files is an object and calls
Object.keys(files) which throws on malformed snapshots; update the guard to
validate snapshot.files before reading it: check that snapshot is defined and
that typeof snapshot.files === 'object' && snapshot.files !== null (or
Array.isArray check if appropriate), then assign const files = snapshot.files
and compute fileCount and manifestContent; if the validation fails, throw the
same buildSourceRecoveryProblemMessage (using input.source and input.operation)
with a reason like 'the published source snapshot is missing or malformed' so
malformed payloads yield the structured recovery error instead of a raw
TypeError. Ensure references to snapshot.files, files, fileCount,
manifestContent, input.source.manifest_path and
buildSourceRecoveryProblemMessage are used as described.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 88d95629-e0c0-4379-9048-6eaad981cda8
📒 Files selected for processing (15)
packages/worker/src/mcp/capabilities/packages/get-git-remote.node.test.tspackages/worker/src/mcp/capabilities/packages/get-git-remote.tspackages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.tspackages/worker/src/mcp/capabilities/packages/publish-external-push.tspackages/worker/src/mcp/capabilities/packages/save-package.tspackages/worker/src/mcp/capabilities/repo/repo-publish-session.tspackages/worker/src/mcp/capabilities/repo/repo-run-commands-text.tspackages/worker/src/mcp/observability.workers.test.tspackages/worker/src/repo/external-publish.node.test.tspackages/worker/src/repo/external-publish.tspackages/worker/src/repo/repo-session-do.tspackages/worker/src/repo/repo-session-rpc.tspackages/worker/src/repo/source-safety-policy.node.test.tspackages/worker/src/repo/source-safety-policy.tspackages/worker/src/repo/source-sync.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3e9f503. Configure here.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/mcp/capabilities/repo/repo-publish-session.ts (1)
16-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the canonical package-source safety policy in this description.
repo_publish_sessionis now marked destructive, but its description no longer surfaces the canonical package-source safety guidance that this PR says should appear on this capability. That leaves this publish entry point out of sync with the new overwrite policy and removes the recovery/confirmation instructions from the agent-facing contract. Please wrap this description with the shared policy helper used by the other package mutation capabilities.🤖 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 `@packages/worker/src/mcp/capabilities/repo/repo-publish-session.ts` around lines 16 - 21, The capability's description string for repo_publish_session was replaced but must be wrapped with the shared package-source safety policy helper used by other package mutation capabilities; update the description field in repo-publish-session.ts to pass the existing helper (withPackageSourceSafetyPolicy) the canonical guidance string so the publish entry point surfaces the recovery/confirmation instructions and remains consistent with the new overwrite policy while leaving readOnly/idempotent/destructive flags as-is.
🧹 Nitpick comments (1)
packages/worker/src/repo/source-safety-policy.node.test.ts (1)
130-146: 💤 Low valueConsider clarifying the test description.
The test description "package mutation capabilities surface the canonical production source safety policy" could be misread as implying all capabilities surface the policy, when in fact it's testing that only
savePackageCapabilitydoes. Consider a more explicit description such as "only savePackageCapability surfaces the canonical production source safety policy" to improve maintainability and clarity for future developers.🤖 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 `@packages/worker/src/repo/source-safety-policy.node.test.ts` around lines 130 - 146, Update the test's description string to explicitly state that only savePackageCapability should surface the canonical production source safety policy (e.g., "only savePackageCapability surfaces the canonical production source safety policy") so the intent is clear; locate the test block that asserts expectations against savePackageCapability, publishExternalPushCapability, getGitRemoteCapability, repoPublishSessionCapability, repoRunCommandsCapabilityDescription and productionPackageSourceSafetyPolicy and replace the current description text with the more explicit wording.
🤖 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.
Outside diff comments:
In `@packages/worker/src/mcp/capabilities/repo/repo-publish-session.ts`:
- Around line 16-21: The capability's description string for
repo_publish_session was replaced but must be wrapped with the shared
package-source safety policy helper used by other package mutation capabilities;
update the description field in repo-publish-session.ts to pass the existing
helper (withPackageSourceSafetyPolicy) the canonical guidance string so the
publish entry point surfaces the recovery/confirmation instructions and remains
consistent with the new overwrite policy while leaving
readOnly/idempotent/destructive flags as-is.
---
Nitpick comments:
In `@packages/worker/src/repo/source-safety-policy.node.test.ts`:
- Around line 130-146: Update the test's description string to explicitly state
that only savePackageCapability should surface the canonical production source
safety policy (e.g., "only savePackageCapability surfaces the canonical
production source safety policy") so the intent is clear; locate the test block
that asserts expectations against savePackageCapability,
publishExternalPushCapability, getGitRemoteCapability,
repoPublishSessionCapability, repoRunCommandsCapabilityDescription and
productionPackageSourceSafetyPolicy and replace the current description text
with the more explicit wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 799881cc-44b0-47b8-90d7-bb8f1e4a79a5
📒 Files selected for processing (8)
packages/worker/src/mcp/capabilities/packages/get-git-remote.tspackages/worker/src/mcp/capabilities/packages/publish-external-push.tspackages/worker/src/mcp/capabilities/packages/save-package.tspackages/worker/src/mcp/capabilities/repo/repo-publish-session.tspackages/worker/src/repo/external-publish.node.test.tspackages/worker/src/repo/external-publish.tspackages/worker/src/repo/source-safety-policy.node.test.tspackages/worker/src/repo/source-safety-policy.ts
💤 Files with no reviewable changes (1)
- packages/worker/src/repo/source-safety-policy.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/worker/src/mcp/capabilities/packages/publish-external-push.ts
- packages/worker/src/mcp/capabilities/packages/save-package.ts
- packages/worker/src/repo/external-publish.node.test.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>

Summary
packages/worker/src/repo/source-safety-policy.ts.package_save, the direct whole-source replacement entry point.Enforcement notes
package_saveverifies the existing package source snapshot before syncing replacement files and requiresconfirm_destructive_overwritefor existing package replacement.ensureEntitySourceclearspublished_commit,package_saveverifies the pre-recreation canonical package entity source snapshot before allowing confirmed recovery/bootstrap.package_saveverifies against the canonical package entity source, not a potentially stale saved-packagesource_idpointer.package_publish_external_push/ external publish rejects non-fast-forward force publishes unlessallow_forceandconfirm_destructive_overwriteare provided and the current published snapshot is restorable.package_get_git_remoteverifies a restorable package source snapshot before minting write access, so agents stop before clone/edit/publish if the original source cannot be recovered.Validation
npx vitest run --project node-unit packages/worker/src/repo/source-safety-policy.node.test.ts packages/worker/src/repo/external-publish.node.test.ts packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.tsnpx vitest run --project workers-unit packages/worker/src/mcp/observability.workers.test.tsnpx vitest run --project node-unit packages/worker/src/mcp/capabilities/packages/get-git-remote.node.test.ts packages/worker/src/repo/source-safety-policy.node.test.tsnpx vitest run --project workers-unit packages/worker/src/mcp/observability.workers.test.ts && npx vitest run --project node-unit packages/worker/src/repo/source-safety-policy.node.test.tsnpx vitest run --project node-unit packages/worker/src/repo/source-safety-policy.node.test.ts packages/worker/src/repo/external-publish.node.test.tsnpx vitest run --project workers-unit packages/worker/src/mcp/observability.workers.test.ts && npx vitest run --project node-unit packages/worker/src/repo/source-safety-policy.node.test.ts packages/worker/src/repo/external-publish.node.test.tsnpm run format:check && npm run lint && npm run typechecknpm run validate✅ Validatepassed, preview deploy passed, CodeRabbit passed, Cursor Bugbot passed.Summary by CodeRabbit
New Features
Improvements