Skip to content

feat: add data plugin with inline dashboard rendering plan#92

Merged
cyyeh merged 2 commits into
mainfrom
worktree-add-data-plugin
Mar 3, 2026
Merged

feat: add data plugin with inline dashboard rendering plan#92
cyyeh merged 2 commits into
mainfrom
worktree-add-data-plugin

Conversation

@cyyeh
Copy link
Copy Markdown
Owner

@cyyeh cyyeh commented Mar 3, 2026

Summary

  • Integrates the Anthropic data plugin (plugins/data/) with Docker volume mounts for sidecar containers
  • Adds implementation plan for inline dashboard rendering via render_dashboard MCP tool + sandboxed iframe
  • Documents the current limitation that /data:build-dashboard generates HTML but cannot display it in the UI yet

Test plan

  • Verify docker compose build completes with plugin volume mount changes
  • Verify plugin commands (/data:analyze, /data:write-query, etc.) work in the sidecar
  • Review implementation plan at docs/plans/2026-03-03-inline-dashboard-rendering.md

🤖 Generated with Claude Code

cyyeh and others added 2 commits March 3, 2026 22:21
…ds and skills

Integrates the Anthropic data plugin (from anthropics/knowledge-work-plugins)
into the sidecar, providing slash commands (/data:analyze, /data:write-query,
etc.) and skills (sql-queries, data-exploration, data-visualization, etc.).

- Bundle plugin at plugins/data/ with neutralized .mcp.json
- Configure sidecar to load the plugin via SDK plugins array
- Mount plugins/ read-only into Docker containers
- Update skill allowlist hook and system prompt to allow plugin-namespaced skills
- Pre-create /app/plugins in Dockerfile for bind-mount compatibility
- Update README with Plugins section, env vars, and architecture docs

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: 4c001114e073
Add implementation plan for rendering data plugin dashboards inline
in the chat UI via a render_dashboard MCP tool and sandboxed iframe.
Document the current limitation that /data:build-dashboard generates
HTML in the sidecar but it is not yet visible to users.

Also includes docker-compose plugin volume mount changes from the
data plugin integration.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Entire-Checkpoint: 4f17de0996a0
@cyyeh cyyeh merged commit fbc1db0 into main Mar 3, 2026
1 check passed
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Mar 3, 2026

Claude finished @cyyeh's task in 2m 46s —— View job


Code Review

  • Gather context and understand PR scope
  • Review infrastructure changes (Docker, container manager)
  • Review sidecar server changes
  • Review plugin structure and documentation
  • Review implementation plan
  • Compile and post final review

This PR integrates the Anthropic data plugin with Docker volume mounts and lays out a detailed plan for inline dashboard rendering. The overall approach is sound and well-documented. Here's a detailed review:


Security

1. Overly broad plugin skill bypass (sidecar/src/server.ts:71-73)

if (skillName && skillName.includes(":")) {
  return {};
}

The allowlist hook bypasses validation for any skill name containing a colon — not just data:* skills. If a malicious actor could register a plugin with a crafted name, or if a future plugin is added that shouldn't be fully trusted, this bypass grants blanket permission. Consider validating against a known set of allowed plugin namespaces:

const ALLOWED_PLUGIN_NAMESPACES = new Set(["data"]);
const [namespace] = skillName.split(":");
if (namespace && ALLOWED_PLUGIN_NAMESPACES.has(namespace)) {
  return {};
}

Fix this →

2. Memory leak in proposed HtmlDashboardWidget (docs/plans/2026-03-03-inline-dashboard-rendering.md:308-315)

The "Open in new tab" button creates a blob URL but never calls URL.revokeObjectURL():

const blob = new Blob([html], { type: 'text/html' });
const url = URL.createObjectURL(blob);
window.open(url, '_blank');
// Missing: URL.revokeObjectURL(url)

The download button (just above) correctly revokes. The "open in tab" case should too. Note this for when implementing Task 5. Fix this →

3. Iframe sandbox notes (plan Task 5)

The sandbox="allow-scripts" without allow-same-origin is the correct approach — it gives the dashboard null origin, blocking access to parent cookies, localStorage, and DOM. The security notes in the plan document this accurately. One minor addition worth noting for the implementation: Chart.js loads from a CDN inside the iframe; if a strict CSP is later added to the parent page, <iframe sandbox="allow-scripts"> without allow-same-origin means the iframe's own CSP is independent, so CDN requests should still work.


Code Quality

4. Hardcoded plugin skill names in system prompt (sidecar/src/server.ts:268)

const pluginSkillNote = " You may also invoke any plugin-namespaced skills ... such as 'data:sql-queries', 'data:analyze', 'data:write-query', 'data:create-viz', 'data:explore-data', 'data:build-dashboard', 'data:validate', etc."

This hardcodes the data plugin's skill names inline. If the plugin adds/renames skills, this string requires a manual update. Since the plugin directory is accessible, consider dynamically reading skill names from ./plugins/data/commands/ or ./plugins/data/skills/ directories at startup. This is low-priority but would reduce maintenance burden.

5. Path dependency on ${PWD} in docker-compose.yml (docker-compose.yml:42)

PLUGINS_HOST_PATH: ${PLUGINS_HOST_PATH:-${PWD}/plugins}

${PWD} works correctly when docker compose is run from the project root (which is standard), but it's worth noting this assumption in the README. The existing SKILLS_HOST_PATH uses the same pattern, so this is consistent with the project convention.

6. plugins_host_path env var is not validated (backend/app/container_manager.py:127-134)

The PLUGINS_HOST_PATH env var is used directly as a host mount path. While os.path.abspath() normalizes it, there's no check that the resolved path is within the expected project directory. Since this is set by a server-side env var rather than user input, the risk is low, but a simple sanity check (e.g., confirming the path exists and is a directory) would make failures more debugible. The os.path.isdir() check on the fallback path already handles the fallback case correctly.


Performance

7. discoverSkills() called multiple times per request (sidecar/src/server.ts:62-84, 266)

discoverSkills() does a filesystem scan and is called: (1) in the hook callback per tool use, and (2) once per request for the skill restriction. For high-frequency usage, consider caching the result for a short TTL (e.g., 5 seconds) with invalidation on file changes, or reading it once per request and passing it down. This is pre-existing behavior extended by this PR.


Testing

8. No test coverage for new functionality

The PR adds no tests for:

  • Plugin volume mount logic in container_manager.py (the plugins_host_path resolution)
  • The plugin skill namespace bypass in the sidecar hook
  • Plugin loading in the SDK query options

Given the existing test pattern in backend/tests/, at minimum a test verifying that PLUGINS_HOST_PATH env var is correctly picked up and that the volume binding is included in the containers.run() call would be valuable.


Documentation

9. README Dashboard limitation (README.md:76) ✅

The limitation is clearly stated:

"The /data:build-dashboard command can generate HTML dashboards inside the sidecar container, but the output is not yet visible in the UI"

This is exactly the right pattern — acknowledging the limitation and pointing to the plan. Well done.

10. CONNECTORS.md placeholder * footnote

The placeholder MCP servers (Snowflake, Databricks) are clearly marked with * and explained in the footnote. This avoids user confusion about unconfigured connections.

11. Implementation plan quality

The plan in docs/plans/2026-03-03-inline-dashboard-rendering.md is exceptionally detailed with exact file paths, line numbers, and copy-pasteable code snippets for each step. The architecture decision to follow the render_chart pattern rather than introduce a file-serving endpoint is sound.


Minor Nits

  • plugins/data/.mcp.json is {"mcpServers": {}} (empty). This is intentional per the README explanation that external connections are neutralized. A comment in that file explaining why it's empty would help future contributors. Fix this →

  • The plugins/data/LICENSE is Apache 2.0, which is compatible with the project. No concerns.


Summary

Area Status
Security ⚠️ Plugin bypass too broad; blob URL leak in plan
Code Quality ✅ Clean, consistent with existing patterns
Performance ℹ️ Minor: multiple skill discovery scans
Testing ❌ No new tests
Documentation ✅ Excellent — plan is thorough, README updated

The PR is well-structured as a foundation for the dashboard feature. The most actionable items before implementing the dashboard plan are: tightening the plugin namespace allowlist and adding the URL.revokeObjectURL call in HtmlDashboardWidget.

@cyyeh cyyeh deleted the worktree-add-data-plugin branch March 3, 2026 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant