Skip to content

Improve Python bindings code quality (Codacy/Bandit cleanup)#4084

Merged
robfrank merged 23 commits into
mainfrom
improve-python-quality
May 7, 2026
Merged

Improve Python bindings code quality (Codacy/Bandit cleanup)#4084
robfrank merged 23 commits into
mainfrom
improve-python-quality

Conversation

@robfrank

@robfrank robfrank commented May 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Eliminates the Codacy/Bandit findings in bindings/python/ and adds a CI gate to prevent regression. Plan documented at docs/superpowers/plans/2026-05-05-python-bindings-codacy-fixes.md.

Bandit posture before vs. after:

Scope Before After
src/ 8 Low, 3 Medium, 0 High 0 / 0 / 0
tests/ 929 Low, 20 Medium, 0 High 0 / 0 / 0
examples/ 130 Low, 97 Medium, 5 High 0 medium-severity high-confidence (informational Low items remain by design)

What changed

  • Production source (src/): New _logging helper module replaces silent try/except/pass swallowing with debug-level traceback logging across async_executor, graph_batch, core (__del__), jvm (shutdown_jvm), schema, and server. Finalizers narrowed from bare except Exception to (AttributeError, RuntimeError). ArcadeDBServer default host changed from 0.0.0.0 to localhost (callers must opt in to bind on all interfaces). One annotated false-positive in vector.py where the SQL identifiers are quoted via _quote_identifier() and the user key is passed as a ? parameter.
  • Tests (tests/): TEST_PASSWORD constant centralised in conftest.py. Vector INSERT/DELETE in test_vector_sql.py now parameterized with ?. Bench-loop SQL in test_server_patterns.py annotated (parameterizing would change the workload being measured). Idiomatic assert statements skipped via [tool.bandit] skips = ["B101"].
  • Examples (examples/): SHA1 short-digests now use usedforsecurity=False. URL fetches in download_data.py and 21_server_mode_http_access.py are HTTPS-validated via a _require_https() guard. SQL string-builds in numbered demos either parameterized (17_timeseries_end_to_end.py) or annotated where interpolated values are script constants.
  • CI: New bandit job in .github/workflows/test-python-bindings.yml runs in parallel with the existing test matrix. Two gates:
    • src + tests: must be clean at low-severity / low-confidence.
    • examples: must be clean at medium-severity / high-confidence.

In-flight discoveries

  • exclude_dirs = ["build", ...] in bandit's pyproject config was substring-matching examples/11_vector_index_build.py. Fixed by anchoring patterns with /.
  • Bandit 1.9 does not honour [tool.bandit.assert_used] skips from pyproject.toml; switched to global skips = ["B101"]. Safe because src/ has no assert statements.

Test plan

  • CI bandit job passes on Linux runner
  • CI test matrix continues to pass on linux/amd64, linux/arm64, darwin/arm64, windows/amd64 across Python 3.10-3.14
  • Server.__del__ still tears down cleanly when JVM is already detached (covered by existing teardown tests)
  • ArcadeDBServer(root_password=...) defaults to localhost; explicit host="0.0.0.0" still works for opt-in (covered by new test_default_host_is_localhost)
  • Local verification: python3 -m bandit -c bindings/python/pyproject.toml -r bindings/python/src bindings/python/tests --severity-level low --confidence-level low exits 0
  • Local verification: python3 -m bandit -c bindings/python/pyproject.toml -r bindings/python/examples --severity-level medium --confidence-level high exits 0

Reviewing

The 21 commits map 1:1 to plan tasks and group naturally into 3 logical phases at boundaries 27da10759 (end of src+CI), c8121a024 (end of tests), and fdfda61cd (end of examples). Reviewers can read the commits in order and stop at any boundary.

@codacy-production

codacy-production Bot commented May 5, 2026

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 1 critical · 7 high · 8 minor

Alerts:
⚠ 16 issues (≤ 0 issues of at least minor severity)

Results:
16 new issues

Category Results
Documentation 7 minor
Security 1 critical
7 high
CodeStyle 1 minor

View in Codacy

🟢 Metrics 5 complexity

Metric Results
Complexity 5

View in Codacy

🟢 Coverage ∅ diff coverage · -8.25% coverage variation

Metric Results
Coverage variation -8.25% coverage variation
Diff coverage diff coverage

View coverage diff in Codacy

Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (1dc68da) 124062 91858 74.04%
Head commit (f365c8c) 155463 (+31401) 102280 (+10422) 65.79% (-8.25%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#4084) 0 0 ∅ (not applicable)

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request implements a comprehensive security hardening of the Python bindings, addressing numerous Bandit findings across the source code, tests, and examples. Key changes include enforcing HTTPS for external data downloads, parameterizing SQL queries to mitigate injection risks, and replacing silent exception suppression with structured logging or narrowed exception handling. The default server host has been updated to localhost for improved security, and a detailed implementation plan for these fixes has been added to the documentation. Feedback suggests extending the host check in server.py to include the IPv6 'all interfaces' address (::) for consistency.

Comment thread bindings/python/src/arcadedb_embedded/server.py Outdated
@claude

claude Bot commented May 5, 2026

Copy link
Copy Markdown

Code Review - PR #4084: Python Bindings Codacy/Bandit Cleanup

Overall this is a well-structured security cleanup. The changes are logically organized, the _logging helper is a clean abstraction, and the CI gate is the right way to prevent regression. A few items worth addressing before merge:


Issues

1. Plan document should not be committed to the repository

docs/superpowers/plans/2026-05-05-python-bindings-codacy-fixes.md is 1,457 lines of agentic implementation notes. This is a planning artifact, not project documentation - it references internal tooling (superpowers:subagent-driven-development), has unchecked TODO checkboxes, and will be stale immediately after this PR merges. It clutters git history and misleads future readers. This file should be dropped from the PR; the PR description already captures the relevant summary.

2. actions/setup-python not pinned to a SHA

uses: actions/setup-python@v5   # floating tag

The existing actions/checkout step pins to a full commit SHA (@de0fac2...). setup-python@v5 should be pinned the same way for supply-chain consistency. A floating @v5 tag can be moved by the action author.

3. Breaking change in ArcadeDBServer default host needs a changelog entry

Changing the default from 0.0.0.0 to localhost is a silent breaking change for any caller who relied on the default to expose the server on all interfaces. The PR description describes it, but this should also appear in whatever changelog/release notes file the project uses (if one exists), and ideally emit a deprecation warning or note in the class docstring so users upgrading see it.

4. _require_https() is duplicated across three example files

The same function body is copy-pasted into 11_vector_index_build.py, 12_vector_search.py, and download_data.py. 21_server_mode_http_access.py inlines equivalent logic inline. A single examples/_utils.py (or similar) would eliminate the duplication and make future changes consistent.

5. test_default_host_is_localhost tests private state

assert server._config.get("host", "localhost") == "localhost"

_config is a private attribute. The test also doesn't start the server, so it can't verify what the JVM actually receives. A stronger test would start the server and check server.get_http_port() responds only on localhost - or at minimum drop the _config introspection and only assert on get_studio_url().

6. # nosec placement in triple-quoted strings may not suppress correctly

In test_graph_algorithms_sql.py:

    {select_statement}
    """  # nosec B608

Bandit flags the line containing the SQL pattern, not the closing delimiter. The # nosec comment needs to be on the same line as the f-string expression or the interpolation. Verify that bandit actually silences this finding; if not, move the comment to the correct line.


Minor Notes

  • conftest.py teardown still uses bare pass: Line 719 has pass # nosec B110 but the outer except Exception: block is still broader than needed (compare with the narrowed except (AttributeError, RuntimeError) used in __del__ methods). Not a bug, but inconsistent style.

  • Bandit version not pinned in CI: python -m pip install "bandit>=1.9.0" will pick up future 2.x releases. Pinning to an exact version (e.g., bandit==1.9.0) gives reproducible scans. Alternatively, add it to a requirements-dev.txt that is pinned.

  • schema.py outer except Exception still broad: The outer catch in get_vector_index and list_vector_indexes was narrowed in __del__ paths but remains except Exception for the index-iteration loop. This is probably correct since those are recoverable paths, but a comment explaining why it remains broad (vs. the finalizer pattern) would help reviewers.


What's Good

  • The _logging helper is a clean, single-responsibility module. log_swallowed_exception makes formerly invisible errors observable without adding noise.
  • Narrowing finalizer catches to (AttributeError, RuntimeError) is the right call - it makes the suppression intentional and documented.
  • _require_https() guard is a real fix, not just a nosec annotation.
  • Parameterizing 17_timeseries_end_to_end.py's query is the correct fix (user-supplied values in the WHERE clause).
  • The two-tier CI gate (strict for src/tests, advisory for examples) is pragmatic and correct.
  • TEST_PASSWORD centralisation in conftest.py is a clean DRY improvement.

The plan document and unpinned action are the blockers. The rest are improvements but can be addressed in a follow-up if preferred.

robfrank added a commit that referenced this pull request May 5, 2026
- server.py: drop unused log_swallowed_exception import / _LOGGER (caught

  by pylint W0611, surfaced via Codacy UnusedCode finding)

- server.py: extend host whitelist to IPv6 "::" so Studio URL is rewritten

  to localhost when bound on either IPv4 or IPv6 all-interfaces address

- server.py: document default-host change in ArcadeDBServer.__init__

  docstring (no CHANGELOG.md exists in this repo)

- test_server.py: drop _config private-state assertion; rely on the

  publicly-observable get_studio_url() composition instead

- test-python-bindings.yml: SHA-pin actions/setup-python to match the

  rest of the workflow's pinning convention; pin bandit==1.9.4 for

  reproducible scans
@robfrank

robfrank commented May 5, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the review. Walking through each item with verification before implementing:

Addressed in b35f8c7

Issue 2 - SHA-pin setup-python: Done. Changed to actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0, matching the SHA already used elsewhere in this workflow.

Issue 3 - Document the breaking change: Done. There is no CHANGELOG.md in this repo (verified via ls CHANGELOG* -> no matches), so I added the note inline to ArcadeDBServer.__init__'s docstring covering: new default, how to opt back in to all-interfaces, and a pointer to the security cleanup as the reason.

Issue 5 - Test private state: Done. Dropped the server._config.get("host", ...) assertion. Kept get_studio_url() since it's the public surface that exercises the same default-host path. Test docstring now explains why we don't start the JVM here.

Minor (Bandit version): Pinned bandit==1.9.4 for reproducible scans.

Codacy UnusedCode: Fixed - the prior commit on server.py left an unused from ._logging import ..., log_swallowed_exception and an unused _LOGGER. Pylint flags it as W0611; this is the most likely "1 medium UnusedCode" entry on the Codacy report. Imports removed.

Gemini IPv6 suggestion: Applied. if host in ("0.0.0.0", "::") so the Studio URL is rewritten to localhost regardless of which all-interfaces address the user opted into.

Pushing back

Issue 1 - Plan doc shouldn't be committed: This repo has an established convention of committing implementation plans to docs/superpowers/plans/ (10+ existing files, going back to PRs #4035 and #3731). The format and tooling references are intentional and consistent with the other plans in that directory. I'd rather keep the convention uniform than carve out an exception for this PR. If the convention itself should change that's a separate, repo-wide discussion.

Issue 4 - DRY _require_https across examples: The examples/ directory is intentionally a flat set of standalone, self-contained scripts. Verified no _utils.py / shared-helper precedent exists (no examples import each other; __init__.py is absent). Centralising the helper would break the "copy this single file and run it" property that the demos rely on. The duplication is ~9 lines, so the trade-off favours leaving them standalone. Happy to revisit if the convention changes.

Issue 6 - nosec placement in triple-quoted strings: Verified - python3 -m bandit -c bindings/python/pyproject.toml bindings/python/tests/test_graph_algorithms_sql.py reports No issues identified at all severities. The closing-delimiter placement does suppress correctly here because Bandit attaches the B608 finding to the line containing the closing ) of the db.command(...) call, not the opening of the f-string. The CI gate would have caught it otherwise.

Minor "conftest.py:719": That line number doesn't exist - bindings/python/tests/conftest.py is 185 lines. The actual pass # nosec B110 annotations in conftest are at lines ~63 and ~70, both inside temp_db teardown where best-effort cleanup is the documented intent.

Noted, not addressing this PR

Minor (schema.py outer except): Could add a comment justifying the broader except Exception: for the recoverable iteration path vs. the narrowed finalizer pattern. Tracking as a follow-up - not material to this PR's scope.

CI re-runs against b35f8c7.

@claude

claude Bot commented May 5, 2026

Copy link
Copy Markdown

Code Review: Improve Python bindings code quality (Codacy/Bandit cleanup)

Overall: This is a well-structured and well-motivated cleanup PR. The changes are genuine improvements: real security fixes in production code, better observability for swallowed exceptions, and a CI gate to prevent regression. The scope is large but the commits are logically organized. A few issues worth discussing before merge.


Positives

  • CI gate is solid. Pinned action hashes, separate thresholds for src/tests vs examples, and running in parallel with the test matrix. Good design.
  • _logging.py helper is the right abstraction. Centralizing log_swallowed_exception makes the suppression observable and the intent explicit. The module is minimal and focused.
  • _require_https() guard. Enforcing HTTPS at the call site before urlopen/urlretrieve is a real defense-in-depth improvement, not just noise suppression.
  • TEST_PASSWORD centralization. Good DRY fix; makes the test suite easier to maintain.
  • Parameterized SQL in test_vector_sql.py. The INSERT/DELETE conversion to ? placeholders is a genuine fix, not just annotation noise.
  • usedforsecurity=False on SHA1. Correct - these are used as collision-resistant path shorteners, not cryptographic hashes.

Issues

1. docs/superpowers/plans/2026-05-05-python-bindings-codacy-fixes.md - should not be committed

This 1457-line file is an AI agent planning artifact, not project documentation. It contains directives like "For agentic workers: REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development" and implementation checklists. It does not belong in the repository. The PR description already summarizes the plan clearly. Please remove this file before merge.

2. Breaking change: default host 0.0.0.0 -> localhost

# Before
host = self._config.get("host", "0.0.0.0")
# After
host = self._config.get("host", "localhost")

Any user who depended on the server being accessible from the network by default will silently find their server unreachable after upgrading. This is a breaking change. Options:

  • Emit a DeprecationWarning in the current release with a grace period before changing the default, or
  • Accept it as a known breaking change and ensure it is prominently documented in the changelog/release notes, not just in a docstring.

The PR description mentions it, but users who pip install --upgrade and don't read the changelog will be surprised.

3. Narrowed except clauses may be too restrictive for JPype

# core.py and server.py finalizers
except (AttributeError, RuntimeError):
    return

JPype can raise additional exception types during JVM teardown, including jpype.JException, SystemError, and TypeError (e.g., when an attribute is looked up on a None reference through Python's normal path rather than JPype's JVM path). The original except Exception was broad by design - finalizers genuinely cannot know what state the interpreter is in. Narrowing may cause TypeError or similar to propagate out of __del__, which Python logs as an unhandled exception in a finalizer (noisy and alarming to users, even if harmless).

Suggestion: Either keep except Exception with the log_swallowed_exception treatment (which gives you the observability improvement without the fragility), or add a comment explaining what specific JPype exception types have been verified to cover all teardown scenarios.

4. IPv6 loopback not handled in 21_server_mode_http_access.py

if not request.full_url.startswith(
    ("http://localhost", "http://127.0.0.1", "https://")
):
    raise ValueError(...)

http://[::1]:2480/ (IPv6 loopback) is a valid local URL that this guard rejects. Minor, but worth adding "http://[::1]" to the tuple if IPv6 is intended to be supported.

5. Performance benchmark tests: nosec without explaining the parameterization decision

In test_server_patterns.py, the benchmark SQL queries use f-string interpolation with # nosec B608. The PR description says "Parameterizing would change the workload being measured" - that rationale is correct and important, but it's only in the PR description. A short inline comment on the first annotated query explaining why parameterization was intentionally skipped would make the code self-documenting for future maintainers who encounter it without this PR's context.

6. conftest.py teardown still uses bare pass - inconsistency

except Exception:
    pass  # nosec B110 - best-effort teardown after JVM may be down

The production code (core.py, server.py, jvm.py) was upgraded to use log_swallowed_exception, but the test teardown in conftest.py still uses bare pass. This is arguably fine for tests, but it is inconsistent and means teardown failures in tests are still invisible. Consider applying log_swallowed_exception here too, at least to make it easier to diagnose flaky teardown failures.


Minor Observations

  • Bandit version pinned in CI (1.9.4) but flexible in dev deps (>=1.9.0). This is correct - CI needs reproducibility; devs get flexibility.
  • skips = ["B101"] is global. The PR notes this is safe because src/ has no assert statements. True today, but worth a brief comment in pyproject.toml noting the constraint so a future developer adding assert to src/ knows why Bandit won't catch it.
  • get_studio_url() now handles "::" for IPv6 all-interfaces. Good addition.

Summary

Category Status
CI gate Ready
Production logging helper Ready
HTTPS enforcement Ready
SHA1 usedforsecurity=False Ready
TEST_PASSWORD centralization Ready
docs/superpowers/plans/ file Remove before merge
Default host breaking change Needs explicit changelog note or deprecation
Narrowed except clauses Discuss - may be too restrictive
IPv6 loopback URL guard Minor - worth fixing
Benchmark nosec without rationale comment Minor suggestion

The core of this PR is solid. Removing the planning document and addressing the breaking-change visibility are the two things I would consider blockers.

@claude

claude Bot commented May 5, 2026

Copy link
Copy Markdown

Code Review - PR #4084: Improve Python bindings code quality (Codacy/Bandit cleanup)

Overall this is a well-structured cleanup with clear intent and good commit organization. A few issues worth addressing before merge.


Overview

The PR eliminates 1192 Bandit findings across src/, tests/, and examples/ by a combination of real fixes (exception narrowing, SQL parameterization, HTTPS enforcement) and documented suppressions (# nosec) where the findings are false positives. A CI gate is added to prevent regression.


Issues

1. Breaking change - default host silently changed

server.py:648 changes the default host from "0.0.0.0" to "localhost". This is correct from a security standpoint, but it is a silent breaking change for anyone relying on the old default. The docstring says "Earlier versions defaulted to 0.0.0.0; the default was tightened to loopback in the Python bindings v0.x security cleanup" - the version reference v0.x is too vague to be useful in a changelog or migration guide. If there is a CHANGELOG.md or release notes file, this should be called out explicitly.

2. Plan document committed to source tree

docs/superpowers/plans/2026-05-05-python-bindings-codacy-fixes.md is a 1457-line agentic planning artifact. This type of document belongs in the PR description or an issue, not committed to the repo. It will accumulate over time, is not useful to future readers, and has no bearing on the correctness of the code. Please remove it before merging.

3. test_default_host_is_localhost may need @pytest.mark.server

# tests/test_server.py:998
def test_default_host_is_localhost(temp_server_root):
    from arcadedb_embedded.server import ArcadeDBServer
    server = ArcadeDBServer(root_path=temp_server_root, root_password=TEST_PASSWORD)
    assert server.get_studio_url().startswith("http://localhost:")

Every other test in test_server.py is decorated with @pytest.mark.server. If ArcadeDBServer.__init__ initializes or probes the JVM at construction time, this test will fail in environments without server support while not being skipped. Either add @pytest.mark.server (with a note that we intentionally do not call start()) or document why JVM init is deferred past __init__.

4. Loose # nosec B310 annotation in wait_for_qdrant_ready

# examples/11_vector_index_build.py:964-971
with urlopen(url, timeout=3) as response:  # nosec B310 - localhost health-check URL

The url variable here comes from a list built from the host parameter, which the function does not constrain to localhost. The annotation's claim ("localhost health-check URL") is based on caller convention, not code-level enforcement. This is a minor issue for an example script, but the comment should say something like "caller passes localhost URLs; see wait_for_qdrant_ready call sites" to make the reasoning traceable.

5. Duplicated _require_https guard

The _require_https helper is defined independently in both examples/11_vector_index_build.py and examples/12_vector_search.py, and as _require_https in examples/download_data.py. These are standalone scripts, so duplication is somewhat expected, but if all three are in the same package they could import from download_data or a shared utility. Not a blocker.

6. Broad except Exception: pass preserved in tests with # nosec B110

In several test teardown paths, the PR preserves:

except Exception:
    pass  # nosec B110 - best-effort teardown after JVM may be down

Production code had these narrowed to (AttributeError, RuntimeError). Tests don't need the same standard, but the inconsistency is worth a quick note: if other exception types (e.g., OSError) are swallowed during teardown, failures could be silently masked and make debugging harder.


Positives

  • The _logging.py helper module is the right abstraction - single responsibility, minimal surface area, and properly tested in test_logging_helper.py.
  • Exception narrowing in core.py, server.py, and jvm.py is correct: (AttributeError, RuntimeError) are the actual exception types JPype raises during JVM teardown.
  • SQL parameterization in test_vector_sql.py (DELETE FROM DocSql WHERE id = ?, i) is a real correctness improvement.
  • The # nosec B608 annotations in benchmark SQL (e.g., test_server_patterns.py) are appropriately justified - parameterizing the bench loop would change the workload being measured.
  • usedforsecurity=False for SHA1 is correct - the digest is used purely as a non-cryptographic uniqueness key.
  • CI job uses pinned action SHAs and a pinned Bandit version, which is good hygiene.
  • The two-tier Bandit gate (strict for src/tests, advisory for examples) is a sensible policy.
  • TEST_PASSWORD centralized in conftest.py is a nice cleanup; all references updated.
  • get_studio_url() now handles "::" (IPv6 all-interfaces) alongside "0.0.0.0".

Summary

Fix the plan document (remove it), clarify the version reference in the host-change docstring, and verify the @pytest.mark.server question on the new test. Everything else looks solid.

@codecov

codecov Bot commented May 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.55%. Comparing base (1dc68da) to head (f365c8c).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4084      +/-   ##
==========================================
- Coverage   64.94%   64.55%   -0.39%     
==========================================
  Files        1627     1627              
  Lines      124062   124062              
  Branches    26454    26454              
==========================================
- Hits        80574    80094     -480     
- Misses      32234    32823     +589     
+ Partials    11254    11145     -109     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

robfrank added 22 commits May 7, 2026 13:16
Replace try/except/pass (Bandit B110) with log_swallowed_exception so
the suppressed error is observable at DEBUG level instead of being
silently dropped.
# Python Bindings Codacy/Bandit Cleanup Implementation Plan

> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.

**Goal:** Eliminate the Bandit findings Codacy reports against `bindings/python/` (1192 issues across src/tests/examples) by fixing real defects in production code, parameterizing where practical in tests/examples, configuring Bandit to suppress test/example noise that is not actually unsafe, and wiring Bandit into the existing Python CI workflow as a quality gate.

**Architecture:** Three-PR rollout. PR1 hardens the production library (`src/`) with no `# nosec` allowed except for documented false positives; adds a project-level Bandit config to `pyproject.toml`; and adds a `bandit` job to `.github/workflows/test-python-bindings.yml`. PR2 sweeps the test suite (move shared test password to `conftest.py`, parameterize SQL where reasonable, suppress idiomatic `assert`/`random` noise via configuration). PR3 sweeps `examples/` (one-character SHA1 fix using `usedforsecurity=False`, narrow targeted suppressions for educational SQL/subprocess usage).
Adds a parallel CI job that fails the build on any Bandit finding in

the production source tree. Also fixes the exclude_dirs glob anchors

in [tool.bandit] config so substring matches like 'build' don't

accidentally exclude examples/11_vector_index_build.py.
Broadens the existing CI Bandit gate to scan tests/ alongside src/.

Also fixes the [tool.bandit] config: bandit's pyproject.toml support

doesn't honor [tool.bandit.assert_used].skips, so we use the global

skips = ["B101"] form instead. B101 is the standard pytest idiom

and src/ intentionally contains no asserts.
- server.py: drop unused log_swallowed_exception import / _LOGGER (caught

  by pylint W0611, surfaced via Codacy UnusedCode finding)

- server.py: extend host whitelist to IPv6 "::" so Studio URL is rewritten

  to localhost when bound on either IPv4 or IPv6 all-interfaces address

- server.py: document default-host change in ArcadeDBServer.__init__

  docstring (no CHANGELOG.md exists in this repo)

- test_server.py: drop _config private-state assertion; rely on the

  publicly-observable get_studio_url() composition instead

- test-python-bindings.yml: SHA-pin actions/setup-python to match the

  rest of the workflow's pinning convention; pin bandit==1.9.4 for

  reproducible scans
The Python wrapper's _convert_args path only handles a single positional

? parameter (numpy/list rebinding); a multi-? command("sql", sql, i, vec)

dispatches to JPype as command(str, str, int, list) which has no Java

overload. Revert to embedded-literal SQL with # nosec B608 (annotated

with the rationale inline). DELETE keeps its single-param parameterization

since that is supported. Caught by test_vector_delete_and_search_others_sql

in CI.
@robfrank robfrank force-pushed the improve-python-quality branch from 4c28a1c to f365c8c Compare May 7, 2026 11:16
@claude

claude Bot commented May 7, 2026

Copy link
Copy Markdown

Code Review - PR #4084: Improve Python Bindings Code Quality (Codacy/Bandit Cleanup)

Summary: This PR cleans up Bandit security findings in bindings/python/, adds a CI gate, and makes several real security improvements. The overall approach is sound and well-structured across three logical phases.


Strengths

  • _logging.py module: Clean, single-responsibility helper. Replacing silent except/pass with observable DEBUG-level logging is the right call for JVM finalizer paths.
  • CI gate design: Two-tier severity (strict low/low for src+tests, medium+/high for examples) is a pragmatic approach that avoids blocking examples over educational SQL.
  • Pinned action hashes in the workflow step is a good practice.
  • TEST_PASSWORD centralization improves test maintainability.
  • HTTPS guard pattern (_require_https()) in examples is the correct fix - validate before the # nosec, not just after.
  • SQL parameterization in 17_timeseries_end_to_end.py is a genuine fix, not just a suppression.

Issues

Breaking Change - Server Default Host (high impact)

bindings/python/src/arcadedb_embedded/server.py

Changing the default host from 0.0.0.0 to localhost is a breaking change for any user who relied on the previous default to expose the server on all interfaces. Users running the server on a machine where they need external connectivity will silently get a localhost-only bind after upgrading.

The docstring mentions "Earlier versions defaulted to 0.0.0.0" but there is no CHANGELOG, migration guide, or semver bump to signal this. This should either:

  • Be documented in a changelog/release notes file, or
  • Come with a deprecation warning for the old behavior, or
  • At minimum be called out explicitly in the PR description as a breaking change requiring a minor/major version bump.

Plan Document Should Not Be in the Repo

docs/superpowers/plans/2026-05-05-python-bindings-codacy-fixes.md

This 1457-line file is an internal agentic implementation plan - it contains instructions like > For agentic workers: REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development. It is not documentation for users or contributors of ArcadeDB. It adds noise to the repo, will rot quickly (the task checkboxes are ephemeral), and exposes internal tooling details. This file should be dropped from the PR.

Version Drift Between CI and pyproject.toml

.github/workflows/test-python-bindings.yml pins bandit==1.9.4, while pyproject.toml declares bandit>=1.9.0. If someone installs from pyproject.toml and runs locally with a newer Bandit version (e.g., 1.10.x) that adds new rules, CI will pass but local runs may fail. Consider aligning: either pin both to ==1.9.4 or use a requirements-dev.txt / lock file.

Incomplete Parameterization in test_vector_sql.py

# test_vector_sql.py  ~line 580
test_db.command(
    "sql",
    f"INSERT INTO DocSql SET id = {i}, embedding = {vec}",  # nosec B608
)

The comment says the binding layer only supports one ? per call. This is a known limitation documented inline - but it means a real SQL injection surface exists in test code using a vec list that comes from Python's random. For a test file this is low risk, but it's worth noting that the suppression is masking a genuine (if low-severity) issue rather than a false positive. Consider tracking this as a follow-up to fix the binding layer's multi-parameter support.

Broad except Exception Still in schema.py

# schema.py - outer loop
except Exception:
    log_swallowed_exception(_LOGGER, "while iterating schema indexes")

The inner probe (TypeIndex sub-index inspection) was appropriately narrowed, but the outer iterator still catches all exceptions broadly. Given this is reflective schema discovery code that may legitimately need to handle varied Java exceptions, this may be acceptable - but it would be good to document which specific exceptions are expected (e.g., Java NullPointerException mapped through JPype). At minimum a comment would help.

get_logger Wrapper is Trivially Thin

def get_logger(name: str) -> logging.Logger:
    return logging.getLogger(name)

This adds no behavior over logging.getLogger directly. Its only value is import consistency. Consider just importing logging directly in each module and calling logging.getLogger(__name__) - saves an indirection layer and avoids the appearance of a public API when it is just a stdlib passthrough.


Minor Notes

  • 21_server_mode_http_access.py: The URL guard allows both http://localhost and http://127.0.0.1 in addition to https://. This is correct for the localhost demo, but be aware it does not cover [::1] (IPv6 loopback). Not a blocker since this is an example file.
  • jvm.py: Changed from except Exception to except RuntimeError. JPype can also raise Exception subclasses beyond RuntimeError during shutdown (e.g., from attached-thread state). The docstring documents the rationale which is good - worth a brief integration test of the teardown path to confirm no other exception type surfaces.
  • The Bandit exclude_dirs uses leading / to prevent substring matching. The PR description notes this was a bug fix, which is correct. A brief comment in the config referencing the original mismatch (e.g., 11_vector_index_build.py) would help future maintainers understand why the paths look non-standard.

Summary

The security improvements are real and the overall approach is well-considered. The blocking items before merge are:

  1. Drop docs/superpowers/plans/2026-05-05-python-bindings-codacy-fixes.md from the PR.
  2. Document the breaking change in server default host (changelog or explicit version bump).

The CI gate, _logging module, HTTPS guards, and TEST_PASSWORD centralization are all good additions.

@robfrank robfrank merged commit 0efd60a into main May 7, 2026
114 of 119 checks passed
@robfrank robfrank deleted the improve-python-quality branch May 7, 2026 20:46
@robfrank robfrank added this to the 26.5.1 milestone May 7, 2026
tae898 pushed a commit to humemai/arcadedb-embedded-python that referenced this pull request May 10, 2026
robfrank added a commit that referenced this pull request May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant