Repository navigation
Add external package invocation API - #275
Merged
kentcdodds merged 8 commits intoApr 27, 2026
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds a new external package invocation API: contributor docs, DB migration for tokens/invocations, worker routing and HTTP handler with token auth and validation, service implementation with idempotency and bundled-module execution, persistence layer, and comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as HTTP Handler (worker)
participant Repo as Token/Invocation Repo (D1)
participant Service as Invocation Service
participant Storage as Package Storage / Artifact
participant Executor as Bundled Module Executor
Client->>Handler: POST /api/package-invocations/{pkgId}/{exportName}\nAuthorization: Bearer <token>\nJSON body (idempotencyKey, params, source, topic)
Handler->>Repo: hash token & lookup active token by hash
Repo->>Handler: token record / scopes (or null -> 401)
Handler->>Repo: update token last_used_at
Handler->>Handler: validate JSON body (idempotencyKey, params, source, topic)
Handler->>Service: invokePackageExport({ tokenScope, request, baseUrl })
Service->>Repo: getPackageInvocationByKey (user, token, pkg, export, idempotencyKey)
alt existing terminal record
Repo-->>Service: stored terminal response
Service-->>Handler: replayed response (mark idempotency.replayed)
else in-progress or mismatch
Repo-->>Service: in-progress or mismatched -> 409
Service-->>Handler: conflict response
else new invocation
Service->>Repo: insertPackageInvocationRow (status: in_progress)
Service->>Storage: ensure/load published bundle artifact
alt artifact missing
Storage->>Storage: build/typecheck and persist artifact
end
Service->>Executor: run bundled module with registry & per-invocation storage
Executor-->>Service: result or execution error + logs
Service->>Repo: updatePackageInvocationResult (completed|failed + response_json)
Service-->>Handler: success or error response (status + JSON)
end
Handler-->>Client: HTTP response with status and JSON body
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kentcdodds
marked this pull request as ready for review
April 27, 2026 15:25
Contributor
|
🔎 Preview deployed: https://kody-pr-275.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
packages/worker/migrations/0029-package-invocations.sql (2)
1-16: Consider a CHECK constraint onstatus.
repo.tsonly ever writes'in_progress' | 'completed' | 'failed', but the column accepts any TEXT. A CHECK constraint catches drift if a future writer (or a manual edit) introduces an unexpected value, and gives a clearer failure than a downstream parse path.♻️ Proposed addition
- status TEXT NOT NULL, + status TEXT NOT NULL CHECK (status IN ('in_progress', 'completed', 'failed')),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/migrations/0029-package-invocations.sql` around lines 1 - 16, Add a CHECK constraint on the package_invocations.status column to restrict values to the expected set ('in_progress', 'completed', 'failed'). Update the migration that creates the package_invocations table (table name: package_invocations, column: status) to include a CHECK (status IN (...)) clause, or provide a follow-up ALTER TABLE ... ADD CONSTRAINT statement to enforce the same rule for existing databases; ensure the constraint name is unique (e.g., package_invocations_status_check) and that existing rows validate or are migrated before applying the constraint.
1-28: Plan a retention/cleanup strategy forpackage_invocations.Idempotency replay requires keeping rows around long enough to deduplicate retries, but D1 row counts and storage are bounded and this table will grow indefinitely on any high-throughput integration (e.g. the Discord gateway proxy). Consider one of:
- An
expires_at(or TTL) column populated at insert and a periodic sweeper job that deletes expired rows.- A scheduled maintenance route (similar to
__maintenance/reindex-*) that prunes rows older than N days.- Documenting the retention guarantee callers can rely on for idempotency replay.
Also worth confirming whether stuck
in_progressrows are reaped on failure or left to be overwritten/replayed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/migrations/0029-package-invocations.sql` around lines 1 - 28, The migration for package_invocations lacks any retention/cleanup strategy which will let the table grow unbounded; add an expires_at (timestamp/text) column to package_invocations, populate it at insert based on the desired retention window, add an index on expires_at (or a partial index if supported), and implement a periodic sweeper job or a maintenance route that deletes rows where expires_at < now() to enforce TTL; also ensure the idempotency identity index (idx_package_invocations_identity) and any insert/update paths (where inserts use idempotency_key/export_name) account for replays by either skipping inserts for expired rows or allowing overwrites, and add logic to detect and reap stale in_progress rows (e.g., transition in_progress to failed after a timeout) so stuck invocations don’t block deduplication.packages/worker/src/env-schema.ts (1)
175-195: Optional: prefer returningfail()overthrow/catchinnormalizeOptionalStringArray.Other schemas in this file (e.g.
optionalRemoteConnectorSecretsSchema) consistently returnfail(...)from validation callbacks. Usingthrow new Error(...)and catching it at line 291 is functionally equivalent but breaks the local convention and obscures control flow. Threadingcontext.path/failthroughnormalizeOptionalStringArray(or returning a discriminated result) would align with the rest of the module.Also applies to: 217-298
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/env-schema.ts` around lines 175 - 195, normalizeOptionalStringArray currently throws Error instead of using the validation callback's fail convention; change its signature to accept the validation context (e.g., ctx or { path, fail }) and replace throws with ctx.fail(...) (or call fail with the same message) and return undefined on failure, then update all callers (e.g., optionalRemoteConnectorSecretsSchema and other places around the function) to pass the validation context through; this preserves the existing control flow/return semantics used elsewhere in the file and keeps validation errors emitted via the schema's fail mechanism.packages/worker/src/package-invocations/service.ts (3)
197-201: Brittle error-message string match for missing-export detection.
isMissingPackageExportErrormatches on the substring'does not define export'. Any future wording change inresolvePackageExportPath(or whichever upstream throws this) silently downgrades a 404export_not_foundinto a generic 500invocation_failed, which is also persisted asfailedinpackage_invocations.Prefer a typed/sentinel error (e.g. a dedicated
PackageExportNotFoundErrorclass or a tagged error code) thrown at the source and caught here byinstanceof/ discriminator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-invocations/service.ts` around lines 197 - 201, The current isMissingPackageExportError function relies on fragile string matching of error.message; change the upstream throw in resolvePackageExportPath (or the module that detects missing exports) to throw a sentinel error (e.g., a PackageExportNotFoundError class or an Error with a stable code property like error.code === 'PACKAGE_EXPORT_NOT_FOUND'), then update isMissingPackageExportError to detect that sentinel via instanceof PackageExportNotFoundError or by checking the stable discriminator (error && typeof error === 'object' && (error as any).code === 'PACKAGE_EXPORT_NOT_FOUND') so the missing-export case is robustly recognized.
504-508: Detecting UNIQUE-constraint conflicts via error-message substring is fragile.
message.toLowerCase().includes('unique')couples this code to the exact wording of D1/SQLite error messages. A wording change (or a different driver path) could turn a benign idempotency race into a hard rethrow. If D1 surfaces a structured error code/cause, prefer that; otherwise consider matchingunique constraintmore specifically and adding a comment documenting the contract you depend on.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-invocations/service.ts` around lines 504 - 508, The catch block that currently decides whether to rethrow based on message.toLowerCase().includes('unique') is fragile; update it to first check for a structured error code or cause (e.g., inspect error.code or error.cause) and treat known D1/SQLite unique-constraint codes as the idempotent case, and if neither structured fields exist, match a more specific phrase like 'unique constraint' (and add a brief comment documenting this fallback contract). In other words, in the catch handling around the variable message and the thrown error, prefer structured checks (error.code / error.cause) for D1 unique constraint values, then fall back to message.includes('unique constraint') with a comment describing the dependency on driver wording.
137-141: Use locale-independent key sort for canonical hashing.
localeCompare(without an explicit locale) uses the runtime's default locale and can in principle produce different orderings across environments or future runtime changes. Since the resulting hash is the idempotency identity persisted in D1, any drift would cause same-payload requests to compute differentrequest_hashvalues and bypass replay protection.♻️ Suggested fix
Object.keys(record) - .sort((left, right) => left.localeCompare(right)) + .sort() .map((key) => [key, canonicalizeJsonValue(record[key])]),(
Array.prototype.sortdefaults to UTF-16 code-unit order and is deterministic.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-invocations/service.ts` around lines 137 - 141, The current key sorting uses left.localeCompare(right) which is locale-dependent; change the sort to a locale-independent, deterministic byte/UTF-16 order (e.g. use Array.prototype.sort() with no localeCompare or a simple lexical comparator like (a,b) => a < b ? -1 : a > b ? 1 : 0) when building the canonicalized record (the Object.fromEntries(... .sort(...) .map(...)) block that calls canonicalizeJsonValue(record[key])) so request_hash/idempotency values remain stable across environments.packages/worker/src/package-invocations/http.ts (1)
244-245: Type-coercedsource/topicmake the dedicated validators unreachable.
sourceandtopicare coerced tonullfor any non-string value here, before the explicit validators at Lines 275-298 run. Since the locals computed here are what eventually flow toinvokePackageExport, theinvalid_source/invalid_topic400 responses will only fire whenbody.source/body.topicare themselves non-string truthy values — which they always pass through unchanged at validation time, so those branches do work. However, by thensource/topiclocals were already coerced tonull, so even when those 400 branches don't fire, a non-string falsy value silently becomesnullinstead of erroring.Recommend computing
source/topicafter the validation block so the rejection of malformed values is consistent and the locals always reflect the validated input.♻️ Suggested reorder
const body = parsedBody.body - const source = typeof body.source === 'string' ? body.source : null - const topic = typeof body.topic === 'string' ? body.topic : null if ( body.params !== undefined && @@ if (body.topic != null && typeof body.topic !== 'string') { ... } + const source = typeof body.source === 'string' ? body.source : null + const topic = typeof body.topic === 'string' ? body.topic : null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-invocations/http.ts` around lines 244 - 245, The code currently coerces locals const source and const topic to null early (using typeof checks) which makes the later validators (the invalid_source/invalid_topic branches) unreachable and causes non-string falsy inputs to be silently converted to null; move the computation of the locals source and topic so they are derived from body only after the explicit validation block has run (i.e., evaluate source/topic after the validators complete and before calling invokePackageExport), ensuring the validators see the original body.source/body.topic and the locals reflect the validated values passed into invokePackageExport.packages/worker/src/package-invocations/repo.ts (1)
99-104: Storing HTTP status as a stringified number is awkward.
response.statusis already anumberonPackageInvocationStoredResponse, but it's serialized asString(input.row.response.status)here (and again at Line 180 inupdatePackageInvocationResult). On read inparseStoredResponse(Line 47-50), the code only accepts string and re-parses withNumber.parseInt, so any future write that emits the integer directly will be silently treated as an unparseable response and trigger needless re-execution instead of replay.Recommend storing the integer directly and accepting
typeof === 'number'(or'string'for backward compatibility) on read.♻️ Suggested fix
- ? JSON.stringify({ - status: String(input.row.response.status), - body: input.row.response.body, - }) + ? JSON.stringify({ + status: input.row.response.status, + body: input.row.response.body, + }) : null…and mirror in
updatePackageInvocationResult, withparseStoredResponseupdated accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-invocations/repo.ts` around lines 99 - 104, The code currently stringifies response.status when writing (in the response construction inside repo.ts and the similar spot in updatePackageInvocationResult), but parseStoredResponse only accepts strings and re-parses them, causing mismatches if an integer is written; update the writers (the response creation in repo.ts and the updatePackageInvocationResult writer) to store response.status as a number (not String(...)) and then update parseStoredResponse to accept either a number or a string (treat a number as the canonical value and still support string by parsing Number.parseInt for backward compatibility) so reads and writes are consistent and do not trigger false replays.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/contributing/environment-variables.md`:
- Around line 132-177: The new "External package invocation API" section has
been inserted between "Home connector bridge" and "Remote connector secrets
(Worker)", causing "Remote connector secrets (Worker)" to be nested incorrectly;
fix by either promoting the "Remote connector secrets (Worker)" heading from
"### Remote connector secrets (Worker)" to a top-level "## Remote connector
secrets (Worker)" or by moving the entire "External package invocation API"
block so it appears after the "Remote connector secrets (Worker)" block,
ensuring the three headings ("Home connector bridge", "External package
invocation API", "Remote connector secrets (Worker)") retain proper sibling
hierarchy.
In `@packages/worker/src/env-schema.ts`:
- Around line 197-302: The validation currently allows duplicate bearer strings
across PACKAGE_INVOCATION_TOKENS entries; add a uniqueness check so identical
token values cause a fail() during schema parsing. While building out[tokenId]
(inside the loop where you compute token, userId, etc.), maintain a Set (e.g.,
seenTokens) and after extracting the token string, check if
seenTokens.has(token) and call fail(`Duplicate PACKAGE_INVOCATION_TOKENS "token"
value for "${tokenId}"`) with context.path if so; otherwise add the token to
seenTokens and continue to populate out. Ensure the check references the local
token variable, the out map, and uses the existing fail(...) and context.path
semantics.
In `@packages/worker/src/package-invocations/repo.ts`:
- Around line 69-79: mapRow currently throws on parseSafe failures which bubbles
up to getPackageInvocationByKey and causes unhandled rejections in
invokePackageExport; change mapRow to treat schema-mismatch rows as absent by
catching parseSafe failures, logging the issue, and returning null (i.e., change
its signature to return PackageInvocationRecord | null and stop throwing). Then
update getPackageInvocationByKey to handle a null return from mapRow and return
null to callers, and ensure invokePackageExport (referenced at
invokePackageExport lines where getPackageInvocationByKey is called) already
handles null as “not found” (or add that handling if missing) so corrupt rows
don’t produce 500s or block retries. Ensure logging includes the parsed.issues
message so the corruption is observable.
In `@packages/worker/src/package-invocations/service.ts`:
- Around line 487-539: The current catch block around insertPackageInvocationRow
rethrows any non-UNIQUE errors which bubbles out of invokePackageExport and
produces an unstructured 500; instead, handle non-unique failures by returning a
structured JSON error response so callers always receive the { ok: false, error:
... } contract. Concretely: in the catch block in invokePackageExport (the block
wrapping insertPackageInvocationRow), when message does NOT include 'unique' do
NOT throw error—return buildJsonErrorResponse({ status: 500, code:
'invocation_failed', message: 'Failed to create package invocation', details:
message }) (or similar) so handlePackageInvocationApiRequest receives a stable
response; keep existing idempotency-handling for UNIQUE errors unchanged. Ensure
you reference insertPackageInvocationRow, invokePackageExport, and
buildJsonErrorResponse when applying this change.
---
Nitpick comments:
In `@packages/worker/migrations/0029-package-invocations.sql`:
- Around line 1-16: Add a CHECK constraint on the package_invocations.status
column to restrict values to the expected set ('in_progress', 'completed',
'failed'). Update the migration that creates the package_invocations table
(table name: package_invocations, column: status) to include a CHECK (status IN
(...)) clause, or provide a follow-up ALTER TABLE ... ADD CONSTRAINT statement
to enforce the same rule for existing databases; ensure the constraint name is
unique (e.g., package_invocations_status_check) and that existing rows validate
or are migrated before applying the constraint.
- Around line 1-28: The migration for package_invocations lacks any
retention/cleanup strategy which will let the table grow unbounded; add an
expires_at (timestamp/text) column to package_invocations, populate it at insert
based on the desired retention window, add an index on expires_at (or a partial
index if supported), and implement a periodic sweeper job or a maintenance route
that deletes rows where expires_at < now() to enforce TTL; also ensure the
idempotency identity index (idx_package_invocations_identity) and any
insert/update paths (where inserts use idempotency_key/export_name) account for
replays by either skipping inserts for expired rows or allowing overwrites, and
add logic to detect and reap stale in_progress rows (e.g., transition
in_progress to failed after a timeout) so stuck invocations don’t block
deduplication.
In `@packages/worker/src/env-schema.ts`:
- Around line 175-195: normalizeOptionalStringArray currently throws Error
instead of using the validation callback's fail convention; change its signature
to accept the validation context (e.g., ctx or { path, fail }) and replace
throws with ctx.fail(...) (or call fail with the same message) and return
undefined on failure, then update all callers (e.g.,
optionalRemoteConnectorSecretsSchema and other places around the function) to
pass the validation context through; this preserves the existing control
flow/return semantics used elsewhere in the file and keeps validation errors
emitted via the schema's fail mechanism.
In `@packages/worker/src/package-invocations/http.ts`:
- Around line 244-245: The code currently coerces locals const source and const
topic to null early (using typeof checks) which makes the later validators (the
invalid_source/invalid_topic branches) unreachable and causes non-string falsy
inputs to be silently converted to null; move the computation of the locals
source and topic so they are derived from body only after the explicit
validation block has run (i.e., evaluate source/topic after the validators
complete and before calling invokePackageExport), ensuring the validators see
the original body.source/body.topic and the locals reflect the validated values
passed into invokePackageExport.
In `@packages/worker/src/package-invocations/repo.ts`:
- Around line 99-104: The code currently stringifies response.status when
writing (in the response construction inside repo.ts and the similar spot in
updatePackageInvocationResult), but parseStoredResponse only accepts strings and
re-parses them, causing mismatches if an integer is written; update the writers
(the response creation in repo.ts and the updatePackageInvocationResult writer)
to store response.status as a number (not String(...)) and then update
parseStoredResponse to accept either a number or a string (treat a number as the
canonical value and still support string by parsing Number.parseInt for backward
compatibility) so reads and writes are consistent and do not trigger false
replays.
In `@packages/worker/src/package-invocations/service.ts`:
- Around line 197-201: The current isMissingPackageExportError function relies
on fragile string matching of error.message; change the upstream throw in
resolvePackageExportPath (or the module that detects missing exports) to throw a
sentinel error (e.g., a PackageExportNotFoundError class or an Error with a
stable code property like error.code === 'PACKAGE_EXPORT_NOT_FOUND'), then
update isMissingPackageExportError to detect that sentinel via instanceof
PackageExportNotFoundError or by checking the stable discriminator (error &&
typeof error === 'object' && (error as any).code === 'PACKAGE_EXPORT_NOT_FOUND')
so the missing-export case is robustly recognized.
- Around line 504-508: The catch block that currently decides whether to rethrow
based on message.toLowerCase().includes('unique') is fragile; update it to first
check for a structured error code or cause (e.g., inspect error.code or
error.cause) and treat known D1/SQLite unique-constraint codes as the idempotent
case, and if neither structured fields exist, match a more specific phrase like
'unique constraint' (and add a brief comment documenting this fallback
contract). In other words, in the catch handling around the variable message and
the thrown error, prefer structured checks (error.code / error.cause) for D1
unique constraint values, then fall back to message.includes('unique
constraint') with a comment describing the dependency on driver wording.
- Around line 137-141: The current key sorting uses left.localeCompare(right)
which is locale-dependent; change the sort to a locale-independent,
deterministic byte/UTF-16 order (e.g. use Array.prototype.sort() with no
localeCompare or a simple lexical comparator like (a,b) => a < b ? -1 : a > b ?
1 : 0) when building the canonicalized record (the Object.fromEntries(...
.sort(...) .map(...)) block that calls canonicalizeJsonValue(record[key])) so
request_hash/idempotency values remain stable across environments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac4ed05e-74be-4064-a106-e632c0d49479
📒 Files selected for processing (15)
.github/workflows/deploy.yml.github/workflows/preview.ymldocs/contributing/environment-variables.mddocs/contributing/index.mddocs/contributing/package-invocation-api.mddocs/contributing/setup-manifest.mdpackages/worker/.env.examplepackages/worker/migrations/0029-package-invocations.sqlpackages/worker/src/env-schema.tspackages/worker/src/index.tspackages/worker/src/package-invocations/http.tspackages/worker/src/package-invocations/http.workers.test.tspackages/worker/src/package-invocations/repo.tspackages/worker/src/package-invocations/service.node.test.tspackages/worker/src/package-invocations/service.ts
Comment on lines
+197
to
+302
| const out: PackageInvocationTokensConfig = {} | ||
| for (const [rawTokenId, rawTokenConfig] of Object.entries(parsed)) { | ||
| const tokenId = rawTokenId.trim() | ||
| if (!tokenId) { | ||
| return fail( | ||
| 'PACKAGE_INVOCATION_TOKENS keys must be non-empty token ids.', | ||
| context.path, | ||
| ) | ||
| } | ||
| if ( | ||
| !rawTokenConfig || | ||
| typeof rawTokenConfig !== 'object' || | ||
| Array.isArray(rawTokenConfig) | ||
| ) { | ||
| return fail( | ||
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" must be an object.`, | ||
| context.path, | ||
| ) | ||
| } | ||
|
|
||
| try { | ||
| const record = rawTokenConfig as Record<string, unknown> | ||
| const token = | ||
| typeof record['token'] === 'string' ? record['token'].trim() : '' | ||
| const userId = | ||
| typeof record['userId'] === 'string' ? record['userId'].trim() : '' | ||
| const email = | ||
| typeof record['email'] === 'string' ? record['email'].trim() : '' | ||
| const displayName = | ||
| typeof record['displayName'] === 'string' | ||
| ? record['displayName'].trim() | ||
| : '' | ||
| const packageIds = normalizeOptionalStringArray( | ||
| record['packageIds'], | ||
| 'packageIds', | ||
| tokenId, | ||
| ) | ||
| const packageKodyIds = normalizeOptionalStringArray( | ||
| record['packageKodyIds'], | ||
| 'packageKodyIds', | ||
| tokenId, | ||
| ) | ||
| const exportNames = normalizeOptionalStringArray( | ||
| record['exportNames'], | ||
| 'exportNames', | ||
| tokenId, | ||
| ) | ||
| const sources = normalizeOptionalStringArray( | ||
| record['sources'], | ||
| 'sources', | ||
| tokenId, | ||
| ) | ||
|
|
||
| if (!token) { | ||
| return fail( | ||
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" requires a non-empty "token" string.`, | ||
| context.path, | ||
| ) | ||
| } | ||
| if (!userId) { | ||
| return fail( | ||
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" requires a non-empty "userId" string.`, | ||
| context.path, | ||
| ) | ||
| } | ||
| if (!email) { | ||
| return fail( | ||
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" requires a non-empty "email" string.`, | ||
| context.path, | ||
| ) | ||
| } | ||
| if (!displayName) { | ||
| return fail( | ||
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" requires a non-empty "displayName" string.`, | ||
| context.path, | ||
| ) | ||
| } | ||
| if (!packageIds && !packageKodyIds) { | ||
| return fail( | ||
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" must declare at least one package scope via "packageIds" or "packageKodyIds".`, | ||
| context.path, | ||
| ) | ||
| } | ||
|
|
||
| out[tokenId] = { | ||
| token, | ||
| userId, | ||
| email, | ||
| displayName, | ||
| ...(packageIds ? { packageIds } : {}), | ||
| ...(packageKodyIds ? { packageKodyIds } : {}), | ||
| ...(exportNames ? { exportNames } : {}), | ||
| ...(sources ? { sources } : {}), | ||
| } | ||
| } catch (error) { | ||
| return fail( | ||
| error instanceof Error | ||
| ? error.message | ||
| : `Invalid PACKAGE_INVOCATION_TOKENS entry "${tokenId}".`, | ||
| context.path, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| return { value: out } | ||
| }) |
There was a problem hiding this comment.
Consider rejecting duplicate bearer tokens across entries.
The schema validates each entry independently but doesn't enforce uniqueness of token values across entries. If two token-ids in PACKAGE_INVOCATION_TOKENS accidentally share the same token string but declare different userId/scope, the runtime bearer-token lookup (presumably an iteration over entries) becomes order-dependent and a misconfiguration could silently grant the wrong scope. A startup-time uniqueness check would make this a fail-fast configuration error.
🛡️ Proposed defensive check
const out: PackageInvocationTokensConfig = {}
+ const seenTokens = new Map<string, string>()
for (const [rawTokenId, rawTokenConfig] of Object.entries(parsed)) {
@@
out[tokenId] = {
token,
userId,
email,
displayName,
...(packageIds ? { packageIds } : {}),
...(packageKodyIds ? { packageKodyIds } : {}),
...(exportNames ? { exportNames } : {}),
...(sources ? { sources } : {}),
}
+ const previousTokenId = seenTokens.get(token)
+ if (previousTokenId) {
+ return fail(
+ `PACKAGE_INVOCATION_TOKENS entries "${previousTokenId}" and "${tokenId}" share the same "token" value.`,
+ context.path,
+ )
+ }
+ seenTokens.set(token, tokenId)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const out: PackageInvocationTokensConfig = {} | |
| for (const [rawTokenId, rawTokenConfig] of Object.entries(parsed)) { | |
| const tokenId = rawTokenId.trim() | |
| if (!tokenId) { | |
| return fail( | |
| 'PACKAGE_INVOCATION_TOKENS keys must be non-empty token ids.', | |
| context.path, | |
| ) | |
| } | |
| if ( | |
| !rawTokenConfig || | |
| typeof rawTokenConfig !== 'object' || | |
| Array.isArray(rawTokenConfig) | |
| ) { | |
| return fail( | |
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" must be an object.`, | |
| context.path, | |
| ) | |
| } | |
| try { | |
| const record = rawTokenConfig as Record<string, unknown> | |
| const token = | |
| typeof record['token'] === 'string' ? record['token'].trim() : '' | |
| const userId = | |
| typeof record['userId'] === 'string' ? record['userId'].trim() : '' | |
| const email = | |
| typeof record['email'] === 'string' ? record['email'].trim() : '' | |
| const displayName = | |
| typeof record['displayName'] === 'string' | |
| ? record['displayName'].trim() | |
| : '' | |
| const packageIds = normalizeOptionalStringArray( | |
| record['packageIds'], | |
| 'packageIds', | |
| tokenId, | |
| ) | |
| const packageKodyIds = normalizeOptionalStringArray( | |
| record['packageKodyIds'], | |
| 'packageKodyIds', | |
| tokenId, | |
| ) | |
| const exportNames = normalizeOptionalStringArray( | |
| record['exportNames'], | |
| 'exportNames', | |
| tokenId, | |
| ) | |
| const sources = normalizeOptionalStringArray( | |
| record['sources'], | |
| 'sources', | |
| tokenId, | |
| ) | |
| if (!token) { | |
| return fail( | |
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" requires a non-empty "token" string.`, | |
| context.path, | |
| ) | |
| } | |
| if (!userId) { | |
| return fail( | |
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" requires a non-empty "userId" string.`, | |
| context.path, | |
| ) | |
| } | |
| if (!email) { | |
| return fail( | |
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" requires a non-empty "email" string.`, | |
| context.path, | |
| ) | |
| } | |
| if (!displayName) { | |
| return fail( | |
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" requires a non-empty "displayName" string.`, | |
| context.path, | |
| ) | |
| } | |
| if (!packageIds && !packageKodyIds) { | |
| return fail( | |
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" must declare at least one package scope via "packageIds" or "packageKodyIds".`, | |
| context.path, | |
| ) | |
| } | |
| out[tokenId] = { | |
| token, | |
| userId, | |
| email, | |
| displayName, | |
| ...(packageIds ? { packageIds } : {}), | |
| ...(packageKodyIds ? { packageKodyIds } : {}), | |
| ...(exportNames ? { exportNames } : {}), | |
| ...(sources ? { sources } : {}), | |
| } | |
| } catch (error) { | |
| return fail( | |
| error instanceof Error | |
| ? error.message | |
| : `Invalid PACKAGE_INVOCATION_TOKENS entry "${tokenId}".`, | |
| context.path, | |
| ) | |
| } | |
| } | |
| return { value: out } | |
| }) | |
| const out: PackageInvocationTokensConfig = {} | |
| const seenTokens = new Map<string, string>() | |
| for (const [rawTokenId, rawTokenConfig] of Object.entries(parsed)) { | |
| const tokenId = rawTokenId.trim() | |
| if (!tokenId) { | |
| return fail( | |
| 'PACKAGE_INVOCATION_TOKENS keys must be non-empty token ids.', | |
| context.path, | |
| ) | |
| } | |
| if ( | |
| !rawTokenConfig || | |
| typeof rawTokenConfig !== 'object' || | |
| Array.isArray(rawTokenConfig) | |
| ) { | |
| return fail( | |
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" must be an object.`, | |
| context.path, | |
| ) | |
| } | |
| try { | |
| const record = rawTokenConfig as Record<string, unknown> | |
| const token = | |
| typeof record['token'] === 'string' ? record['token'].trim() : '' | |
| const userId = | |
| typeof record['userId'] === 'string' ? record['userId'].trim() : '' | |
| const email = | |
| typeof record['email'] === 'string' ? record['email'].trim() : '' | |
| const displayName = | |
| typeof record['displayName'] === 'string' | |
| ? record['displayName'].trim() | |
| : '' | |
| const packageIds = normalizeOptionalStringArray( | |
| record['packageIds'], | |
| 'packageIds', | |
| tokenId, | |
| ) | |
| const packageKodyIds = normalizeOptionalStringArray( | |
| record['packageKodyIds'], | |
| 'packageKodyIds', | |
| tokenId, | |
| ) | |
| const exportNames = normalizeOptionalStringArray( | |
| record['exportNames'], | |
| 'exportNames', | |
| tokenId, | |
| ) | |
| const sources = normalizeOptionalStringArray( | |
| record['sources'], | |
| 'sources', | |
| tokenId, | |
| ) | |
| if (!token) { | |
| return fail( | |
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" requires a non-empty "token" string.`, | |
| context.path, | |
| ) | |
| } | |
| if (!userId) { | |
| return fail( | |
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" requires a non-empty "userId" string.`, | |
| context.path, | |
| ) | |
| } | |
| if (!email) { | |
| return fail( | |
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" requires a non-empty "email" string.`, | |
| context.path, | |
| ) | |
| } | |
| if (!displayName) { | |
| return fail( | |
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" requires a non-empty "displayName" string.`, | |
| context.path, | |
| ) | |
| } | |
| if (!packageIds && !packageKodyIds) { | |
| return fail( | |
| `PACKAGE_INVOCATION_TOKENS entry "${tokenId}" must declare at least one package scope via "packageIds" or "packageKodyIds".`, | |
| context.path, | |
| ) | |
| } | |
| out[tokenId] = { | |
| token, | |
| userId, | |
| email, | |
| displayName, | |
| ...(packageIds ? { packageIds } : {}), | |
| ...(packageKodyIds ? { packageKodyIds } : {}), | |
| ...(exportNames ? { exportNames } : {}), | |
| ...(sources ? { sources } : {}), | |
| } | |
| const previousTokenId = seenTokens.get(token) | |
| if (previousTokenId) { | |
| return fail( | |
| `PACKAGE_INVOCATION_TOKENS entries "${previousTokenId}" and "${tokenId}" share the same "token" value.`, | |
| context.path, | |
| ) | |
| } | |
| seenTokens.set(token, tokenId) | |
| } catch (error) { | |
| return fail( | |
| error instanceof Error | |
| ? error.message | |
| : `Invalid PACKAGE_INVOCATION_TOKENS entry "${tokenId}".`, | |
| context.path, | |
| ) | |
| } | |
| } | |
| return { value: out } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/env-schema.ts` around lines 197 - 302, The validation
currently allows duplicate bearer strings across PACKAGE_INVOCATION_TOKENS
entries; add a uniqueness check so identical token values cause a fail() during
schema parsing. While building out[tokenId] (inside the loop where you compute
token, userId, etc.), maintain a Set (e.g., seenTokens) and after extracting the
token string, check if seenTokens.has(token) and call fail(`Duplicate
PACKAGE_INVOCATION_TOKENS "token" value for "${tokenId}"`) with context.path if
so; otherwise add the token to seenTokens and continue to populate out. Ensure
the check references the local token variable, the out map, and uses the
existing fail(...) and context.path semantics.
Comment on lines
+69
to
+79
| function mapRow(row: Record<string, unknown>): PackageInvocationRecord { | ||
| const parsed = parseSafe(packageInvocationRowSchema, row) | ||
| if (!parsed.success) { | ||
| const message = parsed.issues.map((issue) => issue.message).join(', ') | ||
| throw new Error(`Invalid package invocation record: ${message}`) | ||
| } | ||
| return { | ||
| ...parsed.value, | ||
| storedResponse: parseStoredResponse(parsed.value.response_json), | ||
| } | ||
| } |
There was a problem hiding this comment.
mapRow throws on schema mismatch and the throw is not handled by the caller.
A row that fails parseSafe (e.g. column drift, NULL where unexpected) makes getPackageInvocationByKey reject. invokePackageExport calls it at lines 454 and 509 outside any try/catch that returns a structured error, so a single corrupt row would surface as an unhandled 500 to external callers and also block all subsequent attempts with the same idempotency key from being retried.
Consider either catching mapping errors and returning null (treat-as-absent) plus logging, or wrapping the lookup at the service layer in a structured-error path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/package-invocations/repo.ts` around lines 69 - 79, mapRow
currently throws on parseSafe failures which bubbles up to
getPackageInvocationByKey and causes unhandled rejections in
invokePackageExport; change mapRow to treat schema-mismatch rows as absent by
catching parseSafe failures, logging the issue, and returning null (i.e., change
its signature to return PackageInvocationRecord | null and stop throwing). Then
update getPackageInvocationByKey to handle a null return from mapRow and return
null to callers, and ensure invokePackageExport (referenced at
invokePackageExport lines where getPackageInvocationByKey is called) already
handles null as “not found” (or add that handling if missing) so corrupt rows
don’t produce 500s or block retries. Ensure logging includes the parsed.issues
message so the corruption is observable.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/package-invocations/http.ts`:
- Around line 107-125: resolveTokenScope currently ignores the boolean result of
updatePackageInvocationTokenLastUsed which creates a TOCTOU risk; modify the
logic in resolveTokenScope to treat a false return from
updatePackageInvocationTokenLastUsed as an authentication failure (return
null/deny) instead of proceeding, or replace the two-step
getActivePackageInvocationTokenByHash + updatePackageInvocationTokenLastUsed
flow with a single atomic lookup-and-touch statement so that
getActivePackageInvocationTokenByHash / record lookup and the touch are
combined; specifically locate the call to getActivePackageInvocationTokenByHash
and the subsequent call to updatePackageInvocationTokenLastUsed and either check
the updater's boolean return and abort on false, or refactor into an atomic DB
operation that both validates and updates last_used_at.
In `@packages/worker/src/package-invocations/repo.ts`:
- Around line 203-238: The insert for package_invocations (in the repo.ts insert
block that writes input.row and responseJson) is racy because
getPackageInvocationByKey + unconditional insert allows duplicates; add a UNIQUE
constraint in the DB on (user_id, token_id, package_id, export_name,
idempotency_key) and change the insertion logic in the repo (the current INSERT
block and the analogous block around lines 241-267) to use a conflict-safe
pattern: perform INSERT ... ON CONFLICT (...) DO NOTHING (or DO UPDATE to return
the existing row) and then SELECT the existing row if the insert affected zero
rows, or use RETURNING to obtain the inserted/existing id; alternatively catch
unique-constraint errors from D1 and fetch the existing invocation via
getPackageInvocationByKey, ensuring the operation is atomic at the DB boundary.
- Around line 72-84: The parser parseStringArrayJson must fail-closed instead of
returning [] on malformed JSON; change it to throw (or return null/undefined)
when JSON.parse fails or when parsed value is not an array of strings, and
update the callers that consume package_ids_json, package_kody_ids_json,
export_names_json, and sources_json to treat a thrown error/null as an invalid
token (rejecting the token) rather than treating it as "no restriction." Ensure
all call sites that currently rely on empty-array semantics are updated to
handle the new error/null return and abort auth/token acceptance accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fb76030-f248-4291-9046-f717b34f2f84
📒 Files selected for processing (6)
docs/contributing/package-invocation-api.mdpackages/worker/.env.examplepackages/worker/migrations/0029-package-invocations.sqlpackages/worker/src/package-invocations/http.tspackages/worker/src/package-invocations/http.workers.test.tspackages/worker/src/package-invocations/repo.ts
✅ Files skipped from review due to trivial changes (1)
- packages/worker/.env.example
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/worker/migrations/0029-package-invocations.sql
- packages/worker/src/package-invocations/http.workers.test.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/worker/src/package-invocations/repo.ts (1)
184-197: Touchingupdated_aton every successful auth blurs configuration-change semantics.
updated_atis conventionally reserved for material changes (rename, scope edits, revoke). Bumping it on every API call means any UI sorting or syncing onupdated_atwill treat heavy-use tokens as constantly "edited," and you lose the ability to answer "when was this token last reconfigured?" cheaply. Sincelast_used_atalready captures usage churn, consider leavingupdated_atalone here.♻️ Suggested change
const result = await input.db .prepare( `UPDATE package_invocation_tokens - SET last_used_at = ?, updated_at = ? + SET last_used_at = ? WHERE id = ? AND revoked_at IS NULL`, ) - .bind(new Date().toISOString(), new Date().toISOString(), input.id) + .bind(new Date().toISOString(), input.id) .run()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-invocations/repo.ts` around lines 184 - 197, The updatePackageInvocationTokenLastUsed function is incorrectly touching updated_at on every auth call; change the SQL in updatePackageInvocationTokenLastUsed (and its .bind call) to only SET last_used_at = ? (remove updated_at from the UPDATE and its bind value) so that only last_used_at is updated and updated_at remains reserved for configuration changes.packages/worker/src/package-invocations/http.ts (1)
178-318: Recompute the URL/pathname once.
new URL(request.url).pathnameis reconstructed at lines 178, 202, 217, and 315. Cheap, but easy to consolidate and keeps the audit-log payloads consistent.♻️ Optional refactor
export async function handlePackageInvocationApiRequest( request: Request, env: Env, ) { - const route = parsePackageInvocationPath(new URL(request.url).pathname) + const pathname = new URL(request.url).pathname + const route = parsePackageInvocationPath(pathname) if (!route) { return notFoundResponse() } ... - path: new URL(request.url).pathname, + path: pathname,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-invocations/http.ts` around lines 178 - 318, Compute the request pathname once (e.g., const pathname = new URL(request.url).pathname) near the top and use that variable everywhere instead of calling new URL(request.url).pathname repeatedly; replace its uses in parsePackageInvocationPath(new URL(request.url).pathname), all logAuditEvent calls (path: ...), and the getAppBaseUrl call (requestUrl: request.url — pass pathname or use baseUrl args consistently) so the same pathname is used for route parsing, audit logging, and baseUrl construction.packages/worker/src/package-invocations/service.node.test.ts (1)
293-600: Test coverage is solid and exercises the important branches: replay, corruption, persistence failure, mismatch, execution error sanitization, and terminal failure caching for missing exports.The assertions verify both response shapes and that
runBundledModuleWithRegistryis or isn't called as expected, which guards against regressions where replay accidentally re-executes side effects.Optional: consider adding a test for the
params: undefinedvsparams: {}request-hash equality concern —canonicalizeJsonValuedropsundefinedkeys viaJSON.stringify, while{}is preserved, so the same caller switching between those representations could triggeridempotency_mismatch. Minor and likely benign, but worth pinning down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/package-invocations/service.node.test.ts` around lines 293 - 600, Tests note a potential idempotency mismatch when callers alternate between params: undefined and params: {} because canonicalizeJsonValue (used by invokePackageExport's idempotency key computation) drops undefined keys; to fix, normalize the request payload before hashing: in invokePackageExport ensure the idempotency input canonicalization treats missing vs explicitly empty params the same (e.g., coerce undefined params to {} or otherwise canonicalize undefined -> null consistently) so idempotency_mismatch cannot be triggered by representation differences; update the normalization code path where canonicalizeJsonValue is called and add a unit test verifying params: undefined and params: {} produce the same idempotency key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/contributing/package-invocation-api.md`:
- Around line 80-86: The docs claim `source` is optional but the runtime
enforces it: `service.ts`'s tokenAllowsSource() returns false for null/empty
`source`, causing invokePackageExport to fail with 403; fix by either (A)
updating this docs page and the curl example to state `source` is required and
must match the token's sources_json allowlist (clarify format and behavior), or
(B) change `tokenAllowsSource()` to treat a missing/empty `source` as "no source
claim — skip allowlist check" so omission is allowed; pick one approach and
update the docs and the implementation consistently (reference tokenAllowsSource
and invokePackageExport when making the change).
In `@packages/worker/src/package-invocations/http.ts`:
- Around line 174-318: The audit log calls in handlePackageInvocationApiRequest
(calls to logAuditEvent) are fire-and-forget with "void" and can be cancelled
when the Worker returns; wrap each logAuditEvent call with Cloudflare's
persistent waiter by importing and using waitUntil (import { waitUntil } from
'cloudflare:workers') or by passing an ExecutionContext and using ctx.waitUntil;
update the three invocations in handlePackageInvocationApiRequest (the calls
that log missing_bearer_token, invalid_private_token, and the final
package_invoke result) to use waitUntil(logAuditEvent(...)) so the events are
guaranteed to complete after the response is returned.
In `@packages/worker/src/package-invocations/service.ts`:
- Around line 503-510: The catch blocks that build idempotency error responses
(the one shown returning buildJsonErrorResponse with code
'idempotency_lookup_failed' and the other blocks at the same file with codes
'idempotency_persistence_failed' and the second 'idempotency_lookup_failed')
must not expose raw DB/persistence error.messages to callers; instead log the
raw error server-side and return a stable generic message. Change each catch to
call your server logger (e.g., processLogger.error(error) or the existing logger
variable) to record the full error, and pass a generic human-safe message to
buildJsonErrorResponse (for example "Internal error during idempotency lookup"
or "Internal error persisting idempotency record") while keeping status, code,
and idempotencyKey unchanged; update the three locations (the shown catch plus
the blocks around the other two codes) accordingly.
- Around line 184-191: tokenAllowsSource currently treats a missing source
(null) the same as a disallowed source, causing confusing 403 errors; change
tokenAllowsSource to allow a null source only when the token's sources allowlist
is empty (i.e., treat an empty allowlist as "no source restrictions") otherwise
return false; specifically, in tokenAllowsSource(update the function body) check
if input.source is null and if so return true when (input.token.sources ??
[]).length === 0, otherwise proceed to normal allowlist check, and ensure
callers like invokePackageExport continue to interpret a false return as
"source_not_allowed" while null-is-allowed paths pass through.
---
Nitpick comments:
In `@packages/worker/src/package-invocations/http.ts`:
- Around line 178-318: Compute the request pathname once (e.g., const pathname =
new URL(request.url).pathname) near the top and use that variable everywhere
instead of calling new URL(request.url).pathname repeatedly; replace its uses in
parsePackageInvocationPath(new URL(request.url).pathname), all logAuditEvent
calls (path: ...), and the getAppBaseUrl call (requestUrl: request.url — pass
pathname or use baseUrl args consistently) so the same pathname is used for
route parsing, audit logging, and baseUrl construction.
In `@packages/worker/src/package-invocations/repo.ts`:
- Around line 184-197: The updatePackageInvocationTokenLastUsed function is
incorrectly touching updated_at on every auth call; change the SQL in
updatePackageInvocationTokenLastUsed (and its .bind call) to only SET
last_used_at = ? (remove updated_at from the UPDATE and its bind value) so that
only last_used_at is updated and updated_at remains reserved for configuration
changes.
In `@packages/worker/src/package-invocations/service.node.test.ts`:
- Around line 293-600: Tests note a potential idempotency mismatch when callers
alternate between params: undefined and params: {} because canonicalizeJsonValue
(used by invokePackageExport's idempotency key computation) drops undefined
keys; to fix, normalize the request payload before hashing: in
invokePackageExport ensure the idempotency input canonicalization treats missing
vs explicitly empty params the same (e.g., coerce undefined params to {} or
otherwise canonicalize undefined -> null consistently) so idempotency_mismatch
cannot be triggered by representation differences; update the normalization code
path where canonicalizeJsonValue is called and add a unit test verifying params:
undefined and params: {} produce the same idempotency key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18218581-88c2-4c9e-a535-2421ee1c154e
📒 Files selected for processing (6)
docs/contributing/package-invocation-api.mdpackages/worker/src/package-invocations/http.tspackages/worker/src/package-invocations/http.workers.test.tspackages/worker/src/package-invocations/repo.tspackages/worker/src/package-invocations/service.node.test.tspackages/worker/src/package-invocations/service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/package-invocations/http.workers.test.ts
Comment on lines
+80
to
+86
| Fields: | ||
|
|
||
| - `params` — JSON object passed to the package export as runtime params | ||
| - `idempotencyKey` — required stable key for replay protection | ||
| - `source` — optional source label for auditing and token scoping | ||
| - `topic` — optional event topic label for downstream logic and logs | ||
|
|
There was a problem hiding this comment.
source is documented as optional but is effectively required by the runtime.
service.ts tokenAllowsSource() returns false when source is null/empty, which causes invokePackageExport to return 403 source_not_allowed before any package or export is even resolved. So a request omitting source always fails — it isn't "optional" in practice. Either tighten this doc to say source is required (and that it must match the token's sources_json allowlist), or update tokenAllowsSource to treat a missing source as "no source claim, skip allowlist check" if you actually want it optional.
The same friction shows up in the curl example at lines 196–205, where source is included — readers could reasonably remove it based on the "optional" description and get a confusing 403.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/contributing/package-invocation-api.md` around lines 80 - 86, The docs
claim `source` is optional but the runtime enforces it: `service.ts`'s
tokenAllowsSource() returns false for null/empty `source`, causing
invokePackageExport to fail with 403; fix by either (A) updating this docs page
and the curl example to state `source` is required and must match the token's
sources_json allowlist (clarify format and behavior), or (B) change
`tokenAllowsSource()` to treat a missing/empty `source` as "no source claim —
skip allowlist check" so omission is allowed; pick one approach and update the
docs and the implementation consistently (reference tokenAllowsSource and
invokePackageExport when making the change).
Comment on lines
+184
to
+191
| function tokenAllowsSource(input: { | ||
| token: PackageInvocationTokenScope | ||
| source: string | null | ||
| }) { | ||
| if (!input.source) return false | ||
| const sources = input.token.sources ?? [] | ||
| return sources.includes(input.source) | ||
| } |
There was a problem hiding this comment.
tokenAllowsSource conflates "no source provided" with "source not in allowlist".
When source is null/empty, this returns false and the caller gets 403 source_not_allowed with message "This token is not allowed to invoke the requested source." That message reads as if the operator sent a forbidden source value, but they actually didn't send one at all — confusing for integrators, and contradicts the source — optional line in docs/contributing/package-invocation-api.md.
Pick one of:
- Treat missing
sourceas a hard validation error in the HTTP layer with a distinct code (missing_source) so the docs can call it required, or - Allow
source: nullthrough when the token'ssourcesallowlist is also empty / explicitly opts in.
🔧 Option: explicit "source required" code path
function tokenAllowsSource(input: {
token: PackageInvocationTokenScope
source: string | null
}) {
- if (!input.source) return false
+ if (!input.source) return null // signals "source missing"
const sources = input.token.sources ?? []
- return sources.includes(input.source)
+ return sources.includes(input.source) ? true : false
}…and have invokePackageExport distinguish null (→ 400 missing_source) from false (→ 403 source_not_allowed).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/package-invocations/service.ts` around lines 184 - 191,
tokenAllowsSource currently treats a missing source (null) the same as a
disallowed source, causing confusing 403 errors; change tokenAllowsSource to
allow a null source only when the token's sources allowlist is empty (i.e.,
treat an empty allowlist as "no source restrictions") otherwise return false;
specifically, in tokenAllowsSource(update the function body) check if
input.source is null and if so return true when (input.token.sources ??
[]).length === 0, otherwise proceed to normal allowlist check, and ensure
callers like invokePackageExport continue to interpret a false return as
"source_not_allowed" while null-is-allowed paths pass through.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0932ac2. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kentcdodds
deleted the
cursor/-bc-8a687cc4-2537-4d83-a751-7e00439960e2-9107
branch
April 27, 2026 18:28
This was referenced May 10, 2026
This was referenced Jun 18, 2026
This was referenced Jun 26, 2026
This was referenced Jul 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POST /api/package-invocations/:packageIdOrKodyId/:exportNameTesting
npm run typechecknpx vitest run --project node-unit --project workers-unit packages/worker/src/package-invocations/service.node.test.ts packages/worker/src/package-invocations/http.workers.test.tsNotes
idempotencyKeyreplay the original stored response; mismatched reuses return409 idempotency_mismatchPACKAGE_INVOCATION_TOKENSWorker secret; token material is managed in D1 viapackage_invocation_tokens, and Kody stores only token hashes for request-time authSummary by CodeRabbit
New Features
Documentation
Tests