Stabilize external saved-package publishing - #427
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThis PR adds retry logic for transient Durable Object resets in the publish-external-push capability, refactors runRepoChecks to collect and return sourceFiles (snapshotting all repo files except .git), makes publishFromExternalRef accept optional files with a fallback to checks.sourceFiles, and removes local workspace file collection in the repo session path. ChangesPublish Retry & Source File Propagation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
🔎 Preview deployed: https://kody-pr-427.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
packages/worker/src/repo/repo-session-do.node.test.ts (1)
927-955:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
workspaceGlobbefore asserting zero calls.
mockModule.workspaceGlobis shared across this whole suite, and earlier tests in this file already use it. Without a localmockClear()/mockReset(), this assertion becomes order-dependent and can fail even when this publish path never globbed the workspace.Proposed fix
test('publishFromExternalRef checks fast-forward ancestry through shell git adapter', async () => { setCommonSessionFixtures() + mockModule.workspaceGlob.mockClear() mockModule.getEntitySourceById.mockResolvedValue({ id: 'source-1', user_id: 'user-1',🤖 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/repo-session-do.node.test.ts` around lines 927 - 955, In the test 'publishFromExternalRef checks fast-forward ancestry through shell git adapter' clear the shared mock state for workspaceGlob to make the assertion order-independent: call mockModule.workspaceGlob.mockClear() (or mockReset()) before invoking repoSession.publishFromExternalRef (i.e., after test fixtures and mockModule.git.log setup but before awaiting repoSession.publishFromExternalRef) so the subsequent expect(mockModule.workspaceGlob).not.toHaveBeenCalled() only reflects this test's activity.packages/worker/src/repo/external-publish.ts (1)
161-166:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
checks.sourceFilesis too narrow for the published snapshot.Line 165 now falls back to
checks.sourceFiles, butrunRepoChecksonly harvests**/*.{ts,tsx,js,jsx,json}for bundling/typechecking. That means publishes with other source-root assets (.md,.sql,.css,.html, etc.) will now persist an incomplete snapshot, and downstream readers will miss files that were present at the published commit.Please keep using a publish-grade full snapshot here, or have checks return a separate full file set instead of reusing the bundler-only record.
🤖 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/external-publish.ts` around lines 161 - 166, The call to finalizePublishedEntitySource is falling back to checks.sourceFiles (from runRepoChecks), but runRepoChecks only collects bundler/typecheck globs and omits non-code assets, causing incomplete published snapshots; update the flow so finalizePublishedEntitySource always receives a publish-grade full file list: either (A) ensure runRepoChecks returns a new property like fullFiles (or fullSnapshotFiles) containing the complete repository snapshot and pass that (use checks.fullFiles instead of checks.sourceFiles), or (B) keep input.files as the single source of truth for publishing and make the caller populate input.files with the full snapshot before calling finalizePublishedEntitySource; locate usages around finalizePublishedEntitySource, runRepoChecks, and checks.sourceFiles to implement the chosen approach and update types/tests accordingly.
🤖 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/repo/external-publish.ts`:
- Around line 161-166: The call to finalizePublishedEntitySource is falling back
to checks.sourceFiles (from runRepoChecks), but runRepoChecks only collects
bundler/typecheck globs and omits non-code assets, causing incomplete published
snapshots; update the flow so finalizePublishedEntitySource always receives a
publish-grade full file list: either (A) ensure runRepoChecks returns a new
property like fullFiles (or fullSnapshotFiles) containing the complete
repository snapshot and pass that (use checks.fullFiles instead of
checks.sourceFiles), or (B) keep input.files as the single source of truth for
publishing and make the caller populate input.files with the full snapshot
before calling finalizePublishedEntitySource; locate usages around
finalizePublishedEntitySource, runRepoChecks, and checks.sourceFiles to
implement the chosen approach and update types/tests accordingly.
In `@packages/worker/src/repo/repo-session-do.node.test.ts`:
- Around line 927-955: In the test 'publishFromExternalRef checks fast-forward
ancestry through shell git adapter' clear the shared mock state for
workspaceGlob to make the assertion order-independent: call
mockModule.workspaceGlob.mockClear() (or mockReset()) before invoking
repoSession.publishFromExternalRef (i.e., after test fixtures and
mockModule.git.log setup but before awaiting repoSession.publishFromExternalRef)
so the subsequent expect(mockModule.workspaceGlob).not.toHaveBeenCalled() only
reflects this test's activity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28a32437-4cee-4ab1-9973-df65ae5cc370
📒 Files selected for processing (7)
packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.tspackages/worker/src/mcp/capabilities/packages/publish-external-push.tspackages/worker/src/repo/checks.tspackages/worker/src/repo/external-publish.node.test.tspackages/worker/src/repo/external-publish.tspackages/worker/src/repo/repo-session-do.node.test.tspackages/worker/src/repo/repo-session-do.ts
💤 Files with no reviewable changes (1)
- packages/worker/src/repo/repo-session-do.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/checks.ts`:
- Around line 160-166: The current glob (pattern) and loop in thisRepo's file
collection collects everything under normalizedRoot (via input.workspace.glob
and files) which can include sensitive or large files; implement an explicit
allow/deny snapshot policy before adding file contents to sourceFiles: parse and
honor repository ignore rules and package.json "files" (if present), and apply a
deny-list for known sensitive/build dirs and file patterns (e.g., node_modules,
.gitignored paths, .env, *.pem, build/, dist/, *.log, artifacts/). Modify the
loop that reads files (the files for..of block using normalizeRepoWorkspacePath
and input.workspace.readFile) to first run each normalizedPath through this
policy and skip any denied paths, and ensure the policy logic is centralized
(e.g., a helper like isSnapshotAllowed(path)) so publish finalization reuses it.
🪄 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: e24b31ba-ab9a-4abd-8c4b-5195efabb354
📒 Files selected for processing (2)
packages/worker/src/repo/checks.node.test.tspackages/worker/src/repo/checks.ts
| const pattern = normalizedRoot === '' ? '**/*' : `${normalizedRoot}/**/*` | ||
| const files = await input.workspace.glob(pattern) | ||
| for (const file of files) { | ||
| if (file.type !== 'file') continue | ||
| const normalizedPath = normalizeRepoWorkspacePath(file.path) | ||
| if (normalizedPath.split('/').includes('.git')) continue | ||
| const content = await input.workspace.readFile(file.path) |
There was a problem hiding this comment.
Add a publish snapshot allow/deny policy before collecting **/* files.
Line 160 and Line 165 now include every file under sourceRoot except .git. Since this sourceFiles payload is reused for publish finalization, this can unintentionally ship sensitive or high-volume files (for example local secrets/artifacts) and increase reset risk again. Please gate snapshot inclusion with an explicit policy (e.g., honor package files/ignore rules and block known sensitive/build directories).
🤖 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/checks.ts` around lines 160 - 166, The current glob
(pattern) and loop in thisRepo's file collection collects everything under
normalizedRoot (via input.workspace.glob and files) which can include sensitive
or large files; implement an explicit allow/deny snapshot policy before adding
file contents to sourceFiles: parse and honor repository ignore rules and
package.json "files" (if present), and apply a deny-list for known
sensitive/build dirs and file patterns (e.g., node_modules, .gitignored paths,
.env, *.pem, build/, dist/, *.log, artifacts/). Modify the loop that reads files
(the files for..of block using normalizeRepoWorkspacePath and
input.workspace.readFile) to first run each normalizedPath through this policy
and skip any denied paths, and ensure the policy logic is centralized (e.g., a
helper like isSnapshotAllowed(path)) so publish finalization reuses it.
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 8d11149. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
2 participants
Walkthrough
package_publish_external_push: the RepoSession DO no longer callscollectWorkspaceFiles()beforerunRepoChecks, and publish finalization now reuses the source file snapshot that checks collected.**/*) while excluding.gitinternals, so static assets like CSS/SVG/YAML/etc. remain in published KV snapshots.package_publish_external_push. Each retry re-resolves the package source and Artifacts HEAD, uses a fresh transient RepoSession DO name, and returnsalready_publishedif the prior attempt committed before the reset surfaced.Why this helps
The live failures were consistent with pressure inside the publish/check pipeline rather than package checkout size. The path cloned the external HEAD, read the entire workspace into memory, then
runRepoChecksglobbed/read the same tree again and built another snapshot for bundle/typecheck work. Reusing the checks snapshot removes one full source-tree read and one long-lived copy from the RepoSession DO request. The retry path covers remaining transient DO resets with idempotent recovery instead of exposing raw platform reset errors to callers.Remaining work
A deeper async publish redesign may still be warranted if bundle/typecheck plus projection rebuilds continue to approach DO limits for packages with many targets or dependencies. The likely next step would be moving check/projection artifact rebuilds into a Workflow/queue with durable run status rather than keeping all phases in the MCP/RepoSession request path.
Validation
npx vitest run --project node-unit packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.ts packages/worker/src/repo/repo-session-do.node.test.ts packages/worker/src/repo/checks.node.test.ts packages/worker/src/repo/external-publish.node.test.ts packages/worker/src/mcp/capabilities/meta/search-and-execute.node.test.ts— 52 tests passednpm run typecheck— passednpm run format:check— passednpm run lint— passed with existing unrelated warning inpackages/worker/src/email/inbound.workers.test.tsnpm run validatepassed (format,lint,typecheck, unit tests, Playwright E2E, and MCP E2E all exited 0). Full PR CI is re-running after the latest feedback fix.Summary by CodeRabbit
New Features
Improvements
Tests