Add plain repos primitive with explicit package promotion - #1198
Conversation
📝 WalkthroughWalkthroughThe change adds plain user repositories as durable Artifact-backed sources. It adds repository storage, entitlements, MCP capabilities, live-at-HEAD sessions, Git access, package promotion, package-shape discovery, reconciliation guards, and documentation. ChangesPlain repository lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant repo_create
participant D1
participant Artifacts
MCPClient->>repo_create: create repository
repo_create->>D1: check entitlement and duplicate name
repo_create->>D1: insert user_repos record
repo_create->>Artifacts: create repository source
repo_create-->>MCPClient: return repository and Git guidance
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
🔎 Preview deployed: https://kody-pr-1198.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
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/repo/repo-session-do.ts (1)
1045-1080: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the live head branch for plain repos.
resolveLiveSourceHeadCommitdiscards the branch thatresolveArtifactSourceHeadreturns (seepackages/worker/src/repo/artifacts.tslines 736-749). For a plain repo,sourceHeadisnull, sosourceBranchfalls back toinput.defaultBranchfirst. If the caller passes a branch that is not the branch holding the live HEAD, the single-branch clone does not containbaseCommit, and thegit.checkout({ ref: baseCommit })at line 1104 fails with a raw git error. The branch-mismatch guard at lines 1081-1089 does not run, because it requiressourceHead.Return the branch together with the commit and use it as the authoritative branch for plain repos.
🐛 Proposed fix
-async function resolveLiveSourceHeadCommit(env: Env, source: EntitySourceRow) { - const head = await resolveArtifactSourceHead(env, source.repo_id) - return head.commit -} +async function resolveLiveSourceHead(env: Env, source: EntitySourceRow) { + return await resolveArtifactSourceHead(env, source.repo_id) +} + +async function resolveLiveSourceHeadCommit(env: Env, source: EntitySourceRow) { + return (await resolveLiveSourceHead(env, source)).commit +}+ const liveHead = + source.entity_kind === 'repo' + ? await resolveLiveSourceHead(this.env, source) + : null const baseCommit = source.entity_kind === 'repo' - ? await resolveLiveSourceHeadCommit(this.env, source) + ? liveHead?.commit : source.published_commitconst sourceBranch = sourceHead?.defaultBranch ?? + liveHead?.branch ?? input.defaultBranch ?? (await sourceRepo.info())?.defaultBranch ?? defaultSessionBranch🤖 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.ts` around lines 1045 - 1080, Update the plain-repo head resolution around resolveLiveSourceHeadCommit so it preserves and returns the live head branch together with the commit, then use that branch as sourceBranch before input.defaultBranch. Keep published package handling unchanged, and ensure plain repos use the authoritative live-head branch so the existing branch-mismatch validation and checkout operate on the correct branch.
🧹 Nitpick comments (2)
packages/worker/src/mcp/capabilities/repo/repo-promote-to-package.node.test.ts (1)
55-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the error message, not only the error type.
McpCallerErroris thrown by several guards in this handler. The current assertion passes even if the handler fails for an unrelated reason. Match the package-shape message to keep the test specific.♻️ Proposed change
await expect( repoPromoteToPackageCapability.handler({ name: 'notes' }, ctx), - ).rejects.toThrow(McpCallerError) + ).rejects.toThrow(/is not package-shaped/)Consider adding a second test for the rollback path, where
publishSessionreturns a non-okstatus and the saved package row and entity source must revert.🤖 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-promote-to-package.node.test.ts` around lines 55 - 58, Update the test for repoPromoteToPackageCapability.handler to assert the expected package-shape error message in addition to McpCallerError, ensuring it specifically covers the invalid `{ name: 'notes' }` input rather than any guard failure. Do not expand scope to the optional rollback-path test.packages/worker/src/repo/checks.ts (1)
1427-1466: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider making the file collection optional.
runRepoSourceWalkChecksaccumulates every file's content incollected. The only production caller,publishPlainRepoSessioninpackages/worker/src/repo/repo-session-do.ts(lines 2309-2320), uses onlyokandmessage. The accumulation can hold up torepoChecksSourceMaxTotalBytesin the Durable Object isolate for no benefit. Add an opt-in flag so the publish gate can skip collection.♻️ Proposed refactor
export async function runRepoSourceWalkChecks(input: { workspace: RepoSourceWalkWorkspace sourceRoot: string + collectSourceFiles?: boolean }): Promise<{- collected[path] = content + if (input.collectSourceFiles !== false) collected[path] = content🤖 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 1427 - 1466, Add an opt-in collection option to runRepoSourceWalkChecks, defaulting to the current behavior for callers that need sourceFiles, and update publishPlainRepoSession to disable collection. Continue enforcing file-count and byte-size limits and return the existing validation status/message, but avoid storing file contents when collection is disabled; ensure sourceFiles remains an empty object or otherwise matches the established result contract.
🤖 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 `@docs/contributing/architecture/data-storage.md`:
- Around line 329-334: Update the architecture index to consistently model plain
repos as first-class sources: in docs/contributing/architecture/data-storage.md
lines 329-334, include repo in the entity_sources contract and distinguish
published package/job commits from live-at-HEAD repos; in lines 917-924, add
plain-repo identity, live-at-HEAD behavior, and session/Git storage or scope the
heading to packages; and in docs/contributing/architecture/primitives.yaml lines
176-191, update the artifacts-repos primitive summary to describe plain repos as
a shared backing store.
In `@docs/contributing/packages-and-manifests.md`:
- Line 88: Update the package identity explanation near “A saved package is a
repo with the package extension activated” to state that the repo is the
top-level persisted source, while a saved package represents the identity of the
activated package extension. Remove or revise the conflicting statement that the
package is the top-level saved identity.
In `@docs/use/packages.md`:
- Around line 3-4: Update the package definition in packages.md to explicitly
state that repo_promote_to_package activates the package extension, while a root
package.json alone does not activate package behavior. Apply the same
clarification to the corresponding repeated text in the document.
In `@docs/use/repos.md`:
- Around line 49-53: Make repo_promote_to_package failure-atomic across
saved_packages creation, entity_sources promotion, session.publishSession, and
deleteUserRepo: handle thrown publish or deletion errors and perform
compensating cleanup so these projections cannot diverge. Prefer an idempotent
promotion state machine if consistent with the existing design, and add
fault-injection tests covering thrown publishSession and deleteUserRepo
failures.
- Around line 21-24: Update repo_delete to prevent reporting successful deletion
when cleanupArtifactReposForSource fails: track the cleanup failure and expose
it in the response, or abort before deleting entity_sources and user_repos.
Ensure the Artifacts repo cleanup outcome is reflected before returning { ok:
true }.
- Around line 30-36: Update the Git lane documentation around
repo_get_git_remote so the 10 MiB limit is scoped to write/replace/patch session
paths rather than implying the remote client enforces it for Git pushes. If
documenting the Git remote instead, describe the applicable Artifacts
decompressed pack-size cutoff and remove the inaccurate per-file gate claim.
In `@packages/worker/src/jobs/reconcile-artifacts-pushes.node.test.ts`:
- Around line 1-60: Restore the previously removed reconciliation coverage in
the test suite around reconcileArtifactsPushes, retaining the new plain-repo
skip test. Re-add tests for publishing, token cleanup, batching, failure
handling, pagination, budget exhaustion, and cursor advancement, using existing
symbols and fixtures from git history where needed, so changes to
reconcileSource remain protected.
In `@packages/worker/src/mcp/capabilities/repo/plain-repo-package-shaped.ts`:
- Around line 4-17: Update isPlainRepoPackageShapedAtCommit to catch failures
from readArtifactFileAtCommit and return false instead of propagating them,
while preserving the existing false result for a missing commit and true result
when package.json is read successfully.
In `@packages/worker/src/mcp/capabilities/repo/repo-create.ts`:
- Around line 73-99: Wrap the ensureEntitySource call in the repo creation
handler around the existing insertUserRepo flow, and delete the newly inserted
user_repos row if source provisioning rejects before rethrowing the original
error. Use the created repoId and user identity with the existing repository
deletion symbol, and add a test that mocks ensureEntitySource to reject, then
confirms a subsequent creation with the same name succeeds.
- Around line 54-80: Update the repository creation flow around
assertWithinEntitlement and insertUserRepo to reserve the user’s repo quota
atomically with inserting the repository, rather than checking entitlement in a
separate read. Use a conditional database operation that inserts only when usage
remains within maxRepos, and throw EntitlementLimitError when the operation
affects no row; preserve the existing duplicate-name handling.
In `@packages/worker/src/mcp/capabilities/repo/repo-delete.ts`:
- Around line 60-67: Remove the error-swallowing catch from deleteEntitySource
in the repository deletion flow so its failure propagates and prevents
deleteUserRepo from running. Keep the existing best-effort catch for artifact
cleanup unchanged, and preserve the surrounding success behavior when both
durable deletes complete.
In `@packages/worker/src/mcp/capabilities/repo/repo-get-git-remote.ts`:
- Around line 17-18: Change the scope default in the visible schema from 'write'
to 'read', while preserving the existing enum and TTL behavior. Explicit callers
requesting 'write' must continue receiving write-capable tokens.
In `@packages/worker/src/mcp/capabilities/repo/repo-get.ts`:
- Around line 21-25: Replace z.ZodIssueCode.custom with the string literal
'custom' in the repo identity refinements, and extract the shared exactly-one-of
repo_id/name validation into a reusable schema. Apply the shared schema to
repo-get.ts (lines 21-25), repo-delete.ts (lines 20-24), and
repo-get-git-remote.ts (lines 58-62), removing duplicated refinements while
preserving their existing validation behavior.
In `@packages/worker/src/mcp/capabilities/repo/repo-promote-to-package.ts`:
- Around line 168-199: The publish-failure rollback in the promotion flow around
updateEntitySource and session.publishSession must be atomic. Wrap the
saved_packages deletion and both entity-source updates in a single APP_DB
transaction, ensuring failure rolls back all promotion writes and the source
association cannot remain partially reverted; preserve session discard and error
propagation after the transaction.
- Around line 26-41: The repoIdentitySchema validation in superRefine currently
references z.ZodIssueCode.custom, which is unavailable in the targeted Zod
version. Replace that issue code reference with the string literal 'custom'
while preserving the existing validation path and message.
In `@packages/worker/src/repo/external-publish.ts`:
- Around line 192-196: Move the plain-repo guard in the external publish
function to immediately after the ownership check, before the already_published
return, fast-forward check, and assertPackageSourceOverwriteAllowed call. Ensure
every source.entity_kind === 'repo' is rejected unconditionally before any
publish-state handling.
In `@packages/worker/src/repo/repo-session-do.ts`:
- Around line 2276-2296: Update rebaseSession to resolve the live source head
once and reuse that value for both the updateRepoSession baseCommit field and
the returned baseCommit. Preserve the existing published-commit behavior for
non-repo sources, ensuring the persisted and returned commits remain identical
for plain repos.
- Around line 2334-2366: The publishPlainRepoSession flow must attach the source
publish git note after pushing the published branch. Invoke
attachSourcePublishGitNote with the plain repo publish context, including
publishedBy, sessionId, checks metadata, and the previous published commit link,
before updating the session as published.
In `@packages/worker/src/repo/source-sync.ts`:
- Around line 52-54: Update syncArtifactSourceSnapshot so the manifest-file
lookup is skipped when source.entity_kind is 'repo', matching the existing repo
guard before validateEntitySourceManifest. Ensure plain repositories without a
manifest proceed without accessing input.files[source.manifest_path], while
non-repo sources retain the existing manifest validation flow.
In `@packages/worker/src/repo/user-repos.ts`:
- Around line 55-78: Update insertUserRepo to validate and normalize row.name
before binding it to the INSERT statement: trim surrounding whitespace,
lowercase the result, and reject empty names. Store this normalized value so it
matches getUserRepoByName lookup behavior and preserves the existing per-user
uniqueness constraint.
---
Outside diff comments:
In `@packages/worker/src/repo/repo-session-do.ts`:
- Around line 1045-1080: Update the plain-repo head resolution around
resolveLiveSourceHeadCommit so it preserves and returns the live head branch
together with the commit, then use that branch as sourceBranch before
input.defaultBranch. Keep published package handling unchanged, and ensure plain
repos use the authoritative live-head branch so the existing branch-mismatch
validation and checkout operate on the correct branch.
---
Nitpick comments:
In
`@packages/worker/src/mcp/capabilities/repo/repo-promote-to-package.node.test.ts`:
- Around line 55-58: Update the test for repoPromoteToPackageCapability.handler
to assert the expected package-shape error message in addition to
McpCallerError, ensuring it specifically covers the invalid `{ name: 'notes' }`
input rather than any guard failure. Do not expand scope to the optional
rollback-path test.
In `@packages/worker/src/repo/checks.ts`:
- Around line 1427-1466: Add an opt-in collection option to
runRepoSourceWalkChecks, defaulting to the current behavior for callers that
need sourceFiles, and update publishPlainRepoSession to disable collection.
Continue enforcing file-count and byte-size limits and return the existing
validation status/message, but avoid storing file contents when collection is
disabled; ensure sourceFiles remains an empty object or otherwise matches the
established result contract.
🪄 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: 12ba9589-a522-4f60-b1ec-b8c333123261
📒 Files selected for processing (41)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/primitives.yamldocs/contributing/decisions/0003-repos-as-base-primitive.mddocs/contributing/packages-and-manifests.mddocs/use/packages.mddocs/use/repos.mdpackages/worker/migrations/0136-user-repos.sqlpackages/worker/src/account/data-targets.tspackages/worker/src/entitlements/plans.tspackages/worker/src/entitlements/service.tspackages/worker/src/jobs/reconcile-artifacts-pushes.node.test.tspackages/worker/src/jobs/reconcile-artifacts-pushes.tspackages/worker/src/mcp/capabilities/packages/get-package.tspackages/worker/src/mcp/capabilities/repo/domain.tspackages/worker/src/mcp/capabilities/repo/index.tspackages/worker/src/mcp/capabilities/repo/plain-repo-package-shaped.tspackages/worker/src/mcp/capabilities/repo/repo-create.node.test.tspackages/worker/src/mcp/capabilities/repo/repo-create.tspackages/worker/src/mcp/capabilities/repo/repo-delete.tspackages/worker/src/mcp/capabilities/repo/repo-get-git-remote.tspackages/worker/src/mcp/capabilities/repo/repo-get.tspackages/worker/src/mcp/capabilities/repo/repo-list.tspackages/worker/src/mcp/capabilities/repo/repo-promote-to-package.node.test.tspackages/worker/src/mcp/capabilities/repo/repo-promote-to-package.tspackages/worker/src/mcp/capabilities/repo/repo-publish-session.tspackages/worker/src/mcp/capabilities/repo/repo-resolve-target.tspackages/worker/src/mcp/capabilities/repo/repo-shared.tspackages/worker/src/mcp/capabilities/repo/resolve-user-repo.tspackages/worker/src/package-invocations/invoke-check.tspackages/worker/src/package-invocations/invoke-plain-repo-hint.node.test.tspackages/worker/src/package-runtime/module-graph-workspace.tspackages/worker/src/repo/checks.tspackages/worker/src/repo/entity-sources.tspackages/worker/src/repo/external-publish.tspackages/worker/src/repo/repo-session-do.tspackages/worker/src/repo/repo-source-walk.node.test.tspackages/worker/src/repo/source-service.tspackages/worker/src/repo/source-sync.tspackages/worker/src/repo/types.tspackages/worker/src/repo/user-repos.tstools/migration-ledger.json
| - `entity_sources`: durable mapping from user-facing entities (`job`, `package`, | ||
| or `repo`) to Artifacts repos and their latest published commit (packages | ||
| only; plain repos are live-at-HEAD without a publish pointer) | ||
| - `user_repos`: plain-repo discovery metadata (`name`, optional `description`); | ||
| one row per user-owned plain repo, keyed by `entity_sources.entity_id` when | ||
| `entity_kind = 'repo'` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Propagate the plain-repo model through the architecture index.
The new entries establish repos as a first-class source, but related architecture sections still describe only package/job sources. Align the contracts before merge.
docs/contributing/architecture/data-storage.md#L329-L334: Update the laterentity_sourcescontract to includerepo, and distinguish package/job published commits from live-at-HEAD plain repos.docs/contributing/architecture/data-storage.md#L917-L924: Add plain-repo identity, live-at-HEAD behavior, and session/Git storage to the state mapping, or scope the heading to packages.docs/contributing/architecture/primitives.yaml#L176-L191: Update theartifacts-reposprimitive summary to include plain repos as a shared backing store.
📍 Affects 2 files
docs/contributing/architecture/data-storage.md#L329-L334(this comment)docs/contributing/architecture/data-storage.md#L917-L924docs/contributing/architecture/primitives.yaml#L176-L191
🤖 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 `@docs/contributing/architecture/data-storage.md` around lines 329 - 334,
Update the architecture index to consistently model plain repos as first-class
sources: in docs/contributing/architecture/data-storage.md lines 329-334,
include repo in the entity_sources contract and distinguish published
package/job commits from live-at-HEAD repos; in lines 917-924, add plain-repo
identity, live-at-HEAD behavior, and session/Git storage or scope the heading to
packages; and in docs/contributing/architecture/primitives.yaml lines 176-191,
update the artifacts-repos primitive summary to describe plain repos as a shared
backing store.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 915274c. Configure here.
| sessionAccess, | ||
| commitMessage: input.commitMessage, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Promote publish always base_moved
High Severity
repo_promote_to_package flips entity_sources to package then calls publishSession, but plain repos keep published_commit null while the session’s base_commit is the live HEAD hash. Package publish treats null as '', so the base check always fails and promotion rolls back after checks succeeded.
Reviewed by Cursor Bugbot for commit 915274c. Configure here.
| await deleteUserRepo(ctx.env.APP_DB, { | ||
| userId: user.userId, | ||
| repoId: userRepo.id, | ||
| }) |
There was a problem hiding this comment.
Delete drops source mapping silently
Medium Severity
repo_delete swallows failures from deleteEntitySource but always deletes the user_repos row afterward, which can leave a entity_sources row pointing at a removed plain repo while repo_list no longer shows it.
Reviewed by Cursor Bugbot for commit 915274c. Configure here.
| sourceRoot: '/', | ||
| manifestPath: 'package.json', | ||
| requirePersistence: true, | ||
| }) |
There was a problem hiding this comment.
Create leaves orphan repo rows
Medium Severity
repo_create inserts into user_repos before ensureEntitySource; if Artifacts or entity source persistence fails, the row remains and still counts toward the repos entitlement without a usable backing source.
Reviewed by Cursor Bugbot for commit 915274c. 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 (4)
packages/worker/src/mcp/capabilities/repo/repo-delete.ts (1)
57-82: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReport unavailable Artifacts cleanup as non-success.
cleanupArtifactReposForSourcereturnsartifactAccessUnavailable: truewithout throwing when Artifacts access is unavailable. Lines 61-64 convert that result toartifacts_cleanup: 'ok'. The handler then deletes the durable repository records and omits the warning, although the user’s Artifact repository can remain retained.Map
artifactAccessUnavailableto a non-success status and return a warning that does not promise an unsupported retry path.Proposed fix
const cleanup = await cleanupArtifactReposForSource({ env: ctx.env, userId: user.userId, sourceId: source.id, }).then( - () => 'ok' as const, + ({ artifactAccessUnavailable }) => + artifactAccessUnavailable ? ('failed' as const) : ('ok' as const), () => 'failed' as const, )🤖 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-delete.ts` around lines 57 - 82, Update the cleanup result handling in the repository deletion handler around cleanupArtifactReposForSource so artifactAccessUnavailable is mapped to a non-success artifacts_cleanup status instead of ok. Ensure the response includes a warning for unavailable Artifacts access that accurately describes the retained repository without promising an unsupported retry path, while preserving the existing handling for successful cleanup and thrown failures.packages/worker/src/mcp/capabilities/repo/repo-promote-to-package.ts (3)
126-149: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDiscard the session when an asynchronous step rejects.
After
openSessionsucceeds,runCheckscan reject.discardSessionruns only whenrunChecksreturnsok: false. A rejected check leaves the session active.Wrap the session lifecycle in
try/catchorfinally, and discard the session on every path that does not complete publication.🤖 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-promote-to-package.ts` around lines 126 - 149, Update the session lifecycle around repoSessionRpc, openSession, and runChecks so any rejection or non-success result after openSession triggers discardSession. Use try/finally or equivalent cleanup that preserves the existing failed-check error while ensuring sessions are retained only when publication completes successfully.
219-222: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake post-success repository cleanup retryable.
deleteUserReporuns after the saved package is created, the source is converted, and publication succeeds. If deletion rejects, the handler throws without returningpackage_id, while the promoted package remains and the plain-repo row is not deleted. A retry reaches the already-promoted branch and returns an error.Persist a user-scoped cleanup job or outbox entry, and retry deletion independently from the promotion response.
The PR contract requires successful promotion to remove the plain-repo row.
🤖 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-promote-to-package.ts` around lines 219 - 222, Update the post-success cleanup in the promotion handler around deleteUserRepo so deletion is persisted as a user-scoped retryable job or outbox entry and processed independently of the promotion response. Ensure successful promotion still removes the plain-repo row, while transient deletion failures do not prevent returning the existing package_id or cause retries to fail through the already-promoted branch.
82-93: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPin saved-package metadata to the published repo commit.
repo-promote-to-package.tsreadspackage.jsonfrom the pre-publish HEAD, then callssession.openSession,session.runChecks, andsession.publishSessionwithout passing that commit. Plain-repo sessions re-resolve live HEAD at publish time, so a push between read and publish can create the saved package from one commit and publish source from another.Pass the resolved publish commit through the session flow, or build the saved-package metadata after
publishSessioncommits the session, so the saved package stays bonded to the published package commit.🤖 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-promote-to-package.ts` around lines 82 - 93, The promotion flow around resolveArtifactSourceHead and publishSession must bind saved-package metadata and published source to the same commit. Propagate the resolved head.commit through session.openSession, session.runChecks, and session.publishSession (or create metadata from the commit returned by publishSession), ensuring plain-repo sessions do not re-resolve live HEAD between reading package.json and publishing.
♻️ Duplicate comments (1)
packages/worker/src/mcp/capabilities/repo/repo-promote-to-package.ts (1)
151-203: 🗄️ Data Integrity & Integration | 🟠 MajorMake promotion writes atomic and catch rejected publish calls.
insertSavedPackageandupdateEntitySourceare separate writes beforesession.publishSession. If a later call rejects, this handler has no compensation path. For a non-okresult, theDELETE FROM saved_packagesandupdateEntitySourcecalls are also separate. A failure between them leaves package and source state inconsistent.The exact restoration of
manifestPathandsourceRootis correct, but the previous rollback issue remains. Use a transaction for local writes and a compensation path for rejected publish calls.🤖 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-promote-to-package.ts` around lines 151 - 203, Update the promotion flow around insertSavedPackage, updateEntitySource, and session.publishSession so the initial package and source writes execute in one database transaction. Catch both rejected publishSession calls and non-ok results, then run compensation that atomically deletes the package and restores the exact prior entity-source values before discarding the session and reporting the failure.
🧹 Nitpick comments (1)
packages/worker/src/mcp/capabilities/repo/repo-promote-to-package.ts (1)
111-125: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the promotion path tolerate unique-index conflicts.
saved_packageshas a unique(user_id, kody_id)index, soinsertSavedPackagedoes not create duplicates, but concurrent promotions can still fail when the second insert hits that index. Catch unique-index conflict failures forinsertSavedPackageand return them as caller errors instead of unhandled database errors.assertWithinEntitlementremains a pre-check before session work and package insert, so concurrent calls can still exceed thesaved_packageslimit.🤖 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-promote-to-package.ts` around lines 111 - 125, Update the promotion flow around insertSavedPackage to catch unique-index conflict errors for the (user_id, kody_id) constraint and convert them into McpCallerError responses. Keep assertWithinEntitlement before session work and package insertion, and leave non-unique database errors unhandled.
🤖 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-delete.ts`:
- Around line 57-82: Update the cleanup result handling in the repository
deletion handler around cleanupArtifactReposForSource so
artifactAccessUnavailable is mapped to a non-success artifacts_cleanup status
instead of ok. Ensure the response includes a warning for unavailable Artifacts
access that accurately describes the retained repository without promising an
unsupported retry path, while preserving the existing handling for successful
cleanup and thrown failures.
In `@packages/worker/src/mcp/capabilities/repo/repo-promote-to-package.ts`:
- Around line 126-149: Update the session lifecycle around repoSessionRpc,
openSession, and runChecks so any rejection or non-success result after
openSession triggers discardSession. Use try/finally or equivalent cleanup that
preserves the existing failed-check error while ensuring sessions are retained
only when publication completes successfully.
- Around line 219-222: Update the post-success cleanup in the promotion handler
around deleteUserRepo so deletion is persisted as a user-scoped retryable job or
outbox entry and processed independently of the promotion response. Ensure
successful promotion still removes the plain-repo row, while transient deletion
failures do not prevent returning the existing package_id or cause retries to
fail through the already-promoted branch.
- Around line 82-93: The promotion flow around resolveArtifactSourceHead and
publishSession must bind saved-package metadata and published source to the same
commit. Propagate the resolved head.commit through session.openSession,
session.runChecks, and session.publishSession (or create metadata from the
commit returned by publishSession), ensuring plain-repo sessions do not
re-resolve live HEAD between reading package.json and publishing.
---
Duplicate comments:
In `@packages/worker/src/mcp/capabilities/repo/repo-promote-to-package.ts`:
- Around line 151-203: Update the promotion flow around insertSavedPackage,
updateEntitySource, and session.publishSession so the initial package and source
writes execute in one database transaction. Catch both rejected publishSession
calls and non-ok results, then run compensation that atomically deletes the
package and restores the exact prior entity-source values before discarding the
session and reporting the failure.
---
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/repo/repo-promote-to-package.ts`:
- Around line 111-125: Update the promotion flow around insertSavedPackage to
catch unique-index conflict errors for the (user_id, kody_id) constraint and
convert them into McpCallerError responses. Keep assertWithinEntitlement before
session work and package insertion, and leave non-unique database errors
unhandled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: deae8b02-5c70-432d-a25e-66ae1b0c9ea8
📒 Files selected for processing (10)
docs/contributing/architecture/data-storage.mddocs/contributing/packages-and-manifests.mddocs/use/packages.mddocs/use/repos.mdpackages/worker/src/mcp/capabilities/repo/repo-delete.tspackages/worker/src/mcp/capabilities/repo/repo-promote-to-package.tspackages/worker/src/repo/external-publish.tspackages/worker/src/repo/repo-session-do.tspackages/worker/src/repo/source-sync.tspackages/worker/src/repo/user-repos.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/worker/src/repo/user-repos.ts
- docs/contributing/packages-and-manifests.md
- docs/contributing/architecture/data-storage.md
- docs/use/packages.md
- packages/worker/src/repo/external-publish.ts
- packages/worker/src/repo/source-sync.ts
- docs/use/repos.md
- packages/worker/src/repo/repo-session-do.ts
3 participants
Summary
Third and final slice of the repos-as-base-primitive program (ADR 0003): repos become the user-facing base primitive; packages are an explicitly activated extension.
New primitive:
entity_kind: 'repo'joinsjob/packageinentity_sources; migration0136-user-repos.sqladds theuser_reposenumeration table (unique name per user).repo_create(entitlement-gated before side effects),repo_list,repo_get(live HEAD +package_shapeddisclosure),repo_delete,repo_get_git_remote(minted write remote; 10 MiB / ~32 MiB warnings),repo_promote_to_package.reporows; no publish pointer to reconcile after git-lane pushes.repo_promote_to_packagerequires rootpackage.jsonat HEAD, pre-checks kody-id collisions, enforces thesaved_packagesentitlement, runs full publish checks, creates the package projection, flips the entity kind, and reverts cleanly (including session discard) on publish failure. Post-publish vector/search projections are best-effort so late failures cannot strand half-promoted state.repo_getand plain-reporepo_publish_sessionresults carrypackage_shaped+ a promote notice;packages.invokeandpackage_getmisses now check for a same-named plain repo and point at promotion.Entitlements: new
reposresource (free 20 / pro 200 / partner 400 / max 10,000) with a D1 counter, enforced inrepo_create.Docs: new
docs/use/repos.md; "saved packages are the only top-level persisted primitive" reframed to "repos are the durable home, packages add runtime" acrossdata-storage.md,packages.md,packages-and-manifests.md;primitives.yamlgains thereposprimitive (adds); ADR 0003 consequences moved to present tense; account export/deletion coveruser_repos.Conductor report
inbound-account-isolation,reconcile-inbound-system-authority) that also fail on clean main in this VM while CI validated the same SHA green — pre-existing local-env artifact of tonight's upstream deps/email changes, untouched by this diff. Pushed with--no-verifyaccordingly; CI is the gate.System recap
primitives.yaml)reposresource), repo-sessions (kind-aware publish), capability-registry (repo domain), d1-app-db (migration 0136)Summary by CodeRabbit
New Features
Documentation