Code Review Improvements: PgBouncer Detection, Documentation & Test Clarity #640
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implements code review improvements from the code-reviewer agent: 1. PgBouncer Detection (Warning #2): - Extract detection logic into _is_pgbouncer_connection() function - Use URL parsing (urlparse) instead of string matching for port detection - Avoids false positives from passwords containing ":6543" - Add 4 new unit tests for the detection function 2. Negative Budget Test (Warning #3): - Rename test_negative_budget_returns_validation_error → test_negative_budget_raises_tool_error - Update docstring to clarify Pydantic-level validation behavior - Explains why it raises ToolError instead of returning Error response 3. Budget Format Migration Comments (Suggestion #1): - Add clear migration comments to 3 test files - Documents AdCP v2.2.0 float budget format - Helps future developers understand the budget format change - Format: "📊 BUDGET FORMAT: AdCP v2.2.0 Migration (2025-10-27)" Changes improve code maintainability and reduce edge-case bugs without changing functionality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Implements remaining code review suggestions: 1. Performance Metrics (Suggestion #2): - Add comprehensive performance section to pgbouncer.md - Document connection time: Direct (50-100ms) vs PgBouncer (1-5ms) - Document memory usage: Direct (~300MB) vs PgBouncer (~70MB) - Add pool sizing recommendations table (small/medium/large apps) - Include monitoring and tuning tips 2. Overflow Normalization Comment (Suggestion #3): - Expand comment in get_pool_status() to explain negative values - Document two cases: uninitialized pool and closed connections - Clarify why we normalize to 0 for monitoring - Add specific example: -2 when pool_size=2 These improvements help operators understand PgBouncer's performance benefits and tune settings based on their workload. The expanded comment prevents confusion about negative overflow values during testing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
danf-newton
pushed a commit
to Newton-Research-Inc/salesagent
that referenced
this pull request
Nov 24, 2025
…larity (adcontextprotocol#640) * refactor: Improve PgBouncer detection and test clarity (code review) Implements code review improvements from the code-reviewer agent: 1. PgBouncer Detection (Warning adcontextprotocol#2): - Extract detection logic into _is_pgbouncer_connection() function - Use URL parsing (urlparse) instead of string matching for port detection - Avoids false positives from passwords containing ":6543" - Add 4 new unit tests for the detection function 2. Negative Budget Test (Warning adcontextprotocol#3): - Rename test_negative_budget_returns_validation_error → test_negative_budget_raises_tool_error - Update docstring to clarify Pydantic-level validation behavior - Explains why it raises ToolError instead of returning Error response 3. Budget Format Migration Comments (Suggestion adcontextprotocol#1): - Add clear migration comments to 3 test files - Documents AdCP v2.2.0 float budget format - Helps future developers understand the budget format change - Format: "📊 BUDGET FORMAT: AdCP v2.2.0 Migration (2025-10-27)" Changes improve code maintainability and reduce edge-case bugs without changing functionality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * docs: Add performance metrics and improve code documentation Implements remaining code review suggestions: 1. Performance Metrics (Suggestion adcontextprotocol#2): - Add comprehensive performance section to pgbouncer.md - Document connection time: Direct (50-100ms) vs PgBouncer (1-5ms) - Document memory usage: Direct (~300MB) vs PgBouncer (~70MB) - Add pool sizing recommendations table (small/medium/large apps) - Include monitoring and tuning tips 2. Overflow Normalization Comment (Suggestion adcontextprotocol#3): - Expand comment in get_pool_status() to explain negative values - Document two cases: uninitialized pool and closed connections - Clarify why we normalize to 0 for monitoring - Add specific example: -2 when pool_size=2 These improvements help operators understand PgBouncer's performance benefits and tune settings based on their workload. The expanded comment prevents confusion about negative overflow values during testing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Implements code review improvements from the code-reviewer agent's feedback on the AdCP v2.2.0 budget migration and PgBouncer support PRs.
Changes
1. PgBouncer Detection Enhancement (Warning #2)
Problem: String matching for
:6543could cause false positives (e.g., passwords containing:6543)Solution:
_is_pgbouncer_connection()functionurllib.parse.urlparse()for robust port detectionImpact: Eliminates false positives from passwords or other text containing
:65432. Test Naming & Documentation (Warning #3)
Problem: Test named
test_negative_budget_returns_validation_errorbut actually raises exceptionSolution:
test_negative_budget_raises_tool_errorImpact: Test name now accurately reflects behavior
3. Budget Format Migration Comments (Suggestion #1)
Problem: Budget format changed from Budget objects to floats, but no context in test files
Solution:
Impact: Future developers understand the budget format change
4. Performance Metrics Documentation (Suggestion #2)
Problem: PgBouncer documentation lacks performance characteristics
Solution:
Impact: Operators can tune settings based on workload
5. Code Documentation Enhancement (Suggestion #3)
Problem: Overflow normalization comment lacks detail about negative values
Solution:
Impact: Prevents confusion during testing and debugging
Test Coverage
_is_pgbouncer_connection()Documentation
Review Checklist
Related PRs