fix: add tracing by default and fix bugs#2166
Conversation
SkArchon
requested review from
JivusAyrus,
StarpTech,
endigma and
jensneuse
as code owners
WalkthroughUpdates the router plugin scaffold: bumps the generated router-plugin dependency and adds routerplugin.WithTracing() to the generated NewRouterPlugin call; adjusts the init command to place scaffolded plugins under a project-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
SkArchon
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cli/src/commands/router/commands/plugin/templates/plugin.ts (3)
47-51: Fix hard-coded service import path; use the generated package in the new moduleThe imports currently point to the Cosmo repo ("github.com/wundergraph/cosmo/plugin/generated"). In generated projects, the service package will live under the plugin’s module. This will not compile outside the monorepo.
Apply these diffs:
Main template:
- service "github.com/wundergraph/cosmo/plugin/generated" + service "{modulePath}/generated"Test template:
- service "github.com/wundergraph/cosmo/plugin/generated" + service "{modulePath}/generated"To prevent regressions, consider adding a unit assertion in the CLI that verifies no "github.com/wundergraph/cosmo/plugin/generated" strings are emitted. Do you want me to add such a guard?
Also applies to: 95-101
261-273: README still references plugins/{originalPluginName}/; update to plugin-root layoutProject templates moved to a plugin-root layout, but this README still shows the old plugins/… structure. This will confuse users following the guide.
Apply this diff to the tree block:
- plugins/{originalPluginName}/ + {originalPluginName}/Also scan the file for other "plugins/{originalPluginName}" occurrences and update accordingly. I can send a complete patch if you prefer.
327-342: Cursor rules structure block uses old plugins/… pathFor consistency with the new layout and the updated project-level README, switch to the plugin-root structure.
-plugins/{originalPluginName}/ +{originalPluginName}/cli/src/commands/router/commands/plugin/templates/project.ts (1)
10-33: Add pass-through test/generate targets or adjust READMEThe project-level README advertises make generate/test, but the Makefile doesn’t provide them. Add pass-through targets that delegate into {originalPluginName} to avoid confusing users.
.PHONY: install-wgc build download start compose +.PHONY: generate test make: install-wgc download build compose start @@ build: cd {originalPluginName} && make build + +generate: install-wgc + cd {originalPluginName} && make generate + +test: install-wgc + cd {originalPluginName} && make test
🧹 Nitpick comments (6)
cli/src/commands/router/commands/plugin/templates/plugin.ts (4)
11-16: Prefer a stable SemVer tag over a pseudo-version for router-pluginThe pseudo-version pin can introduce cache/proxy inconsistencies. If v0.4.1 is the intended release, depend on it directly.
Apply this diff in the go.mod template:
- github.com/wundergraph/cosmo/router-plugin v0.0.0-20250824152218-8eebc34c4995 // v0.4.1 + github.com/wundergraph/cosmo/router-plugin v0.4.1
54-59: Enabling tracing by default: good direction — ensure opt-out and docs are alignedAdding routerplugin.WithTracing() is aligned with the PR goal. Given tracing impacts performance and sampling, ensure there’s a documented opt-out (env/config flag) and that this matches the router’s tracing defaults. Based on learnings, the tracer plumbing is non-nil by construction, so additional defensive checks aren’t needed.
If there is an env/config switch, reference it in the README and project docs. Otherwise, I can add a minimal snippet to gate tracing at runtime on an env var without changing the default.
405-427: Unify binary naming and paths between docs and DockerfileDocs refer to bin/plugin, while the Dockerfile produces dist/plugin and renames the artifact to {originalPluginName}-plugin. Aligning on one name reduces confusion and scripting conditionals.
Option A (match docs: bin/plugin):
-RUN --mount=type=cache,target="/root/.cache/go-build" CGO_ENABLED=0 GOOS=$TARGETOS GOARCH=$TARGETARCH go build -o dist/plugin ./src +RUN --mount=type=cache,target="/root/.cache/go-build" CGO_ENABLED=0 GOOS=$TARGETOS GOARCH=$TARGETARCH go build -o bin/plugin ./src ... -COPY --from=builder /build/dist/plugin ./{originalPluginName}-plugin -ENTRYPOINT ["./{originalPluginName}-plugin"] +COPY --from=builder /build/bin/plugin ./plugin +ENTRYPOINT ["./plugin"]Option B (match Dockerfile: keep {originalPluginName}-plugin) — then update README to reflect the artifact name.
6-17: Confirm Go toolchain version across environmentsThe template sets go 1.24.1 and uses golang:1.24-alpine. Please confirm CI, local dev, and wgc scaffolding images have Go 1.24 available to avoid bootstrap friction.
I can add a preflight check in the Makefile to assert the local Go version meets the minimum before building.
cli/src/commands/router/commands/plugin/templates/project.ts (2)
55-58: Router plugins.path switched to '.' — verify discovery works with multiple pluginsPointing to '.' is fine for a single plugin. If multiple plugins will be added under {pluginFolders...}, consider setting a dedicated directory (e.g., plugins/) or clarifying in docs. Otherwise router might scan unrelated content.
I can add an end-to-end verification script that composes graph.yaml and starts the router with the downloaded binary to ensure the plugin loads from '.'.
71-84: Project README structure updated — ensure consistency with plugin README and templatesThe top-level tree now reflects plugin-root layout. Cross-check that the plugin-level README and cursor rules (in plugin.ts) are updated as well to avoid mixed guidance. See related comments in plugin.ts.
Do you want me to submit a follow-up patch that updates all remaining references in one go?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cli/src/commands/router/commands/plugin/templates/plugin.ts(2 hunks)cli/src/commands/router/commands/plugin/templates/project.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
PR: wundergraph/cosmo#2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
cli/src/commands/router/commands/plugin/templates/plugin.ts
📚 Learning: 2025-08-12T13:50:45.964Z
Learnt from: Noroth
PR: wundergraph/cosmo#2132
File: router-plugin/plugin.go:139-146
Timestamp: 2025-08-12T13:50:45.964Z
Learning: In the Cosmo router plugin system, the plugin framework creates its own logger independently. When creating a logger in NewRouterPlugin, it's only needed for the gRPC server setup (passed via setup.GrpcServerInitOpts.Logger) and doesn't need to be assigned back to serveConfig.Logger.
Applied to files:
cli/src/commands/router/commands/plugin/templates/plugin.ts
📚 Learning: 2025-08-20T21:05:58.731Z
Learnt from: SkArchon
PR: wundergraph/cosmo#2158
File: router/pkg/grpcconnector/grpccommon/grpc_plugin_client.go:149-153
Timestamp: 2025-08-20T21:05:58.731Z
Learning: In the Cosmo codebase, the GRPCPluginClient struct fields `tracer` and `getTraceAttributes` are always properly initialized during construction and will never be nil, so defensive nil checks are not needed.
Applied to files:
cli/src/commands/router/commands/plugin/templates/plugin.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
cli/src/commands/router/commands/plugin/templates/project.ts (2)
35-41: LGTM: graphConfig points to the plugin-rootThe path: {originalPluginName} matches the new layout and will work with the updated router plugins.path.
173-186: LGTM: plugin README adopts plugin-root layoutThe structure block now starts at {originalPluginName}/, consistent with project-level docs.
SkArchon
force-pushed
the
milinda/update-plugin-version
branch
from
d2dde87 to
48fa40e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/commands/router/commands/plugin/commands/init.ts (1)
102-117: Create the parent 'plugins' directory before moving; otherwise rename will fail (ENOENT).When options.project is provided, pluginDir points to /plugins/, but only projectDir is created. If /plugins doesn't exist, fs.rename will throw ENOENT on most systems.
Apply this minimal fix:
// Create a project directory structure - await mkdir(projectDir, { recursive: true }); + await mkdir(projectDir, { recursive: true }); + // Ensure "plugins" exists when scaffolding into project/plugins/<name> + await mkdir(resolve(projectDir, 'plugins'), { recursive: true }); // Write router config to the project root await writeFile(resolve(projectDir, 'config.yaml'), ProjectTemplates.routerConfig); @@ // Move plugin from temp directory to project plugins directory await rename(tempDir, pluginDir);
🧹 Nitpick comments (2)
cli/src/commands/router/commands/plugin/commands/init.ts (2)
29-33: Simplify pluginDir calculation; remove redundant cwd and match Prettier style.resolve() ignores preceding segments once an absolute path is encountered, so passing cwd again is redundant. This also aligns with the Prettier complaint.
Apply:
- const pluginDir = (options.project) - ? resolve(cwd, projectDir, 'plugins', name) - : resolve(cwd, projectDir, name); + const pluginDir = options.project + ? resolve(projectDir, 'plugins', name) + : resolve(projectDir, name);
115-117: Optional: handle cross-device moves (EXDEV) when moving from tmpdir to workspace.tmpdir can be on a different filesystem; fs.rename will fail with EXDEV. Fallback to cp + rm to make this robust.
Apply:
-import { access, mkdir, rename, rm, writeFile } from 'node:fs/promises'; +import { access, mkdir, rename, rm, writeFile, cp } from 'node:fs/promises'; @@ - await rename(tempDir, pluginDir); + try { + await rename(tempDir, pluginDir); + } catch (err: any) { + if (err?.code === 'EXDEV') { + // Different device: copy then remove + await mkdir(pluginDir, { recursive: true }); + await cp(tempDir, pluginDir, { recursive: true }); + await rm(tempDir, { recursive: true, force: true }); + } else { + throw err; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
cli/src/commands/router/commands/plugin/commands/init.ts(1 hunks)cli/src/commands/router/commands/plugin/templates/plugin.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/src/commands/router/commands/plugin/templates/plugin.ts
🧰 Additional context used
🪛 GitHub Actions: wgc CI
cli/src/commands/router/commands/plugin/commands/init.ts
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
cli/src/commands/router/commands/plugin/commands/init.ts (2)
29-33: Confirm intent: project scaffolds into 'project/plugins/' but PR text says generated projects currently avoid the plugin folder.This change explicitly uses the plugins folder when --project is set. If the intent is to avoid using plugins/, consider reverting to project/. If the intent is to standardize on plugins/, ensure docs and templates (graph.yaml, README) reference the new path.
Would you like me to scan templates for hard-coded paths and open a follow-up patch to align them?
1-1: Fix Prettier CI failure.The CI indicates a Prettier formatting failure. After applying the above diffs, run your formatter from the repo root.
Typical commands:
- pnpm prettier --write .
- or: pnpm -w format
- or: npx prettier --write .
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/src/commands/router/commands/plugin/commands/init.ts (2)
28-35: Sanitize the plugin name and fix pluginDir resolution (avoid redundant cwd).
- Path traversal/FS injection risk:
nameis used directly to form paths; malicious values (e.g.,../../x) could escape the intended directory. Sanitize before using it in paths.- Also compute
pluginDirafter capturingoriginalPluginName, and drop the redundantcwdsegment inresolve(...)to improve clarity.Apply this diff:
const projectDir = resolve(cwd, options.directory, options.project); - - const pluginDir = options.project ? resolve(cwd, projectDir, 'plugins', name) : resolve(cwd, projectDir, name); - - const originalPluginName = name; + const originalPluginName = name; + // sanitize for filesystem safety: allow letters, numbers, '-', '_' + const pluginDirName = originalPluginName.replace(/[^a-zA-Z0-9_-]/g, '-'); + if (pluginDirName !== originalPluginName) { + console.warn(pc.yellow(`Sanitized plugin directory name to '${pluginDirName}'.`)); + } + const pluginDir = options.project + ? resolve(projectDir, 'plugins', pluginDirName) + : resolve(projectDir, pluginDirName);
115-116: Handle cross-device moves: fs.rename may fail with EXDEV when moving from tmp to project.
rename(tempDir, pluginDir)can fail across devices (common when tmp is on a separate mount). Add a fallback to copy+remove, and importcp.Apply these diffs:
- import { access, mkdir, rename, rm, writeFile } from 'node:fs/promises'; + import { access, mkdir, rename, rm, writeFile, cp } from 'node:fs/promises';- await rename(tempDir, pluginDir); + try { + await rename(tempDir, pluginDir); + } catch (err: any) { + if (err?.code === 'EXDEV') { + await cp(tempDir, pluginDir, { recursive: true }); + await rm(tempDir, { recursive: true, force: true }); + } else { + throw err; + } + }- await rename(tempDir, pluginDir); + try { + await rename(tempDir, pluginDir); + } catch (err: any) { + if (err?.code === 'EXDEV') { + await cp(tempDir, pluginDir, { recursive: true }); + await rm(tempDir, { recursive: true, force: true }); + } else { + throw err; + } + }Also applies to: 147-148, 2-2
🧹 Nitpick comments (3)
cli/src/commands/router/commands/plugin/commands/init.ts (3)
62-65: Use warn instead of fail for unsupported language fallback.
spinner.failimplies the operation failed even though the command continues with Go; preferspinner.warnfor a softer signal.- spinner.fail(pc.yellow(`Language '${options.language}' is not supported yet. Using 'go' instead.`)); + spinner.warn(pc.yellow(`Language '${options.language}' is not supported yet. Using 'go' instead.`));
95-99: Avoid embedding absolute paths in generated docs; use a relative path for better portability.Passing an absolute
pluginDirintocursorRulesbakes machine-specific paths into the scaffold. Prefer a path relative to the CWD (or project root).Apply these diffs:
- import { resolve } from 'pathe'; + import { resolve, relative } from 'pathe';- const pluginDir = options.project + const pluginDir = options.project ? resolve(projectDir, 'plugins', pluginDirName) : resolve(projectDir, pluginDirName); + const pluginDirRel = relative(cwd, pluginDir);- pupa(PluginTemplates.cursorRules, { name, originalPluginName, pluginDir }), + pupa(PluginTemplates.cursorRules, { name, originalPluginName, pluginDir: pluginDirRel }),- pupa(PluginTemplates.cursorRules, { name, originalPluginName, pluginDir }), + pupa(PluginTemplates.cursorRules, { name, originalPluginName, pluginDir: pluginDirRel }),Also applies to: 126-129, 6-6, 30-35
71-114: Reduce duplication between the project and non-project branches.Both branches write nearly identical temp scaffolds. Consider extracting helpers (e.g.,
writeBaseScaffold(tempDir, srcDir, generatedDir, ctx)andfinalizeScaffold(destDir)), differing only in where the result is moved and which project-level files are created.If helpful, I can follow up with a refactor patch that introduces these helpers without changing behavior.
Also applies to: 116-144
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cli/src/commands/router/commands/plugin/commands/init.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
cli/src/commands/router/commands/plugin/commands/init.ts (1)
102-103: LGTM: proactively creating the plugins directory.Creating
${projectDir}/pluginsup front prevents parent-missing errors and aligns with the new layout.
3 participants
This PR Does the following
Summary by CodeRabbit
Checklist