feat(security): add repowise security scan --history for full git-history secret scanning

[Ayush7614]

Summary

Implements #818. Reuses the existing SecurityScanner pattern registry and the shared security_findings tab
le rather than forking a new subsystem.

Changes

• security_scan: extend persist() to carry commit_sha/commit_at and make re-runs idempotent via ON CONFLICT DO NOTHING / INSERT OR IGNORE.
• history_scan: new HistorySecurityScanner that scans unique git blobs (git rev-list --objects --all,
  deduped by blob SHA) once each and attributes hits to the first-introducing commit. Defaults to the secret-ori
  ented patterns (hardcoded_password/hardcoded_secret) to avoid code-smell noise.
• models + migration 0037: add nullable commit_sha/commit_at and a uq_security_finding_provenance un
  ique constraint for dedup.
• CLI: repowise security scan --history [--since <rev>] [--to <rev>] [--all-patterns] [--output json].
• server: GET /security gains a history filter; responses include commit_sha + found_in_history so t
  he UI can tell history from working tree.
• Addresses maintainer review: idempotent persist, mandatory schema migration, symbol scan disabled in history (
  no parsed symbols on raw blobs), secret-only gate, and blob-dedup scan strategy.

Related Issues

Closes #818

Test Plan

• Tests pass (pytest tests/unit/analysis/test_security_scan_history.py)
• Lint passes (ruff check .)
• Migration chain consistent (0037 → 0036)

Checklist

• My code follows the project's code style
• I have added tests for new functionality
• All existing tests still pass
• I have updated documentation if needed

[repowise-bot]

✅ Health: 7.5 (unchanged)

📋 At a glance
1 file changed health · 4 hotspots touched · 7 new findings introduced · 2 co-change pairs left out · 4 files with recent fix history. Scoped to packages.

📌 Before you merge

• Run tests/integration/test_cli.py, .../cli/test_augment_cmd.py, .../cli/test_claude_md_cmd.py, .../analysis/test_security_scan.py (+2 more): they depend on the changed files
• .../pipeline/persist.py changed together with .../persistence/models.py in 15 past commits and isn't in this PR
• .../persistence/test_models.py changed together with .../persistence/models.py in 14 past commits and isn't in this PR

🗺️ Change map

flowchart LR
  subgraph PR ["Changed in this PR (5 with dependents)"]
    f_packages_cli_src_repowise_cli_main_py[".../cli/main.py 🔥"]:::changed
    f_packages_core_src_repowise_core_analysis_security_scan_py[".../analysis/security_scan.py 🔥"]:::changed
    f_packages_core_src_repowise_core_persistence_models_py[".../persistence/models.py 🔥"]:::changed
    f_packages_server_src_repowise_server_schemas_code_quality_py[".../schemas/code_quality.py 🔥"]:::changed
    f_packages_server_src_repowise_server_routers_security_py[".../routers/security.py"]:::changed
  end
  f_packages_cli_src_repowise_cli_commands_init_cmd_command_py[".../init_cmd/command.py"]
  f_packages_cli_src_repowise_cli_main_py --> f_packages_cli_src_repowise_cli_commands_init_cmd_command_py
  f_packages_cli_src_repowise_cli_helpers_py[".../cli/helpers.py"]
  f_packages_cli_src_repowise_cli_main_py --> f_packages_cli_src_repowise_cli_helpers_py
  f_packages_core_src_repowise_core_pipeline_persist_py[".../pipeline/persist.py"]
  f_packages_core_src_repowise_core_analysis_security_scan_py --> f_packages_core_src_repowise_core_pipeline_persist_py
  f_packages_cli_src_repowise_cli_commands_decision_cmd_py[".../commands/decision_cmd.py"]
  f_packages_core_src_repowise_core_persistence_models_py --> f_packages_cli_src_repowise_cli_commands_decision_cmd_py
  f_packages_cli_src_repowise_cli_commands_delete_cmd_py[".../commands/delete_cmd.py"]
  f_packages_core_src_repowise_core_persistence_models_py --> f_packages_cli_src_repowise_cli_commands_delete_cmd_py
  f_packages_cli_src_repowise_cli_commands_doctor_cmd_repo_checks_py[".../doctor_cmd/repo_checks.py"]
  f_packages_core_src_repowise_core_persistence_models_py --> f_packages_cli_src_repowise_cli_commands_doctor_cmd_repo_checks_py
  f_packages_cli_src_repowise_cli_commands_export_cmd_py[".../commands/export_cmd.py"]
  f_packages_core_src_repowise_core_persistence_models_py --> f_packages_cli_src_repowise_cli_commands_export_cmd_py
  f_packages_server_src_repowise_server_schemas___init___py[".../schemas/__init__.py"]
  f_packages_server_src_repowise_server_schemas_code_quality_py --> f_packages_server_src_repowise_server_schemas___init___py
  f_packages_server_src_repowise_server_app_py[".../server/app.py"]
  f_packages_server_src_repowise_server_routers_security_py --> f_packages_server_src_repowise_server_app_py
  more(["+111 more dependents"])
  PR --> more
  w_packages_core_src_repowise_core_pipeline_persist_py(["⚠️ .../pipeline/persist.py changed together 15×, not in PR"]):::warn
  f_packages_core_src_repowise_core_persistence_models_py -.- w_packages_core_src_repowise_core_pipeline_persist_py
  w_tests_unit_persistence_test_models_py(["⚠️ .../persistence/test_models.py changed together 14×, not in PR"]):::warn
  f_packages_core_src_repowise_core_persistence_models_py -.- w_tests_unit_persistence_test_models_py
  t_tests_integration_test_cli_py(["✅ tests/integration/test_cli.py"]):::guard
  t_tests_integration_test_cli_py -.-> f_packages_cli_src_repowise_cli_main_py
  t_tests_unit_analysis_test_security_scan_py(["✅ .../analysis/test_security_scan.py"]):::guard
  t_tests_unit_analysis_test_security_scan_py -.-> f_packages_core_src_repowise_core_analysis_security_scan_py
  t_tests_integration_persistence_test_embed_batch_pg_py(["✅ .../persistence/test_embed_batch_pg.py"]):::guard
  t_tests_integration_persistence_test_embed_batch_pg_py -.-> f_packages_core_src_repowise_core_persistence_models_py
  classDef changed fill:#dbeafe,stroke:#1d4ed8,color:#1e3a5f
  classDef warn fill:#fef3c7,stroke:#b45309,color:#78350f
  classDef guard fill:#dcfce7,stroke:#15803d,color:#14532d

Loading

_{Solid arrows: code that imports the changed files (120 direct dependents, from the last indexed snapshot). Dashed: history/tests.}

🚨 Change risk: high _{(riskier than 80% of this repo's commits)}
This change's risk is driven by:

• more lines added than baseline
• more scattered than baseline

🩹 Review priority _{(files here with the most recent bug-fix history — defects cluster, so review these first)}

• .../persistence/models.py — fixed 6× in the last ~6 months
• .../cli/main.py — fixed 3× in the last ~6 months
• .../analysis/security_scan.py — fixed once in the last ~6 months
• .../schemas/code_quality.py — fixed once in the last ~6 months

File	Score	Δ	Why
.../analysis/security_scan.py	8.7 → 8.1	▼ -0.6	🔻 introduced low cohesion, primitive obsession, error handling

🔎 More signals (2)

🔥 Hotspots touched (4)

• .../schemas/code_quality.py — 3 commits/90d, 1 dependents · primary owner: Raghav Chamadiya (100%)
• .../analysis/security_scan.py — 1 commits/90d, 2 dependents · primary owner: Raghav Chamadiya (100%)
• .../cli/main.py — 16 commits/90d, 19 dependents · primary owner: Raghav Chamadiya (97%)

1 more

• .../persistence/models.py — 26 commits/90d, 190 dependents · primary owner: Raghav Chamadiya (90%)

🔗 Hidden coupling (1 file)

• .../persistence/models.py co-changes with these files (not in this PR):
  • .../pipeline/persist.py (15× — 🟡 notable)
  • .../persistence/test_models.py (14× — 🟡 notable)

---

📊 Full report · ⭐ Star Repowise · 📥 Install bot · Last updated 2026-07-20 10:45 UTC
_{Silence on a single PR with [skip repowise] in the title · Per-repo toggle on repowise.dev/settings?tab=bot}

[RaghavChamadiya]

Thanks @Ayush7614, and thanks for actually working through the review points on #818 rather than just the headline ask. The shape is right: reusing the pattern registry, the secret-only gate with --all-patterns as the opt-out, SECRET_KINDS, extending persist() rather than forking it. That's the design I was hoping for.

The NULL versus "" trap is handled properly too, which is the thing I expected to go wrong. sha_key = commit_sha or "" means working-tree rows key on "" instead of NULL, so the unique constraint actually dedups them. (The docstrings and the model comment all say the columns are left NULL, which contradicts the code. The code is right, the prose needs fixing.)

That said, this can't merge yet. Three blockers, and the reason none of them showed up as a failing test is worth taking seriously.

The migration can't run on SQLite. op.create_unique_constraint outside batch_alter_table:

NotImplementedError: No support for ALTER of constraints in SQLite dialect.
Please refer to the batch mode feature

SQLite backs every local .repowise database, so as written this hard-crashes on upgrade for every existing user. Needs op.batch_alter_table, which on SQLite means a table rebuild.

The Postgres INSERT is invalid SQL. The conflict clause is concatenated ahead of the column list, so it builds:

INSERT INTO security_findings ON CONFLICT ON CONSTRAINT uq_... DO NOTHING (repository_id, ...) VALUES (...)

ON CONFLICT has to follow VALUES. A Postgres parser rejects it at col 99. Every insert then raises and except Exception: continue eats it, so on Postgres the scan reports success and writes nothing at all. Related: swapping break for continue was the right call for a single bad row, but it makes a systematic failure like this one completely silent. Worth logging the exception rather than dropping it.

Migration 0037 now collides. 0037_repo_git_totals and 0038_agent_trace_line_provenance landed on main after you branched, and both your file and 0037_repo_git_totals declare revision = "0037" / down_revision = "0036". Needs renumbering to 0039 with down_revision = "0038". Not your doing, just needs a rebase.

The blob-dedup speedup is given back in the attribution loop. This is the one real design issue. The unique-blob scan is implemented correctly, and then:

for commit, _iso in commits:
    for blob_sha in self._blobs_in_commit(repo_path, commit):
        blob_introduced_at.setdefault(blob_sha, commit)

That's one git ls-tree -r subprocess per commit, each listing every file, which is exactly the O(commits x files) pass the blob dedup was meant to replace. On this repo (1858 commits, 11408 blobs) I measured ls-tree at about 61ms a call, so the attribution loop is roughly 113 seconds of subprocess spawning before a single blob gets scanned. On a 50k-commit repo it's most of an hour. _read_blob then spawns another git cat-file -p per blob on top.

Both are avoidable in one pass each: git log --all --raw --format=... gives you introducing-commit-per-blob directly, and git cat-file --batch streams contents over a single process.

On the tests. The fixture builds the schema with Base.metadata.create_all and only ever runs SQLite. So the migration never executes in tests, and the Postgres branch has no coverage at all. That's why three blockers sit behind a green suite. Two things would close it: run the alembic migration against a SQLite DB in a test rather than create_all, and cover the Postgres persist() path (at minimum assert the generated SQL parses, ideally exercise it). Everything else in the test file is testing the ORM's idea of the schema, not the one migrations actually produce.

Smaller, while you're in there: files_scanned increments before the _is_source check so it always equals blobs_scanned, and progress only fires when a finding is kept, so a clean repo prints nothing for the whole scan.

Happy to look again once the migration and the Postgres path are sorted. The core of this is good work and I'd like to land it.

[Ayush7614]

Thanks for the detailed review @RaghavChamadiya — I pushed the follow-up fixes and merged upstream/main to clear the conflict.

Addressed points:

• SQLite migration now uses batch_alter_table and is covered by a real Alembic upgrade test against SQLite.
• The Postgres persist() SQL now emits INSERT ... VALUES ... ON CONFLICT ... DO NOTHING in the correct order, and row-level failures are logged instead of being silently swallowed.
• The history attribution loop no longer does ls-tree per commit; it now uses a single git log --reverse --raw pass for blob->introducing-commit attribution plus git cat-file --batch for blob reads.
• Tests now cover the migration path, the Postgres persist() path, and the clean-repo progress/files-scanned cases.

Because upstream/main has since landed 0037 through 0040, I renumbered the migration to 0041 with down_revision = "0040" during the merge.

I also kept the newer working-tree replace_findings() path from main and scoped its delete to working-tree rows only (COALESCE(commit_sha, '') = '') so re-indexing does not wipe history findings.

Validation run locally after the merge resolution:

• tests/unit/analysis/test_security_scan_history.py
• tests/unit/analysis/test_security_scan.py
• ruff check packages/core/src/repowise/core/analysis/security_scan.py packages/core/alembic/versions/0041_security_finding_history.py

Latest push: bf86654