feat: generate privacy-safe local diagnostics bundle - #428
Merged
Abhash-Chakraborty merged 6 commits intoAug 3, 2026
Merged
Conversation
…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
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.
- Move ensure_error_log_buffer() init from import-time side effect to app lifespan/startup in main.py, avoiding duplicate handler registration under uvicorn --reload - Add explicit timeouts to PG/Redis/MinIO health probes in collect_diagnostics_bundle(); hangs now report ok: false instead of pinning a threadpool worker - Document intentional over-redaction of substring matches (interface, pgvector, etc.) in _SENSITIVE_KEY_SUBSTRING_RE - Add admin-only / local-only callout to top of docs/diagnostics-bundle.md
PR Context Summary
Suggested issue links
Use |
Contributor
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a local diagnostics bundle with bounded health checks, runtime and application state, sanitized errors, recursive redaction, admin-only download access, startup initialization, tests, and documentation. ChangesDiagnostics bundle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Admin as Admin client
participant Router as diagnostics router
participant Collector as collect_diagnostics_bundle
participant Redactor as redact_payload
Admin->>Router: GET /api/admin/diagnostics/bundle
Router->>Collector: collect local diagnostics
Collector->>Redactor: redact assembled payload
Redactor-->>Collector: sanitized bundle
Collector-->>Router: bundle JSON
Router-->>Admin: downloadable JSON response
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
Contributor
ApprovabilityVerdict: Needs human review Unable to check for correctness in 6f65b2a. New admin diagnostics endpoint feature with ~1700 lines of new code. Multiple unresolved security concerns (credential leakage via regex pattern, unauthenticated access when bound to 0.0.0.0, missing cache headers) and correctness issues flagged by reviewers. Author does not own any of the changed files. You can customize Macroscope's approvability policy. Learn more. |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements a privacy-safe, local-only diagnostics export to support production troubleshooting without telemetry or external uploads, aligning with Find’s local-first posture and Issue #347 acceptance criteria.
Changes:
- Adds a new admin-only endpoint (
GET /api/admin/diagnostics/bundle) that returns a redacted JSON diagnostics bundle as an attachment. - Implements allowlist-first redaction + string scrubbing, plus bundle collectors for migrations, services health, queue stats, model state, and recent errors.
- Adds documentation and test coverage for redaction guarantees and endpoint auth/response behavior.
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 |
|---|---|
| docs/index.md | Links the new diagnostics bundle guide from the docs index. |
| docs/diagnostics-bundle.md | Documents how to generate the bundle plus exact include/exclude and privacy guarantees. |
| backend/src/find_api/diagnostics/init.py | Exposes diagnostics helpers via a small, lazy-import package interface. |
| backend/src/find_api/diagnostics/redact.py | Adds allowlist-first recursive redaction and string scrubbing patterns. |
| backend/src/find_api/diagnostics/bundle.py | Implements bundle collection (health checks, migration/queue/model state) and in-process recent error capture. |
| backend/src/find_api/routers/diagnostics.py | Adds the admin-only download route with local-only headers and sanitized failure response. |
| backend/src/find_api/main.py | Wires the diagnostics router and installs the error buffer during app lifespan startup. |
| backend/tests/test_diagnostics_redact.py | Adds redaction unit tests ensuring seeded secrets/private strings do not leak. |
| backend/tests/test_diagnostics_api.py | Adds route-level tests for headers/schema, auth behavior, and leak prevention (including failure path). |
Suppressed comments (2)
backend/src/find_api/diagnostics/bundle.py:107
_run_with_timeout()uses aThreadPoolExecutorcontext manager; after a timeout, thewithblock will still wait for the worker thread to finish during executor shutdown. If the probe is blocked in a non-interruptible call, this can hang the request despite raisingTimeoutError. Create/shutdown the executor manually withshutdown(wait=False, cancel_futures=True)so the caller can return immediately on timeout.
with ThreadPoolExecutor(max_workers=1) as pool:
future = pool.submit(fn)
try:
return future.result(timeout=timeout_s)
except FuturesTimeout as exc:
backend/src/find_api/diagnostics/bundle.py:305
- In Redis queue mode,
_collect_queue_stats()callsget_redis_connection(), which ultimately usesRedis.from_url(settings.REDIS_URL)without socket timeouts (seecore/queue.py). That means diagnostics bundle generation can hang indefinitely if Redis is unreachable, even though_check_redis()uses explicit timeouts. Use a dedicated Redis client with connect/read timeouts here to keep the bundle export reliably bounded.
from rq import Queue
from rq.registry import FailedJobRegistry, StartedJobRegistry
from find_api.core.queue import get_redis_connection
conn = get_redis_connection()
queue_names = ("high", "default", "low")
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
backend/src/find_api/main.py (1)
72-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider installing the buffer before
init_db().
init_db()runs at Line 70. AnyERRORrecord that it emits is lost because the buffer is not attached yet. Database initialization failures are exactly the class of error a support bundle should show. Move theensure_error_log_buffer()call above the database initialization.♻️ Proposed reordering
+ # Install diagnostics error buffer once at boot (not at import time). + ensure_error_log_buffer() + # Initialize database logger.info("Initializing database...") init_db() - # Install diagnostics error buffer once at boot (not at import time). - ensure_error_log_buffer() - # Initialize configured storage backend🤖 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/main.py` around lines 72 - 73, Move the ensure_error_log_buffer() call before init_db() in the boot sequence so database initialization errors are captured, while preserving the existing one-time installation behavior.backend/src/find_api/diagnostics/bundle.py (2)
281-285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not import the private
_get_backendsymbol.
_get_backendis private tofind_api.core.queue. The diagnostics module depends on an internal detail that can change without notice. Add a public accessor, for examplecount_jobs_by_status(), infind_api/core/queue.pyand call it here. That also keeps queue logic in the queue module.As per coding guidelines: "Keep FastAPI routers thin and place storage, queue, database, and ML logic in existing backend modules."
🤖 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 281 - 285, Replace the private _get_backend import in the sqlite diagnostics path with a public count_jobs_by_status() accessor added to find_api.core.queue. Move the count_by_status delegation into that queue-level function, then call it from the diagnostics code so queue and storage logic remains encapsulated in the queue module.Source: Coding guidelines
255-262: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueClarify the redundant
statuscondition.Line 257 tests
current in heads or (current is not None and set(heads) <= {current}). Ifheadsis empty andcurrentis set, the second clause reportsok. That case means the script directory has no revisions while the database records one, which is not anokstate. Simplify the condition and report that case separately.🤖 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 255 - 262, Update the status classification logic around the current and heads variables to handle an empty heads set with a non-None current as a separate non-ok state. Remove the redundant set(heads) <= {current} condition from the ok branch, preserve the existing empty and unmigrated cases, and assign the appropriate behind status for a database revision with no script revisions.backend/src/find_api/diagnostics/redact.py (1)
197-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMultiple sensitive keys in one dict collapse into a single entry.
Every sensitive key writes to the same
REDACTED_KEYslot. If a dict containspasswordandtoken, the output shows oneredacted_keyentry. The reader then cannot tell how many fields were removed. Use a counter or a list to keep the cardinality without leaking names.♻️ Optional: keep the redacted-key count
if isinstance(data, dict): out: dict[str, Any] = {} + redacted = 0 for key, value in data.items(): key_str = str(key) if _is_sensitive_key(key_str): - out[REDACTED_KEY] = REDACTED + redacted += 1 + out[f"{REDACTED_KEY}_{redacted}"] = REDACTED continue🤖 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 197 - 208, Update the sensitive-key branch in redact_payload so multiple sensitive fields preserve their cardinality instead of overwriting the same REDACTED_KEY entry. Use a counter or list-based representation for redacted entries while keeping sensitive key names hidden and leaving non-sensitive key handling unchanged.
🤖 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 251-253: Update the migration probe around
MigrationContext.configure and get_current_revision to execute the
engine.connect and revision lookup through the module’s existing
_run_with_timeout helper. Preserve the current revision result and connection
context behavior while applying the same timeout handling used by the other
dependency probes.
- Around line 99-111: Update _run_with_timeout to create ThreadPoolExecutor
without a with statement, and ensure timeout handling calls shutdown(wait=False)
so executor cleanup does not wait for an already-running probe. Preserve future
cancellation and TimeoutError propagation, while allowing the worker thread to
finish independently after the request returns.
- Around line 299-330: Wrap the entire RQ collection block, including
get_redis_connection(), queue construction, length calculations, registry
lookups, and registry-property accesses, in _run_with_timeout. Preserve the
existing per-registry exception handling and return the collected counters
through the timeout wrapper so every Redis call is bounded like the _check_redis
probe.
- Around line 405-425: Normalize database timestamps to UTC-aware datetimes
before formatting them in the media-analysis entries loop, using the timestamp
selected by updated_at or created_at. Ensure both naive and aware values are
converted to a consistent UTC representation before calling isoformat(), so
_sort_key compares uniformly formatted timestamps.
In `@backend/src/find_api/diagnostics/redact.py`:
- Around line 106-111: Update the key-sensitivity flow in redact_payload and its
helper _is_sensitive_key so allowlisted keys are accepted before applying the
substring heuristic. Preserve exact-match protection from _SENSITIVE_KEY_RE for
sensitive keys such as embedding, while allowing embedding_dim to remain
available as the non-sensitive configured model dimension.
- Around line 174-184: Update scrub_string to preserve YOLO model identifiers
such as yolo1.pt and the default yolo26n.pt from _FILENAME_RE redaction. Add the
smallest targeted exemption or known-model preserve list for string-only
yolo_model values, while retaining filename scrubbing for other values.
In `@backend/src/find_api/routers/diagnostics.py`:
- Around line 24-27: Add "Cache-Control: no-store" to the _BUNDLE_HEADERS
dictionary used by the diagnostics bundle response, preserving the existing
headers and ensuring clients and intermediaries do not cache the bundle.
- Around line 45-49: Update the 500 failure response in the diagnostics endpoint
to send only the non-caching headers, excluding the attachment-related header
from _BUNDLE_HEADERS. First confirm the endpoint tests assert failure-response
headers, then update or add coverage to preserve the intended header behavior.
- Around line 31-34: Update export_diagnostics_bundle to reject requests unless
they originate from the loopback interface 127.0.0.1, rather than relying solely
on the optional _admin dependency. Preserve authenticated admin access only when
the request is local, and document that the diagnostics bundle endpoint is
restricted to local loopback requests.
In `@backend/tests/test_diagnostics_redact.py`:
- Around line 50-71: Update _assert_no_leakage in
backend/tests/test_diagnostics_redact.py (lines 50-71) and the corresponding
helper in backend/tests/test_diagnostics_api.py (lines 58-69) to traverse
payload string leaves directly instead of searching a json.dumps-escaped blob;
apply this raw-string traversal to the SECRETS, PRIVATE_STRINGS, and fragment
checks so Windows paths with backslashes are detected. Both sites require the
same change.
---
Nitpick comments:
In `@backend/src/find_api/diagnostics/bundle.py`:
- Around line 281-285: Replace the private _get_backend import in the sqlite
diagnostics path with a public count_jobs_by_status() accessor added to
find_api.core.queue. Move the count_by_status delegation into that queue-level
function, then call it from the diagnostics code so queue and storage logic
remains encapsulated in the queue module.
- Around line 255-262: Update the status classification logic around the current
and heads variables to handle an empty heads set with a non-None current as a
separate non-ok state. Remove the redundant set(heads) <= {current} condition
from the ok branch, preserve the existing empty and unmigrated cases, and assign
the appropriate behind status for a database revision with no script revisions.
In `@backend/src/find_api/diagnostics/redact.py`:
- Around line 197-208: Update the sensitive-key branch in redact_payload so
multiple sensitive fields preserve their cardinality instead of overwriting the
same REDACTED_KEY entry. Use a counter or list-based representation for redacted
entries while keeping sensitive key names hidden and leaving non-sensitive key
handling unchanged.
In `@backend/src/find_api/main.py`:
- Around line 72-73: Move the ensure_error_log_buffer() call before init_db() in
the boot sequence so database initialization errors are captured, while
preserving the existing one-time installation behavior.
🪄 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 Plus
Run ID: a2b12ee6-7b24-4274-9e5a-9d359dc8cba0
📒 Files selected for processing (9)
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_api.pybackend/tests/test_diagnostics_redact.pydocs/diagnostics-bundle.mddocs/index.md
…ul fields Three bugs found by running the collector rather than reading it. All three passed the existing suite, so each fix ships with a test that fails without it. 1. The health-probe timeout never bounded anything. `_run_with_timeout` used `with ThreadPoolExecutor(...)`, and `Executor.__exit__` calls `shutdown(wait=True)` — so on a hung probe it raised TimeoutError and then blocked until the probe finished anyway. Measured: a 10s hang with a 1s timeout returned after 10.00s. Now shuts down with wait=False and abandons the thread, which is safe because every probe already sets its own socket timeout. Same hang now returns in 1.01s, and a full collection against a fully-down stack went 18.3s -> 8.6s. 2. `embedding_dim` was being destroyed. It is explicitly on ALLOWED_KEYS, but the substring deny-net matched "embedding" inside it and replaced the whole entry with `redacted_key: [REDACTED]`. The substring heuristic is a net for keys nobody vetted, so it no longer overrides curated allowlist entries; exact-match sensitive names still win over the allowlist. 3. Model names came out as `<filename>`. `yolo26n.pt` is filename-shaped, so the generic filename pattern collapsed `yolo_model` and every entry of `configured_models` — which is most of what the models section exists to report. Model-identifier keys are now exempt from filename scrubbing only; path, credential, token, and private-field scrubbing still apply to them, and the exemption is per-key so siblings are unaffected. Also: several sensitive keys in one dict all wrote to the same `redacted_key` entry, so all but the last silently vanished — they now get distinct placeholders. And the migration-state and failed-media queries open real DB connections but had no bound, so they are routed through the same timeout as the health probes. Tests: +12 covering each fix, including guards proving the two redaction relaxations do not leak (credentials and absolute paths under an exempt key are still scrubbed, non-allowlisted lookalikes are still denied), plus a real collector-through-endpoint test asserting strict JSON serialisation — every other endpoint test patches the collector out. Full backend suite: 665 passed, 6 skipped. ruff check and format clean.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
backend/src/find_api/diagnostics/bundle.py:85
ensure_error_log_buffer()is not thread-safe: multiple concurrent calls can pass the_buffer_installedcheck and add the same handler multiple times, causing duplicate log capture. Guard installation/removal with a module-level lock and double-check_buffer_installedinside the lock (optionally closing removed stale handlers).
if _buffer_installed:
backend/src/find_api/diagnostics/bundle.py:112
- The timeout error message rounds
timeout_sto 0 decimal places ({timeout_s:.0f}), which produces misleading messages for sub-second timeouts (e.g.,timeout_s=0.2becomes "after 0s"). Use a format that preserves fractions.
raise TimeoutError(
f"health probe timed out after {timeout_s:.0f}s"
) from exc
Abhash-Chakraborty
added
backend
Abhash-Chakraborty
added
quality:exceptional
Abhash-Chakraborty
approved these changes
Aug 3, 2026
Abhash-Chakraborty
left a comment
Abhash-Chakraborty
left a comment
Owner
There was a problem hiding this comment.
Ran the collector rather than just reading it and found three bugs the suite missed — a probe timeout that never actually timed out, embedding_dim being eaten by the substring deny-net, and model names like yolo26n.pt collapsing to <filename>. Pushed fixes with regression tests for each; 665 passed, ruff clean, and the redaction guarantees are unchanged. Nice work on the allowlist-first design — approving.
|
@macroscope-app review Please review this PR against its linked issue, local-first privacy rules, and the current Find repo instructions. |
Abhash-Chakraborty
merged commit Aug 3, 2026
e95bd53
into
Abhash-Chakraborty:canary
39 of 41 checks passed
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
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.
Fixes #347
Supersedes #361 (originally opened against
main, which had diverged too far fromcanaryfor a clean retarget — recreated via cherry-pick per review feedback).All review comments from #361 addressed:
See #361 for full review history and discussion.
Summary by CodeRabbit
New Features
Documentation
Tests