Skip to content

ci: H-1 batch 5 — Ruff S (security/bandit) closes the H-1 ratchet for src/#113

Merged
rolandpg merged 1 commit into
masterfrom
fix/audit-h1-ruff-batch-5-security
Apr 25, 2026
Merged

ci: H-1 batch 5 — Ruff S (security/bandit) closes the H-1 ratchet for src/#113
rolandpg merged 1 commit into
masterfrom
fix/audit-h1-ruff-batch-5-security

Conversation

@rolandpg
Copy link
Copy Markdown
Owner

Summary

Adds `S` (flake8-bandit) to the ruff `select` list. Closes the GOV-003 §"Tooling and Automation" rule set for `src/` — only `ANN` remains and it will be ratcheted per-module.

Active `select` list now: `{E, F, I, W, N, T20, B, UP, SIM, RUF, S}`.

Findings + disposition (14 in src/)

Code Description Count Disposition
S110/S112 try-except-pass / -continue 6 per-line `# noqa` with intent (best-effort parsers, defensive fallbacks)
S311 random for non-cryptographic uses 3 per-line `# noqa` (note ID suffix, retry jitter, mock embedding)
S324 hashlib.md5 2 switched to `usedforsecurity=False` (PEP 587, 3.9+)
S608 f-string SQL 1 per-line `# noqa` (column list is module constant; values `?`-bound)

CI lints `src/` only, so the 1095 `assert` statements in `tests/` are not in scope.

Stack

This is the last of 5 H-1 batches:

Test plan

  • `ruff check src/` clean with full select list
  • `ruff format --check src/` clean
  • 69/70 critical tests pass (1 pre-existing env-dependent: `test_ingest_relationship`)
  • CI green

🤖 Generated with Claude Code

Closes the GOV-003 §"Tooling and Automation" rule set for src/. With
this batch the active select list is {E, F, I, W, N, T20, B, UP,
SIM, RUF, S}; only ANN remains and it will be ratcheted per-module to
avoid a 1000+ finding flood.

## Findings + disposition (14 total)

- **S110/S112** (try-except-pass / -continue, 4 cases) — best-effort
  parsers and defensive fallbacks. Each annotated with `# noqa: S110`
  + a sentence explaining the documented intent (timestamp parser
  fallback chain, JSONL corrupt-line drop, pyproject probe, attribute
  enumeration in lance metric serializer).
- **S311** (random for non-cryptographic uses, 3 cases) — note ID
  suffix, retry jitter, deterministic mock embedding. Each annotated
  `# noqa: S311` with intent.
- **S324** (hashlib.md5, 2 cases) — query_id (12-char truncation, not
  a security boundary) and deterministic mock embedding. Both switched
  to the 3.9+ `hashlib.md5(..., usedforsecurity=False)` form which
  ruff accepts as explicit non-crypto use.
- **S608** (f-string SQL, 1 case) — `_INSERT_NOTE_SQL` interpolates a
  module-level constant column list and `?`-binds all values; the only
  variable input goes through bound parameters. `# noqa: S608` with
  the safety reasoning inline.

CI lints `src/` only, so the 1095 `assert` statements in `tests/` are
not in scope.

Stacked on fix/audit-h1-ruff-batch-4-simruf.
Copilot AI review requested due to automatic review settings April 25, 2026 05:12
@rolandpg rolandpg merged commit ce5977f into master Apr 25, 2026
14 checks passed
@rolandpg rolandpg deleted the fix/audit-h1-ruff-batch-5-security branch April 25, 2026 05:14
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Enables Ruff’s S (flake8-bandit) security rules for src/ and triages the newly surfaced findings via targeted # noqa: SXXX annotations and non-crypto-safe hashing/RNG clarifications.

Changes:

  • Add S to Ruff’s select list in pyproject.toml.
  • Update MD5 usage to pass usedforsecurity=False where MD5 is used non-cryptographically.
  • Add targeted # noqa: S110/S112/S311/S608 annotations with intent comments for accepted exceptions.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/zettelforge/vector_memory.py Marks deterministic mock embedding RNG as non-crypto; MD5 updated with usedforsecurity=False; adds # noqa: S311.
src/zettelforge/synthesis_generator.py Updates non-crypto query ID hashing to md5(..., usedforsecurity=False).
src/zettelforge/sqlite_backend.py Adds # noqa: S608 for constant-column SQL string assembly and documents rationale.
src/zettelforge/scripts/compact_lance.py Adds targeted # noqa: S110/S112 for best-effort serialization fallbacks.
src/zettelforge/retry.py Adds # noqa: S311 for non-crypto retry jitter RNG.
src/zettelforge/ocsf.py Adds # noqa: S110 for defensive try/except/pass around version resolution.
src/zettelforge/memory_store.py Adds # noqa: S311 for note-id suffix RNG and # noqa: S110 for corrupt JSONL line handling.
src/zettelforge/knowledge_graph.py Adds # noqa: S110/S112 for best-effort timestamp parsing fallbacks.
pyproject.toml Enables Ruff S rules and updates the ratchet commentary.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +271 to 274
# _NOTE_COLUMNS is a module-level constant; row values are ?-bound. Safe.
_INSERT_NOTE_SQL = (
f"INSERT OR REPLACE INTO notes ({', '.join(_NOTE_COLUMNS)}) "
f"INSERT OR REPLACE INTO notes ({', '.join(_NOTE_COLUMNS)}) " # noqa: S608
f"VALUES ({', '.join('?' for _ in _NOTE_COLUMNS)})"
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

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

_NOTE_COLUMNS is documented here as a “module-level constant”, but it’s currently a mutable list. Since this line is used to justify # noqa: S608 on the f-string SQL, it would be safer/clearer to make the columns truly immutable (e.g., a tuple and/or typing.Final) or adjust the comment to avoid implying immutability.

Copilot uses AI. Check for mistakes.
Comment on lines +144 to 147
# Note IDs are not security-sensitive; collisions are tolerated
# (timestamp-prefixed → vanishing collision probability per second).
suffix = str(random.randint(0, 9999)).zfill(4) # noqa: S311
return f"note_{ts}_{suffix}"
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

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

The new comment says “collisions are tolerated”, but write_note() uses this ID as the primary key for the JSONL store and in-memory cache (dict keyed by note.id), so an ID collision can overwrite/hide an existing note (data loss). Either update the implementation to avoid collisions (e.g., uuid4 suffix like SQLiteBackend.write_note does) or soften/correct the comment to reflect the actual risk/behavior.

Copilot uses AI. Check for mistakes.
rolandpg added a commit that referenced this pull request Apr 25, 2026
… work

The [Unreleased] section was empty since v2.4.3 cut. Captures
everything merged since: RFC-011 local LLM backend (#104), RFC-012
LiteLLM (#108), the H-1 Ruff ratchet (#106 #107 #109 #111 #113),
the L-4 CI shell-precedence fix (#112), the spec-drift validator
broadening + GOV-009 Snyk declarations (#114), and a CONTRIBUTING.md
accuracy pass (#115).

Adds a compliance-audit closure table mirroring the running scoreboard
in TODO.md, scoped to what shipped — outstanding items (H-3 mypy,
H-4 GOV-006, M-2 RFC template, M-4 lock file) listed below the table
as remaining work for v2.5.x.

Targets v2.5.0 release.
rolandpg added a commit that referenced this pull request Apr 25, 2026
… work (#116)

The [Unreleased] section was empty since v2.4.3 cut. Captures
everything merged since: RFC-011 local LLM backend (#104), RFC-012
LiteLLM (#108), the H-1 Ruff ratchet (#106 #107 #109 #111 #113),
the L-4 CI shell-precedence fix (#112), the spec-drift validator
broadening + GOV-009 Snyk declarations (#114), and a CONTRIBUTING.md
accuracy pass (#115).

Adds a compliance-audit closure table mirroring the running scoreboard
in TODO.md, scoped to what shipped — outstanding items (H-3 mypy,
H-4 GOV-006, M-2 RFC template, M-4 lock file) listed below the table
as remaining work for v2.5.x.

Targets v2.5.0 release.
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.

2 participants