feat: generate privacy-safe local diagnostics bundle (#347) - #361
feat: generate privacy-safe local diagnostics bundle (#347)#361Shiva210Jyoti wants to merge 4 commits into
Conversation
PR Context Summary
Suggested issue links
Use |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
There was a problem hiding this comment.
Pull request overview
Adds a local-only, privacy-redacted diagnostics bundle to support production troubleshooting without telemetry or external uploads. This fits Find’s local-first support posture by providing an explicit, admin-requested JSON export with deny-by-default redaction and documentation.
Changes:
- Introduces a diagnostics collector (
collect_diagnostics_bundle) that gathers runtime/app/service/queue/model status plus recent sanitized errors, then redacts the payload before returning it. - Adds an admin-only (shared mode) FastAPI export route
GET /api/admin/diagnostics/bundlewith attachment/download headers. - Adds an allowlist-first redaction + string scrubbing module with a focused test suite and user-facing documentation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/index.md | Adds docs navigation entry for the diagnostics bundle guide. |
| docs/diagnostics-bundle.md | Documents generation steps and what is included/excluded in the bundle. |
| backend/tests/test_diagnostics_redact.py | Adds redaction/scrubbing tests and a collector “shape + no leakage” test. |
| backend/src/find_api/routers/diagnostics.py | Adds the admin export endpoint and installs the in-process error buffer. |
| backend/src/find_api/main.py | Wires the new diagnostics router into the FastAPI app. |
| backend/src/find_api/diagnostics/redact.py | Implements allowlist-first recursive redaction and string scrubbing. |
| backend/src/find_api/diagnostics/bundle.py | Implements bundle collection (health checks, migrations, queue stats, model state, recent errors) and redaction. |
| backend/src/find_api/diagnostics/init.py | Adds a small package entry point with lazy attribute loading. |
| from alembic.config import Config | ||
| from alembic.runtime.migration import MigrationContext | ||
| from alembic.script import ScriptDirectory | ||
| from sqlalchemy import create_engine | ||
|
|
||
| backend_root = Path(__file__).resolve().parents[3] | ||
| ini_path = backend_root / "alembic.ini" | ||
| if not ini_path.is_file(): | ||
| return { | ||
| "status": "unavailable", | ||
| "current": None, | ||
| "heads": [], | ||
| "detail": "alembic.ini not found", | ||
| } | ||
|
|
||
| cfg = Config(str(ini_path)) | ||
| cfg.set_main_option("script_location", str(backend_root / "alembic")) | ||
| script = ScriptDirectory.from_config(cfg) | ||
| heads = list(script.get_heads()) | ||
|
|
||
| engine = create_engine(settings.DATABASE_URL) | ||
| with engine.connect() as conn: |
| _error_buffer = _ErrorLogBuffer() | ||
| _buffer_installed = False | ||
|
|
||
|
|
||
| def ensure_error_log_buffer() -> None: | ||
| """Attach the in-process error ring buffer to the root logger once.""" | ||
| global _buffer_installed | ||
| if _buffer_installed: | ||
| return | ||
| root = logging.getLogger() | ||
| if not any(isinstance(h, _ErrorLogBuffer) for h in root.handlers): | ||
| root.addHandler(_error_buffer) | ||
| _buffer_installed = True |
| return JSONResponse( | ||
| content=bundle, | ||
| headers={ | ||
| "Content-Disposition": 'attachment; filename="find-diagnostics-bundle.json"', | ||
| "X-Find-Diagnostics": "local-only", |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review Unable to check for correctness in c4e2126. This PR introduces a new diagnostics feature with a new admin endpoint and privacy redaction system. The author does not own any of the changed files (all owned by Abhash-Chakraborty), and there are unresolved review comments identifying connection leaks, race conditions, and a CodeQL security alert about information exposure. You can customize Macroscope's approvability policy. Learn more. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/src/find_api/diagnostics/redact.py (1)
86-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSensitive-term lists are duplicated across four patterns; consider a single source of truth.
_SENSITIVE_KEY_RE,_SENSITIVE_KEY_SUBSTRING_RE,_SECRET_ASSIGN_RE, and_PRIVATE_FIELD_ASSIGN_REeach hardcode overlapping (but not identical) term lists. Adding a new sensitive field later requires remembering to update multiple regexes correctly — easy to miss one and silently under-redact.♻️ Sketch: consolidate shared terms
+_CREDENTIAL_TERMS = ( + "password", "passwd", "secret", "token", "api[_-]?key", "access[_-]?key", + "secret[_-]?key", "authorization", "auth", "credential", "credentials", +) +_PRIVATE_MEDIA_TERMS = ( + "caption", "ocr(?:_text)?", "embedding", "vector", "face(?:s)?", + "person", "people", "filename", "filepath", "file_path", "minio_key", + "user(?:_?id)?", "uploader", "email", "username", +) + _SENSITIVE_KEY_RE = re.compile( - r"(?i)^(password|passwd|secret|token|api[_-]?key|access[_-]?key|" - r"secret[_-]?key|authorization|auth|credential|credentials|" - r"session|cookie|bearer|private[_-]?key|minio_key|thumbnail_key|" - r"filename|filepath|file_path|path|object_name|caption|ocr|" - r"ocr_text|embedding|vector|face|faces|person|people|" - r"user(_?id)?|uploader|email|username|display_name|" - r"database_url|redis_url|remote_ml_url|remote_ml_api_key|" - r"metadata_json|exif_json|file_hash)$" + rf"(?i)^(?:{'|'.join(_CREDENTIAL_TERMS + _PRIVATE_MEDIA_TERMS)}|" + r"session|cookie|bearer|private[_-]?key|thumbnail_key|path|" + r"object_name|display_name|database_url|redis_url|remote_ml_url|" + r"remote_ml_api_key|metadata_json|exif_json|file_hash)$" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/find_api/diagnostics/redact.py` around lines 86 - 147, Consolidate the overlapping sensitive field names used by _SENSITIVE_KEY_RE, _SENSITIVE_KEY_SUBSTRING_RE, _SECRET_ASSIGN_RE, and _PRIVATE_FIELD_ASSIGN_RE into a shared source of truth. Build each pattern from that shared term set while preserving each regex’s existing scope and matching semantics, including terms unique to particular contexts. Update future additions to require changing the centralized definition only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/find_api/diagnostics/bundle.py`:
- Around line 104-120: Update _collect_migration_state to reuse the module’s
shared PostgreSQL engine instead of calling create_engine for each diagnostics
request, preserving its existing migration checks. Update _check_redis to ensure
the Redis client created by Redis.from_url is closed on both success and
failure, using cleanup that also runs when ping raises.
- Around line 36-65: Protect the shared deque in _ErrorLogBuffer with a
threading lock: acquire it while emit() appends the formatted record and while
snapshot() copies the records. Initialize the lock in __init__, and keep
formatting/error handling outside or compatible with the lock so logging
failures still use handleError(record).
In `@backend/src/find_api/routers/diagnostics.py`:
- Around line 29-46: Add a route-level test for export_diagnostics_bundle
covering the /api/admin/diagnostics/bundle endpoint: verify admin
authentication, the Content-Disposition and X-Find-Diagnostics response headers,
and that the response JSON matches the collected diagnostics bundle shape. Reuse
the existing test client and authentication fixtures and mock
collect_diagnostics_bundle where appropriate.
In `@backend/tests/test_diagnostics_redact.py`:
- Around line 16-22: Replace the recognizable JWT header literal used by
SECRETS[3] and test_strips_bearer_tokens with an equally long non-standard fake
bearer token, or add a narrowly scoped scanner suppression for this fixture.
Preserve the test coverage of _BEARER_RE and _TOKEN_RE while ensuring
GitGuardian no longer identifies the seeded value as a real JWT.
---
Nitpick comments:
In `@backend/src/find_api/diagnostics/redact.py`:
- Around line 86-147: Consolidate the overlapping sensitive field names used by
_SENSITIVE_KEY_RE, _SENSITIVE_KEY_SUBSTRING_RE, _SECRET_ASSIGN_RE, and
_PRIVATE_FIELD_ASSIGN_RE into a shared source of truth. Build each pattern from
that shared term set while preserving each regex’s existing scope and matching
semantics, including terms unique to particular contexts. Update future
additions to require changing the centralized definition only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9630883-2545-405d-b8e5-8c1e6082b892
📒 Files selected for processing (8)
backend/src/find_api/diagnostics/__init__.pybackend/src/find_api/diagnostics/bundle.pybackend/src/find_api/diagnostics/redact.pybackend/src/find_api/main.pybackend/src/find_api/routers/diagnostics.pybackend/tests/test_diagnostics_redact.pydocs/diagnostics-bundle.mddocs/index.md
| class _ErrorLogBuffer(logging.Handler): | ||
| """Keep the last N ERROR+ log records in memory for diagnostics.""" | ||
|
|
||
| def __init__(self, capacity: int = ERROR_LOG_LIMIT) -> None: | ||
| super().__init__(level=logging.ERROR) | ||
| self._records: deque[dict[str, Any]] = deque(maxlen=capacity) | ||
|
|
||
| def emit(self, record: logging.LogRecord) -> None: | ||
| try: | ||
| message = self.format(record) if self.formatter else record.getMessage() | ||
| self._records.append( | ||
| { | ||
| "timestamp": datetime.fromtimestamp( | ||
| record.created, tz=timezone.utc | ||
| ).isoformat(), | ||
| "level": record.levelname, | ||
| "logger": record.name, | ||
| "message": scrub_string(message), | ||
| "source": "log", | ||
| } | ||
| ) | ||
| except Exception: # noqa: BLE001 — never break the logging pipeline | ||
| self.handleError(record) | ||
|
|
||
| def snapshot(self) -> list[dict[str, Any]]: | ||
| return list(self._records) | ||
|
|
||
|
|
||
| _error_buffer = _ErrorLogBuffer() | ||
| _buffer_installed = False |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,260p' backend/src/find_api/diagnostics/bundle.pyRepository: Abhash-Chakraborty/Find
Length of output: 8554
🏁 Script executed:
sed -n '260,420p' backend/src/find_api/diagnostics/bundle.pyRepository: Abhash-Chakraborty/Find
Length of output: 4858
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('backend/src/find_api/diagnostics/bundle.py')
text = p.read_text()
for start, end in [(1, 260), (260, 420)]:
print(f"\n--- lines {start}-{end} ---")
for i, line in enumerate(text.splitlines(), 1):
if start <= i <= end:
print(f"{i:4}: {line}")
PYRepository: Abhash-Chakraborty/Find
Length of output: 15695
🏁 Script executed:
python3 - <<'PY'
import threading, logging
from collections import deque
import time
d = deque(maxlen=10)
exc = []
def writer():
for i in range(100000):
d.append(i)
def reader():
try:
for _ in range(100000):
list(d)
except Exception as e:
exc.append(repr(e))
ts = [threading.Thread(target=writer) for _ in range(2)] + [threading.Thread(target=reader) for _ in range(2)]
for t in ts:
t.start()
for t in ts:
t.join()
print(exc[:10])
PYRepository: Abhash-Chakraborty/Find
Length of output: 164
🏁 Script executed:
rg -n "snapshot\(\)|_collect_recent_errors|_error_buffer|deque mutated" backend/src/find_api/diagnostics/bundle.pyRepository: Abhash-Chakraborty/Find
Length of output: 389
Guard _error_buffer with a lock
emit() and snapshot() share a module-level deque across logging threads. Converting it to list() while another thread appends can raise RuntimeError: deque mutated during iteration, and _collect_recent_errors() doesn’t catch that path, so the diagnostics bundle can fail intermittently. Lock both append and snapshot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/find_api/diagnostics/bundle.py` around lines 36 - 65, Protect the
shared deque in _ErrorLogBuffer with a threading lock: acquire it while emit()
appends the formatted record and while snapshot() copies the records. Initialize
the lock in __init__, and keep formatting/error handling outside or compatible
with the lock so logging failures still use handleError(record).
| @router.get("/admin/diagnostics/bundle") | ||
| def export_diagnostics_bundle( | ||
| _admin: Optional[User] = Depends(get_admin_user), | ||
| ): | ||
| """Return a privacy-safe local diagnostics bundle as JSON. | ||
|
|
||
| Admin-only in shared mode (open in local mode), matching other | ||
| instance-wide admin endpoints. Requires an explicit HTTP request — | ||
| no background telemetry or outbound upload is performed. | ||
| """ | ||
| bundle = collect_diagnostics_bundle() | ||
| return JSONResponse( | ||
| content=bundle, | ||
| headers={ | ||
| "Content-Disposition": 'attachment; filename="find-diagnostics-bundle.json"', | ||
| "X-Find-Diagnostics": "local-only", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "diagnostics/bundle|export_diagnostics_bundle" backend/testsRepository: Abhash-Chakraborty/Find
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## route references\n'
rg -n "diagnostics/bundle|export_diagnostics_bundle|collect_diagnostics_bundle|X-Find-Diagnostics|find-diagnostics-bundle" backend
printf '\n## diagnostics-related tests\n'
rg -n "diagnostic|redact|bundle|admin/diagnostics" backend/tests
printf '\n## test file inventory\n'
git ls-files backend/testsRepository: Abhash-Chakraborty/Find
Length of output: 7220
Add a route-level test for /api/admin/diagnostics/bundle. backend/tests/test_diagnostics_redact.py exercises collect_diagnostics_bundle(), but the endpoint still needs coverage for admin auth, response headers, and the returned JSON shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/find_api/routers/diagnostics.py` around lines 29 - 46, Add a
route-level test for export_diagnostics_bundle covering the
/api/admin/diagnostics/bundle endpoint: verify admin authentication, the
Content-Disposition and X-Find-Diagnostics response headers, and that the
response JSON matches the collected diagnostics bundle shape. Reuse the existing
test client and authentication fixtures and mock collect_diagnostics_bundle
where appropriate.
Source: Path instructions
…raborty#361 - Fix _URL_CREDS_RE to handle password-only URLs (redis://:pass@host) - Rename sensitive dict keys to redacted_key instead of preserving name - Scrub free-standing quoted strings via _QUOTED_CONTENT_RE - Add lock to snapshot() for thread safety - Extend filename scrub to any word.ext pattern - Local storage check uses is_dir() + os.access W_OK - Reuse global engine in _collect_migration_state - Clarify docs: hostnames may appear, only URL credentials stripped - Router returns scrubbed 500 JSON, no stack traces exposed - 508 tests passing
…rty#347) - Add diagnostics/bundle.py to collect version, runtime, Alembic state, service health, queue stats, ML config, sanitized errors - Add diagnostics/redact.py with allowlist-first recursive redaction - Add GET /api/admin/diagnostics/bundle admin-only endpoint - Add 24 passing redaction tests - Add docs/diagnostics-bundle.md - Wire router in main.py, link from docs/index.md - Local JSON only, no uploads or telemetry fix: address Copilot review and clear GitGuardian secrets in tests - Use existing global engine in _collect_migration_state to avoid pool leaks - Dedupe error log buffer handlers by name for uvicorn reload safety - Add API tests covering headers, schema_version, auth (28 tests passing) - Replace realistic-looking test fixtures with obvious placeholders fix: address all review comments on diagnostics bundle PR Abhash-Chakraborty#361 - Fix _URL_CREDS_RE to handle password-only URLs (redis://:pass@host) - Rename sensitive dict keys to redacted_key instead of preserving name - Scrub free-standing quoted strings via _QUOTED_CONTENT_RE - Add lock to snapshot() for thread safety - Extend filename scrub to any word.ext pattern - Local storage check uses is_dir() + os.access W_OK - Reuse global engine in _collect_migration_state - Clarify docs: hostnames may appear, only URL credentials stripped - Router returns scrubbed 500 JSON, no stack traces exposed - 508 tests passing fix: replace scanner-facing test fixtures to clear GitGuardian - Replace JWT-shaped bearer token with FAKE.TEST.TOKEN - Replace sk-live API key with split sk-test-FAKE-KEY-FOR-TESTING-ONLY - Replace passwords with EXAMPLE_PASSWORD_PLACEHOLDER - 26 tests passing
| ) | ||
|
|
||
| return JSONResponse( | ||
| content=bundle, |
|
Please fix the CI. |
bundle.py: guard error log buffer with threading.Lock in emit()/snapshot() to prevent deque mutation under concurrent logging. redact.py: generalize filename scrubbing regex to catch non-allowlisted extensions (.txt, .csv) and dotfiles (.env, .gitignore) while avoiding false positives on version strings. routers/diagnostics.py: stop leaking exception class/message to clients on failure; log full exception server-side via logger.exception() and return a generic error message. test_diagnostics_redact.py and test_diagnostics_api.py: add coverage for new redaction cases and the diagnostics endpoint (headers, payload shape, secret leakage, generic 500, shared-mode auth). Addresses review feedback from CodeRabbit, Macroscope, GitHub Copilot, and CodeQL on PR Abhash-Chakraborty#361.
bundle.py: ensure_error_log_buffer now removes any stale same-named handler before attaching the current _error_buffer instance, fixing silent error-capture loss after uvicorn --reload. redact.py: fix _TOKEN_RE boundary so tokens ending in '-' are correctly redacted (previously leaked past the \b word boundary since '-' is a non-word character). Adds test_strips_long_token_ending_in_hyphen regression test. Full suite: 561 passed, 5 skipped. Diagnostics: 34 passed. Addresses Macroscope findings on PR Abhash-Chakraborty#361.
|
Really nice work — the allowlist-first redaction with per-string scrubbing on top is the right shape, and the 24-test redaction suite is what makes it trustworthy. Backend checks are green. One blocker: this PR targets Two smaller notes while you're in there:
Retarget to |
Abhash-Chakraborty
added
under-review
|
@macroscope-app review Please review this PR against its linked issue, local-first privacy rules, and the current Find repo instructions. |
|
Macroscope skipped reviewing this pull request. Per-PR cost limit exceeded (workspace setting). Reviews on this PR have cost $4.74 so far. This review would add an estimated $0.50, bringing the total to $5.24 — above your per-PR limit of $5.00. Tip To get this pull request reviewed, you can:
|
Resolves #347
Summary
Adds a privacy-safe local diagnostics bundle for production support without telemetry or external uploads.
Files Created
backend/src/find_api/diagnostics/bundle.py— collector (version, runtime, Alembic state, PG/Redis/MinIO health, queue stats, ML config, last 20 sanitized errors)backend/src/find_api/diagnostics/redact.py— allowlist-first recursive redaction + string scrubbingbackend/src/find_api/diagnostics/__init__.pybackend/src/find_api/routers/diagnostics.py—GET /api/admin/diagnostics/bundleadmin-only endpointbackend/tests/test_diagnostics_redact.py— 24 tests, all passingdocs/diagnostics-bundle.md— full include/exclude documentationmain.py, linked fromdocs/index.mdAcceptance Criteria
Notes
X-Find-Diagnostics: local-onlyheader + download dispositionML_MODEonlySummary by CodeRabbit
New Features
Documentation
Tests