fix: close ~30 findings from a third-party Convex migration - #240
Merged
Conversation
✅ Deploy Preview for lunorash ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Thank you for following the naming conventions! 🙏 |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (35)
📒 Files selected for processing (58)
WalkthroughThis pull request adds documentation, advisor and codegen analysis, CLI workflows, browser session APIs, scheduler capabilities, server metadata contracts, lazy table iteration, and supporting dependency and typing updates. ChangesPlatform APIs, codegen, CLI, and documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
Contributor
|
Thank you for confirming the Contributor License Agreement! 🙏 |
![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/server/src/builder/index.ts (1)
205-221: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
.meta()silently drops for.stream()procedures.
.meta(...)is stamped onto the registration and wired intoctx.metafor the[kind]()terminal (lines 211–221), but the siblingstream:terminal (lines 229–247, samemakeBuilderclosure) never receivesstate.meta—makeStreamHandleris called without it and the returned object omitsmeta. SinceQueryBuilder/InternalQueryBuilderexpose both.meta()and.stream()on the same chain (seepackages/server/src/builder/types.tslines 64-75 and the siblingstreammember), a caller writing.meta({ rateLimit: "..." }).stream(handler)will have that metadata silently discarded: middleware never seesctx.metaand tooling never seesfn.metafor that registration. This directly contradicts the documented guarantee ("Merges across calls, is readable from middleware asctx.meta... stamped onto the registration asfn.meta").🐛 Proposed fix
return { args: state.args, ...(state.expose ? { expose: state.expose } : {}), - handler: makeStreamHandler(state.args, state.middlewares, userHandler), + handler: makeStreamHandler(state.args, state.middlewares, userHandler, state.meta), kind: "stream" as const, + ...(state.meta ? { meta: state.meta } : {}), ...(rls ? { rls } : {}), ...(visibility ? { visibility } : {}), ...(state.x402 ? { x402: state.x402 } : {}), };
makeStreamHandler(defined outside this excerpt) would also need to accept the optionalmetaparam and applywithMeta(context, meta)before invoking the generator, mirroringmakeHandler.🤖 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/server/src/builder/index.ts` around lines 205 - 221, Update the sibling stream terminal in makeBuilder so state.meta is passed to makeStreamHandler and included as meta on the returned registration object, matching the [kind] terminal. Extend makeStreamHandler to accept optional metadata and apply it through withMeta before invoking the stream generator, preserving merged .meta() values in both ctx.meta and fn.meta.packages/codegen/src/assert-required-packages.ts (1)
69-92: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMixes default and named exports in one file.
Lines 89-91 combine
export default assertRequiredPackageswith named exports (requiredPackagesFor,RequiredPackage). As per coding guidelines,**/*.{ts,tsx}: "Never mix a default export with named exports in the same file. A default export is allowed only when it is the file's sole export." See the consolidated comment for the full fix (also touches the importer inrun-codegen.ts).🤖 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/codegen/src/assert-required-packages.ts` around lines 69 - 92, Remove the default export from assert-required-packages.ts and expose assertRequiredPackages as a named export alongside requiredPackagesFor and RequiredPackage; update the importer in run-codegen.ts to use the named import while preserving existing behavior.Source: Coding guidelines
🧹 Nitpick comments (5)
packages/browser/src/create-browser.ts (2)
275-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate
getLaunchandrequirePeer— same guard, duplicated error text.Both throw the identical "peer dependency missing"
LunoraErrorfor aundefinedinjected function.getLaunchcould simply becomerequirePeer(options.launch, "launch")(keeping the extra import-hint sentence via an optional suffix param, or dropping it for consistency).As per path instructions,
packages/**/*.tsshould "Follow DRY principles."♻️ Proposed consolidation
- const getLaunch = (): BrowserLaunchLike => { - if (!options.launch) { - throw new LunoraError( - "INTERNAL", - '`@lunora/browser`: `launch` is not available — install the `@cloudflare/playwright` peer dependency. The generated worker wires it for you; outside codegen pass it via createBrowser({ binding, launch }) (import { launch } from "`@cloudflare/playwright`").', - ); - } - - return options.launch; - }; - - /** Same injection contract as {`@link` getLaunch}, for the session surface. */ - const requirePeer = <F>(function_: F | undefined, name: string): F => { + const requirePeer = <F>(function_: F | undefined, name: string): F => { if (!function_) { throw new LunoraError( "INTERNAL", `@lunora/browser: \`${name}\` is not available — install the \`@cloudflare/playwright\` peer dependency. The generated worker wires it for you; outside codegen pass it via createBrowser({ binding, ${name} }).`, ); } return function_; }; + + const getLaunch = (): BrowserLaunchLike => requirePeer(options.launch, "launch");Also applies to: 286-296
🤖 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/browser/src/create-browser.ts` around lines 275 - 284, Consolidate the duplicated missing-peer guard in getLaunch by reusing the existing requirePeer helper with options.launch. Preserve the intended launch error context, either through requirePeer’s supported suffix mechanism or the shared message, and remove the redundant LunoraError construction while keeping getLaunch’s return behavior unchanged.Source: Path instructions
304-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate close-quietly try/finally pattern between
withBrowserandconnect.The catch-and-swallow-on-close block is copy-pasted verbatim between
withBrowser(lines 315-324) andconnect(lines 512-521). Extract a smallcloseQuietly(browser, use)helper to keep the two call sites in sync as this logic evolves.As per path instructions,
packages/**/*.tsshould "Follow DRY principles."♻️ Proposed helper extraction
+ const runAndClose = async <T>(browser: BrowserLike, use: (browser: BrowserLike) => Promise<T>): Promise<T> => { + try { + return await use(browser); + } finally { + try { + await browser.close(); + } catch { + // Swallow: the session is being torn down anyway, and a close + // failure must not mask the caller's result/error. + } + } + }; + const withBrowser = async <T>(use: (browser: BrowserLike) => Promise<T>, keepAlive?: number): Promise<T> => { const browser = await getLaunch()(options.binding, keepAlive === undefined ? undefined : { keep_alive: keepAlive * 1000 }); if (keepAlive !== undefined) { return await use(browser); } - try { - return await use(browser); - } finally { - try { - await browser.close(); - } catch { - // Swallow: the session is being torn down anyway, and a close - // failure must not mask the caller's result/error. - } - } + return runAndClose(browser, use); };Then in
connect:if (connectOptions.close !== true) { return await function_(browser); } - try { - return await function_(browser); - } finally { - try { - await browser.close(); - } catch { - // Swallow: the session is being torn down anyway, and a close - // failure must not mask the caller's result/error. - } - } + return runAndClose(browser, function_);Also applies to: 505-522
🤖 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/browser/src/create-browser.ts` around lines 304 - 325, Extract the duplicated close-and-swallow logic from withBrowser and connect into a shared closeQuietly helper that runs use(browser), closes the browser in finally, and suppresses close failures. Replace both existing try/finally blocks with this helper while preserving the keepAlive path that intentionally skips closing.Source: Path instructions
packages/browser/src/types.ts (1)
117-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
BrowserSessionis missingconnectionStartTimefrom Cloudflare's actualsessions()payload.Cloudflare's documented Browser Rendering
sessions()response includes aconnectionStartTimefield alongsideconnectionId,sessionId, andstartTime. Since this is a closed interface, callers can't type-safely read that field even though the runtime payload has it.♻️ Suggested addition
export interface BrowserSession { connectionId?: string; + connectionStartTime?: number; sessionId: string; startTime?: number; }🤖 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/browser/src/types.ts` around lines 117 - 121, Update the BrowserSession interface to include an optional connectionStartTime field matching Cloudflare’s sessions() payload, alongside connectionId, sessionId, and startTime.packages/codegen/src/discover-queries.ts (1)
39-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated chain-walk logic between
chainMethodsandfiltersPrimaryKeyOf.Both functions walk the identical property-access/call-expression parent chain from the same
queryCallnode.discoverQueriesnow performs this walk twice per filtered read (line 145 callsfiltersPrimaryKeyOfafterchainMethodsalready computedmethodsat line 136). Consider extendingchainMethods(or a shared internal walker) to also expose each intermediate call node, sofiltersPrimaryKeyOfcan reuse the single traversal instead of re-implementing it.As per path instructions,
packages/**/*.tscode should "Follow DRY principles" and be checked for "performance optimizations".♻️ Sketch of a shared-walk refactor
-const chainMethods = (queryCall: CallExpression): string[] => { - const methods: string[] = []; - let node: TsNode = queryCall; - - for (;;) { - const parent = node.getParent(); - - if (!parent || !Node.isPropertyAccessExpression(parent)) { - break; - } - - const callParent = parent.getParent(); - - if (!callParent || !Node.isCallExpression(callParent)) { - break; - } - - methods.push(parent.getName()); - node = callParent; - } - - return methods; -}; +interface ChainStep { + call: CallExpression; + method: string; +} + +const chainSteps = (queryCall: CallExpression): ChainStep[] => { + const steps: ChainStep[] = []; + let node: TsNode = queryCall; + + for (;;) { + const parent = node.getParent(); + + if (!parent || !Node.isPropertyAccessExpression(parent)) { + break; + } + + const callParent = parent.getParent(); + + if (!callParent || !Node.isCallExpression(callParent)) { + break; + } + + steps.push({ call: callParent, method: parent.getName() }); + node = callParent; + } + + return steps; +}; + +const chainMethods = (queryCall: CallExpression): string[] => chainSteps(queryCall).map((step) => step.method);-const filtersPrimaryKeyOf = (queryCall: CallExpression): boolean => { - let node: TsNode = queryCall; - - for (;;) { - const parent = node.getParent(); - - if (!parent || !Node.isPropertyAccessExpression(parent)) { - return false; - } - - const callParent = parent.getParent(); - - if (!callParent || !Node.isCallExpression(callParent)) { - return false; - } - - if (parent.getName() === "filter") { - const predicate = callParent.getArguments()[0]; - - if (predicate && PRIMARY_KEY_PREDICATE_RE.test(predicate.getText())) { - return true; - } - } - - node = callParent; - } -}; +const filtersPrimaryKeyOfSteps = (steps: ReadonlyArray<ChainStep>): boolean => + steps.some((step) => { + if (step.method !== "filter") { + return false; + } + + const predicate = step.call.getArguments()[0]; + + return predicate !== undefined && PRIMARY_KEY_PREDICATE_RE.test(predicate.getText()); + });Then in
discoverQueries, computeconst steps = chainSteps(call);once and derive bothmethodsandfiltersPrimaryKeyfrom it.Also applies to: 83-109, 145-145
🤖 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/codegen/src/discover-queries.ts` around lines 39 - 61, Eliminate the duplicated parent-chain traversal between chainMethods and filtersPrimaryKeyOf by introducing or extending a shared walker that starts from the queryCall and records each property name with its intermediate call expression. Update discoverQueries to compute the shared chain once, derive methods and the primary-key filter from that result, and remove the second traversal while preserving existing outputs.Source: Path instructions
packages/codegen/src/emit.ts (1)
147-219: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
v.from()columns lose their recovered type in generated Drizzle schemas.
validatorToType's new"from"case now renders the recovered Standard-Schema output type, butvalidatorToDrizzleColumn(unchanged, further down this file) has no matching"from"case — it falls through to thedefaultbranch and emits a plaintextcolumn with no.$type<>()annotation, unlikearray/object/record/unionwhich do gettypeAnnotation: validatorToType(validator). A schema usingv.from(zodSchema)as a table column keeps the recovered type at the API layer but silently loses it at the ORM layer.♻️ Proposed fix: give `"from"` the same JSON+typeAnnotation treatment
const validatorToDrizzleColumn = (validator: ValidatorIR): DrizzleColumn => { switch (validator.kind) { case "array": + case "from": case "object": case "record": case "union": { return { builder: "text", mode: "json", notNull: true, typeAnnotation: validatorToType(validator), }; }🤖 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/codegen/src/emit.ts` around lines 147 - 219, Update validatorToDrizzleColumn for the "from" validator kind so it emits the same JSON column configuration and typeAnnotation: validatorToType(validator) used by array, object, record, and union. Preserve the recovered Standard-Schema output type through generated Drizzle schemas instead of allowing "from" to fall through to the default plain text column.
🤖 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 `@apps/docs/src/content/docs/concepts/caching.mdx`:
- Around line 152-176: Update the caching example to move the expensiveThing
computation into the writeCache mutation, so callers no longer compute between
readCache and runMutation. Have the mutation recheck the cache, compute and
store the value only on a miss, and return the cached or newly computed result;
adjust the action to invoke this mutation after its initial read while
preserving the documented single-flight behavior.
In `@packages/advisor/src/lints/static/filter-on-primary-key.ts`:
- Around line 32-52: Update the lint emission logic around the run callback in
filter-on-primary-key.ts so primary-key scans do not also produce a
filter_without_index advisory. Ensure any shared filter_without_index generation
skips reads where read.filtersPrimaryKey is true, while preserving the existing
filter_on_primary_key finding and all other unindexed-filter findings.
In `@packages/advisor/src/lints/static/filter-without-index.ts`:
- Line 26: Update the lint branching that uses shardKindByTable so tables with
shardKind "root" are not handled by the in-memory-filter else path. Keep the
unknown external fallback for shardKind === undefined, while routing root tables
through the unbounded D1 scan wording and preserving existing handling for
"global" and "shardBy".
In `@packages/cli/src/commands/dev/handler.ts`:
- Around line 124-125: Update the daemon argument construction in the lifecycle
flow to propagate the `worker` option from the `dev` command, ensuring
`--no-worker` is preserved when the background daemon starts. Locate the argv
rebuild logic in the relevant lifecycle function and add the existing option
without changing other command behavior.
In `@packages/cli/src/commands/import/index.ts`:
- Around line 4-8: Update the import command description in the command
definition to mention both NDJSON files and export directories, keeping it
consistent with the argument description and directory example while preserving
the existing bulk-insert context.
In `@packages/cli/src/commands/registry/apply.ts`:
- Around line 393-394: Update the comment adjacent to the `value ===
SKIP_BINDING` check to document `SKIP_BINDING` as the “nothing new to write”
sentinel, rather than stating that `undefined` represents the skip condition;
preserve the explanation that entries are skipped when already present or when
their binding name is claimed.
- Around line 269-297: Update freshArrayEntries and its bindingNameOf identity
handling to recognize section-specific keys: use name for
durable_objects.bindings and send_email entries, and queue for
queues.consumers/producers, while preserving binding-based detection elsewhere.
Also add each accepted incoming entry’s serialized value to seen before pushing
it, preventing duplicate identity-less entries within the same batch.
In `@packages/cli/src/commands/registry/commands.ts`:
- Line 11: Update printPlan() to resolve the CLI dist-tag once before iterating
dependencies, then reuse that value when resolving all workspace dependency
ranges through resolveDepRange(). Avoid the default per-call resolveDistTag()
behavior while preserving existing range rendering for workspace:*, workspace:^,
and workspace:~ entries.
In `@packages/cli/src/commands/rules/handler.ts`:
- Around line 105-148: Extract the duplicated `options.dir === undefined ?
resolveWorkspaceRoot(invokedFrom) : resolve(invokedFrom, options.dir)` logic
into a shared helper near `resolveWorkspaceRoot`, then use that helper in both
`runRulesInstall` and `runRulesCheck`. Preserve the existing `--dir` precedence
and workspace-root fallback behavior.
- Line 110: Update WORKSPACE_ROOT_MARKERS to include Bun’s text-based bun.lock
filename alongside the existing lockfile markers, so resolveWorkspaceRoot()
recognizes Bun 1.2 workspace roots.
In `@packages/codegen/src/discover-functions.ts`:
- Around line 1203-1220: Update fileFindings() so the procedure_not_registered
advisory also scans source.getExportAssignments(), not only exported variable
statements. For each assignment, skip isExportEquals() entries and inspect
assignment.getExpression() to detect unresolvable default-export factory
initializers, while preserving the existing handling for variable exports.
In `@packages/codegen/src/discover-schema.ts`:
- Around line 259-270: Update the field-processing logic around fieldElements to
detect and diagnose every non-string-literal element in the array instead of
filtering it out silently. Preserve the existing assertTopLevelIndexField
validation and returned string fields for valid literals, and use the file’s
established failure behavior for unresolvable static values so invalid entries
cannot produce a partially populated index.
In `@packages/codegen/src/discover-workflows.ts`:
- Around line 163-168: Update resolveWorkflowConfig’s variable-declaration
handling to verify the declaration uses a const binding before returning an
object-literal initializer. Reject let and var declarations, while preserving
the existing behavior for const declarations and other initializer cases.
In `@packages/codegen/src/run-codegen.ts`:
- Line 13: Update the assertRequiredPackages import in run-codegen.ts to match
the revised export style of assert-required-packages.ts, using the corresponding
named or default import form consistently with that module’s exported symbol.
In `@packages/codegen/src/secret-rules.ts`:
- Around line 77-87: Update SECRET_NAME_RE to apply word boundaries around each
secret-related alternative, so isSecretishName only matches standalone
identifier terms rather than substrings within names such as cacheKey,
designing, or author. Preserve the existing alternatives and case-insensitive
behavior while ensuring compound names with explicit separators remain
supported.
In `@packages/runtime/src/create-worker.ts`:
- Around line 2971-2989: Update the scheduler object’s runAt method to validate
timestampMs before calling schedule, rejecting non-finite or negative values
with the same LunoraError BAD_REQUEST behavior and actionable message used by
runAfter. Preserve the existing scheduling flow for valid timestamps.
In `@packages/shard-engine/src/ctx-db.ts`:
- Around line 1151-1184: Make filtered async iteration lazy in
packages/shard-engine/src/ctx-db.ts at lines 1151-1184 by fetching candidates in
bounded keyset pages and yielding matches incrementally, rather than
materializing the remaining table before yielding; preserve the existing
QueryPage pagination, cursor progression, ordering, and early-termination
behavior. Apply the corresponding incremental candidate paging change in
packages/sql-store/src/ctx-db.ts at lines 2757-2782, ensuring filtered search
does not read the entire MAX_SEARCH_SCAN window before producing results.
---
Outside diff comments:
In `@packages/codegen/src/assert-required-packages.ts`:
- Around line 69-92: Remove the default export from assert-required-packages.ts
and expose assertRequiredPackages as a named export alongside
requiredPackagesFor and RequiredPackage; update the importer in run-codegen.ts
to use the named import while preserving existing behavior.
In `@packages/server/src/builder/index.ts`:
- Around line 205-221: Update the sibling stream terminal in makeBuilder so
state.meta is passed to makeStreamHandler and included as meta on the returned
registration object, matching the [kind] terminal. Extend makeStreamHandler to
accept optional metadata and apply it through withMeta before invoking the
stream generator, preserving merged .meta() values in both ctx.meta and fn.meta.
---
Nitpick comments:
In `@packages/browser/src/create-browser.ts`:
- Around line 275-284: Consolidate the duplicated missing-peer guard in
getLaunch by reusing the existing requirePeer helper with options.launch.
Preserve the intended launch error context, either through requirePeer’s
supported suffix mechanism or the shared message, and remove the redundant
LunoraError construction while keeping getLaunch’s return behavior unchanged.
- Around line 304-325: Extract the duplicated close-and-swallow logic from
withBrowser and connect into a shared closeQuietly helper that runs
use(browser), closes the browser in finally, and suppresses close failures.
Replace both existing try/finally blocks with this helper while preserving the
keepAlive path that intentionally skips closing.
In `@packages/browser/src/types.ts`:
- Around line 117-121: Update the BrowserSession interface to include an
optional connectionStartTime field matching Cloudflare’s sessions() payload,
alongside connectionId, sessionId, and startTime.
In `@packages/codegen/src/discover-queries.ts`:
- Around line 39-61: Eliminate the duplicated parent-chain traversal between
chainMethods and filtersPrimaryKeyOf by introducing or extending a shared walker
that starts from the queryCall and records each property name with its
intermediate call expression. Update discoverQueries to compute the shared chain
once, derive methods and the primary-key filter from that result, and remove the
second traversal while preserving existing outputs.
In `@packages/codegen/src/emit.ts`:
- Around line 147-219: Update validatorToDrizzleColumn for the "from" validator
kind so it emits the same JSON column configuration and typeAnnotation:
validatorToType(validator) used by array, object, record, and union. Preserve
the recovered Standard-Schema output type through generated Drizzle schemas
instead of allowing "from" to fall through to the default plain text column.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cdca3707-9c11-490b-a741-064633bc5344
⛔ Files ignored due to path filters (34)
.vis/templates/lunora-cron.tsis excluded by!**/.vis/**and included by noneapi-snapshots/advisor.api.mdis excluded by none and included by noneapi-snapshots/cli.api.mdis excluded by none and included by noneapi-snapshots/codegen.api.mdis excluded by none and included by noneapi-snapshots/runtime.api.mdis excluded by none and included by noneapi-snapshots/scheduler.api.mdis excluded by none and included by noneapi-snapshots/server.api.mdis excluded by none and included by noneapi-snapshots/shard-engine.api.mdis excluded by none and included by noneapps/playground/tsconfig.generated.jsonis excluded by!**/*.generated.*and included byapps/**examples/payment-demo/lunora/_generated/shard.tsis excluded by!**/_generated/**and included by nonepackages/advisor/__tests__/filter-scope.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/advisor/__tests__/owner-field-from-args-not-auth.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/browser/__tests__/create-browser.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/cli/__tests__/commands/data-transfer.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/cli/__tests__/commands/dev.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/cli/__tests__/commands/registry-items.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/cli/__tests__/commands/rules.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/codegen/__tests__/discover-crons.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/codegen/__tests__/discover-secrets.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/codegen/__tests__/discover-workflows.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/codegen/__tests__/emit-api.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/codegen/__tests__/fixtures/simple/expected/_generated/app.tsis excluded by!**/_generated/**,!**/__tests__/**and included bypackages/**packages/codegen/__tests__/fixtures/simple/expected/_generated/drizzle.global.tsis excluded by!**/_generated/**,!**/__tests__/**and included bypackages/**packages/codegen/__tests__/parse-validator-from.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/codegen/__tests__/run-codegen.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/runtime/__tests__/create-worker.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/scheduler/__tests__/cron-jobs.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/scheduler/__tests__/cron-parity.jsonis excluded by!**/__tests__/**and included bypackages/**packages/server/__tests__/builder.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/server/__tests__/functions-v-from.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/server/__tests__/functions.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/server/__tests__/types.test-d.tsis excluded by!**/__tests__/**and included bypackages/**packages/shard-engine/__tests__/ctx-db.iterate.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by none
📒 Files selected for processing (57)
apps/docs/src/content/docs/concepts/caching.mdxapps/docs/src/content/docs/concepts/monorepos-and-iac.mdxapps/docs/src/content/docs/getting-started.mdxapps/docs/src/content/docs/meta.jsonapps/docs/src/content/docs/migrating/from-a-convex-toolkit.mdxapps/docs/src/content/docs/migrating/from-convex.mdxapps/playground/lunora/workflows.tsapps/playground/package.jsonpackages/advisor/src/index.tspackages/advisor/src/lints/static/filter-on-primary-key.tspackages/advisor/src/lints/static/filter-without-index.tspackages/advisor/src/lints/static/owner-field-from-args-not-auth.tspackages/advisor/src/owner-field-writes.tspackages/advisor/src/queries.tspackages/auth/package.jsonpackages/browser/src/create-browser.tspackages/browser/src/types.tspackages/cli/src/commands/codegen/handler.tspackages/cli/src/commands/codegen/index.tspackages/cli/src/commands/data-transfer.tspackages/cli/src/commands/dev/handler.tspackages/cli/src/commands/dev/index.tspackages/cli/src/commands/import/handler.tspackages/cli/src/commands/import/index.tspackages/cli/src/commands/registry/apply.tspackages/cli/src/commands/registry/commands.tspackages/cli/src/commands/rules/handler.tspackages/cli/src/commands/rules/index.tspackages/codegen/src/assert-required-packages.tspackages/codegen/src/discover-crons.tspackages/codegen/src/discover-functions.tspackages/codegen/src/discover-owner-field-writes.tspackages/codegen/src/discover-package-dependencies.tspackages/codegen/src/discover-queries.tspackages/codegen/src/discover-schema.tspackages/codegen/src/discover-secrets.tspackages/codegen/src/discover-unregistered-procedures.tspackages/codegen/src/discover-workflows.tspackages/codegen/src/emit-app.tspackages/codegen/src/emit.tspackages/codegen/src/ir.tspackages/codegen/src/parse-validator.tspackages/codegen/src/run-codegen.tspackages/codegen/src/secret-rules.tspackages/runtime/src/create-worker.tspackages/scheduler/README.mdpackages/scheduler/docs/index.mdxpackages/scheduler/src/index.tspackages/scheduler/src/jobs.tspackages/server/src/builder/index.tspackages/server/src/builder/types.tspackages/server/src/http.tspackages/server/src/types.tspackages/shard-engine/src/ctx-db.tspackages/shard-engine/src/schema-types.tspackages/sql-store/src/ctx-db.tspackages/testing/src/harness.ts
prisis
force-pushed
the
fix/migration-findings-codegen
branch
from
July 31, 2026 06:36
6d81caf to
1983e07
Compare
`_generated/` failed `tsc --noEmit` under a consumer's strict config (strict + noUnusedLocals + noImplicitAny + noUncheckedIndexedAccess). Consumers can't `// @ts-nocheck` someone else's generated file without losing type safety everywhere, so the emitter has to be clean. Four distinct causes, found on a 92-table port: - A self-referential FK emitted `.references(() => t._id)`, which is circular in the table's own initializer — TS7022 on the binding, TS7024 on the callback. Now emits drizzle's documented `(): AnySQLiteColumn` return annotation, unconditionally: it is a no-op on ordinary FKs, and mutual cycles need it just as much. - A `v.object`/`v.union` column carrying a nested `v.id()` renders `Id<"table">` into its `.$type<…>()`, but `drizzle.*.ts` never imported `Id` — TS2304. Now imported when referenced. - `api.ts` imported `FunctionReference` unconditionally, so a project with no discovered functions carried a dangling import — TS6133. Both client imports are now emitted only when the body uses them. - The app builder's storage `pick()` fell back to `buckets.default`, which widens to `Storage | undefined` under noUncheckedIndexedAccess — TS2322. The default is held in a plain binding instead. `referencedTable()` is now shared by the renderer and the import scan so the two cannot disagree about which columns emit a `.references()`. Guard: apps/playground/tsconfig.generated.json typechecks `lunora/_generated/**` at the repo-wide strictness and is wired into the playground's `lint:types`. The app's own tsconfig relaxes noUncheckedIndexedAccess, which is exactly why the `Storage` case was invisible here — that config reproduced it on the first run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A report said a hard codegen error exited 0 and still
wrote `_generated/api.ts` as an empty shell, turning one invalid cron
into ~600 "Property does not exist on InternalApiTypes" errors across
every module — each pointing at a caller, none at the cron.
Neither half reproduces on current alpha: `lunora codegen` exits 1
(defineHandler converts the throw), and because every write happens in
one block after all discovery, a cold run leaves no output and a warm
run leaves the previous good output untouched. Verified both against
the playground with `crons.interval(..., { hours: 24 }, ...)`.
That ordering is load-bearing and nothing asserted it, so this adds the
regression test: cold (no api.ts appears) and warm (an existing api.ts
is byte-identical afterwards, re-read rather than inferred from the
throw, since the failure mode being guarded is a write that happened
anyway).
The report's third ask — keep emitting the API surface and report the
cron error alongside it — is deliberately not adopted. Failing closed
is better than shipping a half-correct API surface that typechecks.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two papercuts from porting eight Convex crons.
Interval errors: `{ hours: 24 }` is the ordinary Convex idiom for "once a day", and
an interval compiles to a cron STEP within its field's period, so it is
rejected. The old message said only "must be an integer in [1, 23]",
which does not say where daily lives — and the natural next guess,
`{ days: 1 }`, hit "must specify exactly one of { seconds, minutes,
hours }", which does not say so either. Two dead ends in a row.
Both messages now name `crons.daily(name, { hourUTC, minuteUTC }, …)`.
The over-23 case is split out from the generic range check so it can
explain *why* (an interval repeats within a day rather than spanning
one), and the "exactly one of" message now echoes the keys it actually
got, so `{ days }` is visible in the error.
No hourly helper: `daily`/`weekly`/`monthly` existed but `hourly` did not, so hourly
jobs went through `interval({ hours: 1 })`. Besides the asymmetry, that
form pins the job to `:00`, where a dozen of them stampede; `hourly`
takes a `minuteUTC` so they can be spread out.
`hourly` is wired through CRON_SCHEDULE_KINDS, so codegen discovers it
with no change beyond widening the cast that had the kinds hardcoded.
Added to the shared cron-parity matrix that asserts the scheduler and
codegen compile identically.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two diagnostics that described a symptom rather than the problem.
Nested index paths: `.index("by_state", ["state.kind"])` is a common Convex idiom (a
discriminated-union column with an indexed `kind`). Lunora indexes only
top-level columns, but the rejection came from the drizzle renderer as
`drizzle index field is not a valid JS identifier: "state.kind"`, which
names neither the constraint nor the workaround. Now caught at schema
discovery, so it carries file:line, states the constraint, suggests the
denormalised column name, and points at the `.triggers()` hook that
keeps it in sync.
Missing add-ons: adding one `.global()` table to a project without `@lunora/d1`
left codegen succeeding and only `tsc` failing, with `Cannot find
module '@lunora/d1'` reported inside `_generated/app.ts` — a file the
user did not write, several steps from the cause. Codegen now fails
before emit, listing every missing add-on at once with the install
command. Covers `.global()` → `@lunora/d1`, `.global({ backend:
"hyperdrive" })` → `@lunora/hyperdrive`, and `.vectorize()` →
`@lunora/bindings`. None are umbrella-provided, so `lunorash` projects
need them too.
The gate keys off the schema, not the emitted text, so it can run
before emit and blame the declaration. It stays silent when no manifest
is readable: `discoverPackageDependencies` collapsed "absent" and
"declares nothing" into an empty set, which is right for studio gating
but would make this fire on every manifest-less project. Split out
`readPackageDependencies`, which returns undefined for the former.
Test manifests gained `@lunora/d1` because the shared fixture schema
has a `.global()` table — the gate was correct about all eleven.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codegen registers an export only when its initializer is literally a builder chain. Anything else exists at runtime and never reaches `api.ts` — silently, exit 0 — and the error surfaces somewhere else entirely as "Property 'x' does not exist", often in another package, reading as a naming mistake rather than a dropped function. Three separate investigations on one port traced back to this. Three changes, in order of how much they remove: - `export default <procedure>` now registers as `<module>.default`, matching Convex. This was the worst-diagnosed form: the module went missing from `api.ts` ENTIRELY, not just the one entry, so the error named a module rather than a function and pointed at a caller that was correct. - `defineTable(fieldsIdentifier)` now fails with a diagnostic instead of emitting a table with no columns. Sharing a field map between the schema and an `.input()` is the obvious thing to write, and it produced a `Doc_*` carrying `_id` and `_creationTime` and nothing else, with no error anywhere. A column-less table is never intended. - Everything else is caught by a new type-level pass: any exported binding whose TYPE is `RegisteredQuery`/`Mutation`/`Action` but which is absent from `api.ts` becomes a `procedure_not_registered` advisory. Being type-level rather than syntactic, it cannot be fooled by the very indirection that causes the bug — a factory, an alias, a re-export. The type pass costs a checker call, so it is spent only on initializers that can actually hide a registration (a call, an identifier, a property access); literals and arrow functions are skipped. When the type cannot be resolved at all it reports nothing rather than everything, so a project without `@lunora/server` installed degrades quietly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`git add -A` in the preceding codegen commit swept in four files from concurrent, unrelated work in this checkout (the read-write-set / index-key-codec change). They are new files that do not exist on alpha, so their correct state is untracked-on-disk, which is exactly what `--cached` restores. No file content is touched. Net diff of this branch against alpha for those paths is now nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codegen already printed advisories but exited 0 whatever their level, so an ERROR reading "the call throws at runtime" was indistinguishable from a style nit. Three workflows could have deployed and failed on first use with a green build. Gating on ERROR is only useful if ERROR is trustworthy, and it was not: the first large port surfaced 21 ERRORs and all 21 were false positives. So the two rules are fixed first, and the gate lands on top. - `owner_field_from_args_not_auth` fired on `internal*` procedures (9 of the 21). An internal procedure is not reachable by a caller, so "any caller can act as any user" is simply false there, and taking the subject from `args` is the CORRECT shape — the trusted caller has already authenticated. Procedure visibility now reaches the lint, and internal writes drop to INFO with wording that redirects at the public procedures dispatching to them, which is where the real vector is. Public writes are untouched and still ERROR. - `hardcoded_secret` fired on the W3C Trace Context spec's example trace ids in a test file (10 of the 21). Only the two heuristic kinds (`hex_secret`, `high_entropy`) are affected: they match on shape alone, and a hash, a fixture id, or a spec's example value has exactly that shape. Those two now need a secret-ish binding name and are suppressed in test files. Vendor-prefixed kinds carry their own evidence and are unchanged — an `sk_live_…` in a test is still a leak. - `filter_without_index` reported a `.shardBy()` table identically to a `.global()` one, though the first reads a single Durable Object and the second scans D1 unbounded. Sharded tables now report INFO saying the read is already shard-scoped; global tables say so explicitly. - New `filter_on_primary_key`: `.filter((d) => d._id === x)` is a scan for a row `ctx.db.get(id)` addresses directly. Always wrong, never a judgement call — which is why it is its own rule instead of another finding to triage inside a sometimes-wrong one. `lunora codegen` then exits non-zero on any remaining ERROR, defaulting to on in CI and off locally, with `--no-strict-advisories` to opt out. The output is still written; the exit code is the gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two gaps that together made the most common HTTP-action shape awkward.
No scheduler on HTTP actions: `HttpActionCtx` had none, yet "receive webhook →
enqueue the real work → return 200 immediately" is what HTTP actions
are for, and Convex's httpAction ctx has one. The omission looked
principled (HTTP actions run in the worker, not a shard) but the
scheduler is a DO of its own, reachable from the worker directly.
Hand-rolling it was not a one-liner. A function reference cannot cross
the RPC boundary, so the mutation-hop workaround had to name targets by
string and resolve them shard-side — and these endpoints are reachable
unauthenticated (platform-signed webhooks), so a free-form target
string is a "call any internal function" primitive. That forces a
closed allow-list with an entry per target.
`ctx.scheduler` now talks to the scheduler DO directly, with the full
`Scheduler` surface (runAfter/runAt/cancel/get/list) and support for
function, workflow and agent targets. It is no more privileged than the
`run*` members beside it: the reference is a literal from the app's own
source, never caller-supplied. `originUrl` is derived from the inbound
request rather than configured — the origin a webhook arrived on is by
construction one that reaches this worker, so previews need no
per-environment setting. The member is optional and absent unless the
app declared `.scheduler(...)`.
No httpRouter sugar: the generated `defineApp` builder sugared everything in
`WorkerOptions` except `httpRouter`, the one field a real app is most
likely to need. `.route()` looks like the answer but takes one handler
per key, so a 20-endpoint hono app with CORS and error handling does
not fit; the actual answer was `.extend(() => ({ httpRouter: app }))`,
which works but is undiscoverable. Added `.httpRouter(app)` next to
`.route()`, with docs saying which is for which.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four adoption papercuts, each one a thing that works in this monorepo and breaks outside it. Peer pinning: `@lunora/auth` carried `better-auth` and `@better-auth/core` as regular dependencies, so a consumer on a different version ended up with two nominally-distinct copies and EVERY plugin passed to `createAuth` failed to typecheck — forty lines of `.pnpm` paths and not one mention of a property the user wrote. They are peers now, with the catalog pin kept as a devDependency. An adapter should not pin the thing it adapts. Registry merges: `lunora registry add` merged array bindings structurally, which deduped exact repeats but happily appended a placeholder entry ALONGSIDE a real one under the same `binding` name: two `DB` entries, one pointing at `replace-me-db`. Wrangler picks one, and the app can deploy against a database that does not exist. The project's entry now wins and the skip is reported. The plan output also showed the manifest's internal `workspace:*` range rather than the range `add` actually writes, so it read as though it were about to break `pnpm install`. Rules root: `lunora rules install` resolved against cwd, so running it from a package subdirectory dropped the skills in `<pkg>/.agents/skills` where no coding agent looks. It now walks up to the workspace root, with `--dir` to override. `check` resolves the same way — otherwise it would report "missing" for skills `install` had just written one level up. Workflow scan: the `defineWorkflow` scan matched only an inline object literal, so passing a typed `WorkflowConfig` variable was rejected. That is exactly what you reach for once handlers grow, since putting three 100-line handlers in the registry file is worse code. It now resolves one local `const` hop. Deliberately one hop: a config assembled by a call cannot be read, and registering it under a default name while its declared `name` went unseen would make `ctx.workflows.get(...)` throw at runtime with nothing pointing back here — so that still fails loudly, now with a message that says what IS supported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`_generated/api.ts` types every entry as a `FunctionReference` — the
client-side handle carrying `__lunoraRef` — but `ctx.runQuery` /
`runMutation` / `runAction` were declared to take `RegisteredQuery` &
co., the server-side registration objects. `api` and `internal` are the
only things most call sites have, so the documented example — straight
out of this package's own JSDoc:
ctx.runQuery(api.todos.list, searchParams)
did not typecheck, and there was no user-side fix short of a cast at
every call site. On the first large third-party port this was ~370
errors and the single largest class remaining after everything else was
cleared.
Both shapes are now accepted. Overloads rather than a union parameter,
because `Args` cannot be inferred backwards through `InferArgs<A>` — the
two shapes need separate inference sites. The registration overload
comes first, so importing a module directly keeps its existing, more
precise behaviour, and args stay checked through the reference form
(asserted in types.test-d.ts).
`FunctionHandle` is a structural mirror of `@lunora/client`'s
`FunctionReference`, declared locally so `@lunora/server` keeps carrying
no dependency on the client package — the same treatment `Scheduler`
gets for `@lunora/scheduler`. `RegisteredFunction` has no `__lunoraRef`,
so the two shapes cannot be confused.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`.output(validator)` is what validates at runtime and what a reader takes as the contract, but the emitted `FunctionReference`'s `Return` came from the handler's inferred type, so the two could disagree. Two consequences, in increasing order of seriousness: - A function declaring a two-arm `v.union` whose handler currently returns only one arm typed as JUST that arm. The other branch was unreachable to every consumer even though the validator permits it and the runtime emits it the moment the handler grows a second path. - A single `as any` in a handler erased the whole signature to `unknown`, which propagated to every `runQuery` result and every field read off it. Ten stray casts left by a port's codemod were worth 20 errors, and the link between cast and error is invisible from either end. `.output()` now wins when declared; the handler still supplies the type when it is not, so a project that never uses `.output()` emits byte-identical output (verified: the codegen fixture is unchanged). The declared validator is carried through discovery as `FunctionIR.output` rather than rendered there, keeping the layering — discovery produces IR, emit renders types. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two halves of the same complaint: policy and types that exist at
runtime but are invisible to tooling.
First: `v.from()` is the advertised Standard Schema bridge and works
correctly at runtime, but codegen parsed it to an opaque node, so every
argument behind one typed as `unknown` in the generated api. That broke
`ctx.run*` calls on those functions, made handler args implicitly `any`
under noImplicitAny, and gave generated clients untyped arguments — a
few hundred call sites on the port that reported it, each reading as an
unrelated bug.
Codegen now recovers the wrapped schema's type through the checker from
`~standard.types.output` — the property Standard Schema v1 exposes so
tooling can do exactly this, and the same one the runtime's
`InferStandardOutput` reads, so the emitted type and the value that
reaches the handler agree by construction.
It runs through the same guards as the handler-return path: an
`any`-degraded render (a checker without tsconfig wiring) falls back to
`unknown` rather than misleading, and a locally-declared type
unreachable from `_generated/` is structurally expanded rather than
emitted as a bare name that would not resolve. A schema that omits the
optional `~standard.types` genuinely carries no recoverable type and
stays `unknown` — falling back beats inventing one.
The resolver is registered by the codegen run rather than imported by
the parser, which would be a cycle.
Second: added `.meta(obj)` to all six procedure builders. It merges across
calls, is readable from middleware as `ctx.meta`, and is stamped onto
the registration as `fn.meta` so codegen and other tooling can
enumerate it. The point is policy as DATA: `.meta({ rateLimit:
"pins/create" })` can be walked to generate a rate-limit registry or
docs, where the same policy expressed only as
`.use(rateLimit("pins/create"))` can only be executed. Mirrors tRPC.
A procedure that never calls `.meta()` is byte-identical.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three were cases of code doing something other than what its own
comments and tests asserted.
**`v.from()` type recovery never worked outside its unit test.** Two
independent causes, either alone enough:
- `setStandardTypeResolver` ran ~230 lines AFTER `discoverSchema` /
`discoverFunctions`, which is where validators are parsed. The module
hook was still `undefined` at parse time, so every `v.from()` emitted
a bare `{ kind: "from" }`.
- `SCALAR_TYPE_BY_KIND` still mapped `from -> "unknown"`, and
`validatorToType` consults that map and returns before reaching the
switch, so the recovered `tsType` was unreachable even once set.
The unit test registered the resolver itself and stopped at the IR, so
it passed over a feature that produced `unknown` in the real pipeline —
exactly the "green test over broken code" shape this branch set out to
fix elsewhere. Registration now happens before discovery, `from` has a
real switch arm, and the guard is an end-to-end test through
`runCodegen` to the emitted `api.ts`. Verified it fails when either
cause is reintroduced.
**`filter_on_primary_key` flagged inequality.** The feeder regex
`[!=]==?` also matched `!==`, so `.filter((d) => d._id !== excludeId)`
— "every row except this one" — drew a warning telling the author to
replace it with `ctx.db.get(id)`, which returns the opposite set. That
also falsified the lint's own claim to need no triage, which was the
whole reason for splitting it out of `filter_without_index`. Narrowed
to equality.
**`--no-worker` would have parked the vite flavor on nothing.** The
gate sits on the shared run path, and nothing branched on flavor — so
on a Vite project it suppressed the FRAMEWORK dev server, not the
standalone wrangler this command owns. Since that plan also sets
codegen and studio to false (Vite runs them in-process), the process
would have waited forever having started literally nothing, while
logging that codegen was running. The flag is now refused with an
explanation on those flavors, and the plan test covers vite as well as
wrangler.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two reviewers ran over the branch. The headline: **it did not compile.**
I had typechecked every package I edited and none of their dependents.
**`@lunora/sql-store` failed `tsc`, and `.global()` tables had no
iterator.** Making `[Symbol.asyncIterator]` required on `TableReaderLike`
broke the D1/PlanetScale reader, which does not implement it — so CI's
`lint:types` failed and `@lunora/d1` could not build. The type error was
the cheap half: the public `TableReader` promised an iterator that only
one backend had, so `for await` over a `.global()` table typechecked and
threw at runtime. Implemented it there too, paging through `paginate`
exactly as the shard reader does, so it hits the same directed
`LEGACY_READER_ERROR` when the chain is not a search stage.
**`@lunora/testing` failed `tsc`.** TS will not contextually type a
parameter against a multi-signature type, so making `ctx.run*`
overloaded degraded `(reference, args) => …` to implicit `any` in every
hand-built ctx double. I tried a single generic signature to avoid the
whole class — it does not work, because a concrete `RegisteredQuery` is
not assignable to a `RegisteredFunction<ArgsValidator, …>` constraint
(`handler`'s args are contravariant). Kept the overloads, annotated the
harness, and wrote the reason down where the next person will hit it.
**`.output()` could downgrade a return type to `unknown`.** `.output(
sharedValidator)` — a hoisted validator rather than an inline call —
parses to `{ kind: "any" }`, and preferring that over the handler
replaced a precise inferred type with `unknown`. The change meant to
stop `unknown` leaking into `runQuery` results introduced a new way for
it to happen. Falls back to the handler when the validator is opaque.
**`--no-strict-advisories` did not exist.** Cerebro only synthesizes a
negation for options declared as `no-*`, so the flag advertised in the
option description AND printed in the failure message was an
unknown-option error — following the advice took the build from "failed
on advisories" to "failed on a bad flag", with the gate defaulting on in
every CI. Declared as `no-strict-advisories`, matching
`--no-studio`/`--no-codegen`/`--no-worker`.
**Scheduler hardening.** `targetFields` accepted a bare `"ns:fn"`
string, on endpoints reachable unauthenticated — the "call any internal
function" primitive the surface exists to avoid, and which `run()`
beside it already rejects. Strings are now refused. Also removed the
`originUrl` sent to the DO: it is never read (the DO takes its callback
origin from `env.LUNORA_ORIGIN_URL`, deliberately, to prevent SSRF), and
the comment claiming otherwise would have taught the next maintainer to
trust a field that exists to be ignored.
**`ctx.meta` was not on the ctx types**, so the documented way to read
it needed a cast — the test's own `(ctx as { meta?: unknown })` was the
tell. Declared on QueryCtx/MutationCtx/ActionCtx; the cast is gone.
**`Browser.connect`'s documented flow was unreachable**: `keepAlive`
held a session open but nothing exposed its id, and `sessions()` lists
all of them with no way to identify yours. Added optional `sessionId()`
to the projection and covered both it and the throws-with-keepAlive
path.
Plus three JSDoc blocks that had been orphaned from their functions by
insertion (including the "no id remapping" rationale, which had attached
to the wrong generator), and a doc that described a different function
than the one it sat on — whose wrapper turned out to have no callers.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review caught that `^1.7.0-rc.2` was wider than the catalog's exact pin. The catalog holds these at an exact prerelease deliberately — the `@better-auth/*` plugins peer on an exact `better-auth`/`@better-auth/core`, so a caret admits a version those plugins reject, and with `strictPeerDependencies: false` a consumer silently gets two copies and a split session/plugin state rather than an error. `catalog:auth` is also what CLAUDE.md requires (never hard-code a version that lives in a catalog) and what the adjacent `@better-auth/sso` peer already used, so the protocol was known to work here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two that changed behaviour rather than wording: - The action-cache doc taught a stampede. The prose promised single-flight while the example computed the value in the action, between a read query and a write mutation. A Durable Object interleaves across `await`, so every concurrent caller missed, every one computed, every one wrote. The example now claims a lease in a mutation — which is the part the DO actually serialises — and the callout says so instead of claiming the whole pattern is free. - `bindingNameOf` keyed array entries on `binding` alone, so queue and route entries (keyed by `queue`/`pattern`) all collapsed onto one another during a registry merge. The rest are local: a lint that named the wrong storage tier in its remediation, a scheduler target that accepted a bare string, `ctx.meta` missing from the ctx types, and an import-command flag that documented a default it did not apply. Also cleared six lint errors introduced by this branch — two cognitive complexity refactors, two hoisted regexes, one abbreviation, and a test binding that discarded the value it was meant to assert on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comments cited an external tracker by id, which told a reader nothing they could act on and leaked where the work came from. Each citation is replaced by the defect itself — the wording that was already there, minus the reference — so the comment now explains the code to someone who has only the code. No behaviour change; one test title also drops its id. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/advisor/src/lints/static/owner-field-from-args-not-auth.ts (1)
57-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefine
cacheKeyonce before the visibility branch.Lines 58 and 67 build the same cache key. Define it with
metadataand reuse it in bothemitcalls.Proposed refactor
const metadata = { exportName: write.exportName, field: write.field, file: write.file, line: write.line, method: write.method, visibility: write.visibility ?? "unknown", }; + const cacheKey = `owner_field_from_args_not_auth:${write.file}:${write.line.toString()}:${write.field}`; if (write.visibility === "internal") { return emit(ownerFieldFromArgsNotAuth, { - cacheKey: `owner_field_from_args_not_auth:${write.file}:${write.line.toString()}:${write.field}`, + cacheKey, // ... }); } return emit(ownerFieldFromArgsNotAuth, { - cacheKey: `owner_field_from_args_not_auth:${write.file}:${write.line.toString()}:${write.field}`, + cacheKey, // ... });As per coding guidelines,
packages/**/*.tsmust “Follow DRY principles.”🤖 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/advisor/src/lints/static/owner-field-from-args-not-auth.ts` around lines 57 - 68, In the owner-field diagnostic flow, define the shared cache key once before the visibility branch, alongside metadata, and reuse that variable in both emit calls for ownerFieldFromArgsNotAuth. Remove the duplicated template-string construction while preserving the existing key format and branch behavior.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/dev/handler.ts`:
- Around line 1030-1038: Update the attached-mode log message in the
plan.workerEnabled branch to build its running-service label from
plan.codegenEnabled and plan.studioEnabled, reporting only services actually
enabled. Preserve the existing workerOrigin context and waitForInterrupt
behavior, including handling when both services are disabled.
In `@packages/codegen/src/discover-unregistered-procedures.ts`:
- Around line 86-90: Update discoverFileFunctions to also process named aliases
from source.getExportStatements(), resolve each alias to its local declaration,
and use the exported alias name when checking registration and creating the
FunctionIR/advisory. Preserve existing direct exported-variable handling, and
add a regression covering export { procedure as renamedProcedure } so the
renamed export is reported.
In `@packages/sql-store/src/ctx-db.ts`:
- Around line 2714-2722: Make the async search iterator’s capped termination
explicit in the flow using ITERATOR_PAGE_SIZE, searchPageScan, and isDone. When
MAX_SEARCH_SCAN limits the result window, report the same explicit LIMIT
condition as collect() instead of treating the single capped page as naturally
exhausted; preserve normal isDone behavior for uncapped searches.
---
Nitpick comments:
In `@packages/advisor/src/lints/static/owner-field-from-args-not-auth.ts`:
- Around line 57-68: In the owner-field diagnostic flow, define the shared cache
key once before the visibility branch, alongside metadata, and reuse that
variable in both emit calls for ownerFieldFromArgsNotAuth. Remove the duplicated
template-string construction while preserving the existing key format and branch
behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b7b8bdaf-93d7-4d80-9d60-df81f5ec355b
⛔ Files ignored due to path filters (34)
.vis/templates/lunora-cron.tsis excluded by!**/.vis/**and included by noneapi-snapshots/advisor.api.mdis excluded by none and included by noneapi-snapshots/cli.api.mdis excluded by none and included by noneapi-snapshots/codegen.api.mdis excluded by none and included by noneapi-snapshots/runtime.api.mdis excluded by none and included by noneapi-snapshots/scheduler.api.mdis excluded by none and included by noneapi-snapshots/server.api.mdis excluded by none and included by noneapi-snapshots/shard-engine.api.mdis excluded by none and included by noneapps/playground/tsconfig.generated.jsonis excluded by!**/*.generated.*and included byapps/**examples/payment-demo/lunora/_generated/shard.tsis excluded by!**/_generated/**and included by nonepackages/advisor/__tests__/filter-scope.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/advisor/__tests__/owner-field-from-args-not-auth.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/browser/__tests__/create-browser.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/cli/__tests__/commands/data-transfer.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/cli/__tests__/commands/dev.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/cli/__tests__/commands/registry-items.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/cli/__tests__/commands/rules.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/codegen/__tests__/discover-crons.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/codegen/__tests__/discover-secrets.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/codegen/__tests__/discover-workflows.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/codegen/__tests__/emit-api.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/codegen/__tests__/fixtures/simple/expected/_generated/app.tsis excluded by!**/_generated/**,!**/__tests__/**and included bypackages/**packages/codegen/__tests__/fixtures/simple/expected/_generated/drizzle.global.tsis excluded by!**/_generated/**,!**/__tests__/**and included bypackages/**packages/codegen/__tests__/parse-validator-from.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/codegen/__tests__/run-codegen.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/runtime/__tests__/create-worker.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/scheduler/__tests__/cron-jobs.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/scheduler/__tests__/cron-parity.jsonis excluded by!**/__tests__/**and included bypackages/**packages/server/__tests__/builder.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/server/__tests__/functions-v-from.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/server/__tests__/functions.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**packages/server/__tests__/types.test-d.tsis excluded by!**/__tests__/**and included bypackages/**packages/shard-engine/__tests__/ctx-db.iterate.test.tsis excluded by!**/__tests__/**,!**/*.test.tsand included bypackages/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by none
📒 Files selected for processing (58)
apps/docs/src/content/docs/concepts/caching.mdxapps/docs/src/content/docs/concepts/monorepos-and-iac.mdxapps/docs/src/content/docs/getting-started.mdxapps/docs/src/content/docs/meta.jsonapps/docs/src/content/docs/migrating/from-a-convex-toolkit.mdxapps/docs/src/content/docs/migrating/from-convex.mdxapps/playground/lunora/workflows.tsapps/playground/package.jsonpackages/advisor/src/index.tspackages/advisor/src/lints/static/filter-on-primary-key.tspackages/advisor/src/lints/static/filter-without-index.tspackages/advisor/src/lints/static/owner-field-from-args-not-auth.tspackages/advisor/src/owner-field-writes.tspackages/advisor/src/queries.tspackages/auth/package.jsonpackages/browser/src/create-browser.tspackages/browser/src/types.tspackages/cli/src/commands/codegen/handler.tspackages/cli/src/commands/codegen/index.tspackages/cli/src/commands/data-transfer.tspackages/cli/src/commands/dev/handler.tspackages/cli/src/commands/dev/index.tspackages/cli/src/commands/dev/lifecycle.tspackages/cli/src/commands/import/handler.tspackages/cli/src/commands/import/index.tspackages/cli/src/commands/registry/apply.tspackages/cli/src/commands/registry/commands.tspackages/cli/src/commands/rules/handler.tspackages/cli/src/commands/rules/index.tspackages/codegen/src/assert-required-packages.tspackages/codegen/src/discover-crons.tspackages/codegen/src/discover-functions.tspackages/codegen/src/discover-owner-field-writes.tspackages/codegen/src/discover-package-dependencies.tspackages/codegen/src/discover-queries.tspackages/codegen/src/discover-schema.tspackages/codegen/src/discover-secrets.tspackages/codegen/src/discover-unregistered-procedures.tspackages/codegen/src/discover-workflows.tspackages/codegen/src/emit-app.tspackages/codegen/src/emit.tspackages/codegen/src/ir.tspackages/codegen/src/parse-validator.tspackages/codegen/src/run-codegen.tspackages/codegen/src/secret-rules.tspackages/runtime/src/create-worker.tspackages/scheduler/README.mdpackages/scheduler/docs/index.mdxpackages/scheduler/src/index.tspackages/scheduler/src/jobs.tspackages/server/src/builder/index.tspackages/server/src/builder/types.tspackages/server/src/http.tspackages/server/src/types.tspackages/shard-engine/src/ctx-db.tspackages/shard-engine/src/schema-types.tspackages/sql-store/src/ctx-db.tspackages/testing/src/harness.ts
🚧 Files skipped from review as they are similar to previous changes (51)
- packages/shard-engine/src/schema-types.ts
- apps/playground/package.json
- packages/scheduler/src/index.ts
- apps/docs/src/content/docs/getting-started.mdx
- packages/server/src/http.ts
- packages/scheduler/README.md
- apps/docs/src/content/docs/meta.json
- packages/scheduler/docs/index.mdx
- apps/docs/src/content/docs/concepts/monorepos-and-iac.mdx
- packages/cli/src/commands/dev/index.ts
- packages/codegen/src/discover-crons.ts
- packages/codegen/src/discover-secrets.ts
- packages/advisor/src/lints/static/filter-on-primary-key.ts
- packages/cli/src/commands/rules/index.ts
- packages/testing/src/harness.ts
- packages/shard-engine/src/ctx-db.ts
- packages/codegen/src/discover-workflows.ts
- packages/codegen/src/emit-app.ts
- packages/advisor/src/index.ts
- packages/cli/src/commands/import/index.ts
- packages/codegen/src/discover-package-dependencies.ts
- packages/cli/src/commands/codegen/index.ts
- packages/codegen/src/parse-validator.ts
- packages/auth/package.json
- packages/cli/src/commands/registry/commands.ts
- packages/codegen/src/run-codegen.ts
- packages/advisor/src/queries.ts
- packages/runtime/src/create-worker.ts
- packages/cli/src/commands/registry/apply.ts
- packages/cli/src/commands/codegen/handler.ts
- packages/cli/src/commands/data-transfer.ts
- packages/codegen/src/assert-required-packages.ts
- packages/cli/src/commands/import/handler.ts
- apps/playground/lunora/workflows.ts
- packages/codegen/src/discover-owner-field-writes.ts
- packages/codegen/src/ir.ts
- packages/codegen/src/discover-schema.ts
- packages/codegen/src/discover-queries.ts
- apps/docs/src/content/docs/concepts/caching.mdx
- packages/browser/src/create-browser.ts
- packages/codegen/src/secret-rules.ts
- packages/advisor/src/owner-field-writes.ts
- packages/cli/src/commands/rules/handler.ts
- packages/server/src/builder/types.ts
- packages/codegen/src/discover-functions.ts
- packages/server/src/types.ts
- apps/docs/src/content/docs/migrating/from-convex.mdx
- packages/browser/src/types.ts
- packages/codegen/src/emit.ts
- packages/server/src/builder/index.ts
- packages/scheduler/src/jobs.ts
prisis
force-pushed
the
fix/migration-findings-codegen
branch
from
July 31, 2026 06:54
1983e07 to
67ac48b
Compare
- `export { handler }` dropped a procedure with nothing to look at.
Discovery asks each variable statement whether it `isExported()`, which
is false when the `export` is its own statement, so the procedure never
reached `api.ts`. The advisory that exists to catch exactly this missed
it for the same reason. Worse than the factory form it already covered:
there the initializer is an unreadable call, here it is an ordinary
builder chain, so nothing in the file looks wrong. Reported under the
exported name, since `export { a as b }` is addressed as `b`.
- Search iteration stopped at the cap and looked complete. A page is
capped at MAX_SEARCH_SCAN and the planner refuses any page reaching
past it, so a page sized to the cap comes back full and `isDone`
whether there were exactly that many matches or ten times as many —
the probe row that separates them cannot be fetched. `for await`
therefore returned a silent prefix while `.collect()` on the same query
threw. It now runs the same unbounded read `.collect()` runs and
refuses identically. Nothing is given up: the page size was already the
cap, so the loop read the whole window in one query and a `break` saved
nothing.
- `--no-worker` reported services it had not started. With `--no-codegen`
alongside it, the attached-mode line still claimed the codegen watcher
was running; with `--no-studio` too, it named a service while nothing
ran. The label is now built from the flags.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merging this PR will degrade performance by 11.49%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
prisis
added a commit
that referenced
this pull request
Jul 31, 2026
`assertRequiredPackages` (added on alpha in #240) reads the project manifest and treats one that EXISTS as authoritative — "declares nothing" — while a missing manifest means "cannot tell" and skips the check. This test writes a bare `{ "packageManager": "npm@10.9.0" }` purely to exercise `detectPackageManager`, so the gate started reading it as a project declaring no dependencies and failed the deploy on the fixture's `.global()` tables. The test's intent is "the project declares npm", not "the project declares nothing", so the manifest now carries `@lunora/d1` alongside `packageManager`. Pre-existing on alpha — reproduced on a clean checkout of 37afb05 with none of this branch's changes, and fixed there before porting. Unrelated to the rest of this PR; split out so it can be cherry-picked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
prisis
added a commit
that referenced
this pull request
Jul 31, 2026
…obal reads (#239) * feat(sql-store): coalesce global point reads `ctx.db.get` against a `.global()` backend is a network round-trip, so the idiomatic join — `Promise.all(ids.map(ctx.db.get))` — paid one hop per id. `Promise.all` starts every read in the same tick, so they can be collected before the microtask queue drains and answered with a single `IN (...)`. Batches are per (table, tick) and capped at 50 ids, matching the chunk size the relation pre-resolver already uses against D1's parameter limit. Covers D1 and Hyperdrive alike, since both `.global()` backends route through `@lunora/sql-store`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(shard-engine): add vnode placement ring Routing groundwork for progressive sharding: a fixed vnode ring, FNV-1a and jump consistent hashing, and a directory that expresses "everything is still local". Placement is a pure function of the document id, so a client never names a shard. Bootstraps at `shardCount: 0`, where every vnode maps to LOCAL_SHARD and placement is byte-identical to today's single-DO topology. Rebalance, the per-shard WAL and watermark protocol, and autoscale are deliberately absent — each needs its own design and a half-built version risks misrouting live rows. Nothing consumes this yet, so it stays off the package barrel. Named for vnodes rather than shards because `@lunora/platform` already owns a `ShardDirectory`/`resolveShard` pair for a different concept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(shard-engine): meter transaction resources A Durable Object is a single-threaded process with a hard memory ceiling, so one unbounded mutation takes the whole shard down and the only diagnostic is an isolate that vanished. That is a bad way to learn a query needed an index. Reads and writes are now charged against per-transaction ceilings (32 MiB written, 50k rows written, 100k rows read) and the transaction stops with a `TRANSACTION_LIMIT_EXCEEDED` naming the ceiling it hit. Metering rides on the `onRead`/`onWrite` hooks every path already awaits, so no write bypasses it; `insertManyUnsafe` charges its batch before the multi-row INSERT, since metering after the fact would let the batch materialize in full first. The counters deliberately have no rollback path — the bytes were materialized whether or not a savepoint kept them, and a meter that rewound could not bound a retry loop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(shard-engine): range-precise query invalidation `ctx.db.query(table)` stamped its dependency eagerly, before the chain revealed whether `.withIndex(...)` was coming, so it had to assume the whole table — and every live query on a table re-ran on every write to it. The dependency now comes from the reader's terminal, where the plan is final, and an indexed read records the contiguous index slice it actually touched. Slices are half-open `[lo, hi)` over an order-preserving encoding that matches SQLite's ordering, including UTF-8 byte order for text (JS compares UTF-16 code units, which disagrees on astral characters). Half-open is what makes exclusive bounds correct on compound indexes. Ranges travel as objects on their own channel, never encoded into the row-dependency string space: a document id is arbitrary user data, so a range packed into the same strings could be forged by an id shaped like one — and a forged range reads as "this table is narrowed", which suppresses invalidations rather than adding them. Write positions are computed once on the write path, unioned across the row's before- and after-images, and carried on the delta. A consumer working from the post-image alone would see only where a patched row landed, never the slice it left, and a subscriber watching that slice would never be woken. Subscriptions report their own read footprint rather than reading a shared per-request field: a deferred re-run interleaves with unrelated dispatches, so a shared field would stamp one request's slices onto another subscriber's memo. A re-run also gets its own resource budget instead of spending the writing mutation's. Narrowing is an optimization, never a correctness input — an unencodable value, an unmodelled comparator, an unknown write position, or a table read in more than one way all fall back to whole-table invalidation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(shard-engine): address PR review findings Review of #239 surfaced three correctness bugs in the new code plus a CI break; all are fixed here. Generated shards imported `IndexKeyEntry` but only ever inferred it, so every downstream project failed `noUnusedLocals` — this was the `Lint (types)` failure. Dropped from the emitted imports; `KeyRange` stays because generated signatures name it. A deferred subscription re-run was selected by a `refreshingSubscriptions` instance flag, which says "a refresh is in flight", not "this caller is one". The drain runs in the background, so a concurrent `/rpc` dispatch building its `ctx.db` during it also took that branch and got a throwaway meter — losing its own ceiling entirely. The re-run's budget is now threaded by value from the generated `executeSubscription`, the way identity already is. `insertManyUnsafe` charged every row twice: once in the pre-write loop that bounds the batch, then again through the metering `onWrite` wrapper. It now notifies through the raw hook, so the effective ceiling is what it says. `.global()` insert broadcasts derived index keys from an image that was never stored — defaults and `_creationTime` are applied on the far side, so a key could name a position the row does not occupy and prove a write outside a slice that contains it. Those broadcasts now omit keys and fall back to whole-table invalidation. Also: the read meter could not see scan-shaped reads (a full scan stamps one `*scan` dep, and a `.filter()` terminal returned only survivors) — both now charge the window they materialized; a non-positive `maxBatch` spun the batcher's chunk loop forever; the cache's unserializable-entry fallback charged the module default rather than the instance cap; `estimateBytes` documents that it measures UTF-16 code units, not bytes; and the ring's 64-bit truncation uses `BigInt.asUintN` so the wrap-around is explicit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(shard-engine): type-check the new batch-meter test `insertManyUnsafe` is optional on `DatabaseWriterLike` — the global/D1 twin has no batch primitive — so calling it directly failed `tsc` under `strictNullChecks`. Narrow it explicitly instead. This was the remaining `Lint (types)` CI failure. It went unnoticed locally because the per-package check was being filtered to `src/` errors, which hid everything under `__tests__/`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(shard-engine): lift the batch-writer guard out of the test `vitest/no-conditional-in-test` (warning, and warnings are errors here) rejects the narrowing guard inside the test body. Move it into a module-level helper so the optional `insertManyUnsafe` is narrowed once, outside any test. Caught by the repo-wide `lint:eslint`, which I had not re-run after the previous fix — the per-package check I was using ran before that edit existed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(shard-engine): bind each bigint step explicitly The static analyzer kept flagging the 64-bit truncation as an implicit undefined-to-number conversion, even after moving to `BigInt.asUintN`. The arithmetic was already correct — `tsc` is clean and the placement tests pin the hash values — but folding a mix of masks, shifts and multiplies into compound assignments leaves the operand widths to be inferred, by a reader as much as by a tool. Each step now binds to an explicitly-typed intermediate, and the LCG multiplier is a named constant rather than a literal buried in the loop. Behaviour is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(cli): declare the fixture's add-on in the deploy test `assertRequiredPackages` (added on alpha in #240) reads the project manifest and treats one that EXISTS as authoritative — "declares nothing" — while a missing manifest means "cannot tell" and skips the check. This test writes a bare `{ "packageManager": "npm@10.9.0" }` purely to exercise `detectPackageManager`, so the gate started reading it as a project declaring no dependencies and failed the deploy on the fixture's `.global()` tables. The test's intent is "the project declares npm", not "the project declares nothing", so the manifest now carries `@lunora/d1` alongside `packageManager`. Pre-existing on alpha — reproduced on a clean checkout of 37afb05 with none of this branch's changes, and fixed there before porting. Unrelated to the rest of this PR; split out so it can be cherry-picked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sql-store): probe the runtime for fts5 in tests The test dialect hard-coded `supportsFts5: true`, but FTS5 is a property of the RUNTIME, not the dialect: Node 24 bundles a SQLite with the module, Node 22 does not. The suite therefore passed on one CI matrix leg and failed the other with `no such module: fts5`. Probe once and report the real answer. Both legs stay meaningful rather than one being skipped — where FTS5 exists the suite exercises the `fts5` search layout, and where it does not it exercises the portable `inverted` layout, which is the split the store already models for backends without FTS5. Verified passing with the probe forced both ways. Latent since the search work landed: these tests only run when `sql-store` is affected, so the matrix split went unnoticed until a change touched it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * style(sql-store): satisfy jsdoc lines-before-block Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(shard-engine): close the remaining read/write meter gaps Three paths still slipped the transaction ceilings, all in the same family as the earlier fixes — the meter was attached to the paths that report rows, not to every path that materializes them. Geo reads: `runGeoFetch` decodes and scores every candidate in the covering geohash set, then slices to the limit, so `.take(1)` over a wide radius could materialize thousands of rows while the meter saw one. It now reports the pre-slice candidate count. Filtered pagination: `paginateStage` skips the SQL `LIMIT` when in-memory predicates are staged, so filling a one-row page can scan the whole table. It now reports the pre-filter row count, and `paginate` charges whichever of scan or page is larger. `.global()` inserts: both global branches return before `onWrite`, which is where the meter charges — so a transaction could write unbounded rows to a global table without consuming `maxWrittenRows` or `maxWrittenBytes` at all. Both branches now charge directly. Adds a regression test for the pagination case, which fails under the old page-length accounting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Jul 31, 2026
Merged
This was referenced Aug 8, 2026
17 tasks
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
1 participant
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.
Closes ~30 findings from a large third-party Convex→Lunora migration (a 314-file / 92-table backend). Each one was verified against this repo before being acted on — three of the reported bugs no longer reproduced, and are called out below rather than silently skipped.
Codegen correctness
_generated/did not compile under a consumer's strict config, and consumers cannot// @ts-nochecksomeone else's generated file without losing type safety everywhere. Four causes:.references(() => t._id), circular in its own initializer (TS7022/TS7024). Now carries drizzle's(): AnySQLiteColumnannotation — emitted unconditionally, since mutual cycles need it just as much.v.object/v.unioncolumn with a nestedv.id()rendersId<"table">into$type<…>(), butdrizzle.*.tsnever importedId(TS2304).api.tsimportedFunctionReferenceunconditionally (TS6133 when no functions were discovered).pick()fell back tobuckets.default, which widens toStorage | undefinedundernoUncheckedIndexedAccess(TS2322).Guard:
apps/playground/tsconfig.generated.jsontypecheckslunora/_generated/**at repo-base strictness, wired into the playground'slint:types. The app's own tsconfig setsnoUncheckedIndexedAccess: false, which is why theStoragebug escaped CI — the new config reproduced it on the first run.Codegen no longer drops things silently
Codegen registered an export only when its initializer was literally a builder chain. Anything else existed at runtime, never reached
api.ts, and surfaced asProperty 'x' does not existin an unrelated file.export default <procedure>now registers as<module>.default, matching Convex. Previously the whole module vanished fromapi.ts, so the error named a module and pointed at a caller that was correct.defineTable(fieldsIdentifier)now fails with a diagnostic instead of emitting a table with no columns.RegisteredQuery/Mutation/Actionbut which is absent fromapi.tsbecomes aprocedure_not_registeredadvisory. Type-level, so the indirection that causes the bug cannot hide it.Plus actionable errors for nested index paths and for a
.global()table without@lunora/d1(previously codegen succeeded and onlytscfailed, inside a generated file).Advisor: make ERROR mean something, then gate on it
Gating is only useful if ERROR is trustworthy, and it was not — the reporter's 21 ERRORs were 21 false positives. Fixed the rules first:
owner_field_from_args_not_authfired oninternal*procedures (9 of 21), where "any caller can act as any user" is simply false. Now INFO there, worded to redirect at the public callers, which is where the real vector is.hardcoded_secretfired on the W3C Trace Context spec's example trace ids in a test file (10 of 21). Only the two heuristic kinds are affected; vendor-prefixed keys are still reported everywhere.filter_without_indexreported a.shardBy()table identically to an unbounded.global()scan.filter_on_primary_key:.filter(d => d._id === x)is always wrong, never a judgement call.lunora codegenthen exits non-zero on ERROR, defaulting on in CI,--no-strict-advisoriesto opt out.Type surface
ctx.run*accepts the reference the generated api actually hands you (~370 errors on their port, the largest remaining class). The documented example in this package's own JSDoc did not typecheck..output()drives the emitted return type, not the handler. A declaredv.unionwhose handler returns one arm no longer makes the other arm unreachable to callers, and a strayas anyno longer erases the whole signature.v.from()argument types are recovered from~standard.types.output, so arguments behind the Standard Schema bridge stop typing asunknown..meta()on all six builders — merges, readable from middleware asctx.meta, visible to codegen, so per-procedure policy is enumerable data again.New capability
HttpActionCtx.scheduler— "receive webhook → enqueue → return 200" is what HTTP actions are for. The workaround needed a mutation hop plus a closed allow-list, because a free-form target string on an unauthenticated endpoint is a "call any internal function" primitive.ctx.browsersession reuse (sessions(),connect(),keepAlive). Without it every step of an agent browsing flow got a fresh browser, soclickran against a blank page — silently.for await (const row of ctx.db.query(t)…). Userland merged-index streams previously materialised each branch with.take(1024), so one row cost 1,024 reads per branch.lunora import <convex-export-dir>andlunora dev --no-worker.crons.hourly, and both interval dead-ends now namecrons.daily.Three findings that did not reproduce
Verified against this repo rather than trusted:
_ctxtypo inInternalActionBuilder— already fixed.api.ts" — it exits 1, and writes nothing on a cold run / leaves prior output untouched on a warm one. That ordering was load-bearing and unasserted, so it now has a regression test.Two asks deliberately not adopted
@lunora/cachepackage. Two of the three reported uses are outbound HTTP, better served byfetch(url, { cf: { cacheTtl } }). The third — a genuinely computed result — is a ~40-line table recipe, now documented. The part that usually justifies a package is single-flight, and a Durable Object gives you the hard half of that (an atomic claim, since a mutation runs to completion without interleaving) in one mutation. Note the docs originally overclaimed here — see the review round below.allowExplicitIdandv.id()validates only "is a string", so Convex ids carry across verbatim and every FK stays correct. One pass. Both self-referential and cross-table cycles are covered by a test that would fail under any scheme that minted new ids.Review rounds already applied
Both
/thermospasses and a full automated review ran against this branch; their findings are fixed in the last four commits. The three that mattered most:@lunora/sql-storeand@lunora/testingboth failedtsc— I had typechecked every package I edited and none of their dependents. Worse than the build break: making the query iterator required meant the publicTableReaderpromised an iterator that only the DO backend had, sofor awaitover a.global()table typechecked and threw. Implemented it there too.await, so every concurrent caller missed, computed, and wrote. The example now claims a lease inside a mutation, and the callout states what the DO does and does not give you for free.v.from()recovery never worked outside its unit test. Two independent causes — the resolver was registered ~230 lines after the discovery that parses validators, andSCALAR_TYPE_BY_KINDshort-circuitedfrombefore the switch arm that reads the recovered type. The test registered the resolver itself and stopped at the IR, so it passed over a feature that producedunknownin the real pipeline. Now guarded end-to-end throughrunCodegento the emittedapi.ts; verified it fails when either cause is reintroduced.Also fixed:
--no-strict-advisorieswas an unknown-option error (the flag printed in the failure message hard-errored the CLI);filter_on_primary_keymatched!==and told you to replace it withctx.db.get;--no-workerwould have parked the vite flavor having started nothing;.output()with a hoisted validator downgraded the return type tounknown; the HTTP scheduler accepted a bare"ns:fn"string on unauthenticated endpoints;ctx.metawas undeclared on the ctx types; andBrowser.connect's documented flow was unreachable.Notes for review
origin/alpha(826bf7591). One real conflict — the platform-abstraction commit vs--strict-advisoriesin the codegen CLI handler; both additive, both kept.api-snapshots/observability.api.mddrifts onalphaalready (committed by f9a506b, not regenerated). Untouched here.@lunora/clitest failure (launches wrangler through npx) — this branch touches neither that test nor the deploy/exec code it covers.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation