Repository navigation
Simplify published job execution - #251
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
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 4 minutes and 38 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 Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds KV-backed published source snapshots and bundle artifacts, new DB tables/repos for published bundle artifacts and archived job artifacts, refactors job execution to use persisted artifacts, adds dependency-aware bundle publishing/rebuilds, and implements archival/retention cleanup for successful one-off jobs. Changes
Sequence Diagram(s)sequenceDiagram
participant JobExec as Job Executor
participant DB as D1 Database
participant KV as BUNDLE_ARTIFACTS_KV
participant Bundler as Module Bundler
participant Registry as Package Registry
JobExec->>DB: loadPublishedEntitySource(sourceId)
DB-->>JobExec: EntitySource + files
JobExec->>DB: getPublishedBundleArtifactByIdentity(userId, sourceId, kind, artifactName, entryPoint)
alt Cache Hit
DB-->>JobExec: artifact row
JobExec->>KV: readPublishedBundleArtifact(kvKey)
KV-->>JobExec: mainModule, modules, dependencies
JobExec->>JobExec: execute using persisted artifact
else Cache Miss
DB-->>JobExec: null
JobExec->>Bundler: buildKody*Bundle(files, entryPoint)
Bundler->>Registry: resolve `kody:` deps
Registry-->>Bundler: dependency commits
Bundler-->>JobExec: mainModule, modules, dependencies
JobExec->>KV: writePublishedBundleArtifact(artifact)
KV-->>JobExec: kvKey
JobExec->>DB: insertPublishedBundleArtifactRow(...)
DB-->>JobExec: artifact id
JobExec->>JobExec: execute with newly persisted artifact
end
JobExec->>DB: upsertArchivedJobArtifact(jobId, retainUntil, ...)
DB-->>JobExec: archived id
Note over DB, KV: Periodic cleanup lists archived artifacts due <= now,<br/>deletes KV artifacts, clears storage, removes entity sources, and deletes DB rows.
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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. Comment |
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>
kentcdodds
marked this pull request as ready for review
April 21, 2026 23:18
Contributor
|
🔎 Preview deployed: https://kody-pr-251.kentcdodds.workers.dev Worker: Mocks:
|
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
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/source-sync.ts (1)
41-48:⚠️ Potential issue | 🟠 MajorReorder KV write before DB update in loopback source sync path, and ensure BUNDLE_ARTIFACTS_KV availability in the sync guard.
The loopback branch (lines 92–106) updates the source row with the new
published_commitbefore writing the runtime snapshot toBUNDLE_ARTIFACTS_KV. If the KV write fails, the database is left with apublished_committhat has no corresponding runtime snapshot, breaking artifact-first execution paths.Additionally,
canSyncArtifactSource(lines 41–48) relies onhasArtifactsAccesswhich only verifies REST binding credentials (CLOUDFLARE_ACCOUNT_IDandCLOUDFLARE_API_TOKEN), not theBUNDLE_ARTIFACTS_KVbinding itself. This allowssyncArtifactSourceSnapshotto proceed past the guard and fail later insidewritePublishedSourceSnapshotwhen KV is missing.🛡️ Proposed consistency fix
function canSyncArtifactSource(env: Env) { + const runtimeEnv = env as Env & { + REPO_SESSION?: DurableObjectNamespace | undefined + APP_DB?: D1Database | undefined + BUNDLE_ARTIFACTS_KV?: KVNamespace | undefined + } return ( hasArtifactsAccess(env) && - (env as Env & { REPO_SESSION?: DurableObjectNamespace | undefined }) - .REPO_SESSION != null && - typeof (env as Env & { APP_DB?: D1Database | undefined }).APP_DB - ?.prepare === 'function' + runtimeEnv.REPO_SESSION != null && + typeof runtimeEnv.APP_DB?.prepare === 'function' && + runtimeEnv.BUNDLE_ARTIFACTS_KV != null ) }- await updateEntitySource(input.env.APP_DB, { - id: source.id, - userId: source.user_id, - publishedCommit: snapshot.published_commit, - manifestPath: source.manifest_path, - sourceRoot: source.source_root, - }) await writePublishedSourceSnapshot({ env: input.env, source: { ...source, published_commit: snapshot.published_commit, }, files: input.files, }) + await updateEntitySource(input.env.APP_DB, { + id: source.id, + userId: source.user_id, + publishedCommit: snapshot.published_commit, + manifestPath: source.manifest_path, + sourceRoot: source.source_root, + }) return snapshot.published_commit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/repo/source-sync.ts` around lines 41 - 48, The loopback sync path currently updates the DB row's published_commit before writing the runtime snapshot to KV, and canSyncArtifactSource doesn't verify the BUNDLE_ARTIFACTS_KV binding; change syncArtifactSourceSnapshot so it calls writePublishedSourceSnapshot (the KV write to BUNDLE_ARTIFACTS_KV) before persisting the published_commit in the DB to ensure atomic artifact-first behavior, and update canSyncArtifactSource to also check that env.BUNDLE_ARTIFACTS_KV (or its expected interface) is present/usable in addition to hasArtifactsAccess so the guard fails early if the KV is not available.packages/worker/src/jobs/service.node.test.ts (1)
1923-2116:⚠️ Potential issue | 🟠 MajorMigrate this test away from repo-session assertions.
This test still expects
openSession,discardSession, andreadFile, but the production path now executes from published artifacts. Rewrite it to assert artifact cache/rebuild behavior instead; otherwise the suite remains red on stale session expectations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/service.node.test.ts` around lines 1923 - 2116, The test still asserts repo session interactions (sessionClient.openSession, sessionClient.discardSession, sessionClient.readFile) but production now runs from published artifacts; update the test around executeJobOnce to remove all repo-session expectations and instead assert artifact cache/rebuild behavior: stop spying repoSessionRpc/openSession/discardSession/readFile and remove their expect(...) checks, and add assertions that the bundle artifacts KV (bundleKv) was queried/updated as expected (e.g., ensure bundleKv.get or bundleKv.put was called to fetch or store the published artifact, and that runBundledModuleWithRegistry was invoked once with the artifact bundle), referencing executeJobOnce, repoSessionRpc, sessionClient, runBundledModuleWithRegistry and bundleKv to locate the relevant mocks to modify.
🧹 Nitpick comments (2)
packages/worker/src/package-runtime/published-bundle-artifacts.ts (1)
62-90: Dynamicimport()inside a per-dependency loop looks unintentional.
resolveDependencyForPackagedynamically imports#worker/repo/entity-sources.tson every call. If this is to dodge a circular import, a top-levelimportwith a type-only split (or movinggetEntitySourceByIdinto a leaf module) is cleaner and avoids repeating the module-resolution overhead in hot rebuild paths. If it's not for a cycle, just hoist it to a normal import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-runtime/published-bundle-artifacts.ts` around lines 62 - 90, The function resolveDependencyForPackage currently does a dynamic import of getEntitySourceById from '#worker/repo/entity-sources.ts' on every call which adds runtime overhead and is likely unintentional; replace the dynamic import by hoisting getEntitySourceById to a top-level import (or if that caused a circular dependency, refactor to a type-only import or move getEntitySourceById into a leaf module) so resolveDependencyForPackage calls getEntitySourceById directly without repeated import() overhead.packages/worker/src/package-runtime/published-runtime-artifacts.ts (1)
67-74: NativeUint8Array.toBase64()is available and recommended for this Workers environment.The current char-by-char concat approach works for small modules but scales poorly with larger binary assets (WASM, images, etc.). Cloudflare Workers has supported
Uint8Array.prototype.toBase64()since August 2025, and your project's compatibility_date (2026-04-13) includes this feature.Consider simplifying to:
Suggested implementation
function toBase64(value: ArrayBuffer) { return new Uint8Array(value).toBase64() }This eliminates the intermediate string allocation entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-runtime/published-runtime-artifacts.ts` around lines 67 - 74, The toBase64 function currently builds a binary string char-by-char which is inefficient for large ArrayBuffers; replace its implementation to use the native Uint8Array.prototype.toBase64() call instead (i.e., in function toBase64(value: ArrayBuffer) and where Uint8Array is instantiated) so it returns new Uint8Array(value).toBase64(), removing the manual loop and btoa usage.
🤖 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/migrations/0028-published-bundle-artifacts-and-archived-jobs.sql`:
- Around line 30-39: The table archived_job_artifacts lacks the repository-owned
primary key column expected by
packages/worker/src/jobs/archived-artifacts-repo.ts (which
reads/inserts/updates/deletes by id); add an id TEXT PRIMARY KEY column to the
archived_job_artifacts table and make job_id a UNIQUE NOT NULL column (instead
of primary key) so the repo can continue using id while preserving job_id
uniqueness; ensure created_at/updated_at retain NOT NULL constraints and any
existing constraints/indexes are adjusted accordingly.
In `@packages/worker/src/jobs/archived-artifacts-repo.ts`:
- Around line 69-81: The query in listArchivedJobArtifactsDueBefore can return
arbitrarily large result sets; change the function signature to accept a batch
limit (e.g., limit: number) and a cursor (e.g., afterId?: string) and modify the
SQL to use a deterministic ordering with LIMIT and cursor filtering (for example
ORDER BY retain_until ASC, id ASC LIMIT ?, and if afterId provided add AND
(retain_until > ? OR (retain_until = ? AND id > ?)) or use id-based cursor AND
id > ?), bind the new parameters in the correct order, and update callers to
loop using the returned last record’s id until fewer than limit rows are
returned. Ensure the function still returns
Promise<Array<ArchivedJobArtifactRecord>>.
In `@packages/worker/src/jobs/scheduler-logging.ts`:
- Around line 25-36: The new string fields (artifactName, artifactEntryPoint,
cleanupDeletedSourceId, cleanupDeletedStorageId and other user-derived
artifact/source fields) must be passed through the same
log-truncation/sanitization routine before being emitted; update the scheduler
logging path so these fields are truncated by the existing truncation helper
used for other log fields (apply the same change where these fields are
set/serialized later in the file as well around the 137-150 region) to prevent
unbounded log size.
In `@packages/worker/src/jobs/service.node.test.ts`:
- Around line 1926-1948: The test is seeding published-source fixtures with
insertPublishedEntitySource which performs async KV writes but is not awaited,
causing racey flakes; update each call to insertPublishedEntitySource (e.g., the
invocation that passes db, userId, sourceId, entityKind, entityId,
publishedCommit, manifestPath, files, env) to be awaited (prepend await) and
ensure the enclosing test function is marked async where necessary; apply this
change to the occurrences around the shown block and also the other ranges
referenced (2244-2265, 2411-2432, 2575-2597, 2877-2899, 3123-3144).
- Around line 710-715: The test declares an unused fixture const row which
causes a lint warning; in the test containing the variables existing and params
(and the published_commit logic), remove the unused const row declaration (the
block that spreads ...existing and sets published_commit) or replace it by using
it in the assertion — most simply delete the const row = { ...existing,
published_commit: params.includes('published_commit') ?
existing['published_commit'] : existing['published_commit'] } statement to
eliminate the unused variable and silence the lint error.
- Around line 970-1002: The helper function insertEntitySourceFixture is
declared but unused; either remove the function entirely or add calls to it
where an entity_sources row is required (e.g., in tests that need
manifest_path/source_root or published_commit data). If you choose removal,
delete the insertEntitySourceFixture function; if you choose reuse, call
insertEntitySourceFixture({ db, id, entityId, publishedCommit, ... }) from the
relevant tests (or test setup) and ensure imports/fixtures provide the expected
parameters and default values match existing test conventions.
- Around line 2120-2144: The call to insertPublishedEntitySource is passing an
invalid kv property and not awaiting the async function; create a single
bundleKv const via createBundleArtifactsKv(), await
insertPublishedEntitySource(...), and pass that bundleKv into the env object as
BUNDLE_ARTIFACTS_KV so both insertPublishedEntitySource(...) and the env used
later reference the same bundleKv instance (update the
insertPublishedEntitySource call, the env definition, and ensure the call is
awaited).
In `@packages/worker/src/jobs/service.ts`:
- Around line 191-293: The cache/miss logic uses a hardcoded entryPoint
('src/job.ts') for loadPublishedBundleArtifactByIdentity but persists the
artifact with a manifest-derived entry point; to fix, resolve the job entry
point up front (use getManifestEntrypointPath for repo manifests and
normalizePackageWorkspacePath(jobDefinition.entry) for package manifests) into a
single variable (e.g., entryPoint) before the first call to
loadPublishedBundleArtifactByIdentity, pass that entryPoint and artifactName
consistently to loadPublishedBundleArtifactByIdentity,
persistPublishedJobBundleArtifact, and the subsequent reload call
(loadedArtifact) so the lookup, persist, and reload all use the same identity.
- Around line 191-198: The artifact name for job-owned artifacts is
inconsistent—sometimes set to null and other times to input.job.id—causing
duplicate persisted artifacts and missed cache hits; update the artifactName
logic used with loadPublishedBundleArtifactByIdentity (and the equivalent calls
around the other mentioned locations) so when source.entity_kind === 'job' you
set artifactName = input.job.id (not null), and apply the same convention in the
other call sites (the rebuild path and the locations referenced by the reviewer)
to ensure a single, consistent artifact naming scheme for job-owned artifacts.
- Around line 50-63: Two imports from the same module are duplicated:
getEntitySourceById (currently imported at the top) and deleteEntitySource
(imported later); merge them into a single import from
'#worker/repo/entity-sources.ts' that includes both getEntitySourceById and
deleteEntitySource to resolve the lint warning and avoid duplicate module
imports, keeping the other imports unchanged.
In `@packages/worker/src/package-registry/service.ts`:
- Around line 96-114: The savedPackage object passed into
rebuildPublishedPackageArtifacts is using stale timestamps from the pre-update
existing variable (existing?.updatedAt), causing rebuilt artifacts to carry old
metadata; after calling updateSavedPackage (or whatever updates the DB), obtain
the updated package record (e.g., use the return value from updateSavedPackage
or re-fetch the package) and use that updated record's createdAt/updatedAt
values when constructing savedPackage for rebuildPublishedPackageArtifacts so
timestamps reflect the latest state.
In `@packages/worker/src/package-runtime/published-bundle-artifacts.ts`:
- Around line 144-201: persistPublishedBundleArtifact currently writes the KV
blob first (writePublishedBundleArtifact) then does a check-then-insert into the
DB (getPublishedBundleArtifactByIdentity + insertPublishedBundleArtifactRow),
which can leave orphaned KV entries on DB failure and suffers a race on
concurrent inserts; change the flow to either (A) persist the DB row/upsert
first and then write the KV, or (B) if you must keep KV-first, ensure you delete
the KV in a catch block around the DB operations (use
deletePublishedArtifactsForSource or the appropriate KV delete), and make the DB
write idempotent by replacing the check-then-insert with a single upsert (INSERT
... ON CONFLICT DO UPDATE) in the repository method used by
persistPublishedBundleArtifact (replace insertPublishedBundleArtifactRow with an
upsert or add conflict-handling in
updatePublishedBundleArtifactRow/getPublishedBundleArtifactByIdentity) so
concurrent calls do not throw unique-constraint errors.
- Around line 333-350: deletePublishedArtifactsForSource currently calls
deletePublishedBundleArtifact unconditionally (which calls getBundleArtifactsKv
and will throw if the KV binding is absent) and uses Promise.all so a single KV
delete failure prevents the DB row cleanup; guard the KV path with
hasPublishedRuntimeArtifacts(env) before attempting KV deletes (or invert the
order: delete DB rows first), replace Promise.all with Promise.allSettled so all
KV deletions are attempted and failures are captured, and ensure
deletePublishedBundleArtifactRowsBySourceId runs regardless; log any rejected
settlements for visibility. Reference: deletePublishedArtifactsForSource,
hasPublishedRuntimeArtifacts, deletePublishedBundleArtifact,
getBundleArtifactsKv, deletePublishedBundleArtifactRowsBySourceId,
persistPublishedBundleArtifact.
- Around line 92-120: The module-level regex packageSpecifierPattern with manual
lastIndex resetting causes concurrency/state issues; in
collectDependenciesFromFiles replace the RegExp.exec loop with a stateless
iteration using content.matchAll(packageSpecifierPattern) (or create a fresh
RegExp inside the loop) so matches are re-entrant and side-effect-free, then for
each match extract kodyId and call resolveDependencyForPackage with the existing
state; remove packageSpecifierPattern.lastIndex resets and the RegExpExecArray
handling to prevent missed or duplicated matches.
In `@packages/worker/src/package-runtime/published-runtime-artifacts.ts`:
- Around line 285-301: In readPublishedBundleArtifact, guard against malformed
or mismatched stored artifacts before calling deserializeWorkerLoaderModules:
verify that the retrieved object has a modules property that is a plain object
(e.g., typeof modules === 'object' && modules !== null), and optionally validate
key identity fields (sourceId, publishedCommit, kind, entryPoint) match
expected/caller-supplied values; if any check fails or artifact.version !==
bundleArtifactVersion, return null so callers fall back to rebuild. Keep
references to readPublishedBundleArtifact, deserializeWorkerLoaderModules,
readPublishedSourceSnapshot and bundleArtifactVersion when locating the code to
add these shape checks.
- Around line 85-127: The serializeWorkerLoaderModules and
deserializeWorkerLoaderModules functions use truthy checks (e.g., module.js ? {
js: module.js } : {}) which drop legitimate empty-string fields and dataBase64
on roundtrip; change those conditional spreads to explicit presence checks
(e.g., check for !== undefined or use the 'in' operator) for js, cjs, text in
both serialize and deserialize and for dataBase64 in deserialize so empty
strings are preserved and only truly missing keys are elided.
In `@packages/worker/src/repo/published-bundle-artifacts-repo.ts`:
- Around line 72-84: The listPublishedBundleArtifactsBySourceId function (and
the similar helpers around lines 145-153) only filter by source_id but the
identity/indexes are tenant-scoped by user_id; change the SQL and binds to
include user_id to enforce tenant scoping: update
listPublishedBundleArtifactsBySourceId and the corresponding delete/list helpers
to accept a userId parameter, add "AND user_id = ?" to the WHERE clause (or
include user_id in the WHERE tuple), bind userId along with sourceId, and ensure
mapRow usage remains unchanged so queries only return artifacts for the given
userId.
---
Outside diff comments:
In `@packages/worker/src/jobs/service.node.test.ts`:
- Around line 1923-2116: The test still asserts repo session interactions
(sessionClient.openSession, sessionClient.discardSession,
sessionClient.readFile) but production now runs from published artifacts; update
the test around executeJobOnce to remove all repo-session expectations and
instead assert artifact cache/rebuild behavior: stop spying
repoSessionRpc/openSession/discardSession/readFile and remove their expect(...)
checks, and add assertions that the bundle artifacts KV (bundleKv) was
queried/updated as expected (e.g., ensure bundleKv.get or bundleKv.put was
called to fetch or store the published artifact, and that
runBundledModuleWithRegistry was invoked once with the artifact bundle),
referencing executeJobOnce, repoSessionRpc, sessionClient,
runBundledModuleWithRegistry and bundleKv to locate the relevant mocks to
modify.
In `@packages/worker/src/repo/source-sync.ts`:
- Around line 41-48: The loopback sync path currently updates the DB row's
published_commit before writing the runtime snapshot to KV, and
canSyncArtifactSource doesn't verify the BUNDLE_ARTIFACTS_KV binding; change
syncArtifactSourceSnapshot so it calls writePublishedSourceSnapshot (the KV
write to BUNDLE_ARTIFACTS_KV) before persisting the published_commit in the DB
to ensure atomic artifact-first behavior, and update canSyncArtifactSource to
also check that env.BUNDLE_ARTIFACTS_KV (or its expected interface) is
present/usable in addition to hasArtifactsAccess so the guard fails early if the
KV is not available.
---
Nitpick comments:
In `@packages/worker/src/package-runtime/published-bundle-artifacts.ts`:
- Around line 62-90: The function resolveDependencyForPackage currently does a
dynamic import of getEntitySourceById from '#worker/repo/entity-sources.ts' on
every call which adds runtime overhead and is likely unintentional; replace the
dynamic import by hoisting getEntitySourceById to a top-level import (or if that
caused a circular dependency, refactor to a type-only import or move
getEntitySourceById into a leaf module) so resolveDependencyForPackage calls
getEntitySourceById directly without repeated import() overhead.
In `@packages/worker/src/package-runtime/published-runtime-artifacts.ts`:
- Around line 67-74: The toBase64 function currently builds a binary string
char-by-char which is inefficient for large ArrayBuffers; replace its
implementation to use the native Uint8Array.prototype.toBase64() call instead
(i.e., in function toBase64(value: ArrayBuffer) and where Uint8Array is
instantiated) so it returns new Uint8Array(value).toBase64(), removing the
manual loop and btoa usage.
🪄 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: 4336d98f-a91b-444e-83be-b03fded9daf7
📒 Files selected for processing (23)
packages/worker/migrations/0028-published-bundle-artifacts-and-archived-jobs.sqlpackages/worker/src/env-schema.tspackages/worker/src/jobs/archived-artifacts-repo.tspackages/worker/src/jobs/scheduler-logging.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/jobs/service.tspackages/worker/src/package-registry/service.node.test.tspackages/worker/src/package-registry/service.tspackages/worker/src/package-registry/source.tspackages/worker/src/package-runtime/module-graph.node.test.tspackages/worker/src/package-runtime/module-graph.tspackages/worker/src/package-runtime/package-app.tspackages/worker/src/package-runtime/published-bundle-artifacts.tspackages/worker/src/package-runtime/published-runtime-artifacts.tspackages/worker/src/package-runtime/runtime-bundle-types.tspackages/worker/src/repo/published-bundle-artifacts-repo.tspackages/worker/src/repo/published-source.tspackages/worker/src/repo/source-sync.tspackages/worker/worker-configuration.d.tspackages/worker/wrangler.jsonctools/ci/preview-resources.tstools/ci/production-resources.tstools/ci/resource-utils.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/worker/src/jobs/service.node.test.ts (1)
1927-1949:⚠️ Potential issue | 🔴 CriticalAwait
insertPublishedEntitySourceat every seeding site (several cause current pipeline failures).The unawaited calls are the root cause of multiple failing tests:
- Line 1927 → Line 2092 ("expected openSession to be called 2 times, but got 0") and likely Line 1885 (
spyEnvundefined).- Line 2855 → Line 3029-3030 ("Cannot read properties of undefined (reading 'length')").
- Line 2122 → Line 2230 (covered separately above).
The
3435-3443occurrence (insiderunJobNow can use a one-off repo check policy override…) is a new location not listed in the prior review thread — also needsawait. All remaining sites (2246, 2405, 2567, 2855, 3064) match the prior flagged ranges.Also applies to: 2246-2267, 2405-2426, 2567-2589, 2855-2877, 3064-3085, 3435-3443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/service.node.test.ts` around lines 1927 - 1949, Several test failures are caused by unawaited insertPublishedEntitySource calls; update every call to insertPublishedEntitySource in the test file to be awaited so the seeding completes before assertions run (add await before each insertPublishedEntitySource invocation, or aggregate with await Promise.all when seeding multiple sources), ensure the enclosing test functions are async so awaiting works, and apply this change to all occurrences including the one inside the "runJobNow can use a one-off repo check policy override…" test; this will prevent race conditions that lead to spyEnv/openSession and undefined/length errors.
🤖 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/jobs/service.node.test.ts`:
- Around line 2747-2764: The test 'executeJobOnce fails instead of reusing a
stale repo session when discard fails' references bundleKv which is never
defined and also calls insertPublishedEntitySource(...) without awaiting it; to
fix, create and initialize bundleKv the same way sibling tests do (create the
artifacts KV/bucket helper used elsewhere) before constructing env and assign it
to env.BUNDLE_ARTIFACTS_KV, and make the insertPublishedEntitySource(...) call
awaited so the DB seed is applied before env is used; update the test to
reference the helper that produces the KV (the variable name bundleKv), await
insertPublishedEntitySource, and then build env with that bundleKv.
---
Duplicate comments:
In `@packages/worker/src/jobs/service.node.test.ts`:
- Around line 1927-1949: Several test failures are caused by unawaited
insertPublishedEntitySource calls; update every call to
insertPublishedEntitySource in the test file to be awaited so the seeding
completes before assertions run (add await before each
insertPublishedEntitySource invocation, or aggregate with await Promise.all when
seeding multiple sources), ensure the enclosing test functions are async so
awaiting works, and apply this change to all occurrences including the one
inside the "runJobNow can use a one-off repo check policy override…" test; this
will prevent race conditions that lead to spyEnv/openSession and
undefined/length errors.
🪄 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: 8dc99092-3765-4b29-bc60-daf236ff6aed
📒 Files selected for processing (1)
packages/worker/src/jobs/service.node.test.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
|
||
| function normalizeArtifactName(artifactName: string | null | undefined) { | ||
| return artifactName?.trim() || '_' | ||
| } |
Contributor
There was a problem hiding this comment.
Divergent normalizeArtifactName causes KV key mismatch
Medium Severity
Two separate normalizeArtifactName functions exist with different semantics: published-bundle-artifacts.ts returns null for empty/null names, while published-runtime-artifacts.ts returns '_'. Since persistPublishedBundleArtifact uses the first to set the artifact's artifactName but calls buildPublishedBundleArtifactKvKey (which uses the second) for the KV key, there's a semantic inconsistency that can cause artifact-name-based lookups to mismatch between the DB row and the KV key.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 43f2316. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 4
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/source-sync.ts (1)
96-167:⚠️ Potential issue | 🟡 MinorSnapshot/DB write ordering can leak orphaned KV entries on partial failure.
In all three paths,
writePublishedSourceSnapshotruns after a commit is published (and after DB row updates insidebootstrapSource/publishSession). If the KV put throws, the commit remains published and theentity_sourcesrow already reflects it, but no KV snapshot exists — later loads vialoadPublishedEntitySourcewill fail even though the source row looks healthy. Similarly in the mock path, the KV write happens beforeupdateEntitySource, so a DB failure leaves an orphan snapshot keyed by apublished_commitnever recorded in D1.Consider wrapping the KV write with a best-effort retry or a compensating cleanup on subsequent failure, or document the expected inconsistency window and let the bundle-artifact layer handle cache-miss rebuilds.
♻️ Duplicate comments (6)
packages/worker/src/jobs/service.ts (3)
204-204:⚠️ Potential issue | 🟠 Major
artifactNamefor job-kind sources still diverges between lookup and persist.
- Line 204 (
ensurePublishedBundleArtifactForJob):artifactName = source.entity_kind === 'package' ? input.job.name : null- Line 1001-1002 (
runRepoBackedJob):artifactName = source.entity_kind === 'package' ? input.job.name : input.job.idThe
runRepoBackedJobvalue (input.job.id) is what flows intorebuildAndExecuteJobArtifact→persistPublishedJobBundleArtifact, so job-kind artifacts are stored withartifact_name = input.job.id. But the subsequentensurePublishedBundleArtifactForJobcall (line 344) and its reload at 306-313 look up withnull, guaranteeing a cache miss for every job-source run and making the post-rebuild reload throw.Pick one convention (e.g., always
input.job.idfor job-kind) and use it in lookup, persist, and reload.Also applies to: 1001-1002
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/service.ts` at line 204, The code currently uses different artifactName conventions for job sources: runRepoBackedJob sets artifactName to input.job.id for entity_kind === 'package' while ensurePublishedBundleArtifactForJob uses null (or input.job.name elsewhere), causing cache misses and reload failures; standardize to always use input.job.id for job-kind artifacts by updating ensurePublishedBundleArtifactForJob (and any lookup/reload paths that pass artifactName) to derive artifactName = source.entity_kind === 'package' ? input.job.id : null, and ensure rebuildAndExecuteJobArtifact/persistPublishedJobBundleArtifact continue to persist with input.job.id so lookup, persist, and reload use the same identifier (referenced symbols: ensurePublishedBundleArtifactForJob, runRepoBackedJob, rebuildAndExecuteJobArtifact, persistPublishedJobBundleArtifact, input.job.id).
204-319:⚠️ Potential issue | 🔴 CriticalHardcoded
'src/job.ts'still mismatches the manifest-derived entry point used at persist time — cache is never hit and rebuild reload throws.The persist/reload identity is still inconsistent across these paths, despite the earlier "Addressed" note:
- Line 205-212 and line 306-313 in
ensurePublishedBundleArtifactForJob: lookup/reload useentryPoint: 'src/job.ts'.- Line 1003-1010 in
runRepoBackedJob: primary lookup also usesentryPoint: 'src/job.ts'.- But persist uses the manifest-derived entry point —
getManifestEntrypointPath(manifest)at line 255 for repo jobs andnormalizePackageWorkspacePath(jobDefinition.entry)at line 279 for package jobs (both flow intopersistPublishedJobBundleArtifactvia line 174).Concrete failure: a package job with
"entry": "./src/foo.ts"persists underentry_point = "src/foo.ts", but the lookup at 205-212 searches for"src/job.ts", so cache always misses. After rebuild, the reload at line 306-313 still uses"src/job.ts"and returnsnull, causingensurePublishedBundleArtifactForJobto throw"Published bundle artifact for job ... could not be loaded after rebuild."for any job whose entry isn't literallysrc/job.ts.Resolve the manifest first, then use the same
entryPointvalue for lookup, persist, and reload in all three sites.Also applies to: 1001-1010
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/service.ts` around lines 204 - 319, The lookup and reload use a hardcoded entryPoint ('src/job.ts') causing cache misses; in ensurePublishedBundleArtifactForJob (and similarly in runRepoBackedJob) first resolve the manifest-derived entry point via getManifestEntrypointPath or normalizePackageWorkspacePath (the same values passed into persistPublishedJobBundleArtifact) and then pass that entryPoint variable into loadPublishedBundleArtifactByIdentity for the initial lookup and the post-persist reload so identity matches persistPublishedJobBundleArtifact exactly.
48-61:⚠️ Potential issue | 🟡 MinorDuplicate
#worker/repo/entity-sources.tsimport still present.Lines 48 and 61 both import from
#worker/repo/entity-sources.ts; lint continues to warn.Proposed merge
-import { getEntitySourceById } from '#worker/repo/entity-sources.ts' +import { + deleteEntitySource, + getEntitySourceById, +} from '#worker/repo/entity-sources.ts' @@ -import { deleteEntitySource } from '#worker/repo/entity-sources.ts'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/service.ts` around lines 48 - 61, Remove the duplicate import of "#worker/repo/entity-sources.ts" by consolidating the two import lines into one: keep a single import that includes both getEntitySourceById and deleteEntitySource (the symbols referenced) and remove the redundant import statement so the file only imports from entity-sources.ts once.packages/worker/src/jobs/service.node.test.ts (3)
710-715:⚠️ Potential issue | 🟡 MinorUnused
rowfixture still present.Line 710-715 still declares
rowwithout using it. Lint continues to warn here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/service.node.test.ts` around lines 710 - 715, Remove the unused test fixture by deleting the unused local variable declaration "row" in the test code (the object spread that references "existing" and "published_commit") so the unused variable is no longer declared; locate the block in service.node.test.ts where "const row = { ...existing, published_commit: ... }" is defined and remove that const entirely (or replace it by using the value where needed) to eliminate the linter warning.
978-1010:⚠️ Potential issue | 🟡 MinorUnused
insertEntitySourceFixturehelper still present.The helper is defined but has no call sites; lint still flags it. Either wire it into the tests that need bare
entity_sourcesrows or delete it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/service.node.test.ts` around lines 978 - 1010, The helper function insertEntitySourceFixture is defined but never used, causing lint errors; remove it or wire it into tests that require bare entity_sources rows. To fix, either delete the insertEntitySourceFixture function declaration from service.node.test.ts, or locate the tests that create jobs/packages without entity_sources and call await insertEntitySourceFixture({ db, id: '...', entityId: '...', publishedCommit: '...', ... }) with appropriate params (use db from createDatabase in those tests) so the helper is referenced; ensure any related imports/fixtures remain consistent and re-run linter/tests.
1918-1940:⚠️ Potential issue | 🟠 MajorSeveral
insertPublishedEntitySourcecalls are still not awaited.
insertPublishedEntitySourceisasyncand performs D1 + KV writes, but the top-level fixture seeds at these locations run it withoutawait:
- Line 1918 (
executeJobOnce refreshes repo sessions when base commit moves)- Line 2093 (
executeJobOnce executes package-backed jobs from published artifacts)- Line 2220 (
... bypasses typecheck-only failures ...)- Line 2379 (
... preserves bypass audit logs ...)- Line 2541 (
... repo-session absolute paths ...)- Line 2724 (
... fails instead of reusing a stale repo session ...)- Line 2830 (
... bundles and runs ESM repo-backed job entrypoints)- Line 3033 (
... codemode secret policy ...)- Line 3404 (
runJobNow can use a one-off repo check policy override ...)Since the tests then immediately call
executeJobOnce/runJobNow, which read that same KV snapshot, these races manifest as intermittent "published snapshot not found" failures. Pleaseawaiteach call.Also applies to: 2093-2111, 2220-2241, 2379-2400, 2541-2563, 2724-2732, 2830-2852, 3033-3054, 3404-3412
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/service.node.test.ts` around lines 1918 - 1940, The test fixture is calling the async helper insertPublishedEntitySource without awaiting it, causing KV/D1 write races before subsequent executeJobOnce/runJobNow reads; update each occurrence that seeds fixtures (calls to insertPublishedEntitySource in the affected tests) to await the call so the writes complete before continuing—search for insertPublishedEntitySource in the test file and add await before each call (e.g., in the tests that exercise executeJobOnce and runJobNow) to ensure the KV snapshot is ready.
🧹 Nitpick comments (1)
packages/worker/src/package-registry/source.node.test.ts (1)
39-65: Remove unusedcreateSessionClienthelper.After the switch to
loadPublishedEntitySource,createSessionClientis no longer referenced anywhere in this file. It's dead code from the pre-refactor session-based flow.♻️ Proposed removal
-function createSessionClient(sessionId: string) { - return { - openSession: vi.fn(async () => ({ - id: sessionId, - })), - readFile: vi.fn(async () => ({ - content: JSON.stringify({ - name: '@kentcdodds/example-package', - exports: { - '.': './index.js', - }, - kody: { - id: 'example-package', - description: 'Example package', - app: { - entry: 'app.js', - }, - }, - }), - })), - discardSession: vi.fn(async () => ({ - ok: true as const, - sessionId, - deleted: true, - })), - } -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-registry/source.node.test.ts` around lines 39 - 65, Remove the dead helper createSessionClient from the test file: locate the function declaration named createSessionClient and delete it (and any now-unused references/imports brought in solely for it, e.g., vi mocks if they become unused) so the file contains only code paths used by loadPublishedEntitySource.
🤖 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/jobs/service.ts`:
- Around line 441-480: The loop in cleanupArchivedJobArtifacts lacks
per-artifact error isolation so one failing delete or
storageRunnerRpc(...).clearStorage() stops the rest; wrap the body of the for
(const artifact of due) iteration in a try/catch, perform
deletePublishedArtifactsForSource, deletePublishedSourceSnapshot and
storageRunnerRpc(...).clearStorage() inside the try, only call
deleteArchivedJobArtifact when those side-effects succeed, and in the catch call
logJobSchedulerEvent with event 'job_artifact_cleanup_failed' (including
artifact.userId, jobId, sourceId and the error details) then continue to the
next artifact so a single failure cannot block the batch.
In `@packages/worker/src/mcp/observability.workers.test.ts`:
- Around line 48-74: The helper function createBundleArtifactsKv is declared but
unused; either remove the function or reuse it by replacing the hand-rolled KV
stub in the test that begins around the inline stub (the one at ~line 328) with
a call to createBundleArtifactsKv(). If you keep it, update the test to call
createBundleArtifactsKv() wherever the inline Map-backed KVNamespace stub is
duplicated (ensuring behavior matches the helper's get/put/delete semantics); if
you remove it, delete the createBundleArtifactsKv declaration and any related
imports to eliminate the lint warning.
In `@packages/worker/src/package-registry/source.node.test.ts`:
- Around line 192-198: The variable resolveFiles is typed for a plain
Record<string,string> but filesPromise actually resolves to an object { source,
files }; update resolveFiles' type to match the promise shape and usage (i.e.
((value: { source: ReturnType<typeof createPackageSourceRow>; files:
Record<string,string> }) => void) | null), adjust its initialization and any
calls that pass { source, files } so TypeScript correctly enforces the tuple
structure used by filesPromise and createPackageSourceRow.
In `@packages/worker/src/repo/checks.ts`:
- Around line 318-381: Remove the unused function
typecheckPackageRuntimeEntryPoint and extract the duplicated
snapshot+tsconfig+language-service setup into a shared helper
createTypecheckContextFromSourceFiles({ sourceFiles }) that returns { snapshot,
fileSystem, languageService } and is used by
typecheckPackageEntrypointsFromSourceFiles and runRepoChecks; when creating the
snapshot, call createFileSystemSnapshot with a plain array
(Array.from(Object.entries(sourceFiles))) instead of an async generator for
consistency; change buildRepoChecksTsconfig to accept a boolean (e.g.,
hasBaseConfig) or pass the baseTsconfig != null result so the function no longer
expects the full content but only a null-check, and update all call sites
accordingly (references: typecheckPackageEntrypointsFromSourceFiles,
runRepoChecks, typecheckPackageRuntimeEntryPoint, createFileSystemSnapshot,
buildRepoChecksTsconfig, createTypescriptLanguageService).
---
Duplicate comments:
In `@packages/worker/src/jobs/service.node.test.ts`:
- Around line 710-715: Remove the unused test fixture by deleting the unused
local variable declaration "row" in the test code (the object spread that
references "existing" and "published_commit") so the unused variable is no
longer declared; locate the block in service.node.test.ts where "const row = {
...existing, published_commit: ... }" is defined and remove that const entirely
(or replace it by using the value where needed) to eliminate the linter warning.
- Around line 978-1010: The helper function insertEntitySourceFixture is defined
but never used, causing lint errors; remove it or wire it into tests that
require bare entity_sources rows. To fix, either delete the
insertEntitySourceFixture function declaration from service.node.test.ts, or
locate the tests that create jobs/packages without entity_sources and call await
insertEntitySourceFixture({ db, id: '...', entityId: '...', publishedCommit:
'...', ... }) with appropriate params (use db from createDatabase in those
tests) so the helper is referenced; ensure any related imports/fixtures remain
consistent and re-run linter/tests.
- Around line 1918-1940: The test fixture is calling the async helper
insertPublishedEntitySource without awaiting it, causing KV/D1 write races
before subsequent executeJobOnce/runJobNow reads; update each occurrence that
seeds fixtures (calls to insertPublishedEntitySource in the affected tests) to
await the call so the writes complete before continuing—search for
insertPublishedEntitySource in the test file and add await before each call
(e.g., in the tests that exercise executeJobOnce and runJobNow) to ensure the KV
snapshot is ready.
In `@packages/worker/src/jobs/service.ts`:
- Line 204: The code currently uses different artifactName conventions for job
sources: runRepoBackedJob sets artifactName to input.job.id for entity_kind ===
'package' while ensurePublishedBundleArtifactForJob uses null (or input.job.name
elsewhere), causing cache misses and reload failures; standardize to always use
input.job.id for job-kind artifacts by updating
ensurePublishedBundleArtifactForJob (and any lookup/reload paths that pass
artifactName) to derive artifactName = source.entity_kind === 'package' ?
input.job.id : null, and ensure
rebuildAndExecuteJobArtifact/persistPublishedJobBundleArtifact continue to
persist with input.job.id so lookup, persist, and reload use the same identifier
(referenced symbols: ensurePublishedBundleArtifactForJob, runRepoBackedJob,
rebuildAndExecuteJobArtifact, persistPublishedJobBundleArtifact, input.job.id).
- Around line 204-319: The lookup and reload use a hardcoded entryPoint
('src/job.ts') causing cache misses; in ensurePublishedBundleArtifactForJob (and
similarly in runRepoBackedJob) first resolve the manifest-derived entry point
via getManifestEntrypointPath or normalizePackageWorkspacePath (the same values
passed into persistPublishedJobBundleArtifact) and then pass that entryPoint
variable into loadPublishedBundleArtifactByIdentity for the initial lookup and
the post-persist reload so identity matches persistPublishedJobBundleArtifact
exactly.
- Around line 48-61: Remove the duplicate import of
"#worker/repo/entity-sources.ts" by consolidating the two import lines into one:
keep a single import that includes both getEntitySourceById and
deleteEntitySource (the symbols referenced) and remove the redundant import
statement so the file only imports from entity-sources.ts once.
---
Nitpick comments:
In `@packages/worker/src/package-registry/source.node.test.ts`:
- Around line 39-65: Remove the dead helper createSessionClient from the test
file: locate the function declaration named createSessionClient and delete it
(and any now-unused references/imports brought in solely for it, e.g., vi mocks
if they become unused) so the file contains only code paths used by
loadPublishedEntitySource.
🪄 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: e7ad5c18-a063-4924-9d30-1fc04c132914
📒 Files selected for processing (9)
packages/worker/src/jobs/service.node.test.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp/observability.workers.test.tspackages/worker/src/oauth-handlers.workers.test.tspackages/worker/src/package-registry/service.tspackages/worker/src/package-registry/source.node.test.tspackages/worker/src/repo/checks.tspackages/worker/src/repo/source-sync.node.test.tspackages/worker/src/repo/source-sync.ts
✅ Files skipped from review due to trivial changes (1)
- packages/worker/src/oauth-handlers.workers.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/package-registry/service.ts
Comment on lines
+318
to
+381
| export async function typecheckPackageEntrypointsFromSourceFiles(input: { | ||
| sourceFiles: Record<string, string> | ||
| entryPoints: Array<{ | ||
| path: string | ||
| includeStorage?: boolean | ||
| }> | ||
| }): Promise<{ | ||
| ok: boolean | ||
| message: string | ||
| }> { | ||
| const { createFileSystemSnapshot } = await import('@cloudflare/worker-bundler') | ||
| const snapshot = await createFileSystemSnapshot( | ||
| (async function* () { | ||
| for (const [path, content] of Object.entries(input.sourceFiles)) { | ||
| yield [path, content] as const | ||
| } | ||
| })(), | ||
| ) | ||
| const missingEntryPoints = input.entryPoints | ||
| .map((target) => target.path) | ||
| .filter((path) => snapshot.read(path) == null) | ||
| if (missingEntryPoints.length > 0) { | ||
| return { | ||
| ok: false, | ||
| message: `Typecheck skipped because package runtime entrypoint(s) are missing from the published source snapshot: ${missingEntryPoints | ||
| .map((path) => `"${path}"`) | ||
| .join(', ')}.`, | ||
| } | ||
| } | ||
| const { createTypescriptLanguageService } = | ||
| await import('@cloudflare/worker-bundler/typescript') | ||
| const typecheckFileSystem = createRepoChecksFileSystem({ | ||
| fileSystem: snapshot, | ||
| }) | ||
| const baseTsconfig = snapshot.read(repoChecksSyntheticTsconfigPath) | ||
| if (baseTsconfig != null) { | ||
| typecheckFileSystem.write( | ||
| repoChecksSyntheticTsconfigExtendsPath.slice('./'.length), | ||
| baseTsconfig, | ||
| ) | ||
| } | ||
| typecheckFileSystem.write( | ||
| repoChecksSyntheticTsconfigPath, | ||
| buildRepoChecksTsconfig(baseTsconfig), | ||
| ) | ||
| const { fileSystem, languageService } = await createTypescriptLanguageService({ | ||
| fileSystem: typecheckFileSystem, | ||
| }) | ||
| const diagnostics = getPackageTypecheckDiagnostics({ | ||
| targets: input.entryPoints.map((entryPoint) => ({ | ||
| path: entryPoint.path, | ||
| includeStorage: entryPoint.includeStorage === true, | ||
| })), | ||
| languageService, | ||
| fileSystem, | ||
| }) | ||
| const ok = diagnostics.every((entry) => entry.diagnostics.length === 0) | ||
| return { | ||
| ok, | ||
| message: ok | ||
| ? `No semantic diagnostics for ${input.entryPoints.length} package runtime entrypoint(s).` | ||
| : formatPackageTypecheckDiagnostics(diagnostics).join('\n'), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\btypecheckPackageRuntimeEntryPoint\s*\(' --type=tsRepository: kentcdodds/kody
Length of output: 162
Remove typecheckPackageRuntimeEntryPoint (lines 525–569) — it is unused throughout the codebase.
Additionally, factor out the duplicated snapshot+tsconfig+language-service setup. The pattern appears three times: in typecheckPackageEntrypointsFromSourceFiles, the removed typecheckPackageRuntimeEntryPoint, and the existing runRepoChecks body (lines 478–503). Extract a shared helper (e.g., createTypecheckContextFromSourceFiles({ sourceFiles }) returning { snapshot, fileSystem, languageService }) and reuse it across all call sites.
Two minor inconsistencies in the remaining code:
- Lines 329–335 pass an async generator to
createFileSystemSnapshot, while other instances pass plain arrays. Use consistent input style. buildRepoChecksTsconfig(baseConfigContent)only uses the argument for a null-check; the content is never used. Rename the parameter tohasBaseConfig: booleanor pass the result of the null-check directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/repo/checks.ts` around lines 318 - 381, Remove the unused
function typecheckPackageRuntimeEntryPoint and extract the duplicated
snapshot+tsconfig+language-service setup into a shared helper
createTypecheckContextFromSourceFiles({ sourceFiles }) that returns { snapshot,
fileSystem, languageService } and is used by
typecheckPackageEntrypointsFromSourceFiles and runRepoChecks; when creating the
snapshot, call createFileSystemSnapshot with a plain array
(Array.from(Object.entries(sourceFiles))) instead of an async generator for
consistency; change buildRepoChecksTsconfig to accept a boolean (e.g.,
hasBaseConfig) or pass the baseTsconfig != null result so the function no longer
expects the full content but only a null-check, and update all call sites
accordingly (references: typecheckPackageEntrypointsFromSourceFiles,
runRepoChecks, typecheckPackageRuntimeEntryPoint, createFileSystemSnapshot,
buildRepoChecksTsconfig, createTypescriptLanguageService).
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>
| last_row_id: 0, | ||
| }, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Test mock matches source_id against userId parameter
Low Severity
The mock handler for DELETE FROM published_bundle_artifacts filters rows using row['source_id'] === params[0], but the actual SQL query is DELETE FROM published_bundle_artifacts WHERE user_id = ? AND source_id = ?, meaning params[0] is userId and params[1] is sourceId. The mock compares source_id with the userId value, which is incorrect and could mask real deletion bugs in tests.
Reviewed by Cursor Bugbot for commit bd60ea1. Configure here.
| ), | ||
| ) as T[], | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
SELECT ALL mock pattern won't match actual SQL query
Low Severity
The mock for listing published_bundle_artifacts checks query.includes('WHERE source_id = ?'), but the actual SQL from listPublishedBundleArtifactsBySourceId is WHERE user_id = ? AND source_id = ?. The substring WHERE source_id = ? does not appear in the actual query (it starts with WHERE user_id), so this mock clause will never match. If listPublishedBundleArtifactsBySourceId is exercised by any test path, it would throw "Unsupported all query" instead of returning results.
Reviewed by Cursor Bugbot for commit bd60ea1. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 5 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 49aa3bd. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This was referenced Apr 22, 2026
This was referenced Apr 30, 2026
This was referenced May 10, 2026
This was referenced Jun 25, 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
2 participants
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
Validation
npm run typechecknpx vitest run packages/worker/src/package-runtime/module-graph.node.test.ts packages/worker/src/package-registry/service.node.test.ts packages/worker/src/jobs/service.node.test.tsSummary by CodeRabbit
New Features
Changes
Chores
Tests