Conversation
📝 WalkthroughWalkthroughAdds a new MCP Registry design and initial FastAPI implementation with SQLAlchemy models, Pydantic schemas, CRUD and REST endpoints, Docker/pyproject scaffolding, and Terraform infra to provision VPC, ALB, ECS, RDS, ECR, S3, Secrets Manager, and IAM for deployment and operation. (34 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer/Client
participant API as MCP Registry (FastAPI)
participant DB as PostgreSQL
participant S3 as OpenAPI Specs (S3)
rect rgb(220,230,255)
note right of Dev: Agent registration
Dev->>API: POST /agents (metadata + spec)
API->>S3: PUT spec -> returns s3_uri + checksum
S3-->>API: s3_uri + checksum
API->>DB: INSERT agent (metadata + s3_uri + checksum + version)
DB-->>API: 201 Created
API-->>Dev: 201 Created (agent id, version, s3_uri)
end
rect rgb(230,255,220)
note right of Dev: Discovery / retrieval
Dev->>API: GET /agents or /search?capability=...
API->>DB: Query/filter agents
DB-->>API: metadata results
alt include spec content
API->>S3: GET spec by s3_uri
S3-->>API: spec
API-->>Dev: 200 (metadata + spec)
else spec uri only
API-->>Dev: 200 (metadata + s3_uri)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mcp/MCP_Registry_Plan.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
mcp/MCP_Registry_Plan.md
47-47: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
50-50: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
54-54: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
57-57: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
61-61: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
62-62: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
65-65: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
66-66: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
68-68: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
71-71: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
72-72: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
73-73: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
75-75: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
78-78: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
79-79: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
94-94: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
162-162: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
163-163: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
165-165: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
173-173: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
189-189: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
190-190: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
191-191: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
210-210: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
211-211: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
212-212: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
246-246: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
247-247: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
248-248: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
249-249: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
250-250: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
252-252: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
253-253: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
254-254: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
264-264: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
265-265: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
266-266: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
267-267: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
268-268: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
269-269: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
275-275: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
276-276: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
277-277: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
278-278: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
284-284: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
285-285: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
286-286: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
287-287: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
288-288: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mcp/MCP_Registry_Plan.md (1)
259-291: Implementation roadmap provides a realistic path to production.The three-phase approach (MVP, CLI/Search, Hardening) is well-scoped and achievable. Phase 1 focuses on core functionality, Phase 2 improves DX with tooling, and Phase 3 hardens for production readiness. Pilot team onboarding in Phase 1 is a smart validation step.
Minor enhancement suggestion: Consider adding observability and alerting requirements to Phase 3 explicitly (e.g., API latency/error rate SLOs, database performance metrics, failed auth attempts). These are foundational for operational confidence.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mcp/MCP_Registry_Plan.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
mcp/MCP_Registry_Plan.md
33-33: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
34-34: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
35-35: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
36-36: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
140-140: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
141-141: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
142-142: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
143-143: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
144-144: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
146-146: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
147-147: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
148-148: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
149-149: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
150-150: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
151-151: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
163-163: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
164-164: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
165-165: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
166-166: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
167-167: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
168-168: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
174-174: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
175-175: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
176-176: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
177-177: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
189-189: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
190-190: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
191-191: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
192-192: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
193-193: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
199-199: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
205-205: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
211-211: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
212-212: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
213-213: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
214-214: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
215-215: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
219-219: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
220-220: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
228-228: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
229-229: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
230-230: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
231-231: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
232-232: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
236-236: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
237-237: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
241-241: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
242-242: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
243-243: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
244-244: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
248-248: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
249-249: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
250-250: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
251-251: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
252-252: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
253-253: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
254-254: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
255-255: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
256-256: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
257-257: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
265-265: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
266-266: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
267-267: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
268-268: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
269-269: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
270-270: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
271-271: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
272-272: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
276-276: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
277-277: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
278-278: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
279-279: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
280-280: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
281-281: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
285-285: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
286-286: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
287-287: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
288-288: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
289-289: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
290-290: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
291-291: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (3)
mcp/MCP_Registry_Plan.md (3)
1-50: Excellent technical planning document—clear vision and well-executed.The MCP Registry plan is comprehensive and well-structured. The distinction between a discovery service and a hosting platform is clearly articulated, and the proposed API endpoints, data model, and phased roadmap are all pragmatic and implementable.
Key strengths:
- Clear agent.json manifest schema with all necessary fields (identity, contract, location).
- Pragmatic architecture (FastAPI + PostgreSQL) aligned with operational simplicity.
- Security model combining API key authentication with role-based access control (Reader/Publisher/Admin).
- Realistic phased roadmap with MVP-first approach.
90-120: Architecture diagram and components are clear and well-explained.The system component breakdown (REST API, PostgreSQL, Validation Logic) is sensible. The data model with indexed fields on
name,version,environment, andtags(JSONB) will support efficient discovery and search operations. The use of soft deletes for dependency safety is pragmatic.
181-220: Security and multi-environment strategy is thoughtful.The RBAC design (Reader/Publisher/Admin) is appropriate for an internal registry. The multi-environment support through the
environmentfield in agent.json is well-considered, though you may want to document the recommended approach for environment-specific API keys as a future enhancement to enforce stricter isolation if governance requirements evolve.The VPC-based deployment with optional external access via API Gateway is sound; consider documenting the expected access patterns (internal-only vs. conditional external access) in operational runbooks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mcp/MCP_Registry_Plan.md (1)
1-463: Markdown linting issues remain unresolved.The static analysis tool reports numerous MD007 (incorrect list indentation) and MD036 (emphasis used instead of heading) violations throughout the document. These issues were noted in previous review cycles and marked as addressed in commit 0d8fb55, but they still appear in the current version (lines 161, 239, and many list items using 2–4 space indentation instead of proper nesting).
Recommendation: Run
markdownlint-cli2on the file and fix the remaining formatting violations to ensure consistency and compliance with the project's markdown standards.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mcp/MCP_Registry_Plan.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
mcp/MCP_Registry_Plan.md
33-33: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
34-34: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
35-35: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
36-36: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
141-141: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
142-142: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
143-143: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
144-144: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
145-145: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
146-146: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
147-147: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
148-148: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
149-149: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
150-150: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
151-151: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
152-152: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
153-153: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
161-161: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
190-190: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
191-191: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
192-192: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
193-193: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
194-194: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
195-195: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
196-196: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
197-197: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
198-198: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
199-199: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
200-200: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
201-201: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
202-202: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
203-203: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
204-204: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
210-210: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
211-211: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
212-212: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
213-213: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
220-220: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
221-221: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
222-222: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
223-223: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
224-224: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
239-239: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
242-242: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
243-243: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
244-244: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
245-245: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
248-248: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
249-249: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
250-250: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
253-253: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
254-254: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
255-255: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
256-256: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
257-257: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
258-258: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
264-264: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
270-270: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
271-271: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
272-272: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
274-274: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
276-276: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
277-277: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
278-278: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
279-279: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
294-294: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
295-295: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
296-296: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
297-297: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
298-298: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
304-304: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
305-305: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
306-306: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
307-307: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
308-308: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
309-309: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
317-317: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
318-318: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
319-319: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
320-320: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
321-321: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
325-325: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
326-326: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
330-330: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
331-331: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
332-332: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
333-333: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
337-337: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
338-338: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
339-339: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
340-340: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
341-341: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
342-342: Unordered list indentation
Expected: 4; Actual: 6
(MD007, ul-indent)
343-343: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
344-344: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
345-345: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
346-346: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
387-387: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
388-388: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
389-389: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
410-410: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
411-411: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
412-412: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
413-413: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
437-437: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
438-438: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
439-439: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
440-440: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
441-441: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
442-442: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
443-443: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
444-444: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
448-448: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
449-449: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
450-450: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
451-451: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
452-452: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
453-453: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
457-457: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
458-458: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
459-459: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
460-460: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
461-461: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
462-462: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
463-463: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (1)
mcp/MCP_Registry_Plan.md (1)
157-175: Clarify environment isolation strategy—Section 4.3 vs Section 6.4 appears contradictory.Section 4.3 makes a strong recommendation for "One Registry per Environment" (separate instances for
dev,stage,prod), but Section 6.4 states "The registry will support multiple environments within a single deployed instance" and describes anenvironmentquery parameter for logical separation. These two approaches are fundamentally different and the plan should commit to one strategy.Recommendation: Decide whether to:
- Implement true environment isolation (one registry per environment), in which case Section 6.4 should be removed or retitled as "Rejected Alternative."
- Implement logical separation (single instance with
environmentfield), in which case Section 4.3's strong recommendation should be downgraded or contextualized as a future enhancement to be pursued only if operationally justified.
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Fix all issues with AI agents
In @mcp/mcp_registry/crud.py:
- Around line 7-8: get_agents currently returns all rows including soft-deleted
ones; update it to only return agents where models.Agent.is_deleted is False by
adding a filter(models.Agent.is_deleted == False) before offset/limit; make the
same change in get_agent (e.g., include models.Agent.is_deleted == False in the
query/filter for the agent lookup by id) and in get_agents_by_capability (ensure
the capability query or join also filters models.Agent.is_deleted == False so
deleted agents are excluded).
- Around line 51-52: Update get_agents_by_capability to exclude soft-deleted
agents: modify the query that joins models.Agent.capabilities (inside
get_agents_by_capability) to add a filter that only returns non-deleted agents
(e.g., add models.Agent.deleted_at.is_(None) or models.Agent.is_deleted == False
depending on your Agent model’s soft-delete field), while keeping the existing
capability filter models.Capability.name == capability_name.
- Around line 10-33: The issue is that create_agent calls create_capability
which currently commits its own transaction, causing orphaned capability rows if
the agent commit later fails; fix by changing create_capability to not perform
its own commit/refresh but instead accept and use the passed Session to add and
return the capability (or add an optional commit flag defaulting to False), then
in create_agent ensure all new capabilities are added to the same session and
perform a single db.commit()/db.refresh(db_agent) at the end; update any callers
of create_capability to handle the new non-committing behavior or explicitly
commit when needed.
In @mcp/mcp_registry/database.py:
- Around line 19-24: The association table agent_capability_association lacks a
primary key which can allow duplicate rows and break ORM operations; update the
Table definition for agent_capability_association to declare a composite primary
key on ('agent_id', 'capability_id') by adding PrimaryKeyConstraint or marking
the Column objects as primary_key=True, ensuring the FK columns reference
agents.id and capabilities.id and prevent duplicate associations.
- Around line 12-13: The fallback DATABASE_URL variable currently contains
hardcoded credentials (DATABASE_URL = os.getenv(...,
"postgresql://user:password@localhost/mcp_registry")), which is unsafe; change
the logic to not provide a default credentialed URL: read DATABASE_URL from the
environment only (os.getenv("DATABASE_URL")), check if it is None/empty, and if
so raise an explicit error (e.g., raise RuntimeError or ValueError with a clear
message) so the process fails fast instead of silently using insecure defaults;
update any callers that assume a value accordingly and document that
DATABASE_URL is required.
In @mcp/mcp_registry/main.py:
- Line 10: Remove the module-level table-creation call
models.Base.metadata.create_all(bind=engine) so table creation only happens
inside the explicit startup handler (on_startup); locate and delete the
standalone invocation at module import while keeping the
models.Base.metadata.create_all(bind=engine) call that lives inside the
on_startup event/function to avoid redundant execution.
- Around line 15-21: There is a duplicate get_db generator here; remove the
local definition (the def get_db that instantiates SessionLocal and yields/
closes it) and instead import the existing get_db from the database module
(e.g., from database import get_db); also remove any now-unused SessionLocal
import/usage in this file and ensure all references still call get_db so the
single shared implementation is used.
In @mcp/mcp_registry/schemas.py:
- Around line 11-16: The Capability schema currently embeds a bidirectional
relation (Capability -> List["Agent"] and Agent -> List[Capability]) which can
cause infinite recursion during serialization; create lightweight response DTOs
(e.g., CapabilitySimple and AgentSimple) that omit the back-reference and
replace the nested fields with these simple types (e.g., in Agent use
capabilities: List[CapabilitySimple] = Field(default_factory=list) and in
Capability use agents: List[AgentSimple] = Field(default_factory=list)); update
CapabilityBase/AgentBase-derived classes to use orm_mode and ensure you
import/declare the new CapabilitySimple and AgentSimple types and replace any
direct circular references in codepaths that return API responses.
In @mcp/terraform/alb.tf:
- Around line 67-76: The ALB currently exposes HTTP only via
aws_lb_listener.http; for production you must add an HTTPS listener and ACM
cert: create an aws_acm_certificate (e.g., aws_acm_certificate.main) with DNS
validation records (aws_route53_record.cert_validation) and an
aws_acm_certificate_validation resource, then replace or add an aws_lb_listener
for port 443 configured with ssl_policy and certificate_arn referencing
aws_acm_certificate.main. Also update the ALB listener default_action to forward
to aws_lb_target_group.main, ensure the ALB security group allows inbound TCP
443, and update any ECS service or terraform dependencies that assume port 80 to
depend on/route to the new listener.
In @mcp/terraform/ecr.tf:
- Around line 4-16: The ECR repo resource aws_ecr_repository.main uses unsafe
prod settings: change image_tag_mutability from "MUTABLE" to "IMMUTABLE" to
prevent tag overwrites and set force_delete from true to false to avoid deleting
repositories with images; update those attributes in the aws_ecr_repository
"main" block and ensure any environments that require different settings use
variable overrides or separate Terraform workspaces for POC vs production.
In @mcp/terraform/ecs.tf:
- Around line 33-38: The DATABASE_URL is being set as a literal
"secretsmanager:${aws_secretsmanager_secret.db_credentials.name}" in the
environment block which ECS will not resolve; instead move DATABASE_URL into the
task definition's secrets array (alongside DB_CREDENTIALS) and reference the
secret ARN/secret manager key so ECS injects the actual value, or if you must
build a non-sensitive URL from RDS outputs, put the computed plain value into
environment rather than the secretsmanager: string; update the resource that
defines environment = [...] to remove DATABASE_URL and add an entry in secrets =
[...] referencing aws_secretsmanager_secret.db_credentials (or the correct
secret ARN/key) so the container receives the real DB URL.
In @mcp/terraform/main.tf:
- Around line 15-37: The aws_subnet.public and aws_subnet.private blocks index
into data.aws_availability_zones.available.names using count.index but there is
no guard that var.public_subnet_cidrs or var.private_subnet_cidrs do not exceed
the number of available AZs; add validation to the corresponding variables
(var.public_subnet_cidrs and var.private_subnet_cidrs) or a pre-check in main
using length(data.aws_availability_zones.available.names) to ensure
length(var.*_subnet_cidrs) <=
length(data.aws_availability_zones.available.names) and surface a clear error
message, or alternatively clamp indexing with min(count.index,
length(data.aws_availability_zones.available.names)-1) to avoid out-of-range
accesses.
In @mcp/terraform/s3.tf:
- Around line 4-10: The aws_s3_bucket resource "aws_s3_bucket.main" currently
lacks versioning and at-rest encryption; update that resource to enable object
versioning and configure server-side encryption. Add a "versioning" block with
"enabled = true" to track and rollback spec changes, and add a
"server_side_encryption_configuration" (or the equivalent
aws_s3_bucket_server_side_encryption_configuration resource) with an
apply_server_side_encryption_by_default rule setting sse_algorithm to "AES256"
(or "aws:kms" plus kms_master_key_id if you require KMS) so all objects are
encrypted at rest.
In @mcp/terraform/variables.tf:
- Around line 40-45: The variable declaration for db_username currently provides
a default sensitive value ("mcpadmin"), which is insecure; remove the default
assignment from the variable "db_username" so it remains required and sensitive,
ensuring callers must explicitly set it (keep type = string and sensitive =
true) and update any docs/terraform.tfvars or module callers to supply
db_username explicitly.
🧹 Nitpick comments (12)
mcp/Dockerfile (1)
1-16: Consider adding a non-root user for security.Running containers as root is a security risk. Consider creating a non-root user and switching to it before running the application.
🔒 Proposed security improvements
# Use an official Python runtime as a parent image FROM python:3.11-slim # Set the working directory in the container WORKDIR /app +# Create a non-root user +RUN useradd --create-home --shell /bin/bash appuser # Copy the pyproject.toml file and install dependencies COPY pyproject.toml . COPY ./mcp_registry ./mcp_registry RUN pip install --no-cache-dir . +# Change ownership and switch to non-root user +RUN chown -R appuser:appuser /app +USER appuser # Expose the port the app runs on EXPOSE 80 +# Add healthcheck +HEALTHCHECK --interval=30s --timeout=5s --start-period=5s --retries=3 \ + CMD python -c "import urllib.request; urllib.request.urlopen('http://localhost:80/')" || exit 1 # Run the application CMD ["uvicorn", "mcp_registry.main:app", "--host", "0.0.0.0", "--port", "80"]mcp/mcp_registry/database.py (2)
41-41:datetime.utcnowis deprecated in Python 3.12+.Use
datetime.now(timezone.utc)instead for timezone-aware datetimes.♻️ Proposed fix
+from datetime import datetime, timezone -from datetime import datetime - created_at = Column(DateTime, default=datetime.utcnow) + created_at = Column(DateTime(timezone=True), default=lambda: datetime.now(timezone.utc))
6-6:declarative_base()is legacy SQLAlchemy 1.x style.SQLAlchemy 2.0 recommends using
DeclarativeBaseclass instead. This is optional for now but worth considering for forward compatibility.♻️ SQLAlchemy 2.0 style
-from sqlalchemy.ext.declarative import declarative_base +from sqlalchemy.orm import DeclarativeBase -Base = declarative_base() +class Base(DeclarativeBase): + passmcp/mcp_registry/main.py (1)
24-26:@app.on_event("startup")is deprecated in FastAPI.FastAPI recommends using the
lifespancontext manager instead ofon_eventdecorators.♻️ Migration to lifespan
+from contextlib import asynccontextmanager + +@asynccontextmanager +async def lifespan(app: FastAPI): + # Startup + create_db_and_tables() + yield + # Shutdown (if needed) -app = FastAPI() +app = FastAPI(lifespan=lifespan) -@app.on_event("startup") -def on_startup(): - create_db_and_tables()mcp/mcp_registry/crud.py (1)
45-45:.dict()is deprecated in Pydantic v2.Use
.model_dump()for Pydantic v2 compatibility.♻️ Proposed fix
- db_capability = models.Capability(**capability.dict()) + db_capability = models.Capability(**capability.model_dump())mcp/mcp_registry/schemas.py (1)
15-16: Consider migrating to Pydantic v2 syntax.
orm_mode = Trueis Pydantic v1 syntax. Pydantic v2 (bundled with FastAPI) renames this tofrom_attributesand requires usingmodel_config = ConfigDict(from_attributes=True)instead.♻️ Pydantic v2 syntax
+from pydantic import ConfigDict + class Capability(CapabilityBase): id: int agents: List["Agent"] = [] - class Config: - orm_mode = True + model_config = ConfigDict(from_attributes=True)Also applies to: lines 38-39 (Agent class)
mcp/pyproject.toml (1)
5-11: Pinning dependency versions is recommended for production readiness.Unpinned dependencies risk unexpected breaking changes when new versions are released. For applications, use a lockfile (poetry.lock, requirements.lock, or Pipenv.lock) to pin exact versions in production. For development, consider using looser range constraints like
>=X.Y.Z,<NEXT_MAJORto balance flexibility with stability.📦 Example with updated version constraints
dependencies = [ - "fastapi", - "uvicorn", - "psycopg2-binary", - "SQLAlchemy", - "python-dotenv", + "fastapi>=0.120.0,<1.0.0", + "uvicorn>=0.35.0,<1.0.0", + "psycopg2-binary>=2.9.9,<3.0.0", + "SQLAlchemy>=2.0.40,<3.0.0", + "python-dotenv>=1.0.0,<2.0.0", ]For production deployments, generate and commit a lockfile (e.g., using
pip-tools, Poetry, or Pipenv) to ensure reproducible installs with exact pinned versions.mcp/terraform/rds.tf (1)
4-20: Consider adding explicit egress rules.The security group only defines ingress rules. While AWS provides a default egress rule allowing all outbound traffic, it's better to explicitly define egress rules for clarity and security hardening.
📝 Recommended improvement
ingress { from_port = 5432 to_port = 5432 protocol = "tcp" security_groups = [aws_security_group.app.id] } + + egress { + from_port = 0 + to_port = 0 + protocol = "-1" + cidr_blocks = ["0.0.0.0/0"] + } tags = { Name = "${var.project_name}-db-sg" }mcp/terraform/s3.tf (2)
4-10: Consider enabling server access logging for audit trails.Server access logging provides detailed records of requests made to the bucket, useful for security audits, compliance, and troubleshooting.
📋 Proposed addition for access logging
First, create a logging bucket:
# S3 bucket for access logs resource "aws_s3_bucket" "logs" { bucket = "${var.project_name}-openapi-specs-logs" tags = { Name = "${var.project_name}-openapi-specs-logs" } } # Block public access to logs bucket resource "aws_s3_bucket_public_access_block" "logs" { bucket = aws_s3_bucket.logs.id block_public_acls = true block_public_policy = true ignore_public_acls = true restrict_public_buckets = true }Then enable logging on the main bucket:
# Enable access logging for the main S3 bucket resource "aws_s3_bucket_logging" "main" { bucket = aws_s3_bucket.main.id target_bucket = aws_s3_bucket.logs.id target_prefix = "access-logs/" }
4-10: Optionally add a lifecycle policy for cost management.The static analysis tool suggests adding a lifecycle configuration. For a POC, this is optional, but consider adding lifecycle rules to transition older spec versions to cheaper storage classes or expire them after a retention period.
💰 Proposed lifecycle configuration
# Lifecycle policy for the S3 bucket resource "aws_s3_bucket_lifecycle_configuration" "main" { bucket = aws_s3_bucket.main.id rule { id = "transition-old-versions" status = "Enabled" noncurrent_version_transition { noncurrent_days = 30 storage_class = "STANDARD_IA" } noncurrent_version_transition { noncurrent_days = 90 storage_class = "GLACIER" } noncurrent_version_expiration { noncurrent_days = 365 } } }mcp/terraform/alb.tf (1)
31-41: Enable ALB security and operational features.The ALB is missing several recommended features:
- Access logging: Provides audit trails and troubleshooting data
- Deletion protection: Prevents accidental deletion
- drop_invalid_header_fields: Mitigates HTTP request smuggling attacks
🛡️ Proposed enhancements
resource "aws_lb" "main" { name = "${var.project_name}-alb" internal = false load_balancer_type = "application" security_groups = [aws_security_group.app.id] subnets = aws_subnet.public.*.id + + enable_deletion_protection = true + drop_invalid_header_fields = true tags = { Name = "${var.project_name}-alb" } } + +# Enable access logging for the ALB +resource "aws_s3_bucket" "alb_logs" { + bucket = "${var.project_name}-alb-logs" + + tags = { + Name = "${var.project_name}-alb-logs" + } +} + +resource "aws_s3_bucket_public_access_block" "alb_logs" { + bucket = aws_s3_bucket.alb_logs.id + + block_public_acls = true + block_public_policy = true + ignore_public_acls = true + restrict_public_buckets = true +} + +resource "aws_s3_bucket_policy" "alb_logs" { + bucket = aws_s3_bucket.alb_logs.id + + policy = jsonencode({ + Version = "2012-10-17" + Statement = [ + { + Sid = "AWSLogDeliveryWrite" + Effect = "Allow" + Principal = { + Service = "elasticloadbalancing.amazonaws.com" + } + Action = "s3:PutObject" + Resource = "${aws_s3_bucket.alb_logs.arn}/*" + } + ] + }) +} + +resource "aws_lb" "main" { + # ... existing configuration ... + + access_logs { + bucket = aws_s3_bucket.alb_logs.id + enabled = true + } +}Note: For a POC environment, you may set
enable_deletion_protection = falseto simplify teardown.mcp/terraform/iam.tf (1)
6-18: Update IAM policy version to "2012-10-17".The assume role policies use
"Version": "2008-10-17", which is an older IAM policy version. While still functional, AWS recommends using"2012-10-17"for all new policies.♻️ Proposed fix
assume_role_policy = jsonencode({ - Version = "2008-10-17", + Version = "2012-10-17", Statement = [ { Action = "sts:AssumeRole",Apply the same change to both
aws_iam_role.taskandaws_iam_role.task_execution.Also applies to: 28-40
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
mcp/.gitignoremcp/Dockerfilemcp/mcp_registry/crud.pymcp/mcp_registry/database.pymcp/mcp_registry/main.pymcp/mcp_registry/schemas.pymcp/pyproject.tomlmcp/terraform/alb.tfmcp/terraform/ecr.tfmcp/terraform/ecs.tfmcp/terraform/iam.tfmcp/terraform/main.tfmcp/terraform/outputs.tfmcp/terraform/providers.tfmcp/terraform/rds.tfmcp/terraform/s3.tfmcp/terraform/secretsmanager.tfmcp/terraform/variables.tf
✅ Files skipped from review due to trivial changes (2)
- mcp/.gitignore
- mcp/terraform/secretsmanager.tf
🧰 Additional context used
🧬 Code graph analysis (3)
mcp/mcp_registry/schemas.py (1)
mcp/mcp_registry/database.py (2)
Capability(58-69)Agent(26-56)
mcp/mcp_registry/crud.py (3)
mcp/mcp_registry/database.py (2)
Agent(26-56)Capability(58-69)mcp/mcp_registry/schemas.py (4)
Agent(34-39)AgentCreate(31-32)Capability(11-16)CapabilityCreate(8-9)mcp/mcp_registry/main.py (2)
create_agent(30-31)create_capability(46-49)
mcp/mcp_registry/main.py (3)
mcp/mcp_registry/database.py (3)
get_db(71-76)Agent(26-56)Capability(58-69)mcp/mcp_registry/schemas.py (4)
Agent(34-39)AgentCreate(31-32)Capability(11-16)CapabilityCreate(8-9)mcp/mcp_registry/crud.py (4)
create_agent(10-33)get_agents(7-8)get_agents_by_capability(51-52)create_capability(44-49)
🪛 Checkov (3.2.334)
mcp/terraform/alb.tf
[medium] 31-41: Ensure that ALB drops HTTP headers
(CKV_AWS_131)
[medium] 67-76: Ensure ALB protocol is HTTPS
(CKV_AWS_2)
[medium] 44-64: Ensure AWS Load Balancer doesn't use HTTP protocol
(CKV_AWS_378)
[high] 67-76: Ensure that load balancer is using at least TLS 1.2
(CKV_AWS_103)
mcp/terraform/s3.tf
[medium] 4-10: Ensure that an S3 bucket has a lifecycle configuration
(CKV2_AWS_61)
mcp/terraform/rds.tf
[medium] 33-49: Ensure RDS database has IAM authentication enabled
(CKV_AWS_161)
[medium] 33-49: Ensure that AWS database instances have deletion protection enabled
(CKV_AWS_293)
[high] 33-49: Ensure RDS Performance Insights are encrypted using KMS CMKs
(CKV_AWS_354)
🪛 Ruff (0.14.10)
mcp/mcp_registry/main.py
30-30: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
35-35: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
41-41: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
47-47: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (7)
mcp/mcp_registry/schemas.py (1)
34-39: Schema is missingcreated_atandis_deletedfields from the database model.The
Agentresponse schema doesn't includecreated_atandis_deletedfields that are defined in the database model (mcp/mcp_registry/database.pylines 41-42). Consider whether these should be exposed in API responses.mcp/mcp_registry/main.py (1)
29-31: The Ruff B008 warnings aboutDepends()in defaults are false positives for FastAPI.Using
Depends()in function argument defaults is the idiomatic FastAPI pattern for dependency injection. This static analysis rule doesn't account for FastAPI's design.mcp/terraform/providers.tf (1)
3-15: LGTM!The provider configuration follows Terraform best practices by using version constraints and parameterizing the region and profile through variables.
mcp/terraform/outputs.tf (1)
1-4: LGTM!The output definition is clear and properly references the ECR repository resource.
mcp/terraform/ecs.tf (1)
17-18: Verify CPU and memory allocation for production workloads.The task is configured with 256 CPU units (0.25 vCPU) and 512 MB memory, which are minimal Fargate allocations. Ensure these resources are sufficient for your FastAPI application with SQLAlchemy under expected load.
Consider load testing or monitoring resource utilization after deployment to determine if scaling to larger task sizes (e.g., 512 CPU / 1024 MB memory) is necessary.
mcp/terraform/iam.tf (1)
54-77: LGTM! IAM policies follow least privilege principles.The custom task policy is properly scoped to specific resources (the Secrets Manager secret and S3 bucket objects) with only the necessary permissions (GetSecretValue, GetObject, PutObject). This follows AWS security best practices for least privilege access.
mcp/terraform/alb.tf (1)
51-59: The health check configuration is appropriate. The FastAPI application responds to GET requests on "/" with HTTP 200, matching the ALB health check matcher.
| def create_agent(db: Session, agent: schemas.AgentCreate): | ||
| db_agent = models.Agent( | ||
| name=agent.name, | ||
| version=agent.version, | ||
| display_name=agent.display_name, | ||
| description=agent.description, | ||
| owner=agent.owner, | ||
| endpoint=agent.endpoint, | ||
| openapi_spec=agent.openapi_spec, | ||
| environment=agent.environment, | ||
| tags=agent.tags, | ||
| openapi_spec_s3_uri=agent.openapi_spec_s3_uri, | ||
| openapi_spec_checksum=agent.openapi_spec_checksum | ||
| ) | ||
| for capability_data in agent.capabilities: | ||
| capability = get_capability_by_name(db, name=capability_data.name) | ||
| if not capability: | ||
| capability = create_capability(db, capability_data) | ||
| db_agent.capabilities.append(capability) | ||
|
|
||
| db.add(db_agent) | ||
| db.commit() | ||
| db.refresh(db_agent) | ||
| return db_agent |
There was a problem hiding this comment.
Transaction integrity issue: nested commits can leave orphan capabilities.
When create_agent calls create_capability (line 27), the capability is committed immediately (line 47). If the subsequent agent creation fails, capabilities remain in the database as orphans. Consider deferring commits until the entire operation succeeds.
♻️ Proposed fix: defer commits
+def _get_or_create_capability_no_commit(db: Session, capability_data: schemas.CapabilityCreate):
+ """Get existing capability or create new one without committing."""
+ capability = get_capability_by_name(db, name=capability_data.name)
+ if not capability:
+ capability = models.Capability(**capability_data.dict())
+ db.add(capability)
+ db.flush() # Assign ID without committing
+ return capability
+
def create_agent(db: Session, agent: schemas.AgentCreate):
db_agent = models.Agent(
name=agent.name,
version=agent.version,
display_name=agent.display_name,
description=agent.description,
owner=agent.owner,
endpoint=agent.endpoint,
openapi_spec=agent.openapi_spec,
environment=agent.environment,
tags=agent.tags,
openapi_spec_s3_uri=agent.openapi_spec_s3_uri,
openapi_spec_checksum=agent.openapi_spec_checksum
)
for capability_data in agent.capabilities:
- capability = get_capability_by_name(db, name=capability_data.name)
- if not capability:
- capability = create_capability(db, capability_data)
+ capability = _get_or_create_capability_no_commit(db, capability_data)
db_agent.capabilities.append(capability)
db.add(db_agent)
db.commit()
db.refresh(db_agent)
return db_agent🤖 Prompt for AI Agents
In @mcp/mcp_registry/crud.py around lines 10 - 33, The issue is that
create_agent calls create_capability which currently commits its own
transaction, causing orphaned capability rows if the agent commit later fails;
fix by changing create_capability to not perform its own commit/refresh but
instead accept and use the passed Session to add and return the capability (or
add an optional commit flag defaulting to False), then in create_agent ensure
all new capabilities are added to the same session and perform a single
db.commit()/db.refresh(db_agent) at the end; update any callers of
create_capability to handle the new non-committing behavior or explicitly commit
when needed.
| # Elastic IP for NAT Gateway | ||
| resource "aws_eip" "nat" { | ||
| domain = "vpc" | ||
| } | ||
|
|
||
| # NAT Gateway | ||
| resource "aws_nat_gateway" "main" { | ||
| allocation_id = aws_eip.nat.id | ||
| subnet_id = aws_subnet.public[0].id | ||
|
|
||
| tags = { | ||
| Name = "${var.project_name}-nat-gw" | ||
| } | ||
|
|
||
| depends_on = [aws_internet_gateway.main] | ||
| } |
There was a problem hiding this comment.
Consider deploying one NAT Gateway per availability zone for high availability.
The current configuration uses a single NAT Gateway in the first public subnet, creating a single point of failure. If this NAT Gateway or its availability zone becomes unavailable, all private subnet workloads will lose internet connectivity, impacting the ECS tasks and RDS access to patch repositories or external APIs.
For production deployments, provision one NAT Gateway per availability zone to eliminate this risk.
♻️ Proposed fix for multi-AZ NAT Gateway setup
-# Elastic IP for NAT Gateway
-resource "aws_eip" "nat" {
- domain = "vpc"
-}
-
-# NAT Gateway
-resource "aws_nat_gateway" "main" {
- allocation_id = aws_eip.nat.id
- subnet_id = aws_subnet.public[0].id
-
- tags = {
- Name = "${var.project_name}-nat-gw"
- }
-
- depends_on = [aws_internet_gateway.main]
-}
+# Elastic IPs for NAT Gateways (one per AZ)
+resource "aws_eip" "nat" {
+ count = length(var.public_subnet_cidrs)
+ domain = "vpc"
+
+ tags = {
+ Name = "${var.project_name}-nat-eip-${count.index + 1}"
+ }
+}
+
+# NAT Gateways (one per AZ)
+resource "aws_nat_gateway" "main" {
+ count = length(var.public_subnet_cidrs)
+ allocation_id = aws_eip.nat[count.index].id
+ subnet_id = aws_subnet.public[count.index].id
+
+ tags = {
+ Name = "${var.project_name}-nat-gw-${count.index + 1}"
+ }
+
+ depends_on = [aws_internet_gateway.main]
+}Update the private route table to use the corresponding NAT Gateway:
-# Private Route Table
-resource "aws_route_table" "private" {
- vpc_id = aws_vpc.main.id
-
- route {
- cidr_block = "0.0.0.0/0"
- nat_gateway_id = aws_nat_gateway.main.id
- }
-
- tags = {
- Name = "${var.project_name}-private-rt"
- }
-}
-
-# Associate Private Subnets with Private Route Table
-resource "aws_route_table_association" "private" {
- count = length(var.private_subnet_cidrs)
- subnet_id = aws_subnet.private[count.index].id
- route_table_id = aws_route_table.private.id
-}
+# Private Route Tables (one per AZ)
+resource "aws_route_table" "private" {
+ count = length(var.private_subnet_cidrs)
+ vpc_id = aws_vpc.main.id
+
+ route {
+ cidr_block = "0.0.0.0/0"
+ nat_gateway_id = aws_nat_gateway.main[count.index].id
+ }
+
+ tags = {
+ Name = "${var.project_name}-private-rt-${count.index + 1}"
+ }
+}
+
+# Associate Private Subnets with their respective Private Route Tables
+resource "aws_route_table_association" "private" {
+ count = length(var.private_subnet_cidrs)
+ subnet_id = aws_subnet.private[count.index].id
+ route_table_id = aws_route_table.private[count.index].id
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Elastic IP for NAT Gateway | |
| resource "aws_eip" "nat" { | |
| domain = "vpc" | |
| } | |
| # NAT Gateway | |
| resource "aws_nat_gateway" "main" { | |
| allocation_id = aws_eip.nat.id | |
| subnet_id = aws_subnet.public[0].id | |
| tags = { | |
| Name = "${var.project_name}-nat-gw" | |
| } | |
| depends_on = [aws_internet_gateway.main] | |
| } | |
| # Elastic IPs for NAT Gateways (one per AZ) | |
| resource "aws_eip" "nat" { | |
| count = length(var.public_subnet_cidrs) | |
| domain = "vpc" | |
| tags = { | |
| Name = "${var.project_name}-nat-eip-${count.index + 1}" | |
| } | |
| } | |
| # NAT Gateways (one per AZ) | |
| resource "aws_nat_gateway" "main" { | |
| count = length(var.public_subnet_cidrs) | |
| allocation_id = aws_eip.nat[count.index].id | |
| subnet_id = aws_subnet.public[count.index].id | |
| tags = { | |
| Name = "${var.project_name}-nat-gw-${count.index + 1}" | |
| } | |
| depends_on = [aws_internet_gateway.main] | |
| } |
| resource "aws_db_instance" "main" { | ||
| allocated_storage = 20 | ||
| storage_type = "gp2" | ||
| engine = "postgres" | ||
| engine_version = "18.1" | ||
| instance_class = "db.t3.micro" | ||
| db_name = "mcpdatabase" | ||
| username = var.db_username | ||
| password = var.db_password | ||
| db_subnet_group_name = aws_db_subnet_group.main.name | ||
| vpc_security_group_ids = [aws_security_group.db.id] | ||
| skip_final_snapshot = true | ||
|
|
||
| tags = { | ||
| Name = "${var.project_name}-db" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "rds.tf" -type fRepository: zsoftly/ai-craft
Length of output: 82
🏁 Script executed:
# Check if the file exists and view its content
if [ -f "mcp/terraform/rds.tf" ]; then
wc -l mcp/terraform/rds.tf
echo "---"
cat -n mcp/terraform/rds.tf | sed -n '30,55p'
else
echo "File not found at mcp/terraform/rds.tf"
fiRepository: zsoftly/ai-craft
Length of output: 761
🌐 Web query:
PostgreSQL 18.1 AWS RDS availability 2024 2025
💡 Result:
Short answer: PostgreSQL 18 (including 18.1) was not available on AWS RDS in 2024. AWS introduced PostgreSQL 18 Beta/Preview in 2025 and added RDS support for major version 18 (starting with 18.1) in late 2025.
Timeline (sources)
- May 15, 2025 — PostgreSQL 18 Beta 1 available in the Amazon RDS Database Preview Environment. [1]
- Sep 25, 2025 — PostgreSQL 18.0 available in the Amazon RDS Database Preview Environment. [2]
- Nov 14, 2025 — Amazon RDS for PostgreSQL added support for major version 18 (starting with PostgreSQL 18.1). [3]
- Dec 17, 2025 — Amazon Aurora PostgreSQL-Compatible Edition added PostgreSQL 18.1 in the RDS Database Preview Environment. [4]
Sources:
[1] AWS announcement — PostgreSQL 18 Beta 1 in RDS Database Preview (May 15, 2025).
[2] AWS announcement — PostgreSQL 18.0 in RDS Database Preview (Sep 25, 2025).
[3] AWS announcement — Amazon RDS for PostgreSQL now supports major version 18 (Nov 14, 2025).
[4] AWS announcement — Amazon Aurora supports PostgreSQL 18.1 in RDS Database Preview (Dec 17, 2025).
Address data loss risk from snapshot configuration and add security hardening.
The RDS configuration has a critical concern:
- Line 44:
skip_final_snapshot = trueprevents a final backup when the database is deleted, risking permanent data loss. Change toskip_final_snapshot = falsewith afinal_snapshot_identifierto preserve data.
Additionally, consider these security enhancements:
- Enable deletion protection to prevent accidental database deletion
- Enable IAM database authentication for credential management
- Enable Performance Insights with KMS encryption for monitoring
- Export CloudWatch logs for PostgreSQL activity tracking
Note: PostgreSQL 18.1 is supported on AWS RDS (available since November 2025).
🔒 Recommended security improvements
resource "aws_db_instance" "main" {
allocated_storage = 20
storage_type = "gp2"
engine = "postgres"
engine_version = "18.1"
instance_class = "db.t3.micro"
db_name = "mcpdatabase"
username = var.db_username
password = var.db_password
db_subnet_group_name = aws_db_subnet_group.main.name
vpc_security_group_ids = [aws_security_group.db.id]
- skip_final_snapshot = true
+ skip_final_snapshot = false
+ final_snapshot_identifier = "${var.project_name}-final-snapshot"
+ deletion_protection = true
+ iam_database_authentication_enabled = true
+ enabled_cloudwatch_logs_exports = ["postgresql", "upgrade"]
+
+ performance_insights_enabled = true
+ performance_insights_kms_key_id = aws_kms_key.rds.arn
tags = {
Name = "${var.project_name}-db"
}
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Checkov (3.2.334)
[medium] 33-49: Ensure RDS database has IAM authentication enabled
(CKV_AWS_161)
[medium] 33-49: Ensure that AWS database instances have deletion protection enabled
(CKV_AWS_293)
[high] 33-49: Ensure RDS Performance Insights are encrypted using KMS CMKs
(CKV_AWS_354)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @mcp/mcp_registry/crud.py:
- Around line 44-47: create_capability currently adds a new models.Capability to
the session but never commits, so calls from the POST /capabilities/ endpoint
won't persist; fix by committing and refreshing the instance before returning:
after creating db_capability in create_capability add db.commit() and
db.refresh(db_capability) so it persists and has its generated fields populated,
or if you prefer to keep create_capability non-committing for internal use
(e.g., create_agent), ensure the POST /capabilities/ endpoint calls db.commit()
and db.refresh(db_capability) after crud.create_capability.
In @mcp/terraform/alb.tf:
- Around line 95-96: Replace the outdated ssl_policy and the hardcoded
certificate_arn: set ssl_policy to a TLS1.2+ managed policy (e.g., use
ELBSecurityPolicy-TLS-1-2-2017-01) and change certificate_arn to reference a
variable (certificate_arn = var.acm_certificate_arn); also add a variable block
named acm_certificate_arn (type = string, description = "ARN of the ACM
certificate for HTTPS") in variables.tf so deployments supply the ARN.
🧹 Nitpick comments (7)
mcp/mcp_registry/database.py (1)
43-43: Consider using timezone-aware datetime.
datetime.utcnowis deprecated in Python 3.12+. Usedatetime.now(timezone.utc)for timezone-aware UTC timestamps.♻️ Proposed fix
-from datetime import datetime +from datetime import datetime, timezone # ... in Agent model: - created_at = Column(DateTime, default=datetime.utcnow) + created_at = Column(DateTime, default=lambda: datetime.now(timezone.utc))mcp/mcp_registry/main.py (1)
16-18: Consider using the lifespan context manager instead of deprecated@app.on_event.
@app.on_event("startup")is deprecated in FastAPI 0.93.0+ and the lifespan context manager is the recommended replacement.♻️ Proposed fix
+from contextlib import asynccontextmanager + +@asynccontextmanager +async def lifespan(app: FastAPI): + create_db_and_tables() + yield + -app = FastAPI() +app = FastAPI(lifespan=lifespan) -@app.on_event("startup") -def on_startup(): - create_db_and_tables()mcp/mcp_registry/schemas.py (1)
14-15: Migrate to Pydantic v2from_attributesconfiguration.
orm_mode = Trueis deprecated in Pydantic v2. Replace withfrom_attributes = Truein theConfigclass at lines 14-15, 33-34, 43-44, and 50-51.mcp/terraform/ecs.tf (2)
25-25: Avoid using:latesttag for production deployments.Using
:latestmakes deployments non-reproducible and can lead to unexpected behavior when the image is updated. Consider using specific image tags (e.g., commit SHA, semantic version) for predictable deployments.♻️ Suggested approach
- image = "${aws_ecr_repository.main.repository_url}:latest" + image = "${aws_ecr_repository.main.repository_url}:${var.image_tag}"Add a variable in
variables.tf:variable "image_tag" { description = "Docker image tag to deploy" type = string default = "latest" # Override in production }
71-71: Update dependency to reference the HTTPS listener.The ECS service depends on
aws_lb_listener.http, but the HTTP listener only redirects to HTTPS. The actual traffic forwarding to the target group happens inaws_lb_listener.https.♻️ Proposed fix
- depends_on = [aws_lb_listener.http] + depends_on = [aws_lb_listener.https]mcp/terraform/alb.tf (1)
38-48: Consider enablingdrop_invalid_header_fieldsfor security hardening.Per CKV_AWS_131, enabling this attribute helps prevent HTTP request smuggling and header injection attacks by dropping requests with invalid HTTP headers.
🛡️ Proposed enhancement
resource "aws_lb" "main" { name = "${var.project_name}-alb" internal = false load_balancer_type = "application" security_groups = [aws_security_group.app.id] subnets = aws_subnet.public.*.id + drop_invalid_header_fields = true tags = { Name = "${var.project_name}-alb" } }mcp/terraform/s3.tf (1)
7-17: Update to use standalone resources for versioning and encryption.The inline
versioningandserver_side_encryption_configurationblocks withinaws_s3_bucketare deprecated in AWS provider version 4.0+. These arguments became read-only and must be moved to separate resources for forward compatibility.♻️ Proposed refactoring using standalone resources
resource "aws_s3_bucket" "main" { bucket = "${var.project_name}-openapi-specs" - versioning { - enabled = true - } - - server_side_encryption_configuration { - rule { - apply_server_side_encryption_by_default { - sse_algorithm = "AES256" - } - } - } - tags = { Name = "${var.project_name}-openapi-specs" } } + +resource "aws_s3_bucket_versioning" "main" { + bucket = aws_s3_bucket.main.id + + versioning_configuration { + status = "Enabled" + } +} + +resource "aws_s3_bucket_server_side_encryption_configuration" "main" { + bucket = aws_s3_bucket.main.id + + rule { + apply_server_side_encryption_by_default { + sse_algorithm = "AES256" + } + } +}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
mcp/mcp_registry/crud.pymcp/mcp_registry/database.pymcp/mcp_registry/main.pymcp/mcp_registry/schemas.pymcp/terraform/alb.tfmcp/terraform/ecr.tfmcp/terraform/ecs.tfmcp/terraform/main.tfmcp/terraform/s3.tfmcp/terraform/variables.tf
🚧 Files skipped from review as they are similar to previous changes (2)
- mcp/terraform/main.tf
- mcp/terraform/variables.tf
🧰 Additional context used
🧬 Code graph analysis (4)
mcp/mcp_registry/database.py (1)
mcp/mcp_registry/schemas.py (2)
Agent(39-44)Capability(46-51)
mcp/mcp_registry/main.py (3)
mcp/mcp_registry/database.py (4)
create_db_and_tables(80-81)get_db(73-78)Agent(28-58)Capability(60-71)mcp/mcp_registry/schemas.py (4)
Agent(39-44)AgentCreate(36-37)Capability(46-51)CapabilityCreate(8-9)mcp/mcp_registry/crud.py (4)
create_agent(10-33)get_agents(7-8)get_agents_by_capability(49-50)create_capability(44-47)
mcp/mcp_registry/schemas.py (1)
mcp/mcp_registry/database.py (2)
Agent(28-58)Capability(60-71)
mcp/mcp_registry/crud.py (3)
mcp/mcp_registry/database.py (2)
Agent(28-58)Capability(60-71)mcp/mcp_registry/schemas.py (4)
Agent(39-44)AgentCreate(36-37)Capability(46-51)CapabilityCreate(8-9)mcp/mcp_registry/main.py (2)
create_agent(22-23)create_capability(38-41)
🪛 Checkov (3.2.334)
mcp/terraform/alb.tf
[medium] 38-48: Ensure that ALB drops HTTP headers
(CKV_AWS_131)
[high] 91-102: Ensure that load balancer is using at least TLS 1.2
(CKV_AWS_103)
mcp/terraform/s3.tf
[medium] 4-22: Ensure that an S3 bucket has a lifecycle configuration
(CKV2_AWS_61)
🪛 Ruff (0.14.10)
mcp/mcp_registry/database.py
15-15: Avoid specifying long messages outside the exception class
(TRY003)
mcp/mcp_registry/main.py
22-22: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
27-27: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
33-33: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
39-39: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
mcp/mcp_registry/crud.py
5-5: Avoid equality comparisons to False; use not models.Agent.is_deleted: for false checks
Replace with not models.Agent.is_deleted
(E712)
8-8: Avoid equality comparisons to False; use not models.Agent.is_deleted: for false checks
Replace with not models.Agent.is_deleted
(E712)
50-50: Avoid equality comparisons to False; use not models.Agent.is_deleted: for false checks
Replace with not models.Agent.is_deleted
(E712)
🔇 Additional comments (16)
mcp/mcp_registry/database.py (4)
12-15: Previously flagged issue has been addressed.The hardcoded default credentials in the fallback
DATABASE_URLhave been removed. The code now properly requires the environment variable and fails explicitly if it's not set.
21-26: Previously flagged issue has been addressed.The association table now correctly defines a composite primary key on both
agent_idandcapability_id, preventing duplicate associations.
28-58: Agent model looks well-structured.The model includes appropriate indexing on frequently queried fields (
name,version,environment), a unique constraint on(name, version), soft-delete support viais_deleted, and JSONB storage for the OpenAPI spec.
60-71: Capability model and utility functions are properly implemented.The bidirectional relationship with
Agentis correctly set up, and theget_dbgenerator properly handles session lifecycle with cleanup in thefinallyblock.Also applies to: 73-81
mcp/mcp_registry/main.py (3)
7-8: Previously flagged issues have been addressed.The duplicate
get_dbfunction has been removed; it's now imported from thedatabasemodule. The duplicate table creation at module level has also been removed.
21-41: Endpoints correctly delegate to CRUD layer.The static analysis warnings about
Depends()in default arguments (B008) are false positives for FastAPI—this is the idiomatic pattern. The endpoints properly use dependency injection for database sessions.
44-46: Root endpoint is fine for health checks.Simple and effective for basic liveness verification.
mcp/mcp_registry/schemas.py (2)
11-15: Previously flagged circular serialization issue has been addressed.The use of
CapabilitySimpleandAgentSimpleschemas (without back-references) breaks the circular dependency and prevents infinite recursion during serialization.Also applies to: 30-34
53-54: Forward reference updates may not be needed in Pydantic v2.In Pydantic v2,
update_forward_refs()is replaced bymodel_rebuild(). However, since you're usingCapabilitySimpleandAgentSimple(already defined), forward references may not require explicit resolution.mcp/mcp_registry/crud.py (3)
4-8: Previously flagged soft-delete filtering issue has been addressed.The
get_agentandget_agentsfunctions now correctly filter out soft-deleted agents usingis_deleted == False.Note: The static analysis warning (E712) about
== Falseis a false positive for SQLAlchemy—using== Falseis correct because SQLAlchemy column expressions don't work with Python'snotoperator.
10-33: Transaction handling increate_agentis now correct.The function properly creates capabilities without committing, associates them with the agent, and commits everything in a single transaction at the end. This addresses the previous orphan-capability concern.
49-50: Previously flagged soft-delete filtering issue has been addressed.The
get_agents_by_capabilityfunction now correctly filters out soft-deleted agents.mcp/terraform/ecr.tf (1)
4-16: LGTM! Past review concerns have been addressed.The ECR configuration now follows production best practices:
image_tag_mutability = "IMMUTABLE"prevents tag overwritesforce_delete = falseprotects against accidental repository deletionscan_on_push = trueenables vulnerability scanningmcp/terraform/ecs.tf (1)
33-42: Verify secret key paths for distinct environment variables.Both
DB_CREDENTIALSandDATABASE_URLreference the same secret ARN without key paths. This means both environment variables will receive the identical value (the full secret content).If the secret contains a JSON object with multiple keys, you should specify the key path to extract distinct values:
🔧 Proposed fix if secret contains separate keys
secrets = [ { name = "DB_CREDENTIALS" - valueFrom = aws_secretsmanager_secret.db_credentials.arn + valueFrom = "${aws_secretsmanager_secret.db_credentials.arn}:credentials::" }, { name = "DATABASE_URL" - valueFrom = aws_secretsmanager_secret.db_credentials.arn + valueFrom = "${aws_secretsmanager_secret.db_credentials.arn}:database_url::" } ]Verify the structure of the secret stored in Secrets Manager and ensure the key paths match.
mcp/terraform/s3.tf (1)
24-49: LGTM! Security configuration is well-designed.
- Public access is properly blocked
- IAM policy follows least privilege with only
s3:GetObjectands3:PutObjectpermissions- Task role-based access is appropriate for ECS integration
Optionally, consider adding a lifecycle configuration to manage versioned objects and control storage costs over time.
mcp/terraform/alb.tf (1)
4-35: LGTM! Security group and listener configuration properly implements HTTPS redirect.
- Security group correctly allows both HTTP and HTTPS for the redirect flow
- HTTP listener properly redirects to HTTPS with 301 status code
- Target group health check configuration is reasonable
Also applies to: 51-88
| def create_capability(db: Session, capability: schemas.CapabilityCreate): | ||
| db_capability = models.Capability(**capability.dict()) | ||
| db.add(db_capability) | ||
| return db_capability |
There was a problem hiding this comment.
Missing commit when create_capability is called directly from the API.
create_capability adds to the session but doesn't commit. This works correctly when called from create_agent (which commits), but when called directly from the POST /capabilities/ endpoint, changes won't be persisted.
🐛 Proposed fix
def create_capability(db: Session, capability: schemas.CapabilityCreate):
db_capability = models.Capability(**capability.dict())
db.add(db_capability)
+ db.commit()
+ db.refresh(db_capability)
return db_capabilityAlternatively, if you want to keep create_capability non-committing for internal use, commit in the endpoint:
# In main.py
@app.post("/capabilities/", response_model=schemas.Capability)
def create_capability(
capability: schemas.CapabilityCreate, db: Session = Depends(get_db)
):
db_capability = crud.create_capability(db=db, capability=capability)
db.commit()
db.refresh(db_capability)
return db_capability🤖 Prompt for AI Agents
In @mcp/mcp_registry/crud.py around lines 44 - 47, create_capability currently
adds a new models.Capability to the session but never commits, so calls from the
POST /capabilities/ endpoint won't persist; fix by committing and refreshing the
instance before returning: after creating db_capability in create_capability add
db.commit() and db.refresh(db_capability) so it persists and has its generated
fields populated, or if you prefer to keep create_capability non-committing for
internal use (e.g., create_agent), ensure the POST /capabilities/ endpoint calls
db.commit() and db.refresh(db_capability) after crud.create_capability.
| ssl_policy = "ELBSecurityPolicy-2016-08" | ||
| certificate_arn = "arn:aws:acm:us-east-1:123456789012:certificate/your-certificate-id" # TODO: Replace with your certificate ARN |
There was a problem hiding this comment.
Update SSL policy and parameterize certificate ARN.
Two issues with the HTTPS listener configuration:
- Outdated SSL policy:
ELBSecurityPolicy-2016-08doesn't enforce TLS 1.2 minimum, failing security best practices (CKV_AWS_103). - Hardcoded certificate ARN: The placeholder needs to be replaced with a variable for deployability.
🔒 Proposed fix
- ssl_policy = "ELBSecurityPolicy-2016-08"
- certificate_arn = "arn:aws:acm:us-east-1:123456789012:certificate/your-certificate-id" # TODO: Replace with your certificate ARN
+ ssl_policy = "ELBSecurityPolicy-TLS13-1-2-2021-06"
+ certificate_arn = var.acm_certificate_arnAdd a variable in variables.tf:
variable "acm_certificate_arn" {
description = "ARN of the ACM certificate for HTTPS"
type = string
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ssl_policy = "ELBSecurityPolicy-2016-08" | |
| certificate_arn = "arn:aws:acm:us-east-1:123456789012:certificate/your-certificate-id" # TODO: Replace with your certificate ARN | |
| ssl_policy = "ELBSecurityPolicy-TLS13-1-2-2021-06" | |
| certificate_arn = var.acm_certificate_arn |
🤖 Prompt for AI Agents
In @mcp/terraform/alb.tf around lines 95 - 96, Replace the outdated ssl_policy
and the hardcoded certificate_arn: set ssl_policy to a TLS1.2+ managed policy
(e.g., use ELBSecurityPolicy-TLS-1-2-2017-01) and change certificate_arn to
reference a variable (certificate_arn = var.acm_certificate_arn); also add a
variable block named acm_certificate_arn (type = string, description = "ARN of
the ACM certificate for HTTPS") in variables.tf so deployments supply the ARN.
1 participant
This pull request introduces the technical plan for the ZSoftly Model Context Protocol (MCP) Registry. The plan is detailed in the
MCP_Registry_Plan.mddocument, which is added to themcpdirectory.Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.