fix(core): key generate-spec method schemas by namespace (fixes #445) by osama-rizk · Pull Request #498 · aws-devtools-labs/aws-blocks

osama-rizk · 2026-09-07T09:58:48Z

Fixes #445.

Problem

blocks-generate-spec's TypeScript extractor (extract-ts-types.ts) keyed each method's param/result schemas by the bare method name (result.set(methodName, info)). When two ApiNamespaces each expose an operation with the same name — widgets.create and subscriptions.create — the second overwrote the first in the map, so generate-spec looked up tsTypes.get('create') and silently assigned one namespace's schemas to the other in the OpenRPC document. Their TS contracts are deliberately incompatible, so the generated client types were wrong with no error.

Fix

Key method schemas by the namespace-qualified name (namespace.method), derived from the enclosing export const <name> = new ApiNamespace(...) binding — the same name generate-spec routes by (routingName = exportName). generate-spec now resolves tsTypes.get(qualifiedName) ?? tsTypes.get(methodName): qualified wins (no collision), with a bare fallback so any method the extractor couldn't attribute to a namespace (e.g. created behind indirection) still resolves exactly as before — no previously-working case regresses.

Covers both extractor paths (the direct new ApiNamespace(...) AST walk and the type-checker-resolved indirect path).

Testing

New regression test (extract-ts-types.test.ts): two namespaces both exposing create with incompatible params → distinct widgets.create / subscriptions.create entries, each keeping its own param type, and no bare create entry.
Updated the existing extractor tests to assert the qualified keys.
Extractor + generate-spec + generate-client suites green.

Verification

npm run lint 0 errors. No public API change (internal codegen). Changeset added.

Changeset

@aws-blocks/core patch + @aws-blocks/blocks patch (umbrella re-export).

changeset-bot · 2026-09-07T09:58:56Z

🦋 Changeset detected

Latest commit: 7418d35

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@aws-blocks/core	Patch
@aws-blocks/blocks	Patch

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

sarayev · 2026-09-08T08:56:44Z

 			// want the JSDoc tag to be honored, so the AST-only scan acts as
 			// a safety net.
-			if (tsTypes.get(methodName)?.skipCodegen || skipCodegenNames.has(methodName)) continue;
+			if (tsInfo?.skipCodegen || skipCodegenNames.has(methodName)) continue;


skipCodegenNames holds bare method names (from extractSkipCodegenMethods), so this has(methodName) check isn't namespace-aware. If two namespaces both expose create and only one is tagged @blocksSkipCodegen, doesn't the other one get dropped from the spec too? That looks like the same collision #445 is about, just hiding in this safety-net branch. Could we qualify the skip set the same way and check has(qualifiedName) || has(methodName)?

Great catch — real bug, fixed. extractSkipCodegenMethods now keys namespace-qualified too (bare only when the namespace is unattributable), and generate-spec checks skipCodegenNames.has(qualifiedName) || has(methodName). So a @blocksSkipCodegen tag on widgets.create no longer drops subscriptions.create. Added a regression test asserting exactly that (tagged widgets.create skipped, untagged subscriptions.create survives, no bare create key).

sarayev · 2026-09-08T08:56:44Z

+			// name for entries the extractor couldn't attribute to a namespace.
+			// Qualified keys stop two namespaces that share a method name (e.g.
+			// widgets.create / subscriptions.create) from cross-assigning schemas.
+			const tsInfo = tsTypes.get(qualifiedName) ?? tsTypes.get(methodName);


Nice fallback. Worth a quick note that two namespaces the extractor can't attribute (default export, factory-wrapped) still land on bare keys and could overwrite each other here, same as #445. Fine to leave as-is, but a one-line comment would save the next person some head-scratching.

Done — added a sentence to the fallback comment noting that namespaces the extractor can't attribute (default export, factory-wrapped, export-rename) still land on bare keys and could overwrite each other there, same class as #445.

sarayev · 2026-09-08T08:56:44Z

+function enclosingBindingName(node: ts.Node): string | undefined {
+	let current: ts.Node | undefined = node.parent;
+	while (current) {
+		if (ts.isVariableDeclaration(current) && ts.isIdentifier(current.name)) return current.name.text;


Small edge case: this keys off the const binding name, but generate-spec looks methods up by the export routing name. For const widgets = ...; export { widgets as gadgets } the keys won't line up and the types quietly fall back to unknown. Rare, and the old bare-keying happened to dodge it. Maybe just a comment flagging the limitation?

Added a comment on enclosingBindingName flagging it: it keys by the local const binding, so an export { widgets as gadgets } rename keys widgets.* while routing looks up gadgets.* → qualified lookup misses and falls back to the bare key (resolves if no collision, else unknown). Left resolving the alias as a noted follow-up since it's rare and the fallback is safe.

sarayev · 2026-09-08T08:56:44Z

+		try {
+			const types = extractMethodTypes(join(dir, 'index.ts'));
+			// Distinct qualified keys — no collision.
+			assert.ok(types.has('widgets.create'), 'widgets.create should be keyed by namespace');


Good repro — this fails on main. Since the fix also changed how generate-spec looks these keys up, would it be worth a matching collision test on the generate-spec side? Two namespaces with a same-named method taking different params, asserting each emitted method keeps its own schema. That's the part customers actually hit in #445.

Added it. New generateSpec regression test: two namespaces each with a create taking different params (string vs number), asserting the emitted widgets.create keeps label: string and subscriptions.create keeps topicCount: number — the user-visible #445 in the OpenRPC output, not just the map. (It feeds a typed .ts foundation to the extractor and a custom loader for runtime discovery, since node --test can't import a raw .ts.)

soberm · 2026-09-08T09:11:12Z

 			// want the JSDoc tag to be honored, so the AST-only scan acts as
 			// a safety net.
-			if (tsTypes.get(methodName)?.skipCodegen || skipCodegenNames.has(methodName)) continue;
+			if (tsInfo?.skipCodegen || skipCodegenNames.has(methodName)) continue;


[minor] skipCodegenNames.has(methodName) here is still keyed by the bare method name. tsInfo above is now namespace-resolved, but this safety-net check is not — so if two namespaces share a method name and only one carries @blocksSkipCodegen, the other can be wrongly dropped from the spec. That's the same #445 collision class, just in the fallback branch. Consider qualifying the skip set and checking has(qualifiedName) || has(methodName).

Great catch — real bug, fixed. extractSkipCodegenMethods now keys namespace-qualified too (bare only when the namespace is unattributable), and generate-spec checks skipCodegenNames.has(qualifiedName) || has(methodName). So a @blocksSkipCodegen tag on widgets.create no longer drops subscriptions.create. Added a regression test asserting exactly that (tagged widgets.create skipped, untagged subscriptions.create survives, no bare create key).

soberm · 2026-09-08T09:11:12Z

+function enclosingBindingName(node: ts.Node): string | undefined {
+	let current: ts.Node | undefined = node.parent;
+	while (current) {
+		if (ts.isVariableDeclaration(current) && ts.isIdentifier(current.name)) return current.name.text;


[minor] enclosingBindingName keys off the const binding name, but generate-spec routes by the export name. An export rename — const widgets = new ApiNamespace(...); export { widgets as gadgets } — keys widgets.* while routing looks up gadgets.*, quietly falling back to the bare/unknown path. Worth a one-line comment flagging the limitation (or resolving the export alias if it's cheap).

Added a comment on enclosingBindingName flagging it: it keys by the local const binding, so an export { widgets as gadgets } rename keys widgets.* while routing looks up gadgets.* → qualified lookup misses and falls back to the bare key (resolves if no collision, else unknown). Left resolving the alias as a noted follow-up since it's rare and the fallback is safe.

soberm · 2026-09-08T09:11:12Z

+		try {
+			const types = extractMethodTypes(join(dir, 'index.ts'));
+			// Distinct qualified keys — no collision.
+			assert.ok(types.has('widgets.create'), 'widgets.create should be keyed by namespace');


[minor] This proves the extractor produces distinct qualified keys — nice repro. Consider also adding a collision test on the generate-spec side: two namespaces, same-named method, different params, asserting each operation keeps its own schema in the generated OpenRPC spec. That locks in the actual user-visible #445 (which manifested in the spec, not the map).

Added it. New generateSpec regression test: two namespaces each with a create taking different params (string vs number), asserting the emitted widgets.create keeps label: string and subscriptions.create keeps topicCount: number — the user-visible #445 in the OpenRPC output, not just the map. (It feeds a typed .ts foundation to the extractor and a custom loader for runtime discovery, since node --test can't import a raw .ts.)

soberm · 2026-09-08T09:11:12Z

+			// name for entries the extractor couldn't attribute to a namespace.
+			// Qualified keys stop two namespaces that share a method name (e.g.
+			// widgets.create / subscriptions.create) from cross-assigning schemas.
+			const tsInfo = tsTypes.get(qualifiedName) ?? tsTypes.get(methodName);


[nit] Good fallback comment. One added sentence noting that namespaces the extractor can't attribute (default export, factory-wrapped) still land on bare keys and could overwrite each other would flag the residual edge for the next reader.

Done — added a sentence to the fallback comment noting that namespaces the extractor can't attribute (default export, factory-wrapped, export-rename) still land on bare keys and could overwrite each other there, same class as #445.

…s-assignment (#445) extractMethodTypes keyed schemas by bare method name, so two ApiNamespaces with a shared method name (widgets.create / subscriptions.create) cross-assigned schemas in the OpenRPC output. Key by namespace-qualified name (derived from the binding, matching generate-spec's routingName); generate-spec looks up qualified-then-bare so unattributed methods still resolve. Updated extractor tests to qualified keys + added a #445 regression test.

…test (review on #498) Address sarayev/soberm review: - The @blocksSkipCodegen safety-net check was still bare-keyed, so a tag on one namespace's method would drop another namespace's same-named method from the spec (same #445 collision, in the fallback branch). extractSkipCodegenMethods now keys namespace-qualified (bare only when unattributable); generate-spec checks has(qualifiedName) || has(methodName). - Comment the residual bare-key edge (default export / factory-wrapped) and the export-alias limitation of enclosingBindingName (const binding vs export name). - Tests: generate-spec-side #445 regression (two namespaces, same method, distinct params → each keeps its own schema in the OpenRPC output) + a skipCodegen collision test; updated existing skip test to qualified keys.

osama-rizk requested a review from a team as a code owner September 7, 2026 09:58

osama-rizk temporarily deployed to publish September 7, 2026 09:59 — with GitHub Actions Inactive

osama-rizk temporarily deployed to publish September 7, 2026 10:01 — with GitHub Actions Inactive

osama-rizk temporarily deployed to publish September 7, 2026 10:09 — with GitHub Actions Inactive

osama-rizk had a problem deploying to publish September 7, 2026 10:10 — with GitHub Actions Failure

osama-rizk temporarily deployed to publish September 7, 2026 10:10 — with GitHub Actions Inactive

sarayev reviewed Sep 8, 2026

View reviewed changes

soberm reviewed Sep 8, 2026

View reviewed changes

osama-rizk added 2 commits September 8, 2026 13:52

osama-rizk force-pushed the fix/generate-spec-qualified-schemas branch from cb21537 to 7418d35 Compare September 8, 2026 11:52

osama-rizk temporarily deployed to publish September 8, 2026 11:52 — with GitHub Actions Inactive

osama-rizk temporarily deployed to publish September 8, 2026 11:55 — with GitHub Actions Inactive

osama-rizk temporarily deployed to publish September 8, 2026 12:03 — with GitHub Actions Inactive

osama-rizk temporarily deployed to publish September 8, 2026 12:04 — with GitHub Actions Inactive

osama-rizk had a problem deploying to publish September 8, 2026 12:04 — with GitHub Actions Error

osama-rizk temporarily deployed to publish September 8, 2026 12:04 — with GitHub Actions Inactive

osama-rizk had a problem deploying to publish September 8, 2026 12:04 — with GitHub Actions Failure

osama-rizk temporarily deployed to publish September 8, 2026 12:04 — with GitHub Actions Inactive

soberm approved these changes Sep 8, 2026

View reviewed changes

Uh oh!

Conversation

osama-rizk commented Sep 7, 2026

Problem

Fix

Testing

Verification

Changeset

Uh oh!

changeset-bot Bot commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot Bot commented Sep 7, 2026 •

edited

Loading