← back to Cli Printing Press
fix(cli): shard sub-resource tables per parent on collision (#707)
102305421d740138e3e5a740af3b47a5bc32b7b1 · 2026-05-08 00:38:46 -0700 · Trevin Chow
* fix(cli): shard sub-resource tables per parent on collision
When the same sub-resource leaf name appears under multiple parents
(e.g. /repos/{owner}/{repo}/commits and /gists/{gist_id}/commits in the
GitHub spec) or collides with a top-level resource of the same name
(e.g. Stytch's top-level connected_apps and users.connected_apps), the
generator silently merged everything into a single table whose FK
column matched only the alphabetically-first parent. The profiler
dropped every parameterized endpoint past the first-seen leaf, so
sync only ever ran for one parent.
Both layers now consult a shared spec.SubResourceShardedNames helper
that returns snake-cased keys flagging any leaf needing a parent
prefix. The schema builder emits per-parent tables (gists_commits,
repos_commits) with correct FK columns; the profiler emits one
DependentResource per parent with a matching sharded Name. Single-
parent sub-resources keep their bare names so existing CLIs without
collisions stay byte-identical (verified by golden suite).
Closes #694
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(cli): encapsulate shard lookup and flatten parent resolution
Wrap SubResourceShardedNames' return in a SubResourceShards struct with
an IsSharded(leaf) method so the schema builder and the profiler stop
reaching for the snake-case key shape directly. Extract firstPathParam
and resolveParentResource from detectDependentResources so the loop
body reads as straight-line code. Trim comments down to the WHY-only
shape required by the project's hygiene rules.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(cli): gate AccessToken reference on HasAuthCommand in config Load
PR #704 gated the AccessToken/RefreshToken/etc. struct fields behind
HasAuthCommand but missed the Load() helper's auth-source fallback,
which referenced cfg.AccessToken unconditionally. Auth-less specs
(e.g. browser-transport CLIs) failed to compile because the field
no longer exists on their generated *Config.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(cli): refresh generate-mcp-api golden for SQL allowlist
PR #708 (close MCP sql tool exfiltration vector) introduced
validateReadOnlyQuery + stripLeadingSQLNoise in the generator
and updated the SQL tool description, but the
generate-mcp-api/mcp-cloudflare golden was not refreshed alongside.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Files touched
M internal/generator/schema_builder.goM internal/generator/schema_builder_test.goM internal/generator/templates/config.go.tmplM internal/profiler/profiler.goM internal/profiler/profiler_test.goA internal/spec/naming.goM testdata/golden/expected/generate-mcp-api/mcp-cloudflare/internal/mcp/tools.go
Diff
commit 102305421d740138e3e5a740af3b47a5bc32b7b1
Author: Trevin Chow <trevin@trevinchow.com>
Date: Fri May 8 00:38:46 2026 -0700
fix(cli): shard sub-resource tables per parent on collision (#707)
* fix(cli): shard sub-resource tables per parent on collision
When the same sub-resource leaf name appears under multiple parents
(e.g. /repos/{owner}/{repo}/commits and /gists/{gist_id}/commits in the
GitHub spec) or collides with a top-level resource of the same name
(e.g. Stytch's top-level connected_apps and users.connected_apps), the
generator silently merged everything into a single table whose FK
column matched only the alphabetically-first parent. The profiler
dropped every parameterized endpoint past the first-seen leaf, so
sync only ever ran for one parent.
Both layers now consult a shared spec.SubResourceShardedNames helper
that returns snake-cased keys flagging any leaf needing a parent
prefix. The schema builder emits per-parent tables (gists_commits,
repos_commits) with correct FK columns; the profiler emits one
DependentResource per parent with a matching sharded Name. Single-
parent sub-resources keep their bare names so existing CLIs without
collisions stay byte-identical (verified by golden suite).
Closes #694
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(cli): encapsulate shard lookup and flatten parent resolution
Wrap SubResourceShardedNames' return in a SubResourceShards struct with
an IsSharded(leaf) method so the schema builder and the profiler stop
reaching for the snake-case key shape directly. Extract firstPathParam
and resolveParentResource from detectDependentResources so the loop
body reads as straight-line code. Trim comments down to the WHY-only
shape required by the project's hygiene rules.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(cli): gate AccessToken reference on HasAuthCommand in config Load
PR #704 gated the AccessToken/RefreshToken/etc. struct fields behind
HasAuthCommand but missed the Load() helper's auth-source fallback,
which referenced cfg.AccessToken unconditionally. Auth-less specs
(e.g. browser-transport CLIs) failed to compile because the field
no longer exists on their generated *Config.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(cli): refresh generate-mcp-api golden for SQL allowlist
PR #708 (close MCP sql tool exfiltration vector) introduced
validateReadOnlyQuery + stripLeadingSQLNoise in the generator
and updated the SQL tool description, but the
generate-mcp-api/mcp-cloudflare golden was not refreshed alongside.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
internal/generator/schema_builder.go | 42 ++--
internal/generator/schema_builder_test.go | 251 ++++++++++++++++++++
internal/generator/templates/config.go.tmpl | 2 +-
internal/profiler/profiler.go | 139 +++++++----
internal/profiler/profiler_test.go | 263 +++++++++++++++++++++
internal/spec/naming.go | 82 +++++++
.../mcp-cloudflare/internal/mcp/tools.go | 66 +++++-
7 files changed, 767 insertions(+), 78 deletions(-)
diff --git a/internal/generator/schema_builder.go b/internal/generator/schema_builder.go
index e04aff70..58a054fb 100644
--- a/internal/generator/schema_builder.go
+++ b/internal/generator/schema_builder.go
@@ -3,7 +3,6 @@ package generator
import (
"sort"
"strings"
- "unicode"
"github.com/mvanhorn/cli-printing-press/v4/internal/spec"
)
@@ -56,6 +55,9 @@ func BuildSchema(s *spec.APISpec) []TableDef {
}
sort.Strings(resourceNames)
+ // Sharded names must agree with the profiler; both call SubResourceShardedNames.
+ subResourceShards := spec.SubResourceShardedNames(s)
+
for _, name := range resourceNames {
resource := s.Resources[name]
fields := collectResponseFields(s, resource)
@@ -126,14 +128,24 @@ func BuildSchema(s *spec.APISpec) []TableDef {
sort.Strings(subNames)
for _, subName := range subNames {
subResource := resource.SubResources[subName]
- subTable := buildSubResourceTable(subName, subResource, tableName)
+ // effectiveName is sharded only when the leaf collides; bare
+ // otherwise keeps existing CLIs byte-identical.
+ effectiveName := subName
+ if subResourceShards.IsSharded(subName) {
+ effectiveName = spec.ShardedSubResourceTableName(name, subName)
+ }
+ subTable := buildSubResourceTable(effectiveName, subResource, tableName)
tables = append(tables, subTable)
}
}
- // Deduplicate tables by name (sub-resources from different parents can collide)
+ // Defensive dedup. Sharding handles the common collisions, but a spec
+ // author naming a top-level resource the same thing the shard synthesizes
+ // (e.g. top-level "gists_commits" plus a multi-parent "commits" under
+ // "gists") would otherwise emit two CREATE TABLE statements and two
+ // duplicate Upsert<X>Tx methods, breaking the build on regen.
seen := make(map[string]bool)
- var deduped []TableDef
+ deduped := make([]TableDef, 0, len(tables))
for _, t := range tables {
if !seen[t.Name] {
seen[t.Name] = true
@@ -424,25 +436,7 @@ func sqlStringLiteral(s string) string {
return `'` + strings.ReplaceAll(s, `'`, `''`) + `'`
}
-// toSnakeCase converts camelCase, PascalCase, or kebab-case to snake_case.
-// Expects ASCII input — callers are SQL identifier paths whose inputs come
-// from parsed APISpec types/fields that already passed through the
-// openapi parser's ASCIIFold chokepoints.
+// toSnakeCase aliases spec.ToSnakeCase; shared so profiler/schema agree.
func toSnakeCase(s string) string {
- s = strings.ReplaceAll(s, ".", "_")
- s = strings.ReplaceAll(s, "-", "_")
-
- var result strings.Builder
- for i, r := range s {
- if unicode.IsUpper(r) && i > 0 {
- prev := rune(s[i-1])
- if unicode.IsLower(prev) || unicode.IsDigit(prev) {
- result.WriteRune('_')
- } else if unicode.IsUpper(prev) && i+1 < len(s) && unicode.IsLower(rune(s[i+1])) {
- result.WriteRune('_')
- }
- }
- result.WriteRune(unicode.ToLower(r))
- }
- return result.String()
+ return spec.ToSnakeCase(s)
}
diff --git a/internal/generator/schema_builder_test.go b/internal/generator/schema_builder_test.go
index 1390e7fb..90882bd9 100644
--- a/internal/generator/schema_builder_test.go
+++ b/internal/generator/schema_builder_test.go
@@ -5,6 +5,7 @@ import (
"github.com/mvanhorn/cli-printing-press/v4/internal/spec"
"github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
)
func TestToSnakeCase(t *testing.T) {
@@ -291,6 +292,256 @@ func TestBuildSchema_NoResponseTypeFallback(t *testing.T) {
}
}
+// TestBuildSchema_SubResourceCollisionShardsByParent pins the fix for issue
+// #694. When the same sub-resource leaf name appears under multiple parents
+// (e.g. /repos/{owner}/{repo}/commits and /gists/{gist_id}/commits in the
+// GitHub spec), each shard becomes its own table named "<parent>_<sub>" so
+// data from one parent does not silently overwrite the other.
+func TestBuildSchema_SubResourceCollisionShardsByParent(t *testing.T) {
+ s := &spec.APISpec{
+ Resources: map[string]spec.Resource{
+ "gists": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {Method: "GET", Path: "/gists"},
+ },
+ SubResources: map[string]spec.Resource{
+ "commits": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {Method: "GET", Path: "/gists/{gist_id}/commits"},
+ },
+ },
+ },
+ },
+ "repos": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {Method: "GET", Path: "/repos"},
+ },
+ SubResources: map[string]spec.Resource{
+ "commits": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {Method: "GET", Path: "/repos/{owner}/{repo}/commits"},
+ },
+ },
+ },
+ },
+ },
+ }
+
+ tables := BuildSchema(s)
+
+ names := make([]string, 0, len(tables))
+ byName := make(map[string]TableDef, len(tables))
+ for _, t := range tables {
+ names = append(names, t.Name)
+ byName[t.Name] = t
+ }
+
+ assert.Contains(t, names, "gists_commits", "collision should produce a sharded gists_commits table")
+ assert.Contains(t, names, "repos_commits", "collision should produce a sharded repos_commits table")
+ assert.NotContains(t, names, "commits", "bare commits table is silent data loss when both parents collide")
+
+ gistsCommits := byName["gists_commits"]
+ require.Greater(t, len(gistsCommits.Columns), 1, "sharded table must have FK column")
+ assert.Equal(t, "gists_id", gistsCommits.Columns[1].Name, "gists_commits FK column is gists_id")
+
+ reposCommits := byName["repos_commits"]
+ require.Greater(t, len(reposCommits.Columns), 1, "sharded table must have FK column")
+ assert.Equal(t, "repos_id", reposCommits.Columns[1].Name, "repos_commits FK column is repos_id")
+}
+
+// TestBuildSchema_SubResourceCollidesWithTopLevel verifies the second
+// sharding trigger: when a sub-resource leaf collides with a top-level
+// resource of the same name (e.g. Stytch's top-level connected_apps and
+// users.connected_apps sub-resource), the sub-resource shards while the
+// top-level keeps its bare name.
+func TestBuildSchema_SubResourceCollidesWithTopLevel(t *testing.T) {
+ s := &spec.APISpec{
+ Resources: map[string]spec.Resource{
+ "connected_apps": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {Method: "GET", Path: "/connected_apps/clients"},
+ },
+ },
+ "users": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {Method: "GET", Path: "/users"},
+ },
+ SubResources: map[string]spec.Resource{
+ "connected_apps": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {Method: "GET", Path: "/users/{user_id}/connected_apps"},
+ },
+ },
+ },
+ },
+ },
+ }
+
+ tables := BuildSchema(s)
+ names := make(map[string]TableDef, len(tables))
+ for _, t := range tables {
+ names[t.Name] = t
+ }
+
+ require.Contains(t, names, "connected_apps", "top-level keeps its bare name")
+ require.Contains(t, names, "users_connected_apps", "sub-resource shards on top-level collision")
+ assert.NotContains(t, names, "connected_apps_users")
+
+ // Top-level table has no FK column; sharded sub-resource's FK column
+ // points to its parent.
+ topLevel := names["connected_apps"]
+ for _, col := range topLevel.Columns {
+ assert.NotEqual(t, "users_id", col.Name, "top-level connected_apps must not carry a users_id FK")
+ }
+ usersConnected := names["users_connected_apps"]
+ require.Greater(t, len(usersConnected.Columns), 1)
+ assert.Equal(t, "users_id", usersConnected.Columns[1].Name)
+}
+
+// TestBuildSchema_TopLevelOnlySharedNameNotSyncable pins the predicate
+// alignment between the schema builder and the profiler. A top-level
+// resource without a flat list endpoint (e.g. POST-only) still triggers
+// sharding for any sub-resource that shares its name, otherwise the
+// generator emits two unrelated tables that the runtime conflates.
+func TestBuildSchema_TopLevelOnlySharedNameNotSyncable(t *testing.T) {
+ s := &spec.APISpec{
+ Resources: map[string]spec.Resource{
+ // POST-only top-level — not in syncable, but still a top-level
+ // resource that the schema builder emits a table for.
+ "audits": {
+ Endpoints: map[string]spec.Endpoint{
+ "create": {Method: "POST", Path: "/audits"},
+ },
+ },
+ "users": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {Method: "GET", Path: "/users"},
+ },
+ SubResources: map[string]spec.Resource{
+ "audits": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {Method: "GET", Path: "/users/{user_id}/audits"},
+ },
+ },
+ },
+ },
+ },
+ }
+
+ tables := BuildSchema(s)
+ names := make([]string, 0, len(tables))
+ for _, t := range tables {
+ names = append(names, t.Name)
+ }
+ assert.Contains(t, names, "audits", "top-level audits keeps its bare table")
+ assert.Contains(t, names, "users_audits", "sub-resource shards even when top-level is POST-only")
+}
+
+// TestBuildSchema_ThreeWayCollision verifies the multi-parent shard predicate
+// emits a per-parent table for every parent, not only the first two.
+func TestBuildSchema_ThreeWayCollision(t *testing.T) {
+ s := &spec.APISpec{
+ Resources: map[string]spec.Resource{
+ "channels": {
+ Endpoints: map[string]spec.Endpoint{"list": {Method: "GET", Path: "/channels"}},
+ SubResources: map[string]spec.Resource{"members": {Endpoints: map[string]spec.Endpoint{"list": {Method: "GET", Path: "/channels/{channel_id}/members"}}}},
+ },
+ "groups": {
+ Endpoints: map[string]spec.Endpoint{"list": {Method: "GET", Path: "/groups"}},
+ SubResources: map[string]spec.Resource{"members": {Endpoints: map[string]spec.Endpoint{"list": {Method: "GET", Path: "/groups/{group_id}/members"}}}},
+ },
+ "teams": {
+ Endpoints: map[string]spec.Endpoint{"list": {Method: "GET", Path: "/teams"}},
+ SubResources: map[string]spec.Resource{"members": {Endpoints: map[string]spec.Endpoint{"list": {Method: "GET", Path: "/teams/{team_id}/members"}}}},
+ },
+ },
+ }
+
+ tables := BuildSchema(s)
+ names := make(map[string]bool, len(tables))
+ for _, t := range tables {
+ names[t.Name] = true
+ }
+ assert.True(t, names["channels_members"])
+ assert.True(t, names["groups_members"])
+ assert.True(t, names["teams_members"])
+ assert.False(t, names["members"], "no bare members table when the leaf collides under three parents")
+}
+
+// TestBuildSchema_CamelCaseShardName verifies the shard helper snake-cases
+// its inputs so a profiler-emitted DependentResource.Name and a schema-builder
+// table name agree byte-for-byte even when the parent resource key is
+// camelCase (common in Google Discovery specs).
+func TestBuildSchema_CamelCaseShardName(t *testing.T) {
+ s := &spec.APISpec{
+ Resources: map[string]spec.Resource{
+ "userData": {
+ Endpoints: map[string]spec.Endpoint{"list": {Method: "GET", Path: "/userData"}},
+ SubResources: map[string]spec.Resource{
+ "loginEvents": {
+ Endpoints: map[string]spec.Endpoint{"list": {Method: "GET", Path: "/userData/{user_id}/loginEvents"}},
+ },
+ },
+ },
+ "adminData": {
+ Endpoints: map[string]spec.Endpoint{"list": {Method: "GET", Path: "/adminData"}},
+ SubResources: map[string]spec.Resource{
+ "loginEvents": {
+ Endpoints: map[string]spec.Endpoint{"list": {Method: "GET", Path: "/adminData/{admin_id}/loginEvents"}},
+ },
+ },
+ },
+ },
+ }
+
+ tables := BuildSchema(s)
+ names := make(map[string]bool, len(tables))
+ for _, t := range tables {
+ names[t.Name] = true
+ }
+ assert.True(t, names["user_data_login_events"], "camelCase parent + sub snake-cases through the shard helper")
+ assert.True(t, names["admin_data_login_events"])
+}
+
+// TestBuildSchema_SubResourceUniqueKeepsBareName verifies the fix for #694
+// does not regress the common case: a sub-resource that appears under exactly
+// one parent (e.g. /channels/{channel_id}/messages with no other parent for
+// "messages") keeps its bare name "messages" and existing FK column.
+func TestBuildSchema_SubResourceUniqueKeepsBareName(t *testing.T) {
+ s := &spec.APISpec{
+ Resources: map[string]spec.Resource{
+ "channels": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {Method: "GET", Path: "/channels"},
+ },
+ SubResources: map[string]spec.Resource{
+ "messages": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {Method: "GET", Path: "/channels/{channel_id}/messages"},
+ },
+ },
+ },
+ },
+ },
+ }
+
+ tables := BuildSchema(s)
+
+ names := make([]string, 0, len(tables))
+ byName := make(map[string]TableDef, len(tables))
+ for _, t := range tables {
+ names = append(names, t.Name)
+ byName[t.Name] = t
+ }
+
+ assert.Contains(t, names, "messages", "unique sub-resource keeps bare name")
+ assert.NotContains(t, names, "channels_messages", "no shard prefix when leaf name is unique")
+
+ messages := byName["messages"]
+ require.Greater(t, len(messages.Columns), 1)
+ assert.Equal(t, "channels_id", messages.Columns[1].Name, "FK column matches sole parent")
+}
+
// findTable returns nil when no match exists so callers can render
// a clearer assertion failure than `tables[0]` panicking.
func findTable(tables []TableDef, name string) *TableDef {
diff --git a/internal/generator/templates/config.go.tmpl b/internal/generator/templates/config.go.tmpl
index 47887509..1e72b242 100644
--- a/internal/generator/templates/config.go.tmpl
+++ b/internal/generator/templates/config.go.tmpl
@@ -122,7 +122,7 @@ func Load(configPath string) (*Config, error) {
// config file path is exposed separately as report["config_path"], and
// embedding it in auth_source leaks the user's home directory through
// doctor's JSON envelope.
- if cfg.AuthSource == "" && (cfg.AuthHeaderVal != "" || cfg.AccessToken != "") {
+ if cfg.AuthSource == "" && (cfg.AuthHeaderVal != ""{{- if .HasAuthCommand}} || cfg.AccessToken != ""{{- end}}) {
cfg.AuthSource = "config"
}
{{- if .Auth.EnvVarSpecs}}
diff --git a/internal/profiler/profiler.go b/internal/profiler/profiler.go
index 43b9c017..6c65f11b 100644
--- a/internal/profiler/profiler.go
+++ b/internal/profiler/profiler.go
@@ -167,8 +167,13 @@ func Profile(s *spec.APISpec) *APIProfile {
}
resourceNames, resourceNameIndex := collectResourceNameMetadata(s.Resources)
- syncable := make(map[string]syncableMeta) // resource name -> chosen list endpoint metadata
- parameterized := make(map[string]syncableMeta) // resource name -> parameterized list endpoint metadata (excluded from flat sync; carries IDField/Critical for dependent-resource emission)
+ syncable := make(map[string]syncableMeta) // resource name -> chosen list endpoint metadata
+ // Keyed by "<parent>/<leaf>" so the same leaf under multiple parents
+ // survives instead of first-seen-wins.
+ parameterized := make(map[string]parameterizedEntry)
+ // Mirrors the schema builder's table-naming so DependentResource.Name
+ // lines up byte-for-byte.
+ shardedSubResources := spec.SubResourceShardedNames(s)
searchable := make(map[string]map[string]struct{})
listResources := make(map[string]struct{})
@@ -182,8 +187,8 @@ func Profile(s *spec.APISpec) *APIProfile {
dateRangeParams := make(map[string]int)
responsePaths := make(map[string]int)
- var walk func(name string, r spec.Resource, inheritedTier string)
- walk = func(name string, r spec.Resource, inheritedTier string) {
+ var walk func(name string, r spec.Resource, inheritedTier string, parentName string)
+ walk = func(name string, r spec.Resource, inheritedTier string, parentName string) {
if r.Tier == "" {
r.Tier = inheritedTier
}
@@ -333,9 +338,15 @@ func Profile(s *spec.APISpec) *APIProfile {
// them for dependent-resource detection below. Carry the
// endpoint's metadata so x-resource-id and x-critical
// annotations on a child path-item flow into the override
- // and critical-resource maps.
- if _, ok := parameterized[resourceName]; !ok {
- parameterized[resourceName] = metaFromEndpoint(s, r, endpoint, s.Types, resourceNameIndex)
+ // and critical-resource maps. Store raw names so
+ // detectDependentResources can snake-case downstream.
+ key := parentName + "/" + name
+ if _, ok := parameterized[key]; !ok {
+ parameterized[key] = parameterizedEntry{
+ name: name,
+ parentName: parentName,
+ meta: metaFromEndpoint(s, r, endpoint, s.Types, resourceNameIndex),
+ }
}
} else {
// Paginated endpoints override the path set above — they have
@@ -413,12 +424,12 @@ func Profile(s *spec.APISpec) *APIProfile {
subNames := sortedKeys(r.SubResources)
for _, subName := range subNames {
sub := r.SubResources[subName]
- walk(subName, sub, r.Tier)
+ walk(subName, sub, r.Tier, name)
}
}
for name, resource := range s.Resources {
- walk(name, resource, "")
+ walk(name, resource, "", "")
}
if p.TotalEndpoints > 0 {
@@ -438,7 +449,7 @@ func Profile(s *spec.APISpec) *APIProfile {
p.NeedsSearch = len(listResources) >= 3 && float64(searchEndpointCount)/float64(len(listResources)) < 0.5
p.SyncableResources = sortedSyncableResources(syncable)
- p.DependentSyncResources = detectDependentResources(parameterized, syncable)
+ p.DependentSyncResources = detectDependentResources(parameterized, syncable, shardedSubResources)
for resource, fields := range searchable {
p.SearchableFields[resource] = sortedKeys(fields)
}
@@ -924,51 +935,45 @@ func sortedKeys[V any](m map[string]V) []string {
return keys
}
-// detectDependentResources examines parameterized paths and identifies parent-child
-// relationships. For example, /channels/{channel_id}/messages becomes a dependent
-// resource of "channels" (only one level of nesting).
-func detectDependentResources(parameterized map[string]syncableMeta, syncable map[string]syncableMeta) []DependentResource {
+// detectDependentResources examines parameterized paths and identifies
+// parent-child relationships. For example, /channels/{channel_id}/messages
+// becomes a dependent resource of "channels" (only one level of nesting).
+// When the same leaf appears under multiple parents (or collides with a
+// top-level resource), each parent emits a sharded Name so its shard syncs
+// to its own table.
+func detectDependentResources(parameterized map[string]parameterizedEntry, syncable map[string]syncableMeta, shardedSubResources spec.SubResourceShards) []DependentResource {
var deps []DependentResource
- for childName, meta := range parameterized {
- path := meta.Path
- // Extract the first {param} from the path
- start := strings.Index(path, "{")
- end := strings.Index(path, "}")
- if start < 0 || end < 0 || end <= start {
+ for _, entry := range parameterized {
+ paramName, ok := firstPathParam(entry.meta.Path)
+ if !ok {
continue
}
- paramName := path[start+1 : end]
-
- // Derive the parent resource name by stripping trailing Id/_id from the param.
- // e.g., "channel_id" -> "channel", "channelId" -> "channel"
- parentCandidate := paramName
- parentCandidate = strings.TrimSuffix(parentCandidate, "_id")
- parentCandidate = strings.TrimSuffix(parentCandidate, "Id")
- parentCandidate = strings.TrimSuffix(parentCandidate, "ID")
- parentCandidate = strings.ToLower(parentCandidate)
-
- // Check if a flat syncable resource exists matching the parent name
- // (try both singular and common plural forms).
- parentResource := ""
- for _, candidate := range []string{parentCandidate, parentCandidate + "s", parentCandidate + "es"} {
- if _, ok := syncable[candidate]; ok {
- parentResource = candidate
- break
- }
- }
+ parentResource := resolveParentResource(entry.parentName, paramName, syncable)
if parentResource == "" {
continue
}
+ emittedName := spec.ToSnakeCase(entry.name)
+ if shardedSubResources.IsSharded(entry.name) {
+ // Prefer the spec-walk parent so multi-param paths
+ // (e.g. /repos/{owner}/{repo}/commits) pick the right shard
+ // prefix; the path-param fallback would derive "owner".
+ shardParent := parentResource
+ if entry.parentName != "" {
+ shardParent = entry.parentName
+ }
+ emittedName = spec.ShardedSubResourceTableName(shardParent, entry.name)
+ }
+
deps = append(deps, DependentResource{
- Name: childName,
+ Name: emittedName,
ParentResource: parentResource,
ParentIDParam: paramName,
- Path: path,
- Tier: meta.Tier,
- IDField: meta.IDField,
- Critical: meta.Critical,
- Discriminator: meta.Discriminator,
+ Path: entry.meta.Path,
+ Tier: entry.meta.Tier,
+ IDField: entry.meta.IDField,
+ Critical: entry.meta.Critical,
+ Discriminator: entry.meta.Discriminator,
})
}
// Sort for deterministic output
@@ -978,6 +983,40 @@ func detectDependentResources(parameterized map[string]syncableMeta, syncable ma
return deps
}
+// firstPathParam returns the name of the first {param} in a path template.
+func firstPathParam(path string) (string, bool) {
+ start := strings.Index(path, "{")
+ end := strings.Index(path, "}")
+ if start < 0 || end < 0 || end <= start {
+ return "", false
+ }
+ return path[start+1 : end], true
+}
+
+// resolveParentResource picks a parent name that matches a syncable resource.
+// Prefers the spec-walk parent (correct for multi-param paths like
+// /repos/{owner}/{repo}/commits) and falls back to stripping Id/_id from the
+// path param. Returns "" when no candidate matches.
+func resolveParentResource(walkParent, paramName string, syncable map[string]syncableMeta) string {
+ if walkParent != "" {
+ candidate := strings.ToLower(walkParent)
+ if _, ok := syncable[candidate]; ok {
+ return candidate
+ }
+ }
+ stem := paramName
+ stem = strings.TrimSuffix(stem, "_id")
+ stem = strings.TrimSuffix(stem, "Id")
+ stem = strings.TrimSuffix(stem, "ID")
+ stem = strings.ToLower(stem)
+ for _, candidate := range []string{stem, stem + "s", stem + "es"} {
+ if _, ok := syncable[candidate]; ok {
+ return candidate
+ }
+ }
+ return ""
+}
+
// syncableMeta carries the chosen list endpoint's metadata while the profiler
// is still selecting between candidates (e.g., flat vs. paginated). It is
// converted into a SyncableResource at the end of Profile().
@@ -989,6 +1028,16 @@ type syncableMeta struct {
Discriminator DiscriminatorDispatch
}
+// parameterizedEntry pairs a parameterized list endpoint with the parent
+// resource it was discovered under during the spec walk. parentName is
+// empty for top-level resources whose paths happen to be parameterized;
+// detectDependentResources then falls back to the path-param heuristic.
+type parameterizedEntry struct {
+ name string
+ parentName string
+ meta syncableMeta
+}
+
// metaFromEndpoint extracts the IDField and Critical fields a parser populated
// from path-item-level extensions (or, for IDField, from response-schema
// inference). Keeps the per-endpoint plumbing in one place so future profiler
diff --git a/internal/profiler/profiler_test.go b/internal/profiler/profiler_test.go
index c305ab39..e9a2b771 100644
--- a/internal/profiler/profiler_test.go
+++ b/internal/profiler/profiler_test.go
@@ -773,6 +773,269 @@ func TestProfileDependentResources(t *testing.T) {
assert.Equal(t, "/channels/{channelId}/messages", dep.Path)
}
+// TestProfileDependentResources_SharedSubResourceShardsByParent pins the
+// fix for issue #694. When the same sub-resource leaf name (e.g. "commits")
+// appears under multiple parents (e.g. /gists/{gist_id}/commits and
+// /repos/{owner}/{repo}/commits), each parent must produce its own
+// DependentResource with a sharded Name so sync writes to the correct
+// per-parent table instead of the first-seen parent silently winning.
+func TestProfileDependentResources_SharedSubResourceShardsByParent(t *testing.T) {
+ s := &spec.APISpec{
+ Name: "github",
+ Resources: map[string]spec.Resource{
+ "gists": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/gists",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "per_page"},
+ },
+ },
+ SubResources: map[string]spec.Resource{
+ "commits": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/gists/{gist_id}/commits",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "per_page"},
+ },
+ },
+ },
+ },
+ },
+ "repos": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/repos",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "per_page"},
+ },
+ },
+ SubResources: map[string]spec.Resource{
+ "commits": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/repos/{repo_id}/commits",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "per_page"},
+ },
+ },
+ },
+ },
+ },
+ },
+ }
+
+ profile := Profile(s)
+
+ depsByName := make(map[string]DependentResource)
+ for _, dep := range profile.DependentSyncResources {
+ depsByName[dep.Name] = dep
+ }
+
+ require.Contains(t, depsByName, "gists_commits", "gists.commits should shard to gists_commits")
+ require.Contains(t, depsByName, "repos_commits", "repos.commits should shard to repos_commits")
+ assert.NotContains(t, depsByName, "commits", "bare commits would silently merge two parents")
+
+ gistsDep := depsByName["gists_commits"]
+ assert.Equal(t, "gists", gistsDep.ParentResource)
+ assert.Equal(t, "/gists/{gist_id}/commits", gistsDep.Path)
+
+ reposDep := depsByName["repos_commits"]
+ assert.Equal(t, "repos", reposDep.ParentResource)
+ assert.Equal(t, "/repos/{repo_id}/commits", reposDep.Path)
+}
+
+// TestProfileDependentResources_MultiParamParentPath confirms the walk-context
+// parent (from the SubResources tree) wins over the path-param heuristic when
+// the path has multiple params and the first one does not match a syncable
+// resource. This is the GitHub /repos/{owner}/{repo}/commits shape that the
+// path-param-only heuristic mishandles by deriving "owner" as the parent.
+func TestProfileDependentResources_MultiParamParentPath(t *testing.T) {
+ s := &spec.APISpec{
+ Name: "github",
+ Resources: map[string]spec.Resource{
+ "gists": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/gists",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "per_page"},
+ },
+ },
+ SubResources: map[string]spec.Resource{
+ "commits": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/gists/{gist_id}/commits",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "per_page"},
+ },
+ },
+ },
+ },
+ },
+ "repos": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/repos",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "per_page"},
+ },
+ },
+ SubResources: map[string]spec.Resource{
+ "commits": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/repos/{owner}/{repo}/commits",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "per_page"},
+ },
+ },
+ },
+ },
+ },
+ },
+ }
+
+ profile := Profile(s)
+
+ depsByName := make(map[string]DependentResource)
+ for _, dep := range profile.DependentSyncResources {
+ depsByName[dep.Name] = dep
+ }
+
+ require.Contains(t, depsByName, "repos_commits", "walk-context parent wins over /repos/{owner}/...'s leading param")
+ assert.Equal(t, "repos", depsByName["repos_commits"].ParentResource)
+}
+
+// TestProfileDependentResources_TopLevelCollisionShards mirrors the schema
+// builder's top-level/sub-resource collision case. When the leaf name matches
+// a top-level resource, the dependent resource emits a sharded Name so it
+// lines up with the sharded sub-resource table.
+func TestProfileDependentResources_TopLevelCollisionShards(t *testing.T) {
+ s := &spec.APISpec{
+ Name: "stytch",
+ Resources: map[string]spec.Resource{
+ "connected_apps": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/connected_apps/clients",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "limit"},
+ },
+ },
+ },
+ "users": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/users",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "limit"},
+ },
+ },
+ SubResources: map[string]spec.Resource{
+ "connected_apps": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/users/{user_id}/connected_apps",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "limit"},
+ },
+ },
+ },
+ },
+ },
+ },
+ }
+
+ profile := Profile(s)
+
+ depsByName := make(map[string]DependentResource)
+ for _, dep := range profile.DependentSyncResources {
+ depsByName[dep.Name] = dep
+ }
+
+ require.Contains(t, depsByName, "users_connected_apps", "sub-resource shards when leaf collides with a top-level resource")
+ assert.NotContains(t, depsByName, "connected_apps")
+}
+
+// TestProfileDependentResources_CamelCaseShardSnakeCased pins the snake-case
+// step in the shared shard helper. A profiler-emitted DependentResource.Name
+// must match the schema builder's table name byte-for-byte even when the
+// parent map key is camelCase.
+func TestProfileDependentResources_CamelCaseShardSnakeCased(t *testing.T) {
+ s := &spec.APISpec{
+ Name: "discovery",
+ Resources: map[string]spec.Resource{
+ "userData": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/userData",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "limit"},
+ },
+ },
+ SubResources: map[string]spec.Resource{
+ "loginEvents": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/userData/{user_id}/loginEvents",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "limit"},
+ },
+ },
+ },
+ },
+ },
+ "adminData": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/adminData",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "limit"},
+ },
+ },
+ SubResources: map[string]spec.Resource{
+ "loginEvents": {
+ Endpoints: map[string]spec.Endpoint{
+ "list": {
+ Method: "GET",
+ Path: "/adminData/{admin_id}/loginEvents",
+ Response: spec.ResponseDef{Type: "array"},
+ Pagination: &spec.Pagination{CursorParam: "after", LimitParam: "limit"},
+ },
+ },
+ },
+ },
+ },
+ },
+ }
+
+ profile := Profile(s)
+
+ names := make(map[string]bool)
+ for _, dep := range profile.DependentSyncResources {
+ names[dep.Name] = true
+ }
+ assert.True(t, names["user_data_login_events"], "DependentResource.Name snake-cases through the shard helper")
+ assert.True(t, names["admin_data_login_events"])
+}
+
func TestProfileDependentResources_NoParentNoDependent(t *testing.T) {
// If the parent resource doesn't exist as a flat syncable, no dependent is created.
s := &spec.APISpec{
diff --git a/internal/spec/naming.go b/internal/spec/naming.go
new file mode 100644
index 00000000..73717aaa
--- /dev/null
+++ b/internal/spec/naming.go
@@ -0,0 +1,82 @@
+package spec
+
+import (
+ "strings"
+ "unicode"
+)
+
+// ToSnakeCase converts camelCase, PascalCase, or kebab-case to snake_case.
+// Expects ASCII input — callers are SQL identifier paths whose inputs come
+// from parsed APISpec types/fields that already passed through the openapi
+// parser's ASCIIFold chokepoints.
+func ToSnakeCase(s string) string {
+ s = strings.ReplaceAll(s, ".", "_")
+ s = strings.ReplaceAll(s, "-", "_")
+
+ var result strings.Builder
+ for i, r := range s {
+ if unicode.IsUpper(r) && i > 0 {
+ prev := rune(s[i-1])
+ if unicode.IsLower(prev) || unicode.IsDigit(prev) {
+ result.WriteRune('_')
+ } else if unicode.IsUpper(prev) && i+1 < len(s) && unicode.IsLower(rune(s[i+1])) {
+ result.WriteRune('_')
+ }
+ }
+ result.WriteRune(unicode.ToLower(r))
+ }
+ return result.String()
+}
+
+// SubResourceShards is a lookup that flags sub-resource leaves needing
+// parent-prefixed table names. Callers ask `IsSharded(leaf)` without
+// knowing the underlying key shape (snake_case), which keeps the schema
+// builder and the profiler from reimplementing the normalization.
+type SubResourceShards struct {
+ keys map[string]bool
+}
+
+// IsSharded reports whether the given leaf name requires parent-prefixed
+// table naming. Snake-cases internally so camelCase, kebab-case, and
+// already-snake-cased inputs all resolve to the same canonical key.
+func (s SubResourceShards) IsSharded(leaf string) bool {
+ return s.keys[ToSnakeCase(leaf)]
+}
+
+// SubResourceShardedNames returns the lookup for sub-resource leaves that
+// require parent-prefixed table naming. A leaf shards when it appears
+// under multiple parents OR when it collides with a top-level resource of
+// the same name (e.g. a top-level "loginEvents" collides with a
+// sub-resource "login_events").
+func SubResourceShardedNames(s *APISpec) SubResourceShards {
+ if s == nil {
+ return SubResourceShards{}
+ }
+ parents := make(map[string]map[string]bool)
+ topLevelKeys := make(map[string]bool, len(s.Resources))
+ for parentName, parent := range s.Resources {
+ topLevelKeys[ToSnakeCase(parentName)] = true
+ for subName := range parent.SubResources {
+ key := ToSnakeCase(subName)
+ if parents[key] == nil {
+ parents[key] = make(map[string]bool)
+ }
+ parents[key][ToSnakeCase(parentName)] = true
+ }
+ }
+ shards := make(map[string]bool, len(parents))
+ for subKey, parentSet := range parents {
+ if len(parentSet) > 1 || topLevelKeys[subKey] {
+ shards[subKey] = true
+ }
+ }
+ return SubResourceShards{keys: shards}
+}
+
+// ShardedSubResourceTableName returns the snake-cased per-parent table name
+// for a sub-resource that requires sharding. Both the profiler (when emitting
+// DependentResource.Name) and the schema builder (when emitting the table)
+// must call this so the names line up exactly.
+func ShardedSubResourceTableName(parent, leaf string) string {
+ return ToSnakeCase(parent + "_" + leaf)
+}
diff --git a/testdata/golden/expected/generate-mcp-api/mcp-cloudflare/internal/mcp/tools.go b/testdata/golden/expected/generate-mcp-api/mcp-cloudflare/internal/mcp/tools.go
index e90d0901..20acfcbd 100644
--- a/testdata/golden/expected/generate-mcp-api/mcp-cloudflare/internal/mcp/tools.go
+++ b/testdata/golden/expected/generate-mcp-api/mcp-cloudflare/internal/mcp/tools.go
@@ -31,7 +31,7 @@ func RegisterTools(s *server.MCPServer) {
s.AddTool(
mcplib.NewTool("sql",
mcplib.WithDescription("Run read-only SQL against local database. Use for ad-hoc analysis, aggregations, and joins across synced resources. Requires sync first."),
- mcplib.WithString("query", mcplib.Required(), mcplib.Description("SQL query (SELECT only). Tables match resource names.")),
+ mcplib.WithString("query", mcplib.Required(), mcplib.Description("SQL query (SELECT or WITH...SELECT). Tables match resource names.")),
mcplib.WithReadOnlyHintAnnotation(true),
mcplib.WithDestructiveHintAnnotation(false),
),
@@ -215,6 +215,60 @@ func dbPath() string {
// Note: MCP tools use their own dbPath() because they are in a separate package (main, not cli).
// The CLI's defaultDBPath() in the cli package uses the same canonical path.
+// validateReadOnlyQuery gates the MCP sql tool. The agent contract advertised
+// to the host is ReadOnlyHintAnnotation(true); a false annotation on a
+// mutating tool lets MCP hosts auto-approve writes and is treated as a real
+// bug per the project's agent-native security model.
+//
+// The gate is an allowlist (SELECT or WITH only) applied AFTER stripping the
+// leading whitespace, line comments, block comments, and semicolons that
+// SQLite itself ignores before parsing. A naive HasPrefix check on a
+// keyword blocklist is bypassable by prefixing the dangerous statement with
+// "/* x */" or "-- x\n" — TrimSpace strips outer whitespace but does not
+// understand SQL comment syntax. Combined with the empirical fact that
+// modernc.org/sqlite's mode=ro does NOT block VACUUM INTO (writes a snapshot
+// to a new file) or ATTACH DATABASE (opens a separate writable handle),
+// such a bypass produces silent exfiltration to an attacker-chosen path.
+//
+// SELECT and WITH are the only allowed leading keywords. WITH supports
+// SELECT-form CTEs; CTE-wrapped writes ("WITH x AS (...) INSERT ...") are
+// caught by OpenReadOnly's mode=ro one layer down. PRAGMA, ATTACH, VACUUM,
+// and every other DDL/DML keyword fail at this gate before reaching SQLite.
+func validateReadOnlyQuery(query string) error {
+ upper := strings.ToUpper(stripLeadingSQLNoise(query))
+ if !strings.HasPrefix(upper, "SELECT") && !strings.HasPrefix(upper, "WITH") {
+ return fmt.Errorf("only SELECT queries are allowed")
+ }
+ return nil
+}
+
+// stripLeadingSQLNoise removes leading whitespace, SQL line comments
+// (-- to end of line), block comments (/* ... */), and statement
+// separators (;) from query. SQLite skips these before parsing the first
+// keyword, so a security gate that does not strip them mismatches what the
+// driver actually executes.
+func stripLeadingSQLNoise(query string) string {
+ for {
+ query = strings.TrimLeft(query, " \t\r\n;")
+ switch {
+ case strings.HasPrefix(query, "--"):
+ if idx := strings.IndexByte(query, '\n'); idx >= 0 {
+ query = query[idx+1:]
+ continue
+ }
+ return ""
+ case strings.HasPrefix(query, "/*"):
+ if idx := strings.Index(query[2:], "*/"); idx >= 0 {
+ query = query[2+idx+2:]
+ continue
+ }
+ return ""
+ default:
+ return query
+ }
+ }
+}
+
func handleSQL(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.CallToolResult, error) {
args := req.GetArguments()
query, ok := args["query"].(string)
@@ -222,15 +276,11 @@ func handleSQL(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.CallToo
return mcplib.NewToolResultError("query is required"), nil
}
- // Block write operations
- upper := strings.ToUpper(strings.TrimSpace(query))
- for _, prefix := range []string{"INSERT", "UPDATE", "DELETE", "DROP", "ALTER", "CREATE"} {
- if strings.HasPrefix(upper, prefix) {
- return mcplib.NewToolResultError("only SELECT queries are allowed"), nil
- }
+ if err := validateReadOnlyQuery(query); err != nil {
+ return mcplib.NewToolResultError(err.Error()), nil
}
- db, err := store.OpenWithContext(ctx, dbPath())
+ db, err := store.OpenReadOnly(dbPath())
if err != nil {
return mcplib.NewToolResultError(fmt.Sprintf("opening database: %v", err)), nil
}
← 9e039c0b fix(cli): source store columns from response schema, not que
·
back to Cli Printing Press
·
chore(main): release 4.0.6 (#711) c9dd54ae →