fix(core): guard /api/ext slug collisions in makeApp (closes #323) - #362
Conversation
makeApp only rejected duplicate extension NAMES, but both mounts key off the normalized slug: two extensions named "Http Probe" and "http-probe" shared /api/ext/http-probe with hono first-wins semantics whenever neither exposed an rpc router. Replace the name check with a single slug-uniqueness assertion over the extension list, which covers the /api/ext mounts and the rpc composite alike. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe application now validates normalized extension slug uniqueness before mounting. Collision errors identify both extension names. Tests cover exact-name collisions and router-less extensions with normalized-name collisions. ChangesExtension slug validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/app.ts`:
- Around line 356-358: Update the startup flow around openDb and
assertUniqueExtensionSlugs so extension construction and slug validation
complete before opening the database, or ensure every validation failure
disposes the already-created db client. Preserve successful mounting while
guaranteeing failed startup does not leak the database resource.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 31ebe9d1-9013-4c26-a070-bfba91be59e6
📒 Files selected for processing (3)
packages/core/src/app.tspackages/core/test/api/extension-mount-isolation.it.test.tspackages/core/test/api/rpc-ws.it.test.ts
There was a problem hiding this comment.
Pull request overview
Adds normalized extension-slug collision detection before extensions mount.
Changes:
- Rejects duplicate normalized slugs with claimant details.
- Adds HTTP-route collision coverage and updates duplicate-name expectations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/core/src/app.ts |
Adds slug uniqueness validation. |
packages/core/test/api/rpc-ws.it.test.ts |
Tests router-less slug collisions. |
packages/core/test/api/extension-mount-isolation.it.test.ts |
Updates duplicate-name error expectation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return mountExtension(extension) | ||
| }), | ||
| ) | ||
| assertUniqueExtensionSlugs(extensions) |
There was a problem hiding this comment.
Fixed in 76ac374: slug validation now runs before openDb, so a collision rejects before any resource is created.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mechanism
Both extension mounts key off the normalized slug —
app.route('/api/ext/<slug>', …)and the rpc composite underext.<slug>— butmakeApponly asserted uniqueness of extension names.slug('Http Probe') === slug('http-probe') === 'http-probe', so two extensions whose names normalize to the same slug silently shared the HTTP prefix with hono first-wins semantics: the second extension's routes were unreachable, with no error at boot. The rpc side was already guarded bymakeCompositeRpcRouter(#319), so the hole only showed when neither extension exposed an rpc router.Fix
Replace the name-only check in
makeAppwith one slug-uniqueness assertion over the extension list, run before mounting. It covers the/api/extmounts and the rpc composite alike, and reports both claimants:Duplicate names still fail (identical names normalize to identical slugs), just with the slug message.
makeCompositeRpcRouter's guard is kept: it validates caller-supplied slug strings at the@conciv/extensionpackage boundary (@conciv/harness-testkitmounts routers there with slugs that never pass throughmakeApp), so it is a different input at a different layer rather than a second copy of this check.Tests
packages/core/test/api/rpc-ws.it.test.ts: new test boots two router-less extensions (Http Probe/http-probe, each with a hono app) and assertsmakeApprejects naming the slug and both extensions. Fails on the pre-fix code (boot resolves normally).packages/core/test/api/extension-mount-isolation.it.test.ts: duplicate-name test now asserts the slug message.Closes #323
🤖 Generated with Claude Code
Summary by CodeRabbit