Add direct Artifacts git push primitives for packages - #352
Conversation
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>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds end-to-end support for external Cloudflare Artifacts git pushes: minting short-lived tokens, resolving Artifacts HEADs, publishing pushed commits through server-side checks and transactional publish (DB + KV snapshot with rollback), a reconcile cron that detects/publishes stale pushes and revokes stale tokens, plus MCP capabilities and tests. ChangesExternal Artifacts Git Push Workflow
Sequence DiagramsequenceDiagram
participant User as User/Client
participant Kody as Kody Server
participant ArtifactsAPI as Artifacts API
participant Git as Git Remote
participant DB as D1 Database
participant KV as KV Storage
participant Reconciler as Reconcile Job
User->>Kody: package_get_git_remote (request token)
Kody->>ArtifactsAPI: Create short-lived token
ArtifactsAPI-->>Kody: token (plaintext, expiresAt)
Kody-->>User: authenticated_remote, git_extra_header, setup_commands
User->>Git: git clone / edit / push (uses http.extraHeader)
Git-->>ArtifactsAPI: update default-branch HEAD
User->>Kody: package_publish_external_push
Kody->>ArtifactsAPI: resolve default-branch HEAD
ArtifactsAPI-->>Kody: current commit OID
Kody->>Kody: runRepoChecks(manifest, files)
alt checks pass
Kody->>DB: update entity_sources.published_commit (new)
Kody->>KV: write published source snapshot
alt snapshot fails
Kody->>DB: revert published_commit to previous
end
Kody-->>User: published (commit, checks)
else checks fail
Kody-->>User: checks_failed (failed_checks, manifest, run_id)
end
Reconciler->>DB: listEntitySourcesForExternalReconcile
DB-->>Reconciler: candidate sources
Reconciler->>ArtifactsAPI: resolve default-branch HEAD per source
Reconciler->>Kody: publishFromExternalRef (for new commits)
Reconciler->>DB: update last_external_check_at
alt 03:00–03:04 UTC
Reconciler->>ArtifactsAPI: listTokens / revokeToken for expired tokens
end
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 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. Review rate limit: 0/1 reviews remaining, refill in 50 minutes and 19 seconds.Comment |
|
🔎 Preview deployed: https://kody-pr-352.kentcdodds.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
packages/worker/src/repo/artifacts.ts (1)
636-657: ⚡ Quick win
resolveArtifactSourceHeadduplicatesresolveArtifactDefaultBranchHeadand drops the|| 'main'fallback
resolveArtifactSourceHeadreinvents whatresolveArtifactDefaultBranchHead(lines 520–543) already does: fetch repo info → mint a short-lived read token → list server refs for the default branch → return the OID. The duplication introduces a silent divergence:resolveArtifactDefaultBranchHeadguards against an emptydefaultBranchwith|| 'main', whileresolveArtifactSourceHeadusesinfo.defaultBranchbare. If the Artifacts API ever returns""fordefault_branch, the prefix becomesrefs/heads/(matches every branch) and thefindpredicateentry.ref === "refs/heads/"never matches, returningcommit: null— silently treating the source as already published and stalling reconciliation.Delegate to the existing helper:
♻️ Proposed refactor
export async function resolveArtifactSourceHead(env: Env, repoId: string) { const repo = await resolveArtifactSourceRepo(env, repoId) - const info = await repo.info() - if (!info?.remote) { - throw new Error('Artifact repo remote URL is unavailable.') - } - const token = await repo.createToken('read', 300) - const auth = buildArtifactsGitAuth({ token: token.plaintext }) - const refs = await git.listServerRefs({ - http, - url: info.remote, - prefix: `refs/heads/${info.defaultBranch}`, - onAuth: () => auth, - }) - const ref = refs.find( - (entry) => entry.ref === `refs/heads/${info.defaultBranch}`, - ) - return { - branch: info.defaultBranch, - commit: ref?.oid ?? null, - } + const result = await resolveArtifactDefaultBranchHead({ repo }) + return { + branch: result?.defaultBranch ?? 'main', + commit: result?.commit ?? null, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/repo/artifacts.ts` around lines 636 - 657, resolveArtifactSourceHead duplicates resolveArtifactDefaultBranchHead but omits the defaultBranch fallback; update resolveArtifactSourceHead to delegate to resolveArtifactDefaultBranchHead(env, repoId) (or replicate its logic including the defaultBranch || 'main' fallback) instead of reimplementing the flow so the empty default_branch case uses 'main' and avoids the silent mismatch that yields commit: null; adjust any callers to use the returned { branch, commit } as before.packages/worker/src/mcp/capabilities/packages/get-git-remote.node.test.ts (1)
95-99: ⚡ Quick win
beforeEachflagged byprefer-dispose-in-tests— prefer disposable setupThe
epic-weblinter prefers moving per-test cleanup into each test body viausing/await usingwithdispose/disposeAsyncrather than a sharedbeforeEachblock. Since each mock in this file is already reset viaObject.values(mockModule), encapsulating that in a disposable is straightforward.♻️ Suggested refactor (inline disposable)
-beforeEach(() => { - for (const fn of Object.values(mockModule)) { - fn.mockReset() - } -}) - test('returns a write remote token expiring within the requested ttl', async () => { + using _ = { + [Symbol.dispose]() { + for (const fn of Object.values(mockModule)) fn.mockReset() + }, + } const { createToken } = mockPackageSource()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/packages/get-git-remote.node.test.ts` around lines 95 - 99, Replace the shared beforeEach teardown with an inline disposable that resets mocks per test: remove the beforeEach block that iterates Object.values(mockModule).mockReset and instead, inside each test use using/await using to create a disposable (e.g., a small helper that captures Object.values(mockModule) and implements dispose/disposeAsync to call mockReset on each fn) so that mockModule mocks are reset automatically at test end; update tests to import/instantiate that disposable at the top of each test and rely on its dispose behavior instead of the global beforeEach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/use/packages.md`:
- Line 189: Docs show a hardcoded "main" in the git push example which can be
wrong for repos with other default branches; change the example to use the
default branch returned by package_get_git_remote instead of "main" (e.g.,
replace "HEAD:main" with "HEAD:<defaultBranch>" where <defaultBranch> is the
value from package_get_git_remote) and update the surrounding text to clarify
that <defaultBranch> should be taken from the package_get_git_remote response.
- Around line 200-205: The docs currently expose the internal column name
entity_sources.published_commit in the description of
package_publish_external_push; change the wording to avoid schema/implementation
leakage by replacing any direct mentions of entity_sources.published_commit with
a generic phrase such as "the published commit marker" or "the published commit
reference" and replace "D1/KV untouched" with "underlying storage/state is left
unchanged" (or similar neutral phrasing) so the doc conveys behavior without
surfacing column or storage engine internals.
In `@packages/worker/src/jobs/reconcile-artifacts-pushes.ts`:
- Around line 57-117: The catch block currently only increments errors and logs,
leaving lastExternalCheckAt unchanged which causes broken sources to be
re-listed; modify the catch in the loop (the try/catch inside
reconcile-artifacts-pushes that iterates sources) to call
updateEntitySource(input.env.APP_DB, { id: source.id, userId: source.user_id,
lastExternalCheckAt: now.toISOString() }) before continuing/after logging so
every source—success, skip, or error—advances the external reconcile cursor used
by listEntitySourcesForExternalReconcile; keep the existing error count/logging
behavior.
- Around line 22-24: minutesAgoIso currently uses Date.now() causing a different
clock than the job's injected time; change minutesAgoIso to accept a reference
Date or timestamp (e.g., baseNow: Date | number) and compute the cutoff from
that value instead of Date.now(), then update all callers in
reconcile-artifacts-pushes.ts (including the uses around the previous 41-43
region) to pass input.now ?? new Date() as the base; ensure signature and
callers reflect the new param so the job uses a single deterministic clock.
In `@packages/worker/src/mcp/capabilities/packages/get-git-remote.ts`:
- Around line 91-95: The setup_commands array builds git commands but misses
changing into the cloned repo: after the clone command in setup_commands (the
entry using shellQuote(cloneDirectory)), insert a cd into the clone directory
before running the subsequent commands so that `git remote add kody
${shellQuote(info.remote)}` and the push (the line referencing
info.defaultBranch) execute inside the cloned repo; update the setup_commands
sequence in get-git-remote.ts (the setup_commands variable) to include a `cd
${shellQuote(cloneDirectory)}` step between the clone and the remote/add/push
commands.
- Around line 33-45: The outputSchema currently marks authenticated_remote and
git_extra_header as secrets but leaves setup_commands unprotected, exposing
embedded Bearer tokens; update the call to markSecretInputFields so the secret
fields list includes "setup_commands" (alongside "authenticated_remote" and
"git_extra_header") to ensure the entire array is redacted, or alternatively
modify the code that builds setup_commands to substitute a placeholder token
(and keep only git_extra_header as the secret) — locate the outputSchema
definition and either add "setup_commands" to the secret list passed to
markSecretInputFields or change the command-generation logic to inject a
non-secret placeholder instead of the live gitExtraHeader value.
In `@packages/worker/src/repo/artifacts-tokens.ts`:
- Around line 19-25: The revokeToken DELETE can raise a 404 when a token was
already removed, causing reconcileArtifactsPushes to count the whole source as
an error; update revokeStaleArtifactsTokens to treat 404 as success by calling
repo.revokeToken with the option to treat404AsNull (or otherwise swallow only
404 errors) so a missing token is considered successfully revoked; reference the
revokeStaleArtifactsTokens function and repo.revokeToken to locate where to pass
treat404AsNull (or handle the 404) and ensure other errors still propagate.
In `@packages/worker/src/repo/external-publish.ts`:
- Around line 72-80: The refreshSavedPackageProjection call can throw and
currently bubbles up after published_commit is set; wrap the call to
refreshSavedPackageProjection in a try/catch that mirrors the other post-publish
mutation handling: catch any error, log a detailed error with context (include
input.source.id, input.source.entity_id/packageId, input.env and the
published_commit if available) using the existing logger (e.g.,
processLogger.error), do not rethrow so the publish does not fail, and
optionally emit a metric/trace for retrying—but do not change the
already-committed state flow.
In `@packages/worker/src/repo/repo-session-do.ts`:
- Around line 1435-1438: The current check comparing input.expectedHead to
input.newCommit is insufficient because callers pass the same resolved commit
for both; instead query the remote repository's actual HEAD for the branch being
published and compare that remote HEAD to input.expectedHead: if they differ,
throw the same error. In practice, inside the repo-session-do.ts operation where
input.expectedHead and input.newCommit are used, call the existing repo/session
method that reads the remote ref (e.g., getRemoteRef/getRefHead or the session's
fetch/ref-read helper) to obtain the current remote HEAD for the target branch,
then compare that value to input.expectedHead and throw the error if mismatched
before allowing the publish to proceed.
- Around line 1465-1476: The call to publishExternalRefSource is passing
repo-relative source.manifest_path and source.source_root (which point into the
cloned repo under /session) so the external-push helper reads the wrong
locations; resolve those paths against the session workspace before calling
publishExternalRefSource (the same way runChecks does) and pass the resolved
manifest path and source root instead of
source.manifest_path/source.source_root. Update the arguments to
publishExternalRefSource (call site in this file) to use the workspace-resolved
values (e.g., join this.workspace or input.workspace with source.manifest_path
and source.source_root) so publishExternalRefSource can read the correct files.
---
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/packages/get-git-remote.node.test.ts`:
- Around line 95-99: Replace the shared beforeEach teardown with an inline
disposable that resets mocks per test: remove the beforeEach block that iterates
Object.values(mockModule).mockReset and instead, inside each test use
using/await using to create a disposable (e.g., a small helper that captures
Object.values(mockModule) and implements dispose/disposeAsync to call mockReset
on each fn) so that mockModule mocks are reset automatically at test end; update
tests to import/instantiate that disposable at the top of each test and rely on
its dispose behavior instead of the global beforeEach.
In `@packages/worker/src/repo/artifacts.ts`:
- Around line 636-657: resolveArtifactSourceHead duplicates
resolveArtifactDefaultBranchHead but omits the defaultBranch fallback; update
resolveArtifactSourceHead to delegate to resolveArtifactDefaultBranchHead(env,
repoId) (or replicate its logic including the defaultBranch || 'main' fallback)
instead of reimplementing the flow so the empty default_branch case uses 'main'
and avoids the silent mismatch that yields commit: null; adjust any callers to
use the returned { branch, commit } as before.
🪄 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: 76b3c3a3-60c8-4181-a0bb-7656369c4f71
📒 Files selected for processing (23)
docs/contributing/architecture/data-storage.mddocs/contributing/packages-and-manifests.mddocs/use/packages.mdpackages/worker/migrations/0032-entity-sources-external-check.sqlpackages/worker/src/index.tspackages/worker/src/jobs/reconcile-artifacts-pushes.node.test.tspackages/worker/src/jobs/reconcile-artifacts-pushes.tspackages/worker/src/mcp/capabilities/packages/domain.tspackages/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/resolve-package-source.tspackages/worker/src/repo/artifacts-tokens.tspackages/worker/src/repo/artifacts.tspackages/worker/src/repo/entity-sources.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-service.tspackages/worker/src/repo/types.tspackages/worker/wrangler.jsonc
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/worker/src/repo/repo-session-do.node.test.ts (1)
95-100: ⚡ Quick winAdd a direct test for
expectedHeadmismatch inpublishFromExternalRef.Since this mock was introduced, add one case that sets
expectedHeadto a stale commit and asserts the method throws the HEAD-changed error. It will lock in the race-prevention behavior that this PR adds.Also applies to: 191-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/repo/repo-session-do.node.test.ts` around lines 95 - 100, Add a unit test that verifies publishFromExternalRef throws the "HEAD changed" error when expectedHead is stale: update the test suite that mocks resolveArtifactDefaultBranchHead (the vi.fn returning defaultBranch 'main' and commit 'commit-published-new') to include an additional test case that calls publishFromExternalRef with expectedHead set to an older commit (e.g., 'commit-stale') and assert the call rejects/throws the HEAD-changed error; place the assertion alongside the existing tests that reference resolveArtifactDefaultBranchHead so the race-prevention behavior is locked in.packages/worker/src/repo/external-publish.node.test.ts (1)
163-186: ⚡ Quick winAdd an
allowForcesuccess-path test for rewritten history.This segment validates the reject path (
isFastForward: false) but not the override path. A companion case withallowForce: trueshould assert publish succeeds and advancespublishedCommit, so force-publish behavior can’t regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/repo/external-publish.node.test.ts` around lines 163 - 186, Add a new test that mirrors the existing non-fast-forward rejection but sets allowForce: true when calling publishFromExternalRef; call publishFromExternalRef with isFastForward: false and allowForce: true and assert the result indicates a successful publish (e.g., status 'published' and published_commit 'commit-rewritten' with previous_commit 'commit-old'), and assert mockModule.runRepoChecks and mockModule.updateEntitySource were called (and updateEntitySource received the new commit). Target the same test helpers/fixtures used in the current test and reference publishFromExternalRef, runRepoChecks, and updateEntitySource when implementing the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/repo/artifacts-tokens.ts`:
- Around line 24-31: The code increments revoked after the try/catch, which
counts tokens that only raised a 404 as revoked; move the revoked += 1 so it
only runs on a successful revoke. Specifically, inside the try block immediately
after await repo.revokeToken(token.id) increment revoked, keep the catch logic
(if (!(error instanceof CloudflareApiError && error.status === 404)) throw
error) unchanged so 404s are ignored but not counted.
In `@packages/worker/src/repo/repo-session-do.ts`:
- Around line 452-471: The current isAncestorCommit implementation uses git.log
with a fixed depth (depth: 1000) which can miss true ancestors; replace the
log-based check with a proper git ancestry check (e.g., run git merge-base
--is-ancestor <ancestor> <descendant> or use the library equivalent) inside
isAncestorCommit so the command's exit status determines ancestry reliably;
apply the same replacement for the other occurrence of this logic (the similar
block referenced around the later isAncestor usage).
---
Nitpick comments:
In `@packages/worker/src/repo/external-publish.node.test.ts`:
- Around line 163-186: Add a new test that mirrors the existing non-fast-forward
rejection but sets allowForce: true when calling publishFromExternalRef; call
publishFromExternalRef with isFastForward: false and allowForce: true and assert
the result indicates a successful publish (e.g., status 'published' and
published_commit 'commit-rewritten' with previous_commit 'commit-old'), and
assert mockModule.runRepoChecks and mockModule.updateEntitySource were called
(and updateEntitySource received the new commit). Target the same test
helpers/fixtures used in the current test and reference publishFromExternalRef,
runRepoChecks, and updateEntitySource when implementing the assertions.
In `@packages/worker/src/repo/repo-session-do.node.test.ts`:
- Around line 95-100: Add a unit test that verifies publishFromExternalRef
throws the "HEAD changed" error when expectedHead is stale: update the test
suite that mocks resolveArtifactDefaultBranchHead (the vi.fn returning
defaultBranch 'main' and commit 'commit-published-new') to include an additional
test case that calls publishFromExternalRef with expectedHead set to an older
commit (e.g., 'commit-stale') and assert the call rejects/throws the
HEAD-changed error; place the assertion alongside the existing tests that
reference resolveArtifactDefaultBranchHead so the race-prevention behavior is
locked in.
🪄 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: 2c666155-3605-4d13-9693-594f2d757a0b
📒 Files selected for processing (11)
docs/use/packages.mdpackages/worker/src/jobs/reconcile-artifacts-pushes.node.test.tspackages/worker/src/jobs/reconcile-artifacts-pushes.tspackages/worker/src/mcp/capabilities/packages/get-git-remote.node.test.tspackages/worker/src/mcp/capabilities/packages/get-git-remote.tspackages/worker/src/repo/artifacts-tokens.tspackages/worker/src/repo/artifacts.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 skipped from review due to trivial changes (2)
- docs/use/packages.md
- packages/worker/src/jobs/reconcile-artifacts-pushes.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/worker/src/jobs/reconcile-artifacts-pushes.node.test.ts
- packages/worker/src/repo/external-publish.ts
- packages/worker/src/repo/artifacts.ts
- packages/worker/src/mcp/capabilities/packages/get-git-remote.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
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>
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 43a5195. Configure here.
| .bind(input.before, input.limit) | ||
| .all<Record<string, unknown>>() | ||
| return (results ?? []).map(mapEntitySourceRow) | ||
| } |
There was a problem hiding this comment.
Reconcile scans all entity sources without kind filter
Low Severity
listEntitySourcesForExternalReconcile selects from all entity_sources rows without filtering by entity_kind. Entity sources for non-package kinds (skill, app, job) will be picked up by the reconcile cron every cycle, each triggering an Artifacts HEAD resolution that may error out, wasting API calls and generating log noise.
Reviewed by Cursor Bugbot for commit 43a5195. Configure here.


Summary
package_get_git_remotefor short-lived Artifacts git credentials andpackage_publish_external_pushfor publishing pushed package HEADslast_external_check_at, and user/contributor docsValidation
npm run typechecknpm run lintnpm testSummary by CodeRabbit
New Features
Documentation
Chores
Database
Tests