Repository navigation
Fix first-publish auth for new repo-backed skills - #200
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 10 seconds. ⌛ 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 Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR introduces and threads a Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Job/Skill/App Handler
participant Service as ensureEntitySource
participant Artifacts as createArtifactsRepoIfMissing
participant Sync as syncArtifactSourceSnapshot
participant Session as RepoSession.bootstrapSource
Handler->>Service: ensureEntitySource(...)
Service->>Artifacts: createArtifactsRepoIfMissing()
alt Repo exists (ready)
Artifacts-->>Service: null
else Repo created (new)
Artifacts-->>Service: ArtifactBootstrapAccess{remote, token, defaultBranch, expiresAt}
end
Service-->>Handler: EnsuredEntitySource{...source, bootstrapAccess}
Handler->>Sync: syncArtifactSourceSnapshot({..., bootstrapAccess})
alt First-time publish (no published_commit)
Sync->>Session: bootstrapSource({..., bootstrapAccess})
Session-->>Sync: publishedCommit
else Already published
Sync->>Session: openSession(...) + applyEdits(...) + publishSession(...)
Session-->>Sync: publishedCommit
end
Sync-->>Handler: success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
kentcdodds
marked this pull request as ready for review
April 18, 2026 05:14
Contributor
|
🔎 Preview deployed: https://kody-pr-200.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Test fixtures have wrong year for epoch timestamp
- Updated the mismatched test fixture expiry timestamps to align with the epoch in the token.
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 03fabc3. Configure here.
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>
cursor
Bot
force-pushed
the
cursor/new-repo-backed-skill-creation-7b8f
branch
from
April 18, 2026 13:20
d1e8da6 to
e71b38d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/worker/src/mcp/skills/run-saved-skill.node.test.ts (1)
74-86: Make thereadFilemock fail on unexpected repo paths.Line 77 returns the skill module for every path other than
kody.json, so this regression test would still pass ifrunSavedSkillread the wrong entrypoint. Gate the mock onskill.tsand validate the read payload so the test proves the manifest-derived wiring.🧪 Proposed test tightening
- readFile: vi.fn(async ({ path }: { path: string }) => ({ - path, - content: - path === 'kody.json' - ? JSON.stringify({ - version: 1, - kind: 'skill', - title: 'Fresh skill', - description: 'Runs from repo', - entrypoint: 'skill.ts', - }) - : 'async () => ({ ok: true, repoBacked: true })', - })), + readFile: vi.fn( + async ({ + path, + sessionId, + userId, + }: { + path: string + sessionId: string + userId: string + }) => { + expect(sessionId).toBe('skill-runtime-skill-1-session') + expect(userId).toBe('user-1') + + if (path === 'kody.json') { + return { + path, + content: JSON.stringify({ + version: 1, + kind: 'skill', + title: 'Fresh skill', + description: 'Runs from repo', + entrypoint: 'skill.ts', + }), + } + } + + if (path === 'skill.ts') { + return { + path, + content: 'async () => ({ ok: true, repoBacked: true })', + } + } + + throw new Error(`Unexpected readFile path: ${path}`) + }, + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/skills/run-saved-skill.node.test.ts` around lines 74 - 86, The readFile mock in the test is too permissive and returns the skill module for any path other than 'kody.json', so change the mock in run-saved-skill.node.test.ts to only return the module string when the path equals 'skill.ts' (and continue returning the manifest JSON for 'kody.json'); for any other path have the mock throw or reject with a clear error to fail the test if runSavedSkill reads the wrong entrypoint, and validate the mock's input shape (i.e., ensure the call provides a { path: string } payload) so the test enforces manifest-derived wiring.packages/worker/src/repo/source-sync.node.test.ts (1)
204-273: Consider also asserting the returnedpublishedCommit.The new test verifies wiring of
bootstrapAccessand thatopenSessionis skipped, but discards the return value ofsyncArtifactSourceSnapshot. Asserting the returned commit ('commit-bootstrap-2') would protect against a regression where the function forwards the input but forgets to returnbootstrapResult.publishedCommit.Suggested addition
- await syncArtifactSourceSnapshot({ + const publishedCommit = await syncArtifactSourceSnapshot({ @@ - expect(sessionClient.bootstrapSource).toHaveBeenCalledWith({ + expect(publishedCommit).toBe('commit-bootstrap-2') + expect(sessionClient.bootstrapSource).toHaveBeenCalledWith({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/repo/source-sync.node.test.ts` around lines 204 - 273, The test for syncArtifactSourceSnapshot should also assert that the function returns the published commit from the bootstrap result; capture the result of calling syncArtifactSourceSnapshot and add an expectation that the returned publishedCommit equals 'commit-bootstrap-2' (since sessionClient.bootstrapSource returns publishedCommit: 'commit-bootstrap-2'), referencing the test's use of syncArtifactSourceSnapshot and sessionClient.bootstrapSource to locate where to add the assertion.packages/worker/src/mcp/capabilities/meta/meta-save-skill.node.test.ts (1)
146-153: Useexpect.objectContainingforensureEntitySourceassertion to avoid brittleness.The current strict
toHaveBeenCalledWithat lines 146-153 will fail if the handler passes any optional fields (e.g.,requirePersistence,id,manifestPath,repoId) that are defined in theensureEntitySourcefunction signature, even though the bootstrap-access propagation behavior under test remains unaffected. This inconsistency with thesyncArtifactSourceSnapshotassertion below (which already usesexpect.objectContaining) makes the test unnecessarily fragile.Suggested change
- expect(mockModule.ensureEntitySource).toHaveBeenCalledWith({ - db: {}, - env: { APP_DB: {} }, - userId: 'user-1', - entityKind: 'skill', - entityId: expect.any(String), - sourceRoot: '/', - }) + expect(mockModule.ensureEntitySource).toHaveBeenCalledWith( + expect.objectContaining({ + db: {}, + env: { APP_DB: {} }, + userId: 'user-1', + entityKind: 'skill', + entityId: expect.any(String), + sourceRoot: '/', + }), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/meta/meta-save-skill.node.test.ts` around lines 146 - 153, The test assertion for mockModule.ensureEntitySource is too strict; replace the exact toHaveBeenCalledWith check with a toHaveBeenCalledWith(expect.objectContaining(...)) so only the required props (db, env, userId, entityKind, entityId, sourceRoot) are asserted and optional fields (e.g., requirePersistence, id, manifestPath, repoId) can be present without breaking the test; update the assertion in meta-save-skill.node.test.ts referencing mockModule.ensureEntitySource and keep entityId as expect.any(String) inside the objectContaining.
🤖 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/repo-session-do.ts`:
- Around line 545-563: The code assumes any input.bootstrapAccess is usable;
change the logic in the section handling input.bootstrapAccess so it verifies
bootstrapAccess.expiresAt against the current time and only reuses the provided
token if not expired. If expiresAt is missing or in the past, call
resolveArtifactSourceRepo(this.env, source.repo_id) and
ensureArtifactRepoRemote({ repo: sourceRepo, scope: 'write' }) to obtain a fresh
sourceAccess and populate sourceInfo via sourceRepo.info(); otherwise reuse
input.bootstrapAccess for sourceAccess but still set sourceInfo (fetch via
resolveArtifactSourceRepo(...).info() if needed) so targetBranch selection
(input.bootstrapAccess?.defaultBranch ?? sourceInfo?.defaultBranch ??
defaultSessionBranch) works correctly. Ensure references to sourceAccess,
sourceInfo, input.bootstrapAccess, resolveArtifactSourceRepo, and
ensureArtifactRepoRemote are updated accordingly.
---
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/meta/meta-save-skill.node.test.ts`:
- Around line 146-153: The test assertion for mockModule.ensureEntitySource is
too strict; replace the exact toHaveBeenCalledWith check with a
toHaveBeenCalledWith(expect.objectContaining(...)) so only the required props
(db, env, userId, entityKind, entityId, sourceRoot) are asserted and optional
fields (e.g., requirePersistence, id, manifestPath, repoId) can be present
without breaking the test; update the assertion in meta-save-skill.node.test.ts
referencing mockModule.ensureEntitySource and keep entityId as
expect.any(String) inside the objectContaining.
In `@packages/worker/src/mcp/skills/run-saved-skill.node.test.ts`:
- Around line 74-86: The readFile mock in the test is too permissive and returns
the skill module for any path other than 'kody.json', so change the mock in
run-saved-skill.node.test.ts to only return the module string when the path
equals 'skill.ts' (and continue returning the manifest JSON for 'kody.json');
for any other path have the mock throw or reject with a clear error to fail the
test if runSavedSkill reads the wrong entrypoint, and validate the mock's input
shape (i.e., ensure the call provides a { path: string } payload) so the test
enforces manifest-derived wiring.
In `@packages/worker/src/repo/source-sync.node.test.ts`:
- Around line 204-273: The test for syncArtifactSourceSnapshot should also
assert that the function returns the published commit from the bootstrap result;
capture the result of calling syncArtifactSourceSnapshot and add an expectation
that the returned publishedCommit equals 'commit-bootstrap-2' (since
sessionClient.bootstrapSource returns publishedCommit: 'commit-bootstrap-2'),
referencing the test's use of syncArtifactSourceSnapshot and
sessionClient.bootstrapSource to locate where to add the assertion.
🪄 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
Run ID: 51e697d7-cfaa-4bfd-871b-f1cd668ab4ad
📒 Files selected for processing (13)
packages/worker/src/jobs/service.tspackages/worker/src/mcp/capabilities/apps/ui-save-app.tspackages/worker/src/mcp/capabilities/meta/meta-save-skill.node.test.tspackages/worker/src/mcp/capabilities/meta/meta-save-skill.tspackages/worker/src/mcp/skills/run-saved-skill.node.test.tspackages/worker/src/repo/artifacts.tspackages/worker/src/repo/repo-session-do.tspackages/worker/src/repo/repo-session-rpc.tspackages/worker/src/repo/source-backfill.tspackages/worker/src/repo/source-service.node.test.tspackages/worker/src/repo/source-service.tspackages/worker/src/repo/source-sync.node.test.tspackages/worker/src/repo/source-sync.ts
Comment on lines
+545
to
+563
| let sourceInfo: ArtifactRepoInfo | null = null | ||
| let sourceAccess: { remote: string; token: string } | ||
| if (input.bootstrapAccess) { | ||
| sourceAccess = { | ||
| remote: input.bootstrapAccess.remote, | ||
| token: input.bootstrapAccess.token, | ||
| } | ||
| } else { | ||
| const sourceRepo = await resolveArtifactSourceRepo(this.env, source.repo_id) | ||
| sourceInfo = await sourceRepo.info() | ||
| sourceAccess = await ensureArtifactRepoRemote({ | ||
| repo: sourceRepo, | ||
| scope: 'write', | ||
| }) | ||
| } | ||
| const targetBranch = | ||
| input.bootstrapAccess?.defaultBranch ?? | ||
| sourceInfo?.defaultBranch ?? | ||
| defaultSessionBranch |
There was a problem hiding this comment.
Respect bootstrapAccess.expiresAt before skipping fresh token minting.
Line 547 treats any provided bootstrap access as usable, but the new type carries expiresAt. If a first publish is retried or delayed past expiry, this bypasses ensureArtifactRepoRemote and fails the push with a stale token instead of falling back to a fresh write token.
🛡️ Proposed freshness guard
let sourceInfo: ArtifactRepoInfo | null = null
let sourceAccess: { remote: string; token: string }
- if (input.bootstrapAccess) {
+ const bootstrapAccessExpiresAt = input.bootstrapAccess
+ ? Date.parse(input.bootstrapAccess.expiresAt)
+ : Number.NaN
+ const activeBootstrapAccess =
+ input.bootstrapAccess &&
+ Number.isFinite(bootstrapAccessExpiresAt) &&
+ bootstrapAccessExpiresAt > Date.now() + 30_000
+ ? input.bootstrapAccess
+ : null
+ if (activeBootstrapAccess) {
sourceAccess = {
- remote: input.bootstrapAccess.remote,
- token: input.bootstrapAccess.token,
+ remote: activeBootstrapAccess.remote,
+ token: activeBootstrapAccess.token,
}
} else {
const sourceRepo = await resolveArtifactSourceRepo(this.env, source.repo_id)
sourceInfo = await sourceRepo.info()
sourceAccess = await ensureArtifactRepoRemote({
@@
}
const targetBranch =
- input.bootstrapAccess?.defaultBranch ??
+ activeBootstrapAccess?.defaultBranch ??
sourceInfo?.defaultBranch ??
defaultSessionBranch📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let sourceInfo: ArtifactRepoInfo | null = null | |
| let sourceAccess: { remote: string; token: string } | |
| if (input.bootstrapAccess) { | |
| sourceAccess = { | |
| remote: input.bootstrapAccess.remote, | |
| token: input.bootstrapAccess.token, | |
| } | |
| } else { | |
| const sourceRepo = await resolveArtifactSourceRepo(this.env, source.repo_id) | |
| sourceInfo = await sourceRepo.info() | |
| sourceAccess = await ensureArtifactRepoRemote({ | |
| repo: sourceRepo, | |
| scope: 'write', | |
| }) | |
| } | |
| const targetBranch = | |
| input.bootstrapAccess?.defaultBranch ?? | |
| sourceInfo?.defaultBranch ?? | |
| defaultSessionBranch | |
| let sourceInfo: ArtifactRepoInfo | null = null | |
| let sourceAccess: { remote: string; token: string } | |
| const bootstrapAccessExpiresAt = input.bootstrapAccess | |
| ? Date.parse(input.bootstrapAccess.expiresAt) | |
| : Number.NaN | |
| const activeBootstrapAccess = | |
| input.bootstrapAccess && | |
| Number.isFinite(bootstrapAccessExpiresAt) && | |
| bootstrapAccessExpiresAt > Date.now() + 30_000 | |
| ? input.bootstrapAccess | |
| : null | |
| if (activeBootstrapAccess) { | |
| sourceAccess = { | |
| remote: activeBootstrapAccess.remote, | |
| token: activeBootstrapAccess.token, | |
| } | |
| } else { | |
| const sourceRepo = await resolveArtifactSourceRepo(this.env, source.repo_id) | |
| sourceInfo = await sourceRepo.info() | |
| sourceAccess = await ensureArtifactRepoRemote({ | |
| repo: sourceRepo, | |
| scope: 'write', | |
| }) | |
| } | |
| const targetBranch = | |
| activeBootstrapAccess?.defaultBranch ?? | |
| sourceInfo?.defaultBranch ?? | |
| defaultSessionBranch |
🤖 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.ts` around lines 545 - 563, The code
assumes any input.bootstrapAccess is usable; change the logic in the section
handling input.bootstrapAccess so it verifies bootstrapAccess.expiresAt against
the current time and only reuses the provided token if not expired. If expiresAt
is missing or in the past, call resolveArtifactSourceRepo(this.env,
source.repo_id) and ensureArtifactRepoRemote({ repo: sourceRepo, scope: 'write'
}) to obtain a fresh sourceAccess and populate sourceInfo via sourceRepo.info();
otherwise reuse input.bootstrapAccess for sourceAccess but still set sourceInfo
(fetch via resolveArtifactSourceRepo(...).info() if needed) so targetBranch
selection (input.bootstrapAccess?.defaultBranch ?? sourceInfo?.defaultBranch ??
defaultSessionBranch) works correctly. Ensure references to sourceAccess,
sourceInfo, input.bootstrapAccess, resolveArtifactSourceRepo, and
ensureArtifactRepoRemote are updated accordingly.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This was referenced Apr 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
meta_save_skill, and immediate repo-backedrunSavedSkillexecutionTesting
npm run test -- --run packages/worker/src/repo/source-service.node.test.ts packages/worker/src/repo/source-sync.node.test.ts packages/worker/src/mcp/capabilities/meta/meta-save-skill.node.test.ts packages/worker/src/mcp/skills/run-saved-skill.node.test.tsnpx vitest run --project node-unit --typecheck packages/worker/src/repo/source-service.node.test.ts packages/worker/src/repo/source-sync.node.test.ts packages/worker/src/mcp/capabilities/meta/meta-save-skill.node.test.ts packages/worker/src/mcp/skills/run-saved-skill.node.test.tsnpx oxlint packages/worker/src/jobs/service.ts packages/worker/src/mcp/capabilities/apps/ui-save-app.ts packages/worker/src/mcp/capabilities/meta/meta-save-skill.ts packages/worker/src/repo/artifacts.ts packages/worker/src/repo/repo-session-do.ts packages/worker/src/repo/repo-session-rpc.ts packages/worker/src/repo/source-backfill.ts packages/worker/src/repo/source-service.ts packages/worker/src/repo/source-sync.ts packages/worker/src/repo/source-service.node.test.ts packages/worker/src/repo/source-sync.node.test.ts packages/worker/src/mcp/capabilities/meta/meta-save-skill.node.test.ts packages/worker/src/mcp/skills/run-saved-skill.node.test.tsnpm run typecheckSummary by CodeRabbit
New Features
Tests