chore: internal audit remediation (25 tasks + security fix + ruff sweep) - #327
Merged
Conversation
…n baseline Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Tasks 1-4, 20: Add governance files, configuration templates, and packaging improvements.
Task 1: Update icon and logo set (14 assets: multiple resolutions and formats).
Task 2: Add CODE_OF_CONDUCT.md (Contributor Covenant v2.1).
Task 3: Add .editorconfig for editor/IDE settings alignment.
Task 4: Add py.typed marker (PEP 561), fix stale license-files entry in pyproject.toml,
pin hatchling>=1.26 build-system floor, add packaging tests.
Task 20: Add .env.example documenting every MCPG_* configuration variable.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…2SQL_API_KEY - Replace CODE_OF_CONDUCT.md body with verbatim Contributor Covenant v2.1 from authoritative source (was paraphrased before). Fixes: missing 'caste, color' in protected characteristics, 'We pledge that we will' → 'We pledge to' per spec, double space in line 109. Enforcement contact remains devopam@gmail.com (matches SECURITY.md). - Add MCPG_NL2SQL_API_KEY to .env.example NL→SQL section (was omitted despite being read via secrets.get() in config.py:870 and documented in docs/user-guide.md:520). - Verified completeness: all 93 MCPG_* variables from config.py now in .env.example. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…or-logging call sites Task 6: adds a logger.debug(..., exc_info=True) call to 7 previously-silent except Exception: pass sites (advisors.py, audit.py, audit_trail.py x3, listen.py, migrations.py). Zero control-flow change - migrations.py keeps its trailing raise unchanged. Task 14: audited every logger.error/warning call inside an except block across src/mcpg (33 candidate sites found via fresh grep, not the earlier non-exhaustive 1-2-hit sample). 23 sites needed exc_info=True added (audit_nl2sql.py, cache.py, cursors.py, database.py, graph_diagram.py, http_runtime.py, nl2sql.py, otel_tracing.py, replicas.py, tenancy.py). 1 site already had exc_info=True. 9 sites left unchanged - either not inside an except block, or logging about something other than the caught exception. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
… 7 traceback tests Review found two Important issues in the prior commit (fe95e70): 1. exc_info=True attaches a formatted traceback whose last line is "ExceptionType: str(exception)", bypassing obfuscate_password() applied to the message argument. Reverted exc_info=True at the 5 sites where the caught exception can carry a DSN/connection string and the message was already redacted for exactly that reason: database.py:117, replicas.py:139, replicas.py:353, nl2sql.py:1077, nl2sql.py:1081. All 5 are now byte-identical to their pre-batch state. The other 18 Task-14 fixes and all 7 Task-6 fixes are untouched. 2. None of the 7 Task-6 caplog tests verified that exc_info was actually attached - only message text + level, so dropping exc_info=True while keeping the message would have stayed green. Strengthened all 7 to also assert record.exc_info is not None. Verified the new assertion actually catches the regression by temporarily removing exc_info=True from advisors.py and confirming the test failed, then restoring it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
Distinct from /healthz (liveness): /readyz reports 503 when the DB pool hasn't produced a live connection (Database.is_connected), so an orchestrator can pull a degraded instance out of rotation without restarting it. Already reserved in _AUTH_EXEMPT_PATHS but never mounted. _health_response_factory has no pool-access pattern to reuse - the DB pool lives in AppContext, delivered by the MCP SDK's lifespan only during tool-call handling, not exposed on app.state or to a bare Starlette Route. create_server now stashes the primary Database on the server object (server.mcpg_database) alongside the existing mcpg_settings/otel_tracer/rate_limiter, so build_http_app can read it via getattr and pass it into the new _readiness_response_factory. OIDC/JWKS readiness gating was considered and deliberately omitted: /readyz must stay auth-exempt, so gating it on "JWKS fetched via a verified request" would deadlock a fresh OIDC instance out of rotation before it ever receives the traffic needed to populate the cache. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
run_select / run_select_tuned fully materialized a query's entire result set into Python dict/RowResult objects before truncating to max_rows. SqlDriver.execute_query / SafeSqlDriver.execute_query (and every SqlDriver subclass override in multidb.py/replicas.py/tenancy.py) now take an optional row_limit parameter and use cursor.fetchmany(row_limit) instead of cursor.fetchall() when set; both call sites in query.py pass max_rows + 1. Note on scope: psycopg's client-side cursor already pulls the whole result set into libpq's buffer during cursor.execute(), so this bounds Python-side object materialization, not the server-side network transfer. A true wire-level bound would need a named server-side cursor or an injected LIMIT, which can't cleanly support run_select_tuned's SET LOCAL ...; SELECT ... pattern or the SHOW/EXPLAIN/VACUUM paths SafeSqlDriver also allows -- flagged as a possible follow-up, not built here. _validate/_validate_node and allowlist.py are untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
BREAKING CHANGE: the HTTP transport now raises ConfigError at startup instead of starting unauthenticated-with-a-warning when neither MCPG_HTTP_AUTH_TOKEN nor MCPG_AUTH_MODE=oidc is set. Set MCPG_HTTP_ALLOW_UNAUTHENTICATED=true to explicitly opt out. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
BREAKING CHANGE: MCPG_RATE_LIMIT_ENABLED now defaults to true. Set it to false explicitly to restore the previous unlimited default. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eware - HSTS max-age default bumped 31536000 (1yr) -> 63072000 (2yr, OWASP's current recommendation) in both the Settings dataclass default and the load_settings parse default (and the _SecurityHeadersMiddleware constructor default for consistency). - New Settings.http_trusted_hosts: tuple[str, ...] = () field, following the exact http_allowed_origins comma-split-env-var / empty-tuple-means-off pattern. MCPG_HTTP_TRUSTED_HOSTS wires Starlette's TrustedHostMiddleware conditionally in build_http_app, mirroring the existing CORS conditional. - Updated README.md / .env.example to reflect the new HSTS default and document MCPG_HTTP_TRUSTED_HOSTS. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rifier NL2SQL providers (AnthropicProvider/OpenAIProvider/GeminiProvider) each opened `async with httpx.AsyncClient(...)` fresh on every `complete()` call, paying a full TCP/TLS handshake per translate_nl_to_sql invocation. build_provider() is called fresh on every tool call (provider selection varies per-request via `provider=`), so holding a client on the provider *instance* wouldn't help -- a new provider still means a new client. Instead, nl2sql.py now holds one lazily-constructed httpx.AsyncClient at module scope, shared by all three provider classes (httpx already pools connections per-host internally, so sharing one client across vendors is correct usage). Closed via aclose_shared_client(), wired into make_lifespan's existing shutdown path in server.py. OIDCVerifier's discovery-document fetch had the same per-call client problem. OIDCVerifier IS constructed once per server lifetime (in http_runtime.build_http_app), so it now holds one httpx.AsyncClient in __init__ and a new aclose(). Since the verifier is built after make_lifespan's closure already exists (build_http_app runs later, inside run_http), that closure can't reach it -- build_http_app instead wraps the Starlette app's own ASGI lifespan_context so the verifier's client still gets closed on shutdown, for both the streamable-http and sse transports. Regression tests exercise the real per-call construction pattern (two separate build_provider() calls / two full ASGI lifespan cycles) rather than two .complete() calls on one hand-held provider instance, which would pass even against the unfixed per-call-client code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…calls Wraps each NL2SQL provider's complete() (AnthropicProvider, OpenAIProvider, GeminiProvider) and OIDCVerifier._resolve_jwks_url (the discovery-document fetch) with circuitbreaker.circuit — 5 consecutive failures opens the breaker for 30s, so a degraded vendor/IdP fails fast instead of every request separately paying the full timeout. circuitbreaker 2.1.3 supports async natively (iscoroutinefunction check dispatches to an async-aware wrapper) — confirmed by reading the installed package's source, not assumed from the audit report's secondary characterization. Parameter names failure_threshold/recovery_timeout/ expected_exception match the brief. @circuit decorates the plain function at class-definition time, so its failure count is one object shared by every instance of that class for the process's lifetime, not per-instance state. This is intentional for the NL2SQL providers: build_provider() constructs a fresh provider on every translate_nl_to_sql call, so per-instance state would never accumulate a failure across calls. A tripped breaker raises circuitbreaker.CircuitBreakerError, not the module's own error type — both call sites (translate_nl_to_sql's except around provider.complete(), OIDCVerifier._ensure_jwks_client's call to _resolve_jwks_url) now also catch CircuitBreakerError and re-raise it as NL2SQLError / OIDCError respectively, so existing callers (including http_runtime.py's `except OIDCError` around verifier.verify) keep working unchanged whether the breaker is open or not. mypy --strict's disallow_untyped_decorators fires on @circuit independently of import resolution (circuitbreaker ships no py.typed marker); each decorated method carries a `# type: ignore[untyped-decorator]` alongside a new ignore_missing_imports override for the module. Tests reset every registered circuit breaker via an autouse fixture in both test_nl2sql.py and test_oidc.py — the breaker's shared-at-class-level state would otherwise leak across unrelated tests in the same session. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…KS calls Layers tenacity's @Retry INSIDE (not outside) the @circuit breaker added in the prior commit, on the same four call sites (AnthropicProvider, OpenAIProvider, GeminiProvider .complete(); OIDCVerifier._resolve_jwks_url): @circuit(...) # outer @Retry(...) # inner async def complete(self, ...): ... This ordering is verified against circuitbreaker's actual failure-counting semantics (read from the installed package's source in the prior commit): `call_async` does `with self: return await func(...)` and counts exactly one failure per invocation of whatever it wraps. With retry innermost, all of a logical call's retry attempts happen inside that single `with self:` block, so an exhausted retry cycle counts as ONE breaker failure — not one per retry. Stacked the other way (retry outer, circuit inner — the shape a stale draft of this task's brief showed), each retry attempt would separately enter/exit the breaker's `with self:`, letting 3 quick retries alone burn 3 of the breaker's failure_threshold slots for a single logical call. The task's own top-level framing used the word "outside" for this same intended behavior ("only the whole retried attempt counts as one failure toward the breaker's threshold") while showing retry-outer code — those two are inconsistent for this library; the code matches the stated behavior, not the word. `reraise=True` is load-bearing, not cosmetic: without it, an exhausted retry cycle raises tenacity.RetryError, which wouldn't match @circuit's expected_exception (httpx.HTTPError / OIDCError) — the breaker would silently never count a single failure. `retry_if_exception_type` scopes retries to the same exception type each breaker already keys on, so a non-network bug can't accidentally get retried either. Backoff is small and jittered (wait_exponential_jitter, initial=0.1s, max=2s, jitter=0.1s, 3 attempts) — the breaker, not the backoff, is the mechanism that handles a sustained outage, so there's no reason to make a doomed request wait long between retries. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…path The `except CircuitBreakerError` branch added to `translate_nl_to_sql` in the circuit-breaker commit (e916ff4) was never exercised by any test — a coverage run confirmed nl2sql.py:1360-1365 as "Missing." The existing circuit-breaker tests call `AnthropicProvider(...).complete(...)` directly, bypassing `translate_nl_to_sql` entirely, so the translation this batch's own CHANGELOG entry advertises ("a tripped breaker still surfaces as the module's existing error type… never a bare CircuitBreakerError") was asserted on the OIDC side only, not on NL2SQL's. Adds a test that trips the breaker via translate_nl_to_sql itself (not provider.complete() directly) and asserts the next call raises NL2SQLError, not a bare CircuitBreakerError. No production code changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
…on test Address task-13 review findings: add a test that logs through the real setup_logging() -> handler -> JSONFormatter pipeline (not just RedactionFilter/JSONFormatter in isolation), and narrow the docstring and CHANGELOG wording to accurately state that record.exc_info (exception tracebacks) is not covered by this filter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rge check Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The comment landed inside the YAML block scalar (if: |), which has no comment syntax -- every line at or above its indentation becomes literal content of the string GitHub Actions evaluates as the job condition. Moved the explanatory comment above the if: key as ordinary YAML comments, outside the block scalar. Verified the parsed if string is byte-identical to commit 05f09f1's value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tion
describe_graph (graph.py) and generate_graph_diagram (graph_diagram.py)
read Apache AGE vertex/edge label names back from ag_catalog.ag_label
and interpolated them directly into f-string SQL (`FROM "{graph}"."{label}"`)
with no identifier validation. Postgres/AGE quoted identifiers can contain
arbitrary characters, including embedded double quotes, so a label name
created via a prior run_cypher write could carry attacker-controlled SQL
into that later re-interpolation.
Fix:
- Every catalog-derived label name is identifier-validated
([A-Za-z_][A-Za-z0-9_]*) before it reaches a generated SQL string or the
diagram's Mermaid output (new mcpg.graph._check_label_identifier,
mirroring graph_projection._check_identifier). An invalid name aborts
the call with GraphError rather than being silently used or skipped,
matching graph_projection's existing precedent for catalog-derived
identifiers.
- Both tools' per-label count/fetch queries now run under
force_readonly=True.
- describe_graph gained the same Capability.READ access-mode gate its
sibling read tools (run_cypher's read path, generate_graph_diagram)
already had, for consistency — Capability.READ is currently permitted
in every access mode, so this is defense-in-depth/consistency, not a
fix for a live privilege bypass (generate_graph_diagram already had
this gate; the original finding's "access-mode bypass" framing for it
did not hold up against the current code).
Tests (RED/GREEN): new tests in test_graph.py and test_graph_diagram.py
construct a label name containing an embedded double-quote/`;` and
confirm the pre-fix code either builds the unsafe SQL text or silently
degrades, while post-fix it raises GraphError before any such query
reaches the driver; a monkeypatch spy confirms check_permission is
invoked. Verified against the pre-fix code via git stash before applying
the fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
…lues Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d values Replaced cosmetic rewrites with genuine strengthening: 1. test_demo.py: assert product name or features are interpolated in review text 2-4. test_http_runtime.py: assert middleware passes scope/receive/send unmodified 5. test_warehousepg_reads.py: assert exactly 1 query, not 2+ Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…chine, pytest-rerunfailures Full unit suite (2950 tests) run twice under pytest-randomly's order randomization with different seeds -- both runs passed with no order-dependence failures. pytest-socket is added but not globally enabled (no --disable-socket, no conftest.py changes). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rescoped ruff sweep, Task 23 Steps 1-3 + the PYI sub-step: fix all pre-existing violations in four small, safe, high-value opt-in Ruff categories, run via explicit --select flags (pyproject.toml's [tool.ruff.lint] select list is untouched -- that lands in a later, final commit once C90/ASYNC/FBT are also fixed). - PTH (flake8-use-pathlib), 9 violations: os.path.* -> pathlib.Path, open() -> Path.open(). Every hit hand-reviewed for symlink/relative- path behavior preservation, including the subprocess bin-allowlist symlink check in shell.py (only os.path.dirname() -> Path(...).parent changed; the os.path.realpath() symlink-resolution call is untouched). - C4 (flake8-comprehensions), 1 violation: unnecessary list-in-tuple(). - SIM (flake8-simplify), 48 violations: 20 autofixed (yoda-conditions, if-with-same-arms), 28 manual (contextlib.suppress for try/except/ pass, if/else-to-ternary, nested-if collapsing, with-statement merging). Includes two fixes in the security-critical SQL-safety AST walker (sql/safety.py) -- confirmed behavior-preserving against the full adversarial + fuzz suite. One SIM103 hit (pgq.py) was NOT applied as ruff's literal suggestion because it would have widened the function's declared bool return type; wrapped in bool(...) instead to preserve both behavior and the mypy --strict contract. - PYI (flake8-pyi), 16 violations: 2 autofixed (redundant float|int union), 14 manual (__aenter__/__enter__ return type -> Self). Full verification: ruff check . -> 0; ruff format --check . -> clean; mypy src/mcpg --strict -> 0 issues; tests/unit (what the pre-commit hook gates) -> 2950 passed, 3 skipped, 0 failed. One pre-existing, unrelated contract-test failure in tests/contract/ (a TranslationResult.schema_context snapshot drift from an earlier, already-merged commit) is out of this batch's scope and does not touch tests/unit -- flagged for a follow-up snapshot regeneration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
…ext field The schema_context field added to TranslationResult (commit 6d55775) was a legitimate, intentional dataclass change but the tool-return-shapes contract snapshot was never regenerated for it, leaving the contract test red outside the pre-commit hook's tests/unit-only scope. Regenerated per the project's own documented process (MCPG_REGENERATE_TOOL_RETURN_SHAPES=1). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviews the remaining two opt-in ruff categories from the audit-remediation
sweep -- ASYNC (flake8-async, 18 violations) and C901 (mccabe complexity,
66) -- individually rather than blanket-fixing or blanket-suppressing.
pyproject.toml's [tool.ruff.lint] select list is still untouched; only a
new `external = ["ASYNC", "C90"]` entry was added so this task's mandated
per-violation `# noqa` justifications don't trip RUF100 (unused-noqa)
under a bare `ruff check .` before a later step formally enables the
categories.
ASYNC (18/18 justified-suppressed, 0 code changes needed):
- All 14 ASYNC109 ("timeout" parameter) hits are pass-throughs to a lower
layer that already owns real timeout enforcement -- httpx's own
per-request timeout (nl2sql.py's LLMProvider implementations,
observe_llm_behaviour.py) or SafeSqlDriver's existing
asyncio.timeout() (query.py's run_select/run_select_tuned/
explain_query/analyze_query_plan/run_select_parallel, confirmed by
reading sql/safety.py:181).
- The 3 ASYNC240 + 1 ASYNC221 blocking-call hits are all test-only or
one-off-script call sites (test_shell.py assertions after the call
under test returned, a one-time pg_dump --version probe, a benchmark
script's teardown unlink), not hot paths.
C901 (7 refactored, 59 justified-suppressed):
- Refactored (cheap, safe, test-verified): diesel.generate_diesel_schema,
sqlc.generate_sqlc_schema (extract-a-section-into-a-helper),
test_row_factory._synth_by_name / _synth_by_type (if-chain to
predicate/generator dispatch tables), test_data.generate_test_data
(extract per-column value formatting), audit.audit_database (collapse
a 6-branch "if not None: append" chain into one filtered extend),
diagrams.generate_schema_diagram (extract entity-block /
relationship-line rendering).
- Suppressed with an individual one-line reason each: the security-
critical SQL-safety kernel (sql/safety.py's _validate_node, per this
task's own mandate -- citing the module's existing fuzz-tested /
adversarially-pinned rationale, not refactored; sql/driver.py's
_execute_with_connection; tenancy.py's _execute_with_role), other
identifier-validation / credential-handling / subprocess-sandboxing /
tamper-evidence paths, the tools.py MCP-tool-registration functions
gated by the frozen 254-tool contract snapshot, numerical algorithms
(k-means, MMR re-ranking, ANN recall sweeps) where restructuring risks
changing the computed result, config.load_settings (complexity 178,
the single process-wide env-var reader), and several schema-generator
functions in the same family as the refactored diesel.py/sqlc.py but
with cross-cutting state that resists the same cheap extraction.
Full per-function detail:
.superpowers/sdd/2026-08-25-audit-remediation/batch-ruffB-report.md
Verification: `ruff check --select ASYNC,C901 .` -> 0. Full sweep --
`ruff check . && ruff format --check . && mypy src/mcpg && pytest -q`
(tests/unit + tests/contract + tests/integration in one run) -- clean,
3001 passed / 115 skipped, no failures, no tool-surface or
tool-return-shape contract drift.
Advances roadmap row: N/A (lint remediation, not a roadmap feature)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
…external's forward side-effect Two review findings on the prior ASYNC/C901 assessment commit (2b3de26), both documentation-of-a-correct-decision issues, no logic changes: 1. query.py:187's noqa comment claimed `run_select_tuned`'s timeout is enforced by SafeSqlDriver's asyncio.timeout() -- copy-pasted from the adjacent run_select/explain_query wording without checking that run_select_tuned only calls safe_driver._validate() (a pure pglast parse), never safe_driver.execute_query(). The SafeSqlDriver constructor's timeout= is dead on this path; real enforcement is the explicit asyncio.wait_for(..., timeout=timeout) around the actual execute_query call further down the same function (which already had a correct inline comment). Fixed the noqa comment, this task's batch report, and the matching CHANGELOG.md wording to name the real mechanism. 2. pyproject.toml's `external = ["ASYNC", "C90"]` (added so RUF100 doesn't flag this task's mandated per-violation noqa comments as unused before a later step enables these categories in `select`) prefix-matches unconditionally -- it will keep blinding RUF100 to stale ASYNC/C901 noqa comments even after `select` is updated. Added an explicit removal reminder in the pyproject.toml comment itself, next to the entry, for whoever does that later select-list update. Verification: `ruff check . && ruff format --check . && mypy src/mcpg` all clean (comment-only fix; full suite not re-run per reviewer guidance). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
…s.py subset) Fixes all 103 FBT (flake8-boolean-trap) violations in src/mcpg/tools.py — the tools.py half of the 196-violation FBT sweep (Task 23, Step 9). The 103 diagnostics (53 FBT001 + 50 FBT002; no FBT003) resolve to 53 boolean parameters across 43 functions. AST decorator inspection confirms all 43 are registered @server.tool MCP tools — there were no internal helper hits in this file, so no helper call sites needed updating. A `*` marker is inserted before each signature's first boolean parameter rather than reordering parameters, preserving declaration order and hence JSON-Schema `required` order. The exposed MCP wire contract is UNCHANGED. The mcp SDK derives each tool's inputSchema via pydantic, which treats keyword-only and positional-or-keyword parameters identically. Verified by asserting before regenerating: tests/contract/ passes with no regeneration env var set (51 passed), and re-running both MCPG_REGENERATE_TOOL_SNAPSHOT=1 and MCPG_REGENERATE_TOOL_RETURN_SHAPES=1 reproduces both snapshots byte-for-byte (empty git diff). No snapshot lines are committed — that is the result, not an omission. MCP clients always dispatch tool arguments by name and are unaffected. Also passes per_record= by keyword into walinspect.read_pg_wal_stats. Ruff's FBT003 only fires on boolean *literals*, not forwarded boolean *variables*, so the pending ~93-hit batch outside tools.py could have made that helper's parameter keyword-only and broken this caller with no lint signal. Verified: ruff check . clean; ruff format --check . clean; mypy src/mcpg clean (109 files); pytest -q → 3001 passed, 115 skipped. Advances roadmap row: n/a (lint remediation) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ruff sweep Part C's implementer report found the true post-commit FBT remainder (157) before the commit was finalized, but the CHANGELOG text still cited the plan's original ~93 estimate. Review (batch-ruffC-review.md) flagged this as an Important, non-contract-affecting finding. Corrects the CHANGELOG to the measured 157 (26 src/, 126 tests/, 5 tools/), per batch-ruffC-report.md's own live breakdown. Commit eab2ef0's message is left as-is (historical record); the report already documents the correction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
Fixes the remaining 30 FBT violations outside tests/ -- 26 in src/ (excluding tools.py, done in eab2ef0) and 4 in tools/ dev scripts. tests/ is deliberately out of scope for this batch. The substantive change is in the SQL-safety kernel: force_readonly is now keyword-only across the whole execute_query / _execute_with_connection override family (sql/driver.py, sql/safety.py, multidb.py, replicas.py, tenancy.py), so a security-relevant read-only flag can no longer be passed in the wrong positional slot. No allowlist, policy, or control flow was touched -- only parameter kind and call-site syntax. No public MCP tool signature and no snapshot changed; these are all internal helpers and driver methods. tests/contract/ passes unmodified (51 passed), and the full suite is 3001 passed / 115 skipped, matching the pre-change baseline exactly. Also fixes 10 positional boolean-forwarding hazards that ruff cannot see -- FBT003 fires only on boolean literals, never on a boolean variable forwarded positionally. Found by an AST arity sweep over src/ + tools/ + tests/ (count cross-checked against ruff's own FBT001). One of them, replicas.py's TenantTimeoutSqlDriver, would otherwise have broken at runtime: its super() call was already multi-line, so a single-line edit pass missed it and neither ruff, mypy (the override is untyped), nor the test suite would have caught it. Zero noqa added. pyproject.toml's [tool.ruff.lint] select list untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
The entry said "10 positional boolean-forwarding hazards"; the precise figure is 28 call sites converted, of which ruff flagged only 3 (the FBT003 literals in pitr.py). The other 25 are boolean variables forwarded positionally, which FBT003 structurally cannot see. Breakdown: force_readonly family 9, indexing 3, _validate_bool 8, tools/ 2, PitrGate 6. Verified by AST arity sweep, not by the linter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
Review (batch-ruffD-review.md) flagged as an actionable Minor: the CHANGELOG entry never stated the real signature count (20 — 18 ruff-flagged plus 4 untyped subclass overrides converted for family consistency), leaving the immutable commit subject's '18' uncorrected. The report itself already reconciled 18 vs 20; this surfaces that reconciliation in the reader-facing CHANGELOG too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
…T, Part E) Completes the FBT (flake8-boolean-trap) sweep across Parts C+D+E: 103 (tools.py) + 30 (src/ + tools/) + 126 (tests/) = 259 total violations fixed, FBT is now clean repo-wide. - 45 test-helper/fixture/fake definitions made keyword-only, every call site fixed to match. - Completed the Part D handoff exactly as specified: tests/unit/_fakes.py's three standalone fakes (FakeDriver, FakeRoutingDriver, FakeParamRoutingDriver) had force_readonly made keyword-only together with all 5 positional call sites into them, in this same commit, verified by grep before and after. - FBT003 call-site literals converted to keyword form, including several dataclass constructions (DomainInfo/PublicationInfo/SubscriptionInfo/ ColumnInfo/CronJob) verified against their actual field order in src/. One *key_args cache-key call (test_multidb_cache.py) was rewritten to pass a named variable instead of a literal, clearing FBT003 without a noqa. - Zero noqa added. One definition (test_database.py's _FakeConnection, mirroring psycopg's set_autocommit) had its bool annotation dropped rather than made keyword-only, since keyword-only would have required also changing its production caller (src/mcpg/database.py) -- out of this batch's scope. src/ is untouched by this commit. - pyproject.toml's [tool.ruff.lint] select list intentionally not touched -- that is a separate later step (adding FBT/C90/ASYNC/C4/SIM/ PTH/PYI) once all of Parts C/D/E are done. Verification: uv run ruff check . clean; uv run mypy src/mcpg clean; full suite 3001 passed/115 skipped; tests/contract/ 51 passed standalone -- both identical to Part D's baseline (no regression). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
…(Task 23, Step 11) Completes the rescoped ruff sweep (Task 23): all 7 categories fully assessed/fixed to zero across Parts A-E are now enforced by a bare ruff check ., not just spot-checked via --select. Removed the external = ["ASYNC", "C90"] entry added during Part B (its own comment mandated removal at this exact point) -- RUF100 now polices the ~113 ASYNC/C901 noqa comments normally; none were stale. No [tool.ruff.lint.mccabe] section added: Part B's 66 C901 findings were measured against ruff's unconfigured default max-complexity of 10 (several findings at exactly complexity 11 corroborate this), so leaving it unset preserves the exact bar already assessed. One new FBT001 hit surfaced once FBT was actually selected, in benchmarks/tokens/tier_b/experiments/real_harness_comparison.py's _result_to_trial helper -- a scope gap (benchmarks/ was never covered by Parts C/D/E's globs: tools.py, src/+tools/, tests/), not interim drift. Fixed directly as a trivial single-call-site change (passed: bool -> *, passed: bool) since it is not a registered MCP tool. Combined total across Parts A-E: 417 violations addressed, verified against .superpowers/sdd/2026-08-25-audit-remediation/progress.md. Verification: ruff check . clean, ruff format --check . clean (338 files), mypy src/mcpg clean (109 files), pytest -q 3001 passed/115 skipped, pytest -q tests/contract/ 51 passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
Fix wave for the 4 Minor findings from the whole-branch final review (.superpowers/sdd/2026-08-25-audit-remediation/final-review-report.md). Doc/config-only, no behavior change. - M-1: collapse the CHANGELOG's 6 overlapping ruff-sweep entries (Step 11, Part D, Part E, Part C, Part A, Part B) into one coherent Changed entry covering the categories added, the final totals (418 violations: 74 + 84 + 260), the force_readonly keyword-only conversion, the two near-miss bugs the AST arity sweep caught, and the mccabe-default-10 justification. Granular per-part narrative moved out to batch-ruff*.md (already the source of truth there). - M-2: re-measured the historical FBT remainder breakdown against the batch reports (not trusted from the old text): 26 in src/, 126 in tests/, 4 in tools/ (not 5), 1 in benchmarks/ (previously uncounted) = 157. Verified against batch-ruffD/E-report.md and batch-ruff-step11-report.md; current ruff check --select FBT... is 0 across src/, tests/, tools/, benchmarks/. - M-3: disclose in the Task 21 dependency entry that only pytest-randomly is active; pytest-mock, pytest-socket, time-machine, and pytest-rerunfailures are staged for future test-hygiene work and unused today (verified by grep). No dependency removed from pyproject.toml. - M-4: docs/security-hardening.md's HSTS default was stale (31536000); updated to the current 63072000 default, matching README.md. Grepped docs/ for other stale 31536000 references; found none needing a fix (the plan doc's mention is a correct historical before-value). Verification: ruff check . clean; full [Unreleased] section read for coherence; pytest -q full suite 3001 passed / 115 skipped (matches the review's baseline exactly). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013bghcqrffRB2LTwv2q2bTj
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @devopam, your pull request is larger than the review limit of 150,000 diff characters
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Remediates every finding from two audit runs against MCPg on 2026-08-25 — the
python-code-reviewskill's scorecard (all 11 domains: standards, quality, security, supply chain, performance,
concurrency, idioms, architecture, observability, scalability, testing) and the
project-incubationskill's Step 2–4 repo-structure audit (
docs/project-incubation-baseline.md). 25 planned tasks, oneunplanned SQL-injection fix discovered mid-branch during the bandit sweep, and a rescoped ruff-lint
sweep, across 37 commits. No architectural change.
Also folds in a remodeled icon/logo set under
docs/assets/.Roadmap linkage
Advances roadmap row: N/A — internal audit remediation (python-code-review + project-incubation skill
runs, 2026-08-25)
Highlights
mcpg.errors.MCPgErrorcommon base for all 65 domain exceptions across 64 modules — catch "anyMCPg-internal error" with one type. No exception's name, message, or call sites changed;
TenancyError/DynamicIntentErrorkeep theirValueErrorbase too (dual inheritance)./readyzreadiness probe (distinct from/healthzliveness), optionalTrustedHostMiddlewareviaMCPG_HTTP_TRUSTED_HOSTS, and an HSTS default bumped to 2 years (currentOWASP guidance).
run_select/run_select_tunednow bound the fetch (fetchmany) instead of materializing thewhole result set and truncating afterward. Partial mitigation — see follow-ups.
attempts, ~0.1–2s), layered retry-inside-breaker so an exhausted retry cycle counts as one breaker
failure. NL→SQL providers and the OIDC discovery fetch now reuse a persistent
httpx.AsyncClientinstead of handshaking per call.
RedactionFilterlog backstop; 7 silentexcept: passsites now log; tracebackspreserved at 23 error-logging call sites.
PTH/C4/SIM/PYI/ASYNC/C901/FBT, and all 7categories are now enforced by a bare
ruff check .(previously spot-checked via--selectonly).py.typedmarker added (theTyping :: Typedclassifier was previously unbacked); stalelicense-filesentry pointing at the removed vendored kernel dropped;hatchling>=1.26floorpinned;
pip-licenseslicense enumeration added to CI as a non-blocking report.CODE_OF_CONDUCT.md(Contributor Covenant v2.1, verbatim),.editorconfig, and a completed.env.exampleadded per the project-incubation audit.docs/assets/.when neither
MCPG_HTTP_AUTH_TOKENnorMCPG_AUTH_MODE=oidcwas configured; it now raisesConfigErrorand refuses to start.Migration: configure auth (
MCPG_HTTP_AUTH_TOKEN=…orMCPG_AUTH_MODE=oidc), or setMCPG_HTTP_ALLOW_UNAUTHENTICATED=trueto opt out explicitly — which is loudly logged on everystart. The default
stdiotransport is unaffected.MCPG_RATE_LIMIT_ENABLED,false→true).Migration: operators who want the previous unlimited behavior must set
MCPG_RATE_LIMIT_ENABLED=falseexplicitly.(Minor, Python-API only, not counted above:
force_readonlyis now keyword-only across theexecute_query/_execute_with_connectionoverride family inmcpg.sql. No MCP tool signature orcontract snapshot changed — MCP clients dispatch by name and are unaffected — but code embedding
mcpg.sqldirectly and passing this argument positionally needs a one-word edit.)Security fix included (unplanned, found mid-branch)
A bandit
B608sweep surfaced a real SQL-injection defect indescribe_graphandgenerate_graph_diagram: Apache AGE label names read back fromag_catalog.ag_labelwereinterpolated unescaped into both generated SQL (
FROM "{graph}"."{label}") and generated Mermaiddiagram text, with no identifier validation — so a label planted by a prior
run_cypherwrite couldcarry attacker-controlled SQL into a later re-interpolation. Now identifier-validated at both sinks
(aborting with
GraphErrorrather than silently skipping), matching the existinggraph_projection._check_identifierprecedent; both tools' backing queries also now runforce_readonly=True.(Note: an early internal framing of this as an "access-mode bypass" was incorrect and is not carried
forward —
policy.pygrantsCapability.READunconditionally in all three access modes, so noprivilege tier was ever bypassed. It's a straightforward identifier-injection defect, now closed.)
Tracked, deliberately not fixed here
into libpq during
execute(); this bounds Python-object materialization, not wire transfer. A truewire-level bound needs a server-side cursor or an injected
LIMIT(the existingcursors.pyDECLARE CURSORpattern is the natural reuse target). The original OOM audit finding stays open.RedactionFilterdoes not cover exception tracebacks.formatExceptionbypasses the filterentirely (
JSONFormatter/text formatter both call it directly). This branch raisedexc_info=Truesites in
src/mcpgfrom 3 to 28 while shipping this filter, including 6 new sites incache.pylogging Redis errors —
MCPG_REDIS_URLmay embed aredis://:password@hostcredential, andobfuscate_password's embedded-URL pattern is anchored topostgres(?:ql)?://only, so aredis://credential in a traceback is covered by neither layer today. No leak has been reproduced (depends on
the specific driver exception's
str()rendering), but it's a real structural gap the branch itselfwidened. Follow-up: redact at the formatter level
(
obfuscate_password(super().formatException(ei))in both formatters) and extendobfuscate_password's regex to coverredis(?:s)?://, closing all 28 current sites plus theRedactionFilter's msg/%-args-only scope in one pass.
B608remains globally skipped in bandit. The sweep that found the vulnerability above stoppedthere by design — ~60 of the original 95 hits were never individually verified, and
B608stays inpyproject.toml's bandit skip list rather than being narrowed to per-site# nosec. This branch isnow direct evidence the rule has real true positives here. The same unvalidated-catalog-identifier
pattern is confirmed to persist, untouched, at
schema_docs.py:153,migrations.py:636,data_movement.py:159,cursors.py:199. Follow-up: finish theB608narrowing and consolidatethe now-three live identifier-validation idioms (
graph_projection._check_identifier, the newgraph._check_label_identifier, andgraph.py's pre-existing looser.isalnum()check three linesaway) into one shared
mcpg.sqlhelper./readyzhas no OIDC/JWKS-readiness gate, by design: gating readiness on "JWKS ever cached" woulddeadlock a fresh OIDC instance out of rotation forever, since JWKS populates lazily on the first
verified request and
/readyzmust stay auth-exempt. Options for later: a proactive startup JWKSfetch, or a time-boxed grace window.
policy.PermissionErrorshadows the Python builtin (pre-existing; neither audit caught it).Verified inert — nothing in the repo imports it and nothing catches
PermissionErroranywhere.Renaming it is a separate, wider breaking change for its own PR.
pytest-mock,pytest-socket,time-machine,pytest-rerunfailures) are staged for planned future use and not yet exercised byany test; only
pytest-randomlyis active today. Disclosed inCHANGELOG.md; small, direct-onlysupply-chain cost (no new transitive dependencies).
Verification
ruff check .✅ ·ruff format --check .✅ (338 files) ·mypy src/mcpg✅ (109 files, strict) ·pytest -q --cov→ 3001 passed, 115 skipped, 0 failed (skips = integration suite, no localPostgreSQL) ·
pytest -q tests/contract/→ 51 passed, zero contract-snapshot drift across thewhole branch.
Coverage measured locally at 87.31% without a database (fails the 90% gate) — but the base commit
(
d6389ce, already onmain) scores 86.78% under identical no-database conditions and also fails, sothis branch moves coverage up (+0.53pp, +41 tests), not down. The shortfall is concentrated in
modules this branch didn't meaningfully touch and that only the integration suite exercises
(
tools.py61%,sqlalchemy_export.py36%,migrations.py54%, …); every module this branch addedcode to sits at 89–100%. CI's
testjob runs the same command with a live PostgreSQL per matrix laneand is the authoritative gate.
Every task/batch on this branch was independently reviewed (several with a fix round, all
independently re-verified rather than trusted on report); the ad-hoc security fix and the SQL-safety
kernel's signature changes both got extra-scrutiny review given their sensitivity. A final
whole-branch review (0 Critical, 2 Important — both filed as follow-ups above, not fixed here; 5
Minor — 4 fixed in a scoped fix wave, 1 informational) closed out the branch.
Checklist
integration, no local PG), 0 failed
ruff,ruff format, andmypy src/mcpgpassCHANGELOG.mdupdated under[Unreleased]N/A)src/mcpg/_vendor/