Skip to content

fix(sdk): skip plugin code during agent discovery - #1026

Open
lifelmy wants to merge 6 commits into
CodebuffAI:mainfrom
lifelmy:agent-tasks/1018
Open

fix(sdk): skip plugin code during agent discovery#1026
lifelmy wants to merge 6 commits into
CodebuffAI:mainfrom
lifelmy:agent-tasks/1018

Conversation

@lifelmy

@lifelmy lifelmy commented Aug 16, 2026

Copy link
Copy Markdown

Summary

  • exclude .agents/plugins trees from local agent-module discovery
  • add a regression test proving a nested MCP CommonJS executable is never imported

Root cause

loadLocalAgents recursively treated every supported JavaScript or TypeScript file under .agents as a candidate agent module and imported it before checking whether it exported an agent definition. Third-party plugin and MCP executables can inspect the host process arguments or call process.exit(), so discovery could terminate the Freebuff CLI.

Closes #1018.

Validation

  • bun test sdk/src/__tests__/load-agents.test.ts --test-name-pattern 'does not execute nested MCP implementation files'
  • bun test sdk/src/__tests__/load-agents.test.ts (34 passed)
  • bun run --cwd sdk typecheck
  • bun run build:sdk
  • git diff --check

@lifelmy
lifelmy marked this pull request as ready for review August 24, 2026 05:22
@codebuff-team

Copy link
Copy Markdown
Contributor

Good, minimal fix that matches the pattern already used for node_modules, scripts, and skills* in shouldSkipAgentDirectory (sdk/src/agents/load-agents.ts:111-116). The added test in load-agents.test.ts is a solid regression check — it writes a marker file from inside a nested plugins/example/mcp/bin/mcp-proxy.cjs and asserts the marker never appears, which is a much stronger assertion than just checking the export wasn't picked up.

One thing worth flagging for the maintainer doing the port: this closes the door specifically on files under a directory literally named plugins, but the underlying problem — loadLocalAgents require/import-ing every .ts/.js/.cjs file under .agents before checking whether it exports an agent definition — is more general. Any other subdirectory containing a JS file with top-level side effects (not just plugin/MCP bins) would still execute on discovery. That's plausibly a follow-up rather than a blocker for this PR, since it directly addresses the reported issue (#1018) and the plugins/MCP case is the concrete exploit path, but it's not a complete fix for the discovery-model risk.

Scope-wise this is entirely in sdk/src, so no forbidden-path issues. The change is small, tested, and matches existing code conventions, so it's a reasonable candidate to port as-is, with a note that broader hardening of loadLocalAgents (e.g., detect exports without executing side effects, or sandbox the require) may be worth a separate issue.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree labels Aug 24, 2026
@lifelmy

lifelmy commented Aug 24, 2026

Copy link
Copy Markdown
Author

Thanks for the thorough review. Agreed on both points:

  • Scope of this PR: it deliberately closes the concrete exploit path from CLI exits after ~6-10s in interactive TUI; bare freebuff prints "Usage: node mcp_proxy.js" (Crostini/Debian 12, 0.0.148+0.0.149) #1018 — a JS/CJS file under a plugins directory (the MCP proxy bin) being required during discovery — mirroring the existing node_modules / scripts / skills* skips in shouldSkipAgentDirectory. The regression test asserts the side-effect marker never fires, so it fails on the pre-fix loader.
  • Broader discovery-model risk: you're right that loadLocalAgents importing every .ts/.js/.cjs under .agents before checking for an agent export is the general hazard, and any other subdir with top-level side effects would still execute. That's a larger change (detect exports without executing side effects, or sandbox the require), so I kept it out of this fix to stay minimal and reviewable. Happy to file a separate issue tracking the general hardening if you'd like — just say the word and I'll open it.

Let me know if you'd prefer any adjustment before the port.

@lifelmy

lifelmy commented Aug 26, 2026

Copy link
Copy Markdown
Author

Refreshed this branch onto the latest public main; new head is ad150c4d6. The merge completed cleanly and only incorporated upstream public snapshot updates. The PR diff remains scoped to the plugin-directory skip in agent discovery and its regression test.

Verification on ad150c4d6:

  • git diff --check -> clean
  • bun test sdk/src/__tests__/load-agents.test.ts -> 34 pass / 0 fail
  • bun run --cwd sdk typecheck -> exit 0
  • bun run build:sdk -> build complete
  • bun run build:freebuff -> build complete

I use a coding assistant to help implement, and I review and take responsibility for the final change.

@lifelmy

lifelmy commented Aug 26, 2026

Copy link
Copy Markdown
Author

Updated once more after public main advanced again; new head is 2158bfe2e. The merge completed cleanly and only incorporated the latest upstream public snapshot changes. The PR diff remains scoped to the plugin-directory skip in agent discovery and its regression test.

Verification on 2158bfe2e:

  • git diff --check -> clean
  • bun test sdk/src/__tests__/load-agents.test.ts -> 34 pass / 0 fail
  • bun run --cwd sdk typecheck -> exit 0
  • bun run build:sdk -> build complete
  • bun run build:freebuff -> build complete

I use a coding assistant to help implement, and I review and take responsibility for the final change.

@lifelmy

lifelmy commented Aug 27, 2026

Copy link
Copy Markdown
Author

Refreshed this branch onto the latest public main; new head is 7cee43529. The merge completed without manual conflicts and only incorporated upstream public snapshot updates (bun.lock, common/src/constants/freebuff-ads.ts).

Verification run after the refresh:

  • git diff --check
  • bun test sdk/src/__tests__/load-agents.test.ts (34 pass / 0 fail)
  • bun run --cwd sdk typecheck
  • bun run build:sdk
  • bun run build:freebuff

I use a coding assistant to help implement, and I review and take responsibility for the final change.

@lifelmy

lifelmy commented Aug 27, 2026

Copy link
Copy Markdown
Author

Refreshed this branch onto current public main; new head is d82aa69ca. The merge completed without manual conflicts and only brought in the upstream common/src/types/freebuff-session.ts change.

Verified after refresh:

  • git diff --check
  • bun test sdk/src/__tests__/load-agents.test.ts (34 pass / 0 fail / 69 expects)
  • bun run --cwd sdk typecheck
  • bun run build:sdk
  • bun run build:freebuff

I use a coding assistant to help implement, and I review and take responsibility for the final change.

@lifelmy

lifelmy commented Aug 27, 2026

Copy link
Copy Markdown
Author

Refreshed this branch onto the latest public main; new head is f95d144e6. The merge completed without manual conflicts and only incorporated the latest upstream public snapshot updates (README*, free-model catalog/constants/tests, and related CLI settings coverage). The PR diff remains scoped to skipping plugin code during agent discovery and its regression test.

Verification after refresh:

  • bun test sdk/src/__tests__/load-agents.test.ts -> 34 pass / 0 fail
  • bun run --cwd sdk typecheck -> exit 0
  • bun run build:sdk -> success
  • bun run build:freebuff -> success

I use a coding assistant to help implement, and I review and take responsibility for the final change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:port-candidate Worth porting into the private source tree

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI exits after ~6-10s in interactive TUI; bare freebuff prints "Usage: node mcp_proxy.js" (Crostini/Debian 12, 0.0.148+0.0.149)

2 participants