Repository navigation
Fix production execute failure: __name is not defined in generated modules - #630
Merged
Merged
Conversation
PR #628 inlined createKodyRemoteProxy via Function.prototype.toString() into every generated execute module. Production wrangler/esbuild rewrites named functions to use a bundle-scoped __name helper, so dynamically loaded executor modules fail at init with '__name is not defined'. Replace the serialized factory with a static source template, add regression coverage, and gate production deploys with an authenticated execute smoke check via POST /__maintenance/execute-smoke.
📝 WalkthroughWalkthroughAdds a worker maintenance endpoint ( ChangesExecute smoke check feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WorkerIndex as index.ts
participant Handler as handleExecuteSmokeRequest
participant Runner as runExecuteSmokeCheck
participant Executor
Client->>WorkerIndex: POST /__maintenance/execute-smoke
WorkerIndex->>Handler: handleExecuteSmokeRequest(request, env)
Handler->>Handler: check CAPABILITY_REINDEX_SECRET bearer auth
Handler->>Runner: runExecuteSmokeCheck(env)
Runner->>Executor: executor.execute(smoke code)
Executor-->>Runner: { result: 42 }
Runner-->>Handler: { result: 42 }
Handler-->>Client: 200 { ok: true, result: 42 }
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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
|
🔎 Preview deployed: https://kody-pr-630.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/deploy.yml (1)
254-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixed retry delay vs. exponential backoff elsewhere in the same job.
The deploy step uses exponential backoff for retries, while this new smoke check uses a flat
delay_seconds=3across all 10 attempts. Not a correctness issue, but slightly inconsistent with the pattern already established in this file.🤖 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 @.github/workflows/deploy.yml around lines 254 - 296, The smoke check retry loop in the deploy workflow uses a fixed `delay_seconds=3`, which is inconsistent with the exponential backoff pattern used elsewhere in the same job. Update the retry logic in the `Execute smoke check (production)` step to follow the same backoff approach as the other retry blocks, keeping the `curl`/`node` success check and `attempts` loop intact while adjusting only the sleep timing behavior.
🤖 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 @.github/workflows/deploy.yml:
- Around line 274-292: The retry loop in the smoke test step can terminate
immediately because the curl invocation inside the status assignment is still
subject to set -euo pipefail. Update the retry logic around the curl call in the
deploy workflow so a curl error/timeout is captured as a nonzero status and the
loop continues retrying, while preserving the existing response validation with
the node check and the execute-smoke.json output handling.
In `@packages/worker/src/execute-maintenance.ts`:
- Around line 6-13: The executeSmokeProviders fixture uses as const, which makes
kodyRemoteConnectors infer as a readonly empty array and breaks assignment to
ResolvedProvider.kodyRemoteConnectors. Remove the as const from
executeSmokeProviders in execute-maintenance.ts and keep the fixture mutable so
the nested kodyRemoteConnectors field is inferred as a normal
Array<KodyRemoteConnectorMetadata>.
In `@packages/worker/src/mcp/kody-remote-proxy-source.ts`:
- Around line 75-81: The bundle-safety check in
assertGeneratedExecutorSourceIsBundleSafe is too broad because it matches plain
serialized metadata as well as real bundler output. Update the regex so it only
flags actual esbuild/wrangler __name call syntax (for example in assembled
executor source from executor.ts) rather than any whole-word "__name" string,
and keep the existing error path unchanged for true bundle-scoped helper
references.
---
Nitpick comments:
In @.github/workflows/deploy.yml:
- Around line 254-296: The smoke check retry loop in the deploy workflow uses a
fixed `delay_seconds=3`, which is inconsistent with the exponential backoff
pattern used elsewhere in the same job. Update the retry logic in the `Execute
smoke check (production)` step to follow the same backoff approach as the other
retry blocks, keeping the `curl`/`node` success check and `attempts` loop intact
while adjusting only the sleep timing 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b4d2d92b-0b27-465d-9ae6-4b502998b898
📒 Files selected for processing (7)
.github/workflows/deploy.ymlpackages/worker/src/execute-maintenance.node.test.tspackages/worker/src/execute-maintenance.tspackages/worker/src/index.tspackages/worker/src/mcp/executor.node.test.tspackages/worker/src/mcp/executor.tspackages/worker/src/mcp/kody-remote-proxy-source.ts
Comment on lines
+274
to
+292
| attempts=10 | ||
| delay_seconds=3 | ||
| for i in $(seq 1 "$attempts"); do | ||
| echo "Attempt $i/$attempts" | ||
| status="$(curl --silent --show-error --location --max-time 30 \ | ||
| -X POST \ | ||
| -H "Authorization: Bearer ${CAPABILITY_REINDEX_SECRET}" \ | ||
| -H "Accept: application/json" \ | ||
| --output execute-smoke.json \ | ||
| --write-out "%{http_code}" \ | ||
| "${smoke_url}")" | ||
| cat execute-smoke.json | ||
| echo | ||
| if [ "$status" -ge 200 ] && [ "$status" -lt 300 ] && \ | ||
| node -e "const fs = require('node:fs'); const json = JSON.parse(fs.readFileSync('execute-smoke.json','utf8')); if (json?.ok !== true || json?.result !== 42) { console.error('execute-smoke-unexpected-response', json); process.exit(1); } console.log('execute-smoke-ok', json);"; then | ||
| exit 0 | ||
| fi | ||
| sleep "$delay_seconds" | ||
| done |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the workflow and inspect the surrounding lines.
git ls-files .github/workflows/deploy.yml
wc -l .github/workflows/deploy.yml
sed -n '240,310p' .github/workflows/deploy.yml
# Probe shell semantics relevant to: status="$(cmd)" under set -e.
python3 - <<'PY'
import subprocess, textwrap, tempfile, os, json, pathlib
cases = {
"bare_assignment_success": r'''
set -e
status="$(python3 - <<'PY2'
import sys
print("ok")
sys.exit(0)
PY2
)"
echo "reached:$status"
''',
"bare_assignment_failure": r'''
set -e
status="$(python3 - <<'PY2'
import sys
print("partial", end="")
sys.exit(1)
PY2
)"
echo "reached:$status"
''',
"if_assignment_failure": r'''
set -e
if status="$(python3 - <<'PY2'
import sys
print("partial", end="")
sys.exit(1)
PY2
)"; then
echo ok
fi
echo "reached:$status"
''',
}
for name, script in cases.items():
p = subprocess.run(["bash", "-lc", script], capture_output=True, text=True)
print(f"## {name}")
print("exit", p.returncode)
print("stdout:", repr(p.stdout))
print("stderr:", repr(p.stderr))
PYRepository: kentcdodds/kody
Length of output: 3921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the specific workflow step for exact context.
nl -ba .github/workflows/deploy.yml | sed -n '268,294p'
# Check shell behavior in a way that mirrors the workflow structure.
python3 - <<'PY'
import subprocess, textwrap
scripts = {
"bare_assignment_success": r'''set -e
status="$(python3 - <<'PY2'
print("ok")
PY2
)"
echo "reached:$status"
''',
"bare_assignment_failure": r'''set -e
status="$(python3 - <<'PY2'
import sys
print("partial", end="")
sys.exit(1)
PY2
)"
echo "reached:$status"
''',
"conditional_assignment_failure": r'''set -e
if status="$(python3 - <<'PY2'
import sys
print("partial", end="")
sys.exit(1)
PY2
)"; then
echo ok
fi
echo "reached:$status"
''',
}
for name, script in scripts.items():
p = subprocess.run(["bash", "-lc", script], capture_output=True, text=True)
print(f"## {name}")
print("exit", p.returncode)
print("stdout:", repr(p.stdout))
print("stderr:", repr(p.stderr))
PYRepository: kentcdodds/kody
Length of output: 193
Retry loop can exit on the first curl failure. status="$(curl ...)" runs under set -euo pipefail, so a transient connection/timeout error aborts the step before any retry happens. Wrap it so curl failures become a retryable status instead of ending the step immediately.
🤖 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 @.github/workflows/deploy.yml around lines 274 - 292, The retry loop in the
smoke test step can terminate immediately because the curl invocation inside the
status assignment is still subject to set -euo pipefail. Update the retry logic
around the curl call in the deploy workflow so a curl error/timeout is captured
as a nonzero status and the loop continues retrying, while preserving the
existing response validation with the node check and the execute-smoke.json
output handling.
Comment on lines
+6
to
+13
| const executeSmokeCode = 'async () => 42' | ||
| const executeSmokeProviders = [ | ||
| { | ||
| name: 'kody', | ||
| fns: {}, | ||
| kodyRemoteConnectors: [], | ||
| }, | ||
| ] as const |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "ResolvedProvider" packages/worker/src/mcp/executor.ts -B2 -A8
rg -n "KodyRemoteConnectorMetadata" packages/worker/src/mcp/executor.ts -B2 -A8Repository: kentcdodds/kody
Length of output: 4898
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== execute-maintenance.ts =="
wc -l packages/worker/src/execute-maintenance.ts
cat -n packages/worker/src/execute-maintenance.ts | sed -n '1,220p'
echo
echo "== kody-remote-types.ts =="
wc -l packages/worker/src/mcp/kody-remote-types.ts
cat -n packages/worker/src/mcp/kody-remote-types.ts | sed -n '1,220p'
echo
echo "== executor.ts relevant section =="
cat -n packages/worker/src/mcp/executor.ts | sed -n '390,460p'Repository: kentcdodds/kody
Length of output: 6943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('packages/worker/src/execute-maintenance.ts')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 'executeSmoke' in line or 'getErrorMessage' in line or 'createKodyProviderProxySource' in line or 'providers:' in line or 'result.result' in line:
start = max(1, i-8)
end = min(len(text.splitlines()), i+16)
print(f"\n--- around line {i} ---")
for j in range(start, end+1):
print(f"{j}:{text.splitlines()[j-1]}")
PYRepository: kentcdodds/kody
Length of output: 5995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [
Path('packages/worker/src/execute-maintenance.ts'),
Path('packages/worker/src/mcp/kody-remote-types.ts'),
Path('packages/worker/src/mcp/executor.ts'),
]:
print(f"\n## {path} ##")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 'executeSmoke' in line or 'getErrorMessage' in line or 'createKodyProviderProxySource' in line or 'KodyResolvedProvider' in line or 'kodyRemoteConnectors' in line or 'ResolvedProvider' in line:
start = max(1, i-6)
end = min(len(lines), i+14)
print(f"\n-- around line {i} --")
for j in range(start, end+1):
print(f"{j}:{lines[j-1]}")
PYRepository: kentcdodds/kody
Length of output: 19496
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = [
'packages/worker/src/execute-maintenance.ts',
'packages/worker/src/mcp/kody-remote-types.ts',
'packages/worker/src/mcp/executor.ts',
]
for file in files:
print(f"\n## {file}")
lines = Path(file).read_text().splitlines()
for idx, line in enumerate(lines, 1):
if any(tok in line for tok in ['executeSmoke', 'getErrorMessage', 'createKodyProviderProxySource', 'KodyResolvedProvider', 'kodyRemoteConnectors', 'ResolvedProvider']):
start = max(1, idx - 6)
end = min(len(lines), idx + 14)
print(f"\n-- around line {idx} --")
for j in range(start, end + 1):
print(f"{j}:{lines[j-1]}")
PYRepository: kentcdodds/kody
Length of output: 19487
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== execute-maintenance.ts =="
cat -n packages/worker/src/execute-maintenance.ts | sed -n '1,220p'Repository: kentcdodds/kody
Length of output: 2229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== getErrorMessage helper =="
fd -a "error-message.ts" packages/worker/src
if [ -f packages/worker/src/mcp/capabilities/error-message.ts ]; then
wc -l packages/worker/src/mcp/capabilities/error-message.ts
cat -n packages/worker/src/mcp/capabilities/error-message.ts | sed -n '1,200p'
fi
echo
echo "== TypeScript tool availability =="
command -v tsc || true
node -e "try { const ts = require('typescript'); console.log('typescript module:', ts.version); } catch (e) { console.log('typescript module unavailable') }"
echo
echo "== assignability probe =="
tmpdir="$(mktemp -d)"
cat > "$tmpdir/probe.ts" <<'TS'
type KodyRemoteConnectorMetadata = {
name: string
}
type ResolvedProvider = {
name: string
fns: Record<string, unknown>
kodyRemoteConnectors?: Array<KodyRemoteConnectorMetadata>
}
const providers = [
{
name: 'kody',
fns: {},
kodyRemoteConnectors: [],
},
] as const
const x: Array<ResolvedProvider> = [...providers]
TS
if command -v tsc >/dev/null 2>&1; then
tsc --noEmit --strict "$tmpdir/probe.ts" 2>&1 || true
else
echo "tsc not available"
fi
rm -rf "$tmpdir"Repository: kentcdodds/kody
Length of output: 1732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
cat > "$tmpdir/probe.ts" <<'TS'
type KodyRemoteConnectorMetadata = {
name: string
}
type ResolvedProvider = {
name: string
fns: Record<string, unknown>
kodyRemoteConnectors?: Array<KodyRemoteConnectorMetadata>
}
const executeSmokeProviders = [
{
name: 'kody',
fns: {},
kodyRemoteConnectors: [],
},
] as const
const providersA: Array<ResolvedProvider> = [...executeSmokeProviders]
const providersB: Array<ResolvedProvider> = executeSmokeProviders
declare const p: ResolvedProvider
const connectorsA: Array<KodyRemoteConnectorMetadata> = p.kodyRemoteConnectors!
const connectorsB: Array<KodyRemoteConnectorMetadata> = executeSmokeProviders[0].kodyRemoteConnectors
TS
tsc --noEmit --strict --ignoreConfig "$tmpdir/probe.ts" 2>&1 || true
rm -rf "$tmpdir"Repository: kentcdodds/kody
Length of output: 951
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== assignability probe =="
tmpdir="$(mktemp -d)"
cat > "$tmpdir/probe.ts" <<'TS'
type KodyRemoteConnectorMetadata = {
name: string
}
type ResolvedProvider = {
name: string
fns: Record<string, unknown>
kodyRemoteConnectors?: Array<KodyRemoteConnectorMetadata>
}
const executeSmokeProviders = [
{
name: 'kody',
fns: {},
kodyRemoteConnectors: [],
},
] as const
const providersA: Array<ResolvedProvider> = [...executeSmokeProviders]
const providersB: Array<ResolvedProvider> = executeSmokeProviders
declare const p: ResolvedProvider
const connectorsA: Array<KodyRemoteConnectorMetadata> = p.kodyRemoteConnectors!
const connectorsB: Array<KodyRemoteConnectorMetadata> = executeSmokeProviders[0].kodyRemoteConnectors
TS
tsc --noEmit --strict --ignoreConfig "$tmpdir/probe.ts" 2>&1 || true
rm -rf "$tmpdir"
echo
echo "== ExecuteResult / getErrorMessage context =="
rg -n "type ExecuteResult|interface ExecuteResult|getErrorMessage\\(" packages/worker/src -B2 -A6Repository: kentcdodds/kody
Length of output: 24337
Drop as const from this fixture packages/worker/src/execute-maintenance.ts:7-13 — kodyRemoteConnectors is inferred as readonly [], which isn’t assignable to ResolvedProvider.kodyRemoteConnectors: Array<KodyRemoteConnectorMetadata>. The outer spread doesn’t remove the nested readonly type; use a mutable fixture instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/src/execute-maintenance.ts` around lines 6 - 13, The
executeSmokeProviders fixture uses as const, which makes kodyRemoteConnectors
infer as a readonly empty array and breaks assignment to
ResolvedProvider.kodyRemoteConnectors. Remove the as const from
executeSmokeProviders in execute-maintenance.ts and keep the fixture mutable so
the nested kodyRemoteConnectors field is inferred as a normal
Array<KodyRemoteConnectorMetadata>.
Comment on lines
+75
to
+81
| export function assertGeneratedExecutorSourceIsBundleSafe(source: string) { | ||
| if (/\b__name\b/u.test(source)) { | ||
| throw new Error( | ||
| 'Generated executor source must not reference bundle-scoped __name helpers.', | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Regex can false-positive on interpolated data, not just bundler output.
This helper is invoked against fully-assembled source that includes JSON-serialized connector/capability metadata (metadataJson in executor.ts). If a remote connector or capability is ever named "__name", the whole-word check will match that data and incorrectly throw "must not reference bundle-scoped __name helpers", blocking legitimate execution even though no actual bundler artifact is present.
Real esbuild/wrangler output always uses __name as a call, e.g. __name(fn, "name"). Narrowing the pattern to require the call syntax avoids matching plain string data.
🐛 Proposed fix
export function assertGeneratedExecutorSourceIsBundleSafe(source: string) {
- if (/\b__name\b/u.test(source)) {
+ if (/\b__name\s*\(/u.test(source)) {
throw new Error(
'Generated executor source must not reference bundle-scoped __name helpers.',
)
}
}📝 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
| export function assertGeneratedExecutorSourceIsBundleSafe(source: string) { | |
| if (/\b__name\b/u.test(source)) { | |
| throw new Error( | |
| 'Generated executor source must not reference bundle-scoped __name helpers.', | |
| ) | |
| } | |
| } | |
| export function assertGeneratedExecutorSourceIsBundleSafe(source: string) { | |
| if (/\b__name\s*\(/u.test(source)) { | |
| throw new Error( | |
| 'Generated executor source must not reference bundle-scoped __name helpers.', | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/src/mcp/kody-remote-proxy-source.ts` around lines 75 - 81,
The bundle-safety check in assertGeneratedExecutorSourceIsBundleSafe is too
broad because it matches plain serialized metadata as well as real bundler
output. Update the regex so it only flags actual esbuild/wrangler __name call
syntax (for example in assembled executor source from executor.ts) rather than
any whole-word "__name" string, and keep the existing error path unchanged for
true bundle-scoped helper references.
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.
Root cause (confirmed)
PR #628 (
de3b3fb4) addedcreateKodyProviderProxySource, which embedscreateKodyRemoteProxy.toString()into every generated execute-module preamble whenever thekodyprovider includeskodyRemoteConnectors(including[], which is truthy).Production wrangler/esbuild bundles the worker with name preservation (
keep_names), rewriting named functions to call a bundle-scoped__name(...)helper. That helper exists only inside the deployed worker bundle — not inside dynamically loaded executor modules assembled at runtime. Because the kody provider proxy preamble is injected into every MCPexecutecall, all execute invocations fail at module init with:Local vitest/dev do not apply the same transform, so
npm run validatestayed green while production was broken./healthcontinued to pass because it never exercises the dynamic execute path.Fix
createKodyRemoteProxy.toString()with a static source-string factory inkody-remote-proxy-source.tsand assert generated executor source contains no__namereferences..toString()-based worker module assembly (run-kody-registry.ts,package-runtime/package-app.ts, MCP execute codegen). Onlyexecutor.tshad this landmine; other.toString()uses are URL/body serialization, not function codegen.__name(...)failure mode and verifies generated module source is bundle-safe.POST /__maintenance/execute-smoke(reusesCAPABILITY_REINDEX_SECRET) and a post-deploy smoke step in the production deploy workflow. This catches execute regressions that/healthmisses.Post-deploy verification (production)
After this deploy lands:
Health (existing):
Expect
{ "ok": true, "commitSha": "<deploy-sha>" }.Execute smoke (new deploy gate):
Expect
{ "ok": true, "result": 42 }.MCP execute (manual production check):
Authenticate to MCP and run a trivial module with no imports, e.g.:
Expect a successful result — not
Error: __name is not defined.System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@de3b3fb4· Head:042eb5d8Classification: extends — fixes dynamic execute codegen and adds a deploy smoke gate; no new primitives.
Primitives touched
mcp-serverworker-loaderSystem map
Change flow
Summary by CodeRabbit
New Features
Bug Fixes