Skip to content

feat: add MCP API key functionality for ACP agents - #368

Merged
pikann merged 4 commits into
masterfrom
feature/add-mcp-api-key-functionality-for-acp-agents
Aug 7, 2026
Merged

feat: add MCP API key functionality for ACP agents#368
pikann merged 4 commits into
masterfrom
feature/add-mcp-api-key-functionality-for-acp-agents

Conversation

@pikann

@pikann pikann commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Gives each ACP agent its own MCP API key, and overhauls the local-bridge setup guide around it.

Backend — per-agent MCP API key

  • Each ACP agent can now generate its own MCP API key, mirroring the existing ACP bridge token pattern: only the SHA-256 hash is persisted, the plaintext is shown once, and generating a new key immediately revokes whatever key was live before (one key per agent at a time).
  • New endpoints: POST /projects/:projectId/agents/:agentId/mcp-agent-key and its global-agent sibling POST /admin/agents/:agentId/mcp-agent-key.
  • The authn middleware now resolves an incoming MCP key directly to its owning agent (FindAgentByMCPAPIKeyHash) instead of relying on the old shared AGENT_API_KEY + a client-supplied X-Agent-ID header — this closes a cross-agent impersonation gap, since a leaked key can now only ever authenticate as the one agent it was generated for, not any agent in the deployment.
  • Migration 000032 adds agents.mcp_api_key_hash with a unique partial index.
  • New/updated unit and integration tests covering key generation, the authn fallback path, and the new handler endpoints.

Frontend — local bridge setup guide

  • Reordered the setup steps so "run the bridge" is last — it starts the CLI process that depends on the skill/MCP config from the earlier steps, so starting it first would launch the CLI before that config exists.
  • Merged "install the skill" and "connect the MCP server" into one step: a single key-generation action produces two commands shown underneath it, rather than one &&-chained line that got too long to read or copy comfortably.
  • Both the ACP bridge token and the new MCP key are now generated automatically when an ACP agent is created, so the post-creation setup dialog shows ready-to-copy commands immediately instead of requiring an extra click per credential.
  • Claude Code: added the claude setup-tokenexport CLAUDE_CODE_OAUTH_TOKEN=... two-step flow before the run command — the bridge invokes claude non-interactively, so the CLI needs a valid token already exported when it starts.
  • Codex / Gemini CLI: added an equivalent reminder to export OPENAI_API_KEY / GEMINI_API_KEY before starting the bridge.
  • All new and changed strings translated across all 9 supported locales.

- Introduced MCP API key generation for ACP agents, allowing for unique identification of agent actions.
- Updated agent entity to include MCP API key hash and related fields.
- Implemented API endpoints for generating MCP API keys for both project-specific and global agents.
- Enhanced authentication middleware to support MCP API key validation, ensuring requests can be attributed to the correct agent.
- Added necessary database migrations to support the new MCP API key structure.
- Updated relevant tests to cover new functionality and ensure proper integration.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR — per-agent MCP API keys are a solid improvement over the shared AGENT_API_KEY. I read the full diff, ran the Go unit tests for the touched packages (all passed), and attempted the web typecheck, but the ephemeral runner has no node_modules installed so I could not validate the TypeScript build.

Overall the shape looks correct: SHA-256-only storage, key regeneration that immediately invalidates the old key, a partial unique index, and authn middleware that resolves the key directly to an agent. I do have a few requested changes and suggestions before merging.

Requested changes

  1. Add unit tests for the new MCP key service methods. GenerateAgentMCPKey and GenerateGlobalAgentMCPKey currently have no direct test coverage that I could find, and the authentication fallback path in apikeysvc.Authenticate and FindAgentByMCPAPIKeyHash is only exercised indirectly. Please add tests for:

    • happy-path generation returns a 64-hex plaintext and stores a SHA-256 hash;
    • regenerating overwrites the old hash (old key no longer authenticates);
    • non-ACP agents are rejected;
    • project/global scope validation works (project agent via GetAgent, global agent via GetGlobalAgent);
    • Authenticate resolves a valid per-agent MCP key to the correct AgentID;
    • a wrong key still returns ErrNotFound/ErrInvalidKey rather than resolving to an agent.
  2. Do not swallow non-ErrNotFound errors in FindAgentByMCPAPIKeyHash. Currently the repository returns ErrAgentNotFound for any error, including a database failure. This can turn a transient DB outage into a misleading 401/404. Return the underlying error unless it is sql.ErrNoRows.

  3. Update apps/mcp/README.md. The README still describes a single AGENT_API_KEY and says PACA_PROJECT_ID is required when PACA_AGENT_ID is set. After this PR, ACP agents use a per-agent PACA_API_KEY and PACA_PROJECT_ID is optional for global agents. Please rewrite the setup table and the "Agent Mode vs. User Mode" section to match the new flow shown in the UI.

Suggestions (non-blocking)

  • Consider adding an explicit "Revoke MCP key" action in the UI/API. Right now the only way to invalidate a key is to generate a new one, which is fine but not obvious to operators.
  • The migration comment explains the partial unique index well, but consider adding the same safety note to the domain/repository code so future readers understand why mcp_api_key_hash is unique only among ACP agents.
  • In Authenticate, the MCP key check happens only after the personal key lookup fails. That ordering is fine because personal keys have revocation/expiration semantics that agent keys do not, but consider documenting why agent keys intentionally bypass last_used_at and expiration checks.

I'll approve once the tests and README are addressed and the error-handling fix is in place.


Note: 1 inline comment(s) dropped because they did not anchor to lines inside the PR diff:

  • apps/mcp/README.md:113 (RIGHT) — file not in PR diff

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

Comment thread services/api/internal/service/agent/agent_service.go
Comment thread services/api/internal/repository/postgres/agent_repository.go
Comment thread services/api/internal/service/apikey/apikey_service.go Outdated
pikann added 2 commits August 6, 2026 10:46
CI was failing lint on both the web app (biome format) and services/api
(gofmt) — no logic changes, formatting only.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The latest push resolves the prior review's test-coverage and repository error-handling concerns, but apps/mcp/README.md still documents the old shared-key model and needs to be updated before this ships.

Reviewed changes

This incremental review covers the commits since the prior Pullfrog review at 1f0dd8a5.

  • Refactored credential generation in create-agent-dialog.tsx to issue the ACP bridge token and MCP key concurrently via Promise.allSettled, so agent creation remains fast and a failure in either step does not block the other.
  • Extracted generateHashedSecret in agent_service.go and reused it for bridge tokens and MCP keys, removing duplicated random/SHA-256 code.
  • Added MCP key unit tests for GenerateAgentMCPKey and GenerateGlobalAgentMCPKey covering success, non-ACP rejection, and project/global scope enforcement.
  • Fixed FindAgentByMCPAPIKeyHash error handling so database errors propagate instead of being swallowed as ErrAgentNotFound.
  • Added Authenticate fallback tests confirming a per-agent MCP key resolves to the correct AgentID, personal keys take precedence, and unknown keys fail closed.
  • Added integration tests exercising the full per-agent MCP key auth path, including key regeneration invalidating the old key and X-Agent-ID being ignored when a per-agent key is presented.
  • Added handler tests for POST /admin/agents/:agentId/mcp-agent-key.

⚠️ Stale MCP README will mislead users

apps/mcp/README.md still describes the old shared AGENT_API_KEY model and states that PACA_PROJECT_ID is required whenever PACA_AGENT_ID is set. After this PR, ACP agents authenticate with a per-agent PACA_API_KEY and global agents work without PACA_PROJECT_ID. Please update the environment-variable table, the "Agent Mode vs. User Mode" section, and any examples that reference the shared key.

Technical details
# Stale MCP README

## Affected sites
- apps/mcp/README.md — env var table still references shared `AGENT_API_KEY`
- apps/mcp/README.md — "Agent Mode vs. User Mode" table still references `Global AGENT_API_KEY from server config`
- apps/mcp/README.md — `PACA_PROJECT_ID` documented as required when `PACA_AGENT_ID` is set

## Required outcome
- README describes the per-agent `PACA_API_KEY` generated via the UI/API.
- README clarifies that `PACA_PROJECT_ID` is optional for global ACP agents.
- README removes references to the server-side shared `AGENT_API_KEY`.

## Suggested approach
Mirror the updated UI copy in `acp-bridge-setup.tsx` and the integration-test scenarios in `services/api/test/integration/agent_apikey_test.go`.

Pullfrog  | Fix it ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found. All prior review feedback has been addressed.

Reviewed changes

This incremental review covers the commit since the prior Pullfrog review at 20f7fdee.

  • Rewrote apps/mcp/README.md to document the per-agent PACA_API_KEY model: removed references to the shared server-side AGENT_API_KEY, clarified that PACA_PROJECT_ID is optional for global ACP agents, added a global-agent configuration example, and updated the mode table to show agent-global scope.
  • Hardened apikeysvc.Authenticate so that only ErrAgentNotFound falls through to the final ErrNotFound; any other error from the MCP-key fallback lookup (e.g. a transient DB failure) now propagates as-is instead of being silently reported as an invalid API key.
  • Added TestAuthenticate_AgentStoreDBError_Propagates to lock in that behavior.

All touched Go unit tests pass (internal/service/agent, internal/service/apikey, internal/repository/postgres, internal/transport/http/handler).

Pullfrog  | View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@pikann
pikann merged commit c601841 into master Aug 7, 2026
7 checks passed
@pikann
pikann deleted the feature/add-mcp-api-key-functionality-for-acp-agents branch August 7, 2026 04:48
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