Remove leftover legacy skills feature code - #832
Conversation
Delete the unused skill-parameters module and dead runKodyWithRegistry expression path, drop skill-runner secret reservations and skill/app publish-note compat, and rename contributor execute-pattern docs away from the old skills naming.
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Cloudflare execute-pattern documentation, removes legacy inline execution and parameterization paths, tightens publish-note and secret-name contracts, updates package runtime fixtures, and simplifies related tests. ChangesCloudflare execute patterns
Module runtime cleanup
Publish note schema tightening
Secret-name guard updates
Maintenance route test update
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
🔎 Preview deployed: https://kody-pr-832.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/contributing/execute-patterns/cloudflare-developer-docs.md`:
- Around line 55-70: Update assertAllowedPath to construct and return the
canonical URL, then validate url.pathname against the allowed prefixes and
traversal rules before returning it for fetch use. Reject leading or trailing
whitespace instead of normalizing it with trim(), and update the caller to use
the validator’s returned URL rather than constructing a separate URL afterward.
In `@packages/worker/src/package-runtime/package-storage.workers.test.ts`:
- Around line 250-251: Update the comment near the package-storage test setup to
state that package invocations leave ambient storage unbound and that the runner
seeds the package’s bucket for packageStorage(). Remove the outdated claim that
invocations bind ambient storage to the same ID.
🪄 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: 301f6318-f4e2-44c1-8e77-ad1826e6268f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
docs/contributing/execute-patterns/cloudflare-api-v4.mddocs/contributing/execute-patterns/cloudflare-developer-docs.mddocs/use/packages.mdpackages/worker/package.jsonpackages/worker/src/jobs/service.node.test.tspackages/worker/src/mcp/capabilities/repo/repo-show-publish-note.tspackages/worker/src/mcp/cloudflare/cloudflare-rest-client.tspackages/worker/src/mcp/run-kody-registry.node.test.tspackages/worker/src/mcp/run-kody-registry.tspackages/worker/src/mcp/secrets/name-guards.tspackages/worker/src/mcp/secrets/service.node.test.tspackages/worker/src/mcp/skills/skill-parameters.node.test.tspackages/worker/src/mcp/skills/skill-parameters.tspackages/worker/src/module-source.tspackages/worker/src/package-runtime/package-storage.workers.test.tspackages/worker/src/repo/publish-git-notes.tspackages/worker/src/security/public-route-hardening.workers.test.ts
💤 Files with no reviewable changes (5)
- packages/worker/src/mcp/skills/skill-parameters.node.test.ts
- packages/worker/src/module-source.ts
- packages/worker/package.json
- packages/worker/src/mcp/skills/skill-parameters.ts
- packages/worker/src/mcp/run-kody-registry.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/contributing/execute-patterns/cloudflare-developer-docs.md`:
- Around line 55-70: Update assertAllowedPath to construct and return the
canonical URL, then validate url.pathname against the allowed prefixes and
traversal rules before returning it for fetch use. Reject leading or trailing
whitespace instead of normalizing it with trim(), and update the caller to use
the validator’s returned URL rather than constructing a separate URL afterward.
In `@packages/worker/src/package-runtime/package-storage.workers.test.ts`:
- Around line 250-251: Update the comment near the package-storage test setup to
state that package invocations leave ambient storage unbound and that the runner
seeds the package’s bucket for packageStorage(). Remove the outdated claim that
invocations bind ambient storage to the same ID.
🪄 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: 301f6318-f4e2-44c1-8e77-ad1826e6268f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
docs/contributing/execute-patterns/cloudflare-api-v4.mddocs/contributing/execute-patterns/cloudflare-developer-docs.mddocs/use/packages.mdpackages/worker/package.jsonpackages/worker/src/jobs/service.node.test.tspackages/worker/src/mcp/capabilities/repo/repo-show-publish-note.tspackages/worker/src/mcp/cloudflare/cloudflare-rest-client.tspackages/worker/src/mcp/run-kody-registry.node.test.tspackages/worker/src/mcp/run-kody-registry.tspackages/worker/src/mcp/secrets/name-guards.tspackages/worker/src/mcp/secrets/service.node.test.tspackages/worker/src/mcp/skills/skill-parameters.node.test.tspackages/worker/src/mcp/skills/skill-parameters.tspackages/worker/src/module-source.tspackages/worker/src/package-runtime/package-storage.workers.test.tspackages/worker/src/repo/publish-git-notes.tspackages/worker/src/security/public-route-hardening.workers.test.ts
💤 Files with no reviewable changes (5)
- packages/worker/src/mcp/skills/skill-parameters.node.test.ts
- packages/worker/src/module-source.ts
- packages/worker/package.json
- packages/worker/src/mcp/skills/skill-parameters.ts
- packages/worker/src/mcp/run-kody-registry.ts
🛑 Comments failed to post (2)
docs/contributing/execute-patterns/cloudflare-developer-docs.md (1)
55-70: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
node -e "for (const p of ['/api/%2e%2e/secret', ' /api/resources/']) console.log(p, '=>', new URL(p, 'https://developers.cloudflare.com').pathname)"Repository: kentcdodds/kody
Length of output: 219
🏁 Script executed:
sed -n '1,140p' docs/contributing/execute-patterns/cloudflare-developer-docs.md python3 - <<'PY' from urllib.parse import urljoin tests = ['/api/%2e%2e/secret', ' /api/resources/', '/api/resources/%2e%2e/secret', '/api/resources/%2e%2e%2fsecret', '/api/resources/..%2fsecret'] base='https://developers.cloudflare.com' for t in tests: print(t, '=>', __import__('subprocess').run( ['node','-e',f"console.log(new URL({t!r}, {base!r}).pathname)"], capture_output=True,text=True ).stdout.strip()) PYRepository: kentcdodds/kody
Length of output: 3031
🏁 Script executed:
sed -n '1,140p' docs/contributing/execute-patterns/cloudflare-developer-docs.md python3 - <<'PY' tests = ['/api/%2e%2e/secret', ' /api/resources/', '/api/resources/%2e%2e/secret', '/api/resources/%2e%2e%2fsecret', '/api/resources/..%2fsecret'] base='https://developers.cloudflare.com' import subprocess for t in tests: out = subprocess.run( ['node','-e',f"console.log(new URL({t!r}, {base!r}).pathname)"], capture_output=True,text=True,check=True ).stdout.strip() print(f"{t} => {out}") PYRepository: kentcdodds/kody
Length of output: 3031
Validate the canonical pathname before allowlisting it.
/api/%2e%2e/secretpasses the string checks here butnew URL(...).pathnamenormalizes it to/secret, so the fetch can escape the intended prefix. Return theURLfrom the validator and checkurl.pathname; the currenttrim()also accepts leading/trailing whitespace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/contributing/execute-patterns/cloudflare-developer-docs.md` around lines 55 - 70, Update assertAllowedPath to construct and return the canonical URL, then validate url.pathname against the allowed prefixes and traversal rules before returning it for fetch use. Reject leading or trailing whitespace instead of normalizing it with trim(), and update the caller to use the validator’s returned URL rather than constructing a separate URL afterward.packages/worker/src/package-runtime/package-storage.workers.test.ts (1)
250-251: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the comment to the current package-storage contract.
Lines 250-251 state that package invocations bind ambient
storage, but package invocations now leave it unbound; this runner seeds the bucket forpackageStorage()instead.Proposed fix
- // Seed the package's own bucket the way the package would in its own - // runtime (package invocations bind ambient storage to the same id). + // Seed the package's own bucket for `packageStorage()`.📝 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.// Seed the package's own bucket for `packageStorage()`.🤖 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/package-runtime/package-storage.workers.test.ts` around lines 250 - 251, Update the comment near the package-storage test setup to state that package invocations leave ambient storage unbound and that the runner seeds the package’s bucket for packageStorage(). Remove the outdated claim that invocations bind ambient storage to the same ID.
Validate Cloudflare docs paths against the canonical URL pathname, and correct the package-storage test comment about unbound ambient storage.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Also require the resolved URL origin to match developers.cloudflare.com so allowlisting cannot be bypassed via //evil.com/... paths.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort 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 5778b17. Configure here.
Reject raw .. / %2e segments and require both the input path and the resolved pathname to start with an allowlisted prefix.
3 participants
Summary
Strip the last product-code remnants of the old Kody skills feature so the tree reads as if that feature never existed (historical D1 migrations kept as-is).
Removed
packages/worker/src/mcp/skills/(skill-parameters+ tests) and the deadrunKodyWithRegistryexpression/snippet path that was its only callerskill-runner-token:reserved secret-name reservationentityKind: 'skill' | 'app'acorndependency (only used by skill-parameters)stripCodeFences/hasTopLevelModuleSyntaxhelpers that only served that dead pathRenamed / cleaned
docs/contributing/skill-patterns/→execute-patterns/Live execute continues to use
runModuleWithRegistry/runBundledModuleWithRegistrywith params passed to the default export.npm run validatepasses.System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@51c8e8d5· Head:e17be872Classification: extends — removes dead skill-era execute surface, secret-name reservation, and publish-note parse compat; no new primitives.
Primitives touched
capabilities-executerunKodyWithRegistry/ skill-params wrappingmcp-servermcp/skillsmodule and skill-runner secret guardrepo-sessionscapability-registryrepo_show_publish_noteoutput schema followspackage-runtimejobssecretsSystem map
Dead skill-era execute helpers and secret/publish compat are removed; live module execute is unchanged.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
runKodyWithRegistry+buildParameterizedSkillCodeskill-runner-token:*reserved/hiddenskill/appentity kindsjob/packageonly (raw_notestill returned)Summary by CodeRabbit