feat: ADMP Comprehensive MVP Implementation - #6
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f0f477c48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const envelope = { | ||
| version: '1.0', | ||
| id: messageId, | ||
| type, | ||
| from: from_agent ? `agent://${from_agent}` : `agent://${process.env.AGENT_ID || 'unknown'}`, | ||
| to: `agent://${to_agent}`, | ||
| subject, |
There was a problem hiding this comment.
The MCP sendMessage tool still constructs from/to as agent://..., but the new MessageHandler.validateEnvelope enforces local@domain identities. As a result, any message sent via the MCP server will be rejected by the API’s validation (invalid from/to), breaking MCP message delivery for all callers. Update the MCP envelope formatting to emit the canonical local@domain form (or convert input to it) to keep the MCP tool functional under the new spec.
Useful? React with 👍 / 👎.
|
Claude finished @dundas's task in 2m 32s —— View job PR Review: ADMP Comprehensive MVP ImplementationTodo List
OverviewThis is an impressive, comprehensive implementation of the ADMP MVP with 86 files changed and substantial new functionality. The architecture is well-designed with proper separation of concerns, comprehensive security layers, and production-ready deployment configuration. Critical Issues1. MCP Identity Format Mismatch (P0 - Blocking)Location: The MCP server is still using the old from: from_agent ? `agent://${from_agent}` : `agent://${process.env.AGENT_ID || 'unknown'}`,
to: `agent://${to_agent}`,But the message validation enforces the new } else if (!/^[a-zA-Z0-9._-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/.test(envelope.from)) {
errors.push('from must be a valid identity address (local@domain)');
}Impact: All messages sent via the MCP Fix needed: Fix MCP identity format → 2. Example Code Uses Old Format (P1)Location: The example code still uses fromAgent = 'agent://client-agent',
toAgent = 'agent://mech-storage',This will mislead developers trying to use the SDK. Fix needed: Update example code → Security ReviewStrengths ✅
Areas for Improvement1. Inbox Keys Query Inefficiency (P2)Location: const keys = await this.storageClient.queryMessages({ limit: 1000 });
const key = keys.find(k => k.key_hash === keyHash);This loads ALL inbox keys (up to 1000) on every validation. For production:
2. Missing Mailgun Webhook Signature Verification (P1)Location: // Verify Mailgun signature (implement webhook signature verification)
// For now, accept all (add HMAC verification in production)This is documented as TODO but is a security risk. Without webhook signature verification, anyone can forge delivery status updates. Recommendation: Implement Mailgun webhook HMAC verification before production deployment (Mailgun provides HMAC signature in webhook headers). 3. Agent Registry Linear Scan (P2)Location: async getAgent(agentId) {
const agents = await this.storageClient.queryMessages({ limit: 1000 });
return agents.find(a => a.agent_id === agentId) || null;
}Similar to inbox keys, this loads all agents on every lookup. Consider:
Implementation QualityExcellent ✅
Minor Issues1. Inconsistent Table Name Management (P3)Services repeatedly set agentsStorageClient.tableName = 'agents';
messagesClient.tableName = 'messages';
jobsStorageClient.tableName = 'jobs';Consider passing table name as a constructor parameter or creating table-specific client classes. 2. Hard-coded Limits (P3)Many queries use const keys = await this.storageClient.queryMessages({ limit: 1000 });Consider making this configurable via environment variables for different deployment scales. 3. Missing Input Sanitization (P2)Location: const decodedAgentId = decodeURIComponent(agentId);While URL decoding is necessary, there's no validation that the decoded value is safe. The identity validation happens later, but consider validating immediately after decoding. Documentation & SpecsStrengths ✅
Database SchemaThe schema design is solid:
Note: Migration scripts assume mech-storage as the backend. Ensure schema compatibility is tested during setup. RecommendationsBefore Merge
Post-Merge (Production Hardening)
Testing Recommendations
SummaryThis is a high-quality, well-architected implementation that demonstrates strong engineering practices: ✅ Comprehensive security with proper cryptographic primitives Blocking Issues: 2 (MCP format, example code) Recommendation: Approve with required changes. Fix the MCP identity format and example code before merging. The webhook verification and performance optimizations can be addressed in the production hardening phase. The 4+ week dogfooding period mentioned in the PRD is well-justified given the scope of this implementation. |
Fix MCP identity format to use local@domain - Update src/mcp/server.js to use canonical identity format - Change from agent://id to local@domain (e.g., billing@acme.com) - Add URL encoding for agent IDs in API calls - Add AGENT_ID and AGENT_DOMAIN environment variables Update examples to use canonical identity format - Fix examples/sendBugReport.js to use local@domain - Update default values: client@yourco.com, mech-storage@mechdna.net - Add URL encoding for @ symbols in agent IDs - Remove obsolete agent:// prefix stripping logic Implement Mailgun webhook HMAC verification - Add crypto import for HMAC-SHA256 signature verification - Implement verifyMailgunSignature() helper function - Verify timestamp, token, and signature from webhook - Add replay attack protection (5-minute timestamp window) - Reject webhooks with invalid or missing signatures - Add MAILGUN_SIGNING_KEY environment variable Security improvements: - Prevent webhook forgery with HMAC verification - Protect against replay attacks with timestamp validation - Return 401 for invalid signatures, 500 if not configured Testing: - All 19 existing tests pass - Syntax validation passed for all modified files - Identity format now consistent across MCP, examples, and API Refs: PR #6 code review Fixes: P0-1 (MCP format), P0-2 (examples), P1-1 (webhook security)
Add detailed analysis and action plans for PR #6 code review: - GAP_ANALYSIS.md: Complete issue breakdown with checklists - MERGE_ACTION_PLAN.md: Step-by-step fix instructions - CODE_REVIEW_FIXES.md: Quick reference with code diffs - ISSUE_MATRIX.md: Priority matrix and fix sequences - PR_REVIEW_SUMMARY.txt: Executive summary These documents track the resolution of all P0, P1, P2, and P3 issues identified in the code review process.
|
Addressed stale test/compatibility issues in commit b58bfc6:\n- Fixed test scripts for current Node test runner (tests/*.test.js)\n- Updated agent registry tests to local@domain identity format\n- Updated message handler tests to direct identity storage semantics\n\nValidation:\n- npm test\n- Result: 53 pass, 0 fail |
b58bfc6 to
b617805
Compare
Summary
Complete implementation of the Agent Dispatch Messaging Protocol (ADMP) MVP with canonical
local@domainidentity format, capability-based permissions, cryptographic signature verification, policy enforcement, and production-ready SMTP infrastructure.Implementation Overview
✅ Identity & Addressing
local@domain(e.g.,billing@acme.com)local@agents.domain✅ Security (3-Layer Anti-Prompt-Injection)
✅ Core Services Added
InboxKeysService- Capability token management (SHA-256 hashed storage)KeyDiscoveryService- Public key discovery via HTTPS JWKS and DNS TXTPolicyEngine- Trust lists + subject patterns + constraint enforcementMailgunClient- Outbound SMTP relay via Mailgun APIJobsService- Durable job queue with exponential backoff✅ HTTP API Endpoints
POST /v1/agents/:agentId/keys- Create inbox keyPOST /v1/agents/:agentId/messages- Send message (with signature + policy)POST /v1/agents/:agentId/inbox/pull- Pull message with leasePOST /v1/agents/:agentId/messages/:messageId/ack- AcknowledgePOST /v1/agents/:agentId/messages/:messageId/nack- Requeue or extend leasePOST /v1/agents/:agentId/messages/:messageId/reply- Send correlated replyGET /v1/messages/:messageId/status- Get message status✅ SMTP Transport
POST /v1/inbound/smtpPOST /v1/webhooks/mailgun/deliveryfor status tracking✅ Database Schema (4 Tables)
admp_agents- Identity + public_key + policies (trusted_agents, allowed_subjects, size limits)admp_messages- Inbox with lease-based processingadmp_inbox_keys- Capability tokens (hashed, scoped, TTL)admp_jobs- Durable job queue (smtp_send, webhook_delivery, cleanup)✅ Background Worker
src/worker.js)✅ Deployment (Fly.io)
web(API) +worker(jobs)✅ Tests
File Statistics
Key Files
Core Implementation
src/services/inboxKeysService.js- Capability tokenssrc/services/keyDiscovery.js- Public key discoverysrc/services/policyEngine.js- Trust + constraintssrc/services/mailgunClient.js- SMTP outboundsrc/services/jobsService.js- Job queuesrc/worker.js- Background processorRoutes & Middleware
src/routes/keys.js- Inbox key managementsrc/routes/smtp.js- SMTP inbound + webhookssrc/middleware/inboxKeys.js- Capability enforcementDeployment
Dockerfile- Multi-stage Node.js buildfly.toml- Process groups + health checkscloudflare-worker/- Email Worker for inbound SMTPDocumentation
ARCHITECTURE.md- System overview with diagramsIMPLEMENTATION_SUMMARY.md- Detailed task breakdownDEPLOYMENT_CHECKLIST.md- Step-by-step deploymentCHANGES.md- Complete changesetspec/ADDRESSING.md- Formal addressing specTest Plan
Local Development
Production Deployment
fly deploy fly scale count web=2 worker=1 # See DEPLOYMENT_CHECKLIST.md for full validationBreaking Changes
Identity Format
agent://service.namelocal@domainfrom/tofields in stored messages and client codeAuthentication
/v1/agents/:agentId/keysPath Parameters
/v1/agents/storage/messages/v1/agents/billing%40acme.com/messages(URL-encoded)Alignment with Specs
This implementation follows:
spec/ADMP-SPEC.md- Core protocol specificationspec/ADDRESSING.md- Identity and routing rulestasks/0001-prd-agent-dispatch-mvp.md- Product requirementswhitepaper/v1.md- Technical architectureNext Steps
References