This repository was archived by the owner on May 31, 2026. It is now read-only.
fix: health_integration refactor#66
Closed
KooshaPari wants to merge 1 commit into
Closed
Conversation
Co-Authored-By: Claude Opus 4.7 <[email protected]>
|
Skipping CodeAnt AI review — this PR changes more than 100 files, which usually means a migration, codemod, or vendored drop. Line-level review on diffs this large produces duplicate findings on the same rewrite pattern and drowns out anything that actually matters. If you still want a review, comment |
|
Important Review skippedToo many files! This PR contains 299 files, which is 149 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (299)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Owner
Author
|
Accidentally committed target/ build artifacts — redoing |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
Reviewed by Cursor Bugbot for commit fb5bc0f. Configure here.
| @@ -0,0 +1 @@ | |||
| {"rustc_fingerprint":8767480097696730991,"outputs":{"16573520171876592124":{"success":true,"status":"","code":0,"stdout":"___.exe\nlib___.rlib\n___.dll\n___.dll\n___.lib\n___.dll\nC:\\Users\\koosh\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\npacked\n___\ndebug_assertions\npanic=\"unwind\"\nproc_macro\ntarget_abi=\"\"\ntarget_arch=\"x86_64\"\ntarget_endian=\"little\"\ntarget_env=\"msvc\"\ntarget_family=\"windows\"\ntarget_feature=\"cmpxchg16b\"\ntarget_feature=\"fxsr\"\ntarget_feature=\"sse\"\ntarget_feature=\"sse2\"\ntarget_feature=\"sse3\"\ntarget_has_atomic=\"128\"\ntarget_has_atomic=\"16\"\ntarget_has_atomic=\"32\"\ntarget_has_atomic=\"64\"\ntarget_has_atomic=\"8\"\ntarget_has_atomic=\"ptr\"\ntarget_os=\"windows\"\ntarget_pointer_width=\"64\"\ntarget_vendor=\"pc\"\nwindows\n","stderr":""},"351583688373434005":{"success":true,"status":"","code":0,"stdout":"rustc 1.95.0 (59807616e 2026-04-14)\nbinary: rustc\ncommit-hash: 59807616e1fa2540724bfbac14d7976d7e4a3860\ncommit-date: 2026-04-14\nhost: x86_64-pc-windows-msvc\nrelease: 1.95.0\nLLVM version: 22.1.2\n","stderr":""}},"successes":{}} No newline at end of file | |||
There was a problem hiding this comment.
Build artifacts directory committed to repository
High Severity
The entire rust/target/ directory (Cargo's build output) is being committed to the repository. This includes .rustc_info.json, CACHEDIR.TAG, and hundreds of .fingerprint files. These are machine-generated build artifacts that bloat the repo, cause merge conflicts, and leak local environment details (e.g., C:\Users\koosh\.rustup\... paths). A .gitignore entry for target/ is missing or not effective.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fb5bc0f. Configure here.
| &self, | ||
| project_paths: &[impl AsRef<std::path::Path> + Send + Sync + Clone], | ||
| ) -> HealthRegistry { | ||
| let registry = HealthRegistry::new(); |
There was a problem hiding this comment.
Dead ComplianceHealthCheck struct with unused fields
Low Severity
ComplianceHealthCheck is defined with scanner and project_path fields and a pub fn new() constructor, but it no longer implements any trait or has any methods that use those fields. It's structurally identical to the new ComplianceHealthChecker but without the HealthChecker impl. This appears to be leftover code from the refactor that now serves no purpose.
Reviewed by Cursor Bugbot for commit fb5bc0f. Configure here.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the compliance scanner's health integration to align with the updated phenotype-health library, replacing the old HealthCheck and HealthRegistry with HealthChecker and HealthResponse. Additionally, Cargo build artifacts under the rust/target/ directory were accidentally committed. Feedback focuses on removing the redundant ComplianceHealthCheck struct, parallelizing the sequential project scans to improve performance, and untracking the committed build artifacts.
Comment on lines
+105
to
123
| /// Health check based on compliance scan results | ||
| #[derive(Debug)] | ||
| pub struct ComplianceHealthCheck { | ||
| scanner: Arc<crate::ComplianceScanner>, | ||
| project_path: std::path::PathBuf, | ||
| } | ||
|
|
||
| impl ComplianceHealthCheck { | ||
| /// Create a new compliance health check | ||
| pub fn new( | ||
| scanner: Arc<crate::ComplianceScanner>, | ||
| project_path: impl AsRef<std::path::Path>, | ||
| ) -> Self { | ||
| Self { | ||
| scanner, | ||
| project_path: project_path.as_ref().to_path_buf(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The ComplianceHealthCheck struct and its implementation are redundant and unused. It appears that ComplianceHealthChecker was introduced to replace it, but the old struct was left behind. Removing this dead code will improve maintainability and keep the codebase clean.
Comment on lines
178
to
194
| for path in project_paths { | ||
| if let Ok(report) = self.check_project(path).await { | ||
| // Create a component health check based on the report | ||
| let status = report.overall_status; | ||
| overall = overall.worse(report.overall_status); | ||
| let component_name = path | ||
| .as_ref() | ||
| .file_name() | ||
| .and_then(|n| n.to_str()) | ||
| .unwrap_or("unknown") | ||
| .to_string(); | ||
|
|
||
| debug!("Creating health check for {}: {:?}", component_name, status); | ||
| let _check = ComponentHealthCheck::new(component_name, status); | ||
| // Note: We'd need to register this, but HealthRegistry::register is async | ||
| // For now, just log it | ||
| debug!( | ||
| "Creating health response entry for {}: {:?}", | ||
| component_name, report.overall_status | ||
| ); | ||
| checks.extend(report.checks.iter().map(snapshot_to_check_result)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Scanning multiple projects sequentially using .await in a loop can be a significant performance bottleneck, especially when there are many projects or when the scans take some time. Running these scans in parallel using futures_util::future::join_all will greatly improve efficiency.
let futures = project_paths.iter().cloned().map(|path| async move {
self.check_project(path).await.ok().map(|report| (path, report))
});
let results = futures_util::future::join_all(futures).await;
for (path, report) in results.into_iter().flatten() {
overall = overall.worse(report.overall_status);
let component_name = path
.as_ref()
.file_name()
.and_then(|n| n.to_str())
.unwrap_or("unknown")
.to_string();
debug!(
"Creating health response entry for {}: {:?}",
component_name, report.overall_status
);
checks.extend(report.checks.iter().map(snapshot_to_check_result));
}| @@ -0,0 +1 @@ | |||
| {"rustc_fingerprint":8767480097696730991,"outputs":{"16573520171876592124":{"success":true,"status":"","code":0,"stdout":"___.exe\nlib___.rlib\n___.dll\n___.dll\n___.lib\n___.dll\nC:\\Users\\koosh\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\npacked\n___\ndebug_assertions\npanic=\"unwind\"\nproc_macro\ntarget_abi=\"\"\ntarget_arch=\"x86_64\"\ntarget_endian=\"little\"\ntarget_env=\"msvc\"\ntarget_family=\"windows\"\ntarget_feature=\"cmpxchg16b\"\ntarget_feature=\"fxsr\"\ntarget_feature=\"sse\"\ntarget_feature=\"sse2\"\ntarget_feature=\"sse3\"\ntarget_has_atomic=\"128\"\ntarget_has_atomic=\"16\"\ntarget_has_atomic=\"32\"\ntarget_has_atomic=\"64\"\ntarget_has_atomic=\"8\"\ntarget_has_atomic=\"ptr\"\ntarget_os=\"windows\"\ntarget_pointer_width=\"64\"\ntarget_vendor=\"pc\"\nwindows\n","stderr":""},"351583688373434005":{"success":true,"status":"","code":0,"stdout":"rustc 1.95.0 (59807616e 2026-04-14)\nbinary: rustc\ncommit-hash: 59807616e1fa2540724bfbac14d7976d7e4a3860\ncommit-date: 2026-04-14\nhost: x86_64-pc-windows-msvc\nrelease: 1.95.0\nLLVM version: 22.1.2\n","stderr":""}},"successes":{}} No newline at end of file | |||
There was a problem hiding this comment.
The target/ directory contains build artifacts generated by Cargo. These files should not be committed to SCM. Please remove the target/ directory from git tracking and add it to your .gitignore file.
|
🤖 Augment PR SummarySummary: Refactors the Rust compliance scanner health integration to work with the current Changes:
Technical Notes: The health HTTP payload shape appears to shift from a registry model to a concrete 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| @@ -0,0 +1 @@ | |||
| {"rustc_fingerprint":8767480097696730991,"outputs":{"16573520171876592124":{"success":true,"status":"","code":0,"stdout":"___.exe\nlib___.rlib\n___.dll\n___.dll\n___.lib\n___.dll\nC:\\Users\\koosh\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\npacked\n___\ndebug_assertions\npanic=\"unwind\"\nproc_macro\ntarget_abi=\"\"\ntarget_arch=\"x86_64\"\ntarget_endian=\"little\"\ntarget_env=\"msvc\"\ntarget_family=\"windows\"\ntarget_feature=\"cmpxchg16b\"\ntarget_feature=\"fxsr\"\ntarget_feature=\"sse\"\ntarget_feature=\"sse2\"\ntarget_feature=\"sse3\"\ntarget_has_atomic=\"128\"\ntarget_has_atomic=\"16\"\ntarget_has_atomic=\"32\"\ntarget_has_atomic=\"64\"\ntarget_has_atomic=\"8\"\ntarget_has_atomic=\"ptr\"\ntarget_os=\"windows\"\ntarget_pointer_width=\"64\"\ntarget_vendor=\"pc\"\nwindows\n","stderr":""},"351583688373434005":{"success":true,"status":"","code":0,"stdout":"rustc 1.95.0 (59807616e 2026-04-14)\nbinary: rustc\ncommit-hash: 59807616e1fa2540724bfbac14d7976d7e4a3860\ncommit-date: 2026-04-14\nhost: x86_64-pc-windows-msvc\nrelease: 1.95.0\nLLVM version: 22.1.2\n","stderr":""}},"successes":{}} No newline at end of file | |||
There was a problem hiding this comment.
rust/target/.rustc_info.json:1 — This looks like Cargo build output under rust/target/** and is machine-specific; committing it will cause noisy diffs and non-reproducible changes. Consider removing rust/target from the repo and adding it to .gitignore.
Severity: high
Other Locations
rust/target/CACHEDIR.TAG:1rust/target/debug/.cargo-lock:1rust/target/debug/incremental/phenotype_testing-09k6i6jmfxc55/s-hixhubs6eu-0jb9h6n.lock:1
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| @@ -0,0 +1 @@ | |||
| C:\Users\koosh\Dev\TestingKit\rust\target\debug\libphenotype_compliance_scanner.rlib: C:\Users\koosh\Dev\TestingKit\rust\phenotype-compliance-scanner\src\async_scan.rs C:\Users\koosh\Dev\TestingKit\rust\phenotype-compliance-scanner\src\lib.rs | |||
There was a problem hiding this comment.
rust/target/debug/libphenotype_compliance_scanner.d:1 — This dependency file contains absolute local Windows paths, which are not portable and can leak developer-specific paths. These .d artifacts typically belong in target/ and shouldn’t be committed.
Severity: medium
Other Locations
rust/target/debug/libphenotype_mock.d:1rust/target/debug/libphenotype_test_infra.d:1rust/target/debug/libphenotype_testing.d:1
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| let mut overall = HealthStatus::Healthy; | ||
|
|
||
| for path in project_paths { | ||
| if let Ok(report) = self.check_project(path).await { |
There was a problem hiding this comment.
rust/phenotype-compliance-scanner/src/health_integration.rs:179 — In build_health_registry, failures from check_project() are silently ignored, which can yield an overall Healthy response even when scans are failing. Consider how you want errors to affect overall and whether failed projects should produce an explicit check entry.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| "Creating health response entry for {}: {:?}", | ||
| component_name, report.overall_status | ||
| ); | ||
| checks.extend(report.checks.iter().map(snapshot_to_check_result)); |
There was a problem hiding this comment.
rust/phenotype-compliance-scanner/src/health_integration.rs:192 — HealthResponse.checks entries are derived only from HealthSnapshot.component (rule/message), while the computed component_name (project name) is only logged. If consumers expect per-project checks, consider whether the check service should include the project identifier to avoid ambiguous names.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)These issues were noted but cannot receive inline comments as they are in unchanged code:
Files Reviewed (4 files)
Reviewed by laguna-m.1-20260312:free · 1,393,586 tokens |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Refactor health integration test.
Note
Medium Risk
Health monitoring behavior changes (errors become unhealthy status; HTTP payload shape shifts from registry to HealthResponse), which can affect operators and any code still expecting the old HealthCheck trait on ComplianceHealthCheck.
Overview
Updates compliance scanner health integration to compile against the current
phenotype-healthcrate (HealthChecker,HealthResponse,HealthCheckResult) instead of removed types (HealthCheck,HealthRegistry,HealthCheckError, sharedHealthSnapshot/HealthReport).health_integration.rsnow defines localHealthSnapshot,ReportSummary, andHealthReportfor scan-derived reporting, maps snapshots toHealthCheckResult, and addsComplianceHealthCheckerimplementingHealthChecker(boxed asynccheck, same severity rules as before). Scan and task failures returnHealthStatus::Unhealthywith logging instead of propagating health errors.ComplianceHealthMonitor::build_health_registryreturns a populatedHealthResponse(aggregated status viaworse, per-finding checks) rather than an empty registry stub.The diff also adds
rust/Cargo.lockand manyrust/target/**build/cache files; reviewers may want those artifacts dropped from the PR if they were not intentional.Reviewed by Cursor Bugbot for commit fb5bc0f. Bugbot is set up for automated code reviews on this repo. Configure here.