ci: add trufflehog secrets scan#41
Closed
KooshaPari wants to merge 8 commits into
Closed
Conversation
- Pin actions/checkout, setup-python, setup-node, etc. - Ensures reproducible and secure CI runs
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…te advisory - Remove deprecated default = "allow" (cargo-deny 0.18+ removed this key) - Add version = 2 to licenses section - Ignore RUSTSEC-2024-0436 (paste unmaintained, transitive via proc-macro2) Refs: org-wide cargo-deny maintenance
Pin all action refs to immutable SHAs across workflow files: - checkout@v4 → @11bd71901bbe5b1630ceea73d27597364c9af683 - checkout@v6 → @de0fac2e4500dabe0009e67214ff5f5447ce83dd - setup-node@v4/v5, setup-python@v4/v5, setup-go@v5 - upload-artifact@v4/v7, download-artifact@v4 - cache@v3/v4, github-script@v7 - configure-pages@v5/v6, deploy-pages@v4/v5 - upload-pages-artifact@v3/v5, dependency-review-action@v4 Fixes version-tag normalization (add v4/v5 tags where missing). Fixes double-SHA corruption artifacts from prior patching rounds. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (20)
📒 Files selected for processing (31)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 14 minutes and 16 seconds.Comment |
|
codeant-ai
Bot
added
the
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 | ||
| uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.1.1 |
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
Multiple workflows use an invalid checkout syntax with a double '@' in the ref (e.g. actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11@11bd71901bbe5b1630ceea73d27597364c9af683), which is not a valid GitHub Actions uses: reference and will cause those jobs to fail before running their steps.
Suggestion: Update every affected uses: actions/checkout@...@... line to use a single valid ref (either a tag like @v4 or a single pinned commit SHA) and apply that consistently across all modified workflows.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** .github/workflows/cargo-audit.yml
**Line:** 20:20
**Comment:**
*CRITICAL: Multiple workflows use an invalid checkout syntax with a double '@' in the ref (e.g. `actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11@11bd71901bbe5b1630ceea73d27597364c9af683`), which is not a valid GitHub Actions `uses:` reference and will cause those jobs to fail before running their steps.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+15
to
+17
| - run: trufflehog github --only-verified --no-update | ||
| env: | ||
| GH_TOKEN: \${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The Trufflehog workflow sets GH_TOKEN as \${{ secrets.GITHUB_TOKEN }}, which escapes the expression so the job receives the literal string instead of the secret, breaking authenticated GitHub scanning.
Suggestion: Remove the backslash and pass the token as GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} so the expression is interpolated and Trufflehog can use a real GitHub token.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** .github/workflows/trufflehog.yml
**Line:** 15:17
**Comment:**
*HIGH: The Trufflehog workflow sets `GH_TOKEN` as `\${{ secrets.GITHUB_TOKEN }}`, which escapes the expression so the job receives the literal string instead of the secret, breaking authenticated GitHub scanning.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+97
to
+115
| fn run_calibration_ui<B: ratatui::backend::Backend>( | ||
| terminal: &mut Terminal<B>, | ||
| camera_index: usize, | ||
| _num_points: usize, | ||
| ) -> Result<Calibrator> { | ||
| // Create camera | ||
| let mut config = CameraConfig::eye_tracking(); | ||
| config.camera_index = Some(camera_index); | ||
| let mut camera = Camera::new(config)?; | ||
|
|
||
| // Create inference pipeline | ||
| let mut pipeline = InferencePipeline::real_time_pipeline(); | ||
|
|
||
| // Create calibrator | ||
| let calibrator = Arc::new(Mutex::new(Calibrator::new())); | ||
| let _estimator = Arc::new(Mutex::new(GazeEstimator::new(None))); | ||
|
|
||
| // Calibration state | ||
| let mut points = generate_calibration_points(); |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The num_points calibration CLI argument is accepted but ignored: run_calibration_ui takes _num_points and always uses a fixed 9-point grid, so user-specified point counts have no effect.
Suggestion: Thread num_points through into run_calibration_ui and generate_calibration_points, or validate and reject unsupported values, so the CLI either honors or explicitly constrains the requested calibration point count.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** crates/eyetracker-cli/src/calibration.rs
**Line:** 97:115
**Comment:**
*HIGH: The `num_points` calibration CLI argument is accepted but ignored: `run_calibration_ui` takes `_num_points` and always uses a fixed 9-point grid, so user-specified point counts have no effect.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+34
to
+39
| #[arg(short, long, value_enum, default_value = "json")] | ||
| output: OutputFormat, | ||
|
|
||
| /// Output file (default: stdout) | ||
| #[arg(short, long)] | ||
| output_file: Option<std::path::PathBuf>, |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The --output and --output-file options are exposed on the CLI, but track always runs the TUI loop and ignores both (the OutputFormat argument is unused and output_file is never referenced), so advertised JSON/Text/CSV and file output modes do not work.
Suggestion: Either implement handling for the non-TUI output formats and optional output file in track (e.g. stream gaze data in the selected format to stdout or a file), or restrict the CLI to the supported TUI mode and reject incompatible option combinations.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** crates/eyetracker-cli/src/main.rs
**Line:** 34:39
**Comment:**
*HIGH: The `--output` and `--output-file` options are exposed on the CLI, but `track` always runs the TUI loop and ignores both (the `OutputFormat` argument is unused and `output_file` is never referenced), so advertised JSON/Text/CSV and file output modes do not work.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![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 introduces a comprehensive eye-tracking system by adding three new workspace crates: eyetracker-camera for hardware-abstracted frame capture, eyetracker-inference for facial landmark detection and gaze estimation using ONNX, and eyetracker-cli which implements a terminal-based user interface for real-time tracking and 9-point calibration. Additionally, the PR includes iconography specifications, SVG assets, and journey traceability documentation. I have no feedback to provide.
| fn run_calibration_ui<B: ratatui::backend::Backend>( | ||
| terminal: &mut Terminal<B>, | ||
| camera_index: usize, | ||
| _num_points: usize, |
There was a problem hiding this comment.
Suggestion: The points CLI argument is effectively ignored because _num_points is never used and calibration points are always hardcoded to 9. This breaks the command contract (--points) and will confuse callers expecting a different number of targets. Use num_points to generate the requested number of calibration points (or reject unsupported values explicitly). [logic error]
Severity Level: Major ⚠️
- ⚠️ `eyetracker calibrate --points` flag is silently ignored.
- ⚠️ Calibration CLI help misrepresents configurable point counts.Steps of Reproduction ✅
1. Build and run the CLI binary, then invoke `eyetracker calibrate --points 5` (Calibrate
subcommand with `points` argument defined in `crates/eyetracker-cli/src/main.rs:74-83` and
dispatched in `main.rs:101-117`).
2. Observe that `Commands::Calibrate { points, .. }` passes the user-specified value (5)
into `calibration::run_calibration(cli.camera, *points, save.clone())` at
`main.rs:115-116`.
3. In `crates/eyetracker-cli/src/calibration.rs:69-73`, `run_calibration` receives
`num_points: u32` (value 5) and forwards it as `num_points as usize` into
`run_calibration_ui(&mut terminal, camera_index, num_points as usize)` at
`calibration.rs:81`.
4. In `run_calibration_ui` at `calibration.rs:97-101`, the parameter is named
`_num_points` and is never used; instead the calibration grid is always created by
`generate_calibration_points()` at `calibration.rs:50-66`, which returns exactly 9 fixed
points, and the UI reports `points.len()` at `calibration.rs:287-292` as 9, so the
`--points` CLI argument has no effect.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-cli/src/calibration.rs
**Line:** 100:100
**Comment:**
*Logic Error: The `points` CLI argument is effectively ignored because `_num_points` is never used and calibration points are always hardcoded to 9. This breaks the command contract (`--points`) and will confuse callers expecting a different number of targets. Use `num_points` to generate the requested number of calibration points (or reject unsupported values explicitly).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+203
to
+211
| match cal.finalize() { | ||
| Ok(_) => { | ||
| println!("Calibration successful!"); | ||
| println!("Accuracy: {:.2} degrees", cal.accuracy()); | ||
| } | ||
| Err(e) => { | ||
| eprintln!("Calibration failed: {}", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Calibration finalization errors are swallowed and converted into a successful return path, so failed calibration still exits as success. Propagate the finalize() error instead of only printing it, otherwise callers cannot detect failure and may proceed with invalid state. [logic error]
Severity Level: Major ⚠️
- ⚠️ Calibration command exits successfully despite failed calibration.
- ⚠️ Scripts relying on exit codes misinterpret calibration failures.Steps of Reproduction ✅
1. Run `eyetracker calibrate` so the CLI enters the Calibrate branch in
`crates/eyetracker-cli/src/main.rs:101-117`, which calls
`calibration::run_calibration(cli.camera, *points, save.clone())` at `main.rs:115-116`.
2. During calibration, behave in a way that causes
`eyetracker_core::Calibrator::finalize()` to fail, e.g. by providing highly inconsistent
gaze samples so accuracy exceeds the 1.5° limit enforced in
`crates/eyetracker-core/src/lib.rs:32-71` (where `finalize` sets `state` to
`CalibrationState::Failed` and returns `Err(EyetrackerError::CalibrationFailed(...))`).
3. When all points are marked complete in `run_calibration_ui` at
`crates/eyetracker-cli/src/calibration.rs:198-201`, the code locks the calibrator and
calls `match cal.finalize()` at `calibration.rs:202-203`; on error it executes the
`Err(e)` arm at `calibration.rs:208-210`, printing `"Calibration failed: {e}"` to stderr
but not returning an error from the function, then sets `calibrating = false` at
`calibration.rs:212`.
4. After leaving the loop, `run_calibration_ui` calls `camera.stop()?` and then returns
`Ok(cal)` at `calibration.rs:221-224`, so `run_calibration` returns `Ok(cal)` at
`calibration.rs:94`; consequently `main` at `main.rs:101-129` also returns `Ok(())` and
the process exits with status 0 even though calibration has failed, leaving callers (e.g.
scripts checking exit codes) unable to distinguish success from failure.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-cli/src/calibration.rs
**Line:** 203:211
**Comment:**
*Logic Error: Calibration finalization errors are swallowed and converted into a successful return path, so failed calibration still exits as success. Propagate the `finalize()` error instead of only printing it, otherwise callers cannot detect failure and may proceed with invalid state.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+107
to
+134
| pub fn from_index(index: usize) -> Option<Self> { | ||
| match index { | ||
| 1 => Some(LandmarkType::NoseTip), | ||
| 10 => Some(LandmarkType::Forehead), | ||
| 33 => Some(LandmarkType::LeftEyeOuter), | ||
| 133 => Some(LandmarkType::LeftEyeInner), | ||
| 145 => Some(LandmarkType::LeftEyeBottom), | ||
| 152 => Some(LandmarkType::Chin), | ||
| 159 => Some(LandmarkType::LeftEyeTop), | ||
| 234 => Some(LandmarkType::LeftJaw), | ||
| 263 => Some(LandmarkType::RightEyeInner), | ||
| 275 => Some(LandmarkType::NoseRight), | ||
| 362 => Some(LandmarkType::RightEyeOuter), | ||
| 374 => Some(LandmarkType::RightEyeBottom), | ||
| 454 => Some(LandmarkType::RightJaw), | ||
| 468 => Some(LandmarkType::LeftPupil), | ||
| 469 => Some(LandmarkType::LeftIrisLeft), | ||
| 470 => Some(LandmarkType::LeftIrisTop), | ||
| 471 => Some(LandmarkType::LeftIrisRight), | ||
| 472 => Some(LandmarkType::LeftIrisBottom), | ||
| 473 => Some(LandmarkType::RightPupil), | ||
| 474 => Some(LandmarkType::RightIrisLeft), | ||
| 475 => Some(LandmarkType::RightIrisTop), | ||
| 476 => Some(LandmarkType::RightIrisRight), | ||
| 477 => Some(LandmarkType::RightIrisBottom), | ||
| _ => Some(LandmarkType::Unknown), | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: LandmarkType::NoseBottom is defined and used by face_pitch(), but from_index never maps index 2 to NoseBottom. As a result, generated landmarks at index 2 are tagged Unknown, get(NoseBottom) returns None, and pitch collapses to zero. Add the missing mapping for index 2. [logic error]
Severity Level: Major ⚠️
- ⚠️ GazeEstimationResult.head_pose pitch component always zeroed.
- ⚠️ Any future consumer of head pose gets wrong data.
- ⚠️ Synthetic landmark generator inconsistent with LandmarkType definition.Steps of Reproduction ✅
1. Generate landmarks via the face mesh detector: `FaceMesh::detect` at
`crates/eyetracker-inference/src/face_mesh.rs:352-372` calls
`generate_synthetic_landmarks` (lines 374-405), which loops `for i in 0..478` and assigns
`lm_type = LandmarkType::from_index(i).unwrap_or(LandmarkType::Unknown)` (lines 378-379).
2. For index `2` in that loop, `LandmarkType::from_index(2)` falls into the wildcard arm
`_ => Some(LandmarkType::Unknown)` at `face_mesh.rs:132`, so the landmark at index 2 is
created with `landmark_type = Unknown` instead of `NoseBottom` (lines 380-403).
3. When gaze is estimated (e.g., through the public pipeline), `GazeEstimation::estimate`
at `crates/eyetracker-inference/src/gaze_estimator.rs:147-181` builds a
`GazeEstimationResult` and sets `head_pose: face_mesh.face_pose()` at
`gaze_estimator.rs:168`.
4. `FaceMeshResult::face_pose` at `face_mesh.rs:259-260` calls `face_pitch`, which looks
up `LandmarkType::NoseBottom` via `self.get(LandmarkType::NoseBottom)` at
`face_mesh.rs:227`; `get` (lines 154-158) indexes by `landmark_type.index()` (2) and
filters on `l.landmark_type == landmark_type`, which fails because the stored type is
`Unknown`, so `face_pitch` hits the `else { return 0.0; }` path (lines 223-232), making
the pitch component of `head_pose` always zero for every frame.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-inference/src/face_mesh.rs
**Line:** 107:134
**Comment:**
*Logic Error: `LandmarkType::NoseBottom` is defined and used by `face_pitch()`, but `from_index` never maps index `2` to `NoseBottom`. As a result, generated landmarks at index 2 are tagged `Unknown`, `get(NoseBottom)` returns `None`, and pitch collapses to zero. Add the missing mapping for index 2.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+361
to
+371
| // For now, return a synthetic result for testing | ||
| let landmarks = Self::generate_synthetic_landmarks(width, height); | ||
| let face_box = FaceBox::from_center(0.5, 0.45, 0.4, 0.4); | ||
|
|
||
| Ok(FaceMeshResult { | ||
| landmarks, | ||
| confidence: 0.95, | ||
| face_box, | ||
| timestamp: std::time::SystemTime::now(), | ||
| latency: start.elapsed(), | ||
| }) |
There was a problem hiding this comment.
Suggestion: detect() ignores pixels and always returns a synthetic face with confidence 0.95, so the pipeline reports face detections even when no real face exists. This breaks downstream gaze validity and makes detection status unreliable; detection must depend on model output or explicitly signal unsupported mode. [logic error]
Severity Level: Critical 🚨
- ❌ CLI reports a face even when none is present.
- ❌ Calibration records samples from synthetic, not real, gaze.
- ⚠️ InferenceResult.face_detected cannot indicate real face presence.
- ⚠️ Downstream gaze confidence and status become misleading. Steps of Reproduction ✅
1. Run the CLI tracker via `run_tracker` in `crates/eyetracker-cli/src/app.rs:26-82`; it
constructs an `InferencePipeline` using `InferencePipeline::real_time_pipeline()` or
`high_accuracy_pipeline()` at `app.rs:48-53`.
2. In the main loop `run_ui` (`app.rs:122-182`), each captured camera frame is
preprocessed with `preprocess_frame` and passed into `pipeline.process_frame(&pixels,
frame.width, frame.height)` at `app.rs:141-142`.
3. Inside `InferencePipeline::process_frame`
(`crates/eyetracker-inference/src/pipeline.rs:159-221`), when `detect_face_mesh` is true
(default in `PipelineConfig::default` at `pipeline.rs:35-47`), it calls
`self.face_mesh.detect(pixels, width, height)` at `pipeline.rs:181-182`;
`FaceMesh::detect` (`face_mesh.rs:352-372`) ignores `pixels` entirely and returns
synthetic landmarks and `confidence: 0.95` at lines 361-371, regardless of actual frame
content.
4. `InferencePipeline` then sets `face_detected` from `face_mesh.as_ref().map(|r|
r.confidence >= self.config.min_face_confidence).unwrap_or(false)` at
`pipeline.rs:193-197`, with `min_face_confidence` defaulting to `0.5` at
`pipeline.rs:41-42`, so `face_detected` is always `true`; `TrackingState::update` in
`crates/eyetracker-cli/src/app.rs:97-120` copies this into `state.face_detected`, which is
rendered in the UI as `"Faces: 1"` and `"Status: TRACKING"`
(`app.rs:231-241,269-279,285-289`) even if the camera sees no face, and the calibration
flow in `crates/eyetracker-cli/src/calibration.rs:137-178` uses the resulting synthetic
`screen_position` values to record calibration samples.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-inference/src/face_mesh.rs
**Line:** 361:371
**Comment:**
*Logic Error: `detect()` ignores `pixels` and always returns a synthetic face with confidence `0.95`, so the pipeline reports face detections even when no real face exists. This breaks downstream gaze validity and makes detection status unreliable; detection must depend on model output or explicitly signal unsupported mode.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+318
to
+330
| let (scale_x, scale_y) = if let Some(ref cal) = self.screen_calibration { | ||
| (cal.horizontal_scale, cal.vertical_scale) | ||
| } else { | ||
| (0.8, 0.6) // Default scaling | ||
| }; | ||
|
|
||
| // Offset from gaze direction | ||
| let offset_x = gaze.x * scale_x; | ||
| let offset_y = gaze.y * scale_y; | ||
|
|
||
| // Combine face position with gaze offset | ||
| let screen_x = (face_cx + offset_x * face_mesh.ipd() * 5.0).clamp(0.0, 1.0); | ||
| let screen_y = (face_cy - offset_y * face_mesh.ipd() * 3.0).clamp(0.0, 1.0); |
There was a problem hiding this comment.
Suggestion: Calibration offsets are never applied in gaze-to-screen conversion: only scale factors are read from calibration, while horizontal_offset/vertical_offset are ignored. This makes offset calibration ineffective and produces systematically shifted gaze output. Apply full calibration transform (including offsets) before returning screen coordinates. [logic error]
Severity Level: Major ⚠️
- ⚠️ InferencePipeline consumers cannot adjust gaze offset via calibration.
- ⚠️ ScreenCalibration horizontal/vertical offsets have no runtime effect.
- ⚠️ Future UIs relying on offset tuning get systematically shifted gaze.Steps of Reproduction ✅
1. In the inference crate, note that `InferencePipeline` exposes `set_calibration(&mut
self, calibration: ScreenCalibration)` which forwards to `GazeEstimation::set_calibration`
(`crates/eyetracker-inference/src/pipeline.rs:147-150` and
`crates/eyetracker-inference/src/gaze_estimator.rs:137-140`), allowing callers to
configure a `ScreenCalibration` with non-zero `horizontal_offset`/`vertical_offset`.
2. Observe that `InferencePipeline::process_frame`
(`crates/eyetracker-inference/src/pipeline.rs:159-221`) calls
`self.gaze_estimator.estimate(fm)` when a face is detected (`line 201`), so any
calibration set via `set_calibration` is used inside `GazeEstimation::estimate`
(`crates/eyetracker-inference/src/gaze_estimator.rs:147-181`).
3. Inside `GazeEstimation::estimate`, the final screen coordinates are computed by
`self.gaze_to_screen(&combined, face_mesh)`
(`crates/eyetracker-inference/src/gaze_estimator.rs:158-159`), which is the only place
that maps gaze to `screen_position` surfaced to callers via
`InferenceResult::screen_position()`
(`crates/eyetracker-inference/src/pipeline.rs:95-103`).
4. In `gaze_to_screen` (`crates/eyetracker-inference/src/gaze_estimator.rs:308-333`),
calibration is used only to read `horizontal_scale` and `vertical_scale` (`lines
318-321`), while the documented offset fields and `ScreenCalibration::transform` method
(`horizontal_offset`, `vertical_offset`, and `transform` at
`crates/eyetracker-inference/src/gaze_estimator.rs:342-407`) are never referenced;
therefore, for any caller that sets a `ScreenCalibration` with non-zero offsets via
`InferencePipeline::set_calibration`, the resulting `screen_position` returned from
`process_frame` is unaffected by those offsets, demonstrating that offset calibration is
effectively ignored.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-inference/src/gaze_estimator.rs
**Line:** 318:330
**Comment:**
*Logic Error: Calibration offsets are never applied in gaze-to-screen conversion: only scale factors are read from calibration, while `horizontal_offset`/`vertical_offset` are ignored. This makes offset calibration ineffective and produces systematically shifted gaze output. Apply full calibration transform (including offsets) before returning screen coordinates.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+122
to
+125
| #[cfg(not(target_os = "macos"))] | ||
| pub fn is_available(&self) -> bool { | ||
| true | ||
| } |
There was a problem hiding this comment.
Suggestion: This implementation reports every provider as available on non-macOS platforms, including Metal and CoreML (and potentially unavailable CUDA setups). That violates the provider contract and can lead to selecting an unsupported backend and failing at runtime during inference initialization. [logic error]
Severity Level: Major ⚠️
- ⚠️ InferenceProvider API misleads callers about backend availability.
- ⚠️ External consumers may select unsupported GPU providers.
- ⚠️ Future pipeline hardware selection logic becomes unreliable.Steps of Reproduction ✅
1. In a consumer of this crate, import `InferenceProvider` from
`crates/eyetracker-inference/src/lib.rs:20` (`pub use model::{ModelConfig, ModelLoader,
ModelType, InferenceProvider};`).
2. On a non-macOS target (e.g., Linux or Windows), call
`InferenceProvider::Metal.is_available()` or `InferenceProvider::CoreML.is_available()`
from your application code.
3. The compiled implementation on non-macOS uses the `#[cfg(not(target_os = "macos"))]`
branch at `crates/eyetracker-inference/src/model.rs:122-125`, where `pub fn
is_available(&self) -> bool { true }` unconditionally returns `true` for every provider.
4. Observe that the result contradicts the documented intent "Check if this provider is
available on the current system" at `model.rs:112-113`, incorrectly reporting
Metal/CoreML/CUDA as available on platforms where they are not, which would mislead any
caller that uses `is_available()` to decide which backend to initialize (even though no
current code in this repo calls `is_available()`, verified via Grep).Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-inference/src/model.rs
**Line:** 122:125
**Comment:**
*Logic Error: This implementation reports every provider as available on non-macOS platforms, including `Metal` and `CoreML` (and potentially unavailable CUDA setups). That violates the provider contract and can lead to selecting an unsupported backend and failing at runtime during inference initialization.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+151
to
+154
| let get_pixel = |x: u32, y: u32| -> f32 { | ||
| let idx = (y * src_width + x) as usize; | ||
| src_data.get(idx).copied().unwrap_or(0) as f32 | ||
| }; |
There was a problem hiding this comment.
Suggestion: RGB sampling is incorrect here because the index is computed as if each pixel were 1 byte. When to_grayscale is false and input is RGB, this reads misaligned bytes and effectively treats interleaved RGB data as grayscale-like data. Use channel-aware indexing (stride of 3 bytes per pixel) when processing color frames. [logic error]
Severity Level: Major ⚠️
- ⚠️ `preprocess_frame` mis-samples RGB frames when color enabled.
- ⚠️ ONNX color models would receive spatially misaligned inputs.
- ⚠️ Current CLI always uses grayscale options, avoiding bug.Steps of Reproduction ✅
1. Note that camera frames are captured as RGB24 and stored as `FrameFormat::Rgb8` with 3
bytes per pixel in `crates/eyetracker-camera/src/camera.rs:172-175` and
`crates/eyetracker-camera/src/camera.rs:238-44`.
2. Observe that the inference preprocessing entry point `preprocess_frame` is defined at
`crates/eyetracker-inference/src/processing.rs:112-120` and is exported via `pub use
processing::preprocess_frame` in `crates/eyetracker-inference/src/lib.rs:16-22`.
3. In a new unit test in `crates/eyetracker-inference/src/processing.rs` (tests module
starting at line 396), construct a 2x1 RGB buffer with distinct channels, e.g. `let pixels
= vec![10u8, 20, 30, 40, 50, 60]; // (10,20,30),(40,50,60)` and call `let out =
preprocess_frame(&pixels, 2, 1, &PreprocessOptions::for_onnx());` using the ONNX-optimized
options defined at `processing.rs:83-93` where `to_grayscale` is set to `false`.
4. Inside `preprocess_frame`, when `to_grayscale` is `false`, `src_data` points directly
to the interleaved RGB buffer (`processing.rs:131-135`), but the `get_pixel` closure at
`processing.rs:151-154` computes `idx = (y * src_width + x)` as if there were one byte per
pixel; for `(x=1,y=0)` this reads `pixels[1]` (the green channel of the first RGB pixel)
instead of `pixels[3]` (the red channel of the second pixel). Asserting `out[0] == 10.0`
and `out[1] == 20.0` demonstrates that RGB data are sampled with single-channel indexing
and channels/positions are misaligned.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-inference/src/processing.rs
**Line:** 151:154
**Comment:**
*Logic Error: RGB sampling is incorrect here because the index is computed as if each pixel were 1 byte. When `to_grayscale` is `false` and input is RGB, this reads misaligned bytes and effectively treats interleaved RGB data as grayscale-like data. Use channel-aware indexing (stride of 3 bytes per pixel) when processing color frames.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| adjusted | ||
| }; | ||
|
|
||
| output.push(normalized); |
There was a problem hiding this comment.
Suggestion: The preprocessing loop always pushes exactly one value per output pixel, even when to_grayscale is false. That returns target_width * target_height elements instead of * 3, which breaks the expected tensor shape for RGB model inputs and will cause downstream inference shape mismatches. [possible bug]
Severity Level: Major ⚠️
- ⚠️ RGB preprocessing returns width*height floats instead of width*height*3.
- ⚠️ Incompatible with `ModelConfig` RGB input shapes (model.rs:70).
- ⚠️ Future ONNX integration would fail due to tensor mismatch.Steps of Reproduction ✅
1. Confirm the preprocessing function `preprocess_frame` in
`crates/eyetracker-inference/src/processing.rs:112-120` reserves capacity for either 1 or
3 channels via `Vec::with_capacity((dst_width * dst_height) as usize * if
options.to_grayscale { 1 } else { 3 })` at line 118.
2. In a new test in the `tests` module of `processing.rs` (starting at line 396), create a
synthetic RGB buffer `let pixels = vec![0u8; 224 * 224 * 3];` and call `let out =
preprocess_frame(&pixels, 224, 224, &PreprocessOptions::for_onnx());` using
`PreprocessOptions::for_onnx()` defined at `processing.rs:83-93`, which sets
`to_grayscale` to `false` and keeps RGB data.
3. Follow the resize loop at `processing.rs:137-181`: for every destination pixel `(dst_x,
dst_y)`, the code computes a single interpolated scalar `value` (lines 156-165), applies
brightness/contrast and normalization (lines 167-178), and then pushes exactly one
`normalized` value per output pixel at `processing.rs:180` regardless of `to_grayscale`.
4. Assert in the test that `out.len() == 224 * 224` even though the capacity reserved for
`to_grayscale == false` corresponds to `224 * 224 * 3` and the model configuration expects
3 channels (see `ModelConfig::input_shape = vec![1, 224, 224, 3]` for gaze models in
`crates/eyetracker-inference/src/model.rs:239-243`). This demonstrates that non-grayscale
preprocessing returns a 1-channel tensor where 3-channel RGB is expected.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-inference/src/processing.rs
**Line:** 180:180
**Comment:**
*Possible Bug: The preprocessing loop always pushes exactly one value per output pixel, even when `to_grayscale` is `false`. That returns `target_width * target_height` elements instead of `* 3`, which breaks the expected tensor shape for RGB model inputs and will cause downstream inference shape mismatches.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| for pixel in pixels.iter_mut() { | ||
| let bin = ((*pixel * 255.0).round() as usize).clamp(0, 255); | ||
| let cdf_val = cdf[bin]; | ||
| let equalized = ((cdf_val - cdf_min) as f32 / (total - cdf_min) as f32) * max_val; |
There was a problem hiding this comment.
Suggestion: Histogram equalization can divide by zero when all pixels fall into one bin (total == cdf_min). In that case the denominator becomes zero and produces NaN/Inf values in the output buffer. Guard this case and skip or short-circuit equalization when dynamic range is zero. [possible bug]
Severity Level: Major ⚠️
- ⚠️ Equalization on flat frames converts pixels to NaNs.
- ⚠️ CLI preprocessing uses equalization by default for tracking.
- ⚠️ Current face mesh stub ignores pixels, masking this bug.Steps of Reproduction ✅
1. Confirm that `preprocess_frame` in
`crates/eyetracker-inference/src/processing.rs:111-190` applies histogram equalization
when `options.equalize_histogram` is true by calling `equalize_histogram_inplace(&mut
output);` at lines 184-187; the default options enable this
(`PreprocessOptions::default()` at lines 66-78 sets `equalize_histogram: true`).
2. In the `tests` module of `processing.rs` (starting at line 396), add a test
`test_equalize_histogram_flat_image` that constructs a flat normalized buffer `let mut
pixels = vec![0.5f32; 10];` and calls `equalize_histogram_inplace(&mut pixels);` using the
function defined at `processing.rs:212-241`.
3. Inside `equalize_histogram_inplace`, the histogram built at `processing.rs:217-222`
places all counts into a single bin because every `p` is 0.5; the cumulative distribution
`cdf` computed at `processing.rs:224-229` then has `cdf_min == total == pixels.len() as
u32` as determined at `processing.rs:232-234`.
4. For each pixel, the equalization formula at `processing.rs:239` computes `equalized =
((cdf_val - cdf_min) as f32 / (total - cdf_min) as f32) * max_val;`; with a flat image
this becomes `0.0 / 0.0 * 255.0`, yielding `NaN` under IEEE-754. The loop at
`processing.rs:236-240` writes these NaNs back into `pixels`, which you can verify with
`assert!(pixels.iter().all(|v| v.is_nan()));`. This shows that flat images produce invalid
outputs via division by zero.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-inference/src/processing.rs
**Line:** 239:239
**Comment:**
*Possible Bug: Histogram equalization can divide by zero when all pixels fall into one bin (`total == cdf_min`). In that case the denominator becomes zero and produces NaN/Inf values in the output buffer. Guard this case and skip or short-circuit equalization when dynamic range is zero.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+285
to
+290
| pub async fn capture_frame_async(&mut self, timeout: Duration) -> Result<Frame> { | ||
| if !self.running { | ||
| return Err(CameraError::NotRunning.into()); | ||
| } | ||
|
|
||
| tokio::time::timeout(timeout, async { self.capture_frame() }).await? |
There was a problem hiding this comment.
Suggestion: The async capture method runs a blocking camera grab directly inside an async future, which blocks the Tokio worker thread and defeats cooperative scheduling; timeout also cannot preempt the blocking call itself. Move frame capture to spawn_blocking (or a dedicated thread) and await that task inside timeout. [possible bug]
Severity Level: Major ⚠️
- ❌ Async camera consumers block Tokio worker during frame capture.
- ⚠️ capture_frame_async timeout fails to cancel long camera grabs.
- ⚠️ Concurrent eye tracking tasks starve on same executor thread.Steps of Reproduction ✅
1. Within a Tokio runtime, construct a `Camera` using `Camera::new` from
`crates/eyetracker-camera/src/camera.rs:155-192` with a valid `CameraConfig`, and call
`camera.start()` from `camera.rs:199-218` to begin capture.
2. Spawn an async task that calls `camera.capture_frame_async(Duration::from_secs(5))`
implemented at `camera.rs:285-290`, while also spawning another async task on the same
runtime that periodically logs or updates UI.
3. When `capture_frame_async` runs, it executes `tokio::time::timeout(timeout, async {
self.capture_frame() }).await?` at `camera.rs:290`, and inside that future,
`self.capture_frame()` calls the blocking `provider.grab_frame(1000)` at
`camera.rs:239-247`, which performs a synchronous frame grab.
4. Observe that the Tokio worker thread running `capture_frame_async` is blocked for the
full duration of `grab_frame(1000)`, preventing the other async task on that worker from
making progress, and that the `timeout` cannot preempt or cancel the underlying blocking
call; it only returns after `capture_frame` finishes, defeating the intended non-blocking
timeout behavior.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-camera/src/camera.rs
**Line:** 285:290
**Comment:**
*Possible Bug: The async capture method runs a blocking camera grab directly inside an async future, which blocks the Tokio worker thread and defeats cooperative scheduling; timeout also cannot preempt the blocking call itself. Move frame capture to `spawn_blocking` (or a dedicated thread) and await that task inside `timeout`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+236
to
+241
| let mut hist = vec![0u32; bins]; | ||
| let bin_size = 256 / bins; | ||
|
|
||
| for &pixel in &gray.data { | ||
| let bin = (pixel as usize / bin_size).min(bins - 1); | ||
| hist[bin] += 1; |
There was a problem hiding this comment.
Suggestion: Histogram bin sizing does integer division without validating bins; bins == 0 panics, and bins > 256 makes bin_size zero, causing division by zero in bucket computation. Validate bins (e.g., 1..=256) before calculating bin size. [possible bug]
Severity Level: Major ⚠️
- ❌ eyetracker-camera Frame::brightness_histogram panics on invalid bins.
- ⚠️ Future callers with user-configured bins risk crashes.Steps of Reproduction ✅
1. Open `crates/eyetracker-camera/src/frame.rs` and locate the `brightness_histogram`
method at lines 234–245 and the `test_frame_histogram` test at lines 312–320 (as seen via
Grep results showing calls at `frame.rs:317`).
2. In `test_frame_histogram` (around line 317), change the call `let hist =
frame.brightness_histogram(16);` to `let hist = frame.brightness_histogram(0);` so the
test now passes `bins == 0`.
3. From the repository root `/workspace/eyetracker`, run `cargo test -p eyetracker-camera`
to execute the `eyetracker-camera` crate tests, which will invoke `test_frame_histogram`
and then `Frame::brightness_histogram` at line 234.
4. Observe that the test process panics with `attempt to divide by zero` when executing
`let bin_size = 256 / bins;` at line 237 in `brightness_histogram`, and similarly would
panic when `bins > 256` once `bin_size` becomes 0 and `pixel as usize / bin_size` at line
240 attempts another division by zero.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-camera/src/frame.rs
**Line:** 236:241
**Comment:**
*Possible Bug: Histogram bin sizing does integer division without validating `bins`; `bins == 0` panics, and `bins > 256` makes `bin_size` zero, causing division by zero in bucket computation. Validate `bins` (e.g., 1..=256) before calculating bin size.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+202
to
+210
| let cdf_min = cdf.iter().find(|&&v| v > 0).copied().unwrap_or(0); | ||
| let total = pixels.len() as u32; | ||
| let max_val = 255.0; | ||
|
|
||
| for pixel in pixels.iter_mut() { | ||
| let bin = ((*pixel * 255.0).round() as usize).clamp(0, 255); | ||
| let cdf_val = cdf[bin]; | ||
| let equalized = ((cdf_val - cdf_min) as f32 / (total - cdf_min) as f32) * max_val; | ||
| *pixel = equalized / 255.0; |
There was a problem hiding this comment.
Suggestion: Histogram equalization can divide by zero when all pixels fall into one cumulative bucket (total == cdf_min), which yields NaN/Inf output values and corrupts downstream inference input. Guard this case and skip equalization (or return zeros) when the denominator is zero. [possible bug]
Severity Level: Critical 🚨
- ❌ CLI tracking may feed NaNs into inference.
- ❌ Calibration pipeline processes NaN-normalized pixels.
- ⚠️ Uniform or low-texture frames yield corrupted preprocessing.
- ⚠️ Downstream gaze estimates become unstable or meaningless.Steps of Reproduction ✅
1. In the inference pipeline, call `preprocess_frame` from
`crates/eyetracker-inference/src/processing.rs:111-190` with a uniform grayscale image:
for example, in a test or consumer, construct `let pixels = vec![128u8; 224 * 224];` and
invoke `preprocess_frame(&pixels, 224, 224, &PreprocessOptions::default())` where
`equalize_histogram` defaults to `true` (lines 66-78).
2. Inside `preprocess_frame`, after resampling and normalization, execution reaches the
equalization step at `crates/eyetracker-inference/src/processing.rs:184-187`, which calls
`equalize_histogram_inplace(&mut output);` with `output` containing identical values (all
pixels map to the same normalized brightness).
3. In `equalize_histogram_inplace`
(`crates/eyetracker-inference/src/processing.rs:211-241`), the histogram (`hist`) and
cumulative distribution (`cdf`) are built so that exactly one bin carries the entire
count; `cdf_min` (line 232) becomes `total` (line 233), where `total` is `pixels.len() as
u32`. The normalization step at lines 236-239 computes `let equalized = ((cdf_val -
cdf_min) as f32 / (total - cdf_min) as f32) * max_val;` which for these inputs is `0.0 /
0.0`, producing `NaN` for every pixel.
4. `preprocess_frame` returns a `Vec<f32>` containing `NaN` values; this vector is then
passed into `InferencePipeline::process_frame` (exported from
`crates/eyetracker-inference/src/lib.rs:20-22` and used in
`crates/eyetracker-cli/src/app.rs:141-142` and
`crates/eyetracker-cli/src/calibration.rs:138-139`), meaning that the CLI's tracking and
calibration flows can feed NaN inputs into downstream ONNX inference when a near-uniform
frame (e.g., lens-covered or saturated) is processed.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-camera/src/processing.rs
**Line:** 202:210
**Comment:**
*Possible Bug: Histogram equalization can divide by zero when all pixels fall into one cumulative bucket (`total == cdf_min`), which yields NaN/Inf output values and corrupts downstream inference input. Guard this case and skip equalization (or return zeros) when the denominator is zero.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+35
to
+46
| enable_raw_mode()?; | ||
| let mut stdout = io::stdout(); | ||
| execute!(stdout, EnterAlternateScreen, EnableMouseCapture)?; | ||
| let backend = CrosstermBackend::new(stdout); | ||
| let mut terminal = Terminal::new(backend)?; | ||
|
|
||
| // Create camera | ||
| let mut config = CameraConfig::eye_tracking(); | ||
| config.camera_index = Some(camera_index); | ||
| config.target_fps = fps.min(60); | ||
|
|
||
| let mut camera = Camera::new(config)?; |
There was a problem hiding this comment.
Suggestion: Terminal raw mode and alternate-screen mode are enabled before fallible setup calls, but early ? returns (for example from camera creation) bypass the cleanup block and leave the user terminal in a broken state. Wrap terminal setup/cleanup in a guard pattern so cleanup always runs even when initialization fails. [resource leak]
Severity Level: Major ⚠️
- ❌ Tracking command can leave terminal stuck in raw mode.
- ⚠️ Affects eyetracker-cli `track` command error paths.
- ⚠️ Users must manually reset terminal after failures.Steps of Reproduction ✅
1. From the repo root, run the CLI tracking command, e.g. `cargo run -p eyetracker-cli --
track --camera 10`, which enters `fn main()` in `crates/eyetracker-cli/src/main.rs:12-24`
and calls `app::run_tracker(cli.camera, *fps, *smooth, *debug, cli.output)?;` at
`main.rs:24`.
2. Inside `run_tracker` in `crates/eyetracker-cli/src/app.rs:27-39`, the code enables
terminal raw mode and alternate screen before any camera setup: `enable_raw_mode()?;` at
`app.rs:35`, then `execute!(stdout, EnterAlternateScreen, EnableMouseCapture)?;` at
`app.rs:37`, and constructs the TUI `Terminal::new(backend)?;` at `app.rs:39`.
3. `run_tracker` then configures and creates the camera at `app.rs:41-46`, calling
`Camera::new(config)?;` at `app.rs:46`, which in turn calls `Provider::with_device(...)`
and other fallible setup in `crates/eyetracker-camera/src/camera.rs:7-26`; if the selected
camera index is invalid or no camera device is present, `Camera::new` returns an `Err` via
the `?` at `camera.rs:10-26`.
4. Because the error from `Camera::new(config)?` (or earlier `Terminal::new(backend)?`)
propagates out of `run_tracker` before the cleanup block at `app.rs:73-75`, the calls to
`disable_raw_mode()?;` and `execute!(..., LeaveAlternateScreen, DisableMouseCapture)?;`
never execute, so the process exits with the terminal still in raw mode and alternate
screen, leaving the user's shell in a broken state until they manually reset it.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-cli/src/app.rs
**Line:** 35:46
**Comment:**
*Resource Leak: Terminal raw mode and alternate-screen mode are enabled before fallible setup calls, but early `?` returns (for example from camera creation) bypass the cleanup block and leave the user terminal in a broken state. Wrap terminal setup/cleanup in a guard pattern so cleanup always runs even when initialization fails.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+112
to
+115
| if let Some(last) = self.last_update { | ||
| let elapsed = last.elapsed().as_secs_f64(); | ||
| if elapsed > 0.0 { | ||
| self.fps = self.frame_count as f64 / elapsed; |
There was a problem hiding this comment.
Suggestion: FPS is computed using total processed frame count divided by only the elapsed time since the previous update, which makes the displayed FPS grow incorrectly over time. Compute FPS from per-frame delta (1.0 / elapsed) or use elapsed time from a fixed start timestamp consistently. [logic error]
Severity Level: Major ⚠️
- ⚠️ FPS metric in footer/debug view becomes meaningless.
- ⚠️ Misleads performance monitoring during tracking sessions.Steps of Reproduction ✅
1. Run the tracking command, e.g. `cargo run -p eyetracker-cli -- track`, which calls
`app::run_tracker` from `crates/eyetracker-cli/src/main.rs:12-25` and enters the main loop
`run_ui` at `crates/eyetracker-cli/src/app.rs:122-182`.
2. In each iteration of the loop at `app.rs:136-159`, a frame is captured and processed,
and `TrackingState::update` is called via `s.update(&result, result.total_latency);` at
`app.rs:147`, with `TrackingState::update` defined at `app.rs:97-120`.
3. Inside `TrackingState::update`, `self.frame_count` is incremented at `app.rs:109`, then
if `self.last_update` is `Some(last)` at `app.rs:112`, `elapsed` is computed as
`last.elapsed().as_secs_f64()` at `app.rs:113`, and FPS is set to `self.frame_count as f64
/ elapsed` at `app.rs:115`, even though `elapsed` only measures the time since the
previous frame while `frame_count` is the total number of frames since start.
4. As tracking continues with roughly constant frame times, each call uses a growing
`frame_count` divided by a near-constant per-frame `elapsed`, so the `fps` field grows
without bound and is rendered incorrectly in both the footer (`state.fps` used at
`app.rs:249-250` in `draw_ui`) and the debug view (`state.fps` displayed at
`app.rs:264-265` in `draw_debug_view`), producing clearly invalid FPS values during normal
use.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-cli/src/app.rs
**Line:** 112:115
**Comment:**
*Logic Error: FPS is computed using total processed frame count divided by only the elapsed time since the previous update, which makes the displayed FPS grow incorrectly over time. Compute FPS from per-frame delta (`1.0 / elapsed`) or use elapsed time from a fixed start timestamp consistently.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
![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 5 potential issues.
Bugbot Autofix is ON, but it could not run because on-demand usage is turned off. To enable Bugbot Autofix, turn on on-demand usage and set a spend limit in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit caf54d9. Configure here.
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 | ||
| uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.1.1 |
There was a problem hiding this comment.
Invalid double-@ syntax breaks checkout in multiple workflows
High Severity
The uses field in multiple workflow files now contains two @ symbols (e.g., actions/checkout@hash1@hash2 or actions/checkout@v4@hash). GitHub Actions only supports the format owner/repo@ref with a single @. The double-@ format is not a valid action reference and will cause workflow parse/resolution failures across cargo-audit.yml, cargo-deny.yml, cargo-machete.yml, cargo-semver-checks.yml, scorecard.yml, and journey-gate.yml.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit caf54d9. Configure here.
| - uses: trufflehog/actions/setup@main | ||
| - run: trufflehog github --only-verified --no-update | ||
| env: | ||
| GH_TOKEN: \${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Escaped dollar sign prevents secret token evaluation
High Severity
The GH_TOKEN environment variable uses \${{ secrets.GITHUB_TOKEN }} with a backslash before the $. GitHub Actions expression evaluation looks for the literal ${{ pattern to begin expression substitution. The \${{ does not match this pattern, so the env var will be set to the literal string \${{ secrets.GITHUB_TOKEN }} instead of the actual token value. This means trufflehog won't have valid authentication credentials.
Reviewed by Cursor Bugbot for commit caf54d9. Configure here.
| with: | ||
| fetch-depth: 0 | ||
| - uses: trufflehog/actions/setup@main | ||
| - run: trufflehog github --only-verified --no-update |
There was a problem hiding this comment.
Trufflehog github command missing required --repo flag
High Severity
The command trufflehog github --only-verified --no-update is missing the required --repo flag. The trufflehog github subcommand scans a GitHub repository via the API and needs --repo to know which repository to scan (e.g., --repo ${{ github.repository }}). Without it, the command will fail. Alternatively, trufflehog git file://. could be used to scan the locally checked-out repository.
Reviewed by Cursor Bugbot for commit caf54d9. Configure here.
| self.fps = self.frame_count as f64 / elapsed; | ||
| } | ||
| } | ||
| self.last_update = Some(Instant::now()); |
There was a problem hiding this comment.
FPS calculation divides total frames by per-frame interval
Medium Severity
The TrackingState::update method computes FPS as self.frame_count / elapsed, where frame_count is the cumulative total but elapsed is only the time since the previous frame (since last_update is overwritten every call). This causes FPS to grow linearly with frame count — e.g., on frame 100 at ~33ms intervals, the displayed FPS would be ~3030 instead of ~30. The denominator needs to be the total elapsed time from a fixed start point, or the formula should use 1.0 / elapsed for instantaneous FPS.
Reviewed by Cursor Bugbot for commit caf54d9. Configure here.
|
|
||
| output.push(normalized); | ||
| } | ||
| } |
There was a problem hiding this comment.
Preprocessing ignores to_grayscale: false, always outputs single-channel
Medium Severity
When to_grayscale is false (as set by PreprocessOptions::for_onnx() with the comment "Keep RGB for ML models"), preprocess_frame still indexes into the source data using single-channel addressing (y * src_width + x) and pushes only one value per pixel. This produces garbled single-channel output from what should be 3-channel RGB data — the capacity is allocated for 3 channels but only dst_width * dst_height values are written. Any consumer expecting proper RGB tensor data for ML inference would get incorrect results.
Reviewed by Cursor Bugbot for commit caf54d9. Configure here.
Owner
Author
|
Closing as superseded by PR #42 — trufflehog.yml added cleanly via ci/trufflehog-scan branch. |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
Sequence DiagramThis diagram shows how GitHub runs the new Trufflehog secrets scanning workflow on pushes and pull requests, scanning repository history for verified secrets and reporting the result as a CI status check. sequenceDiagram
participant Developer
participant GitHub
participant TrufflehogWorkflow
participant GitHubAPI
Developer->>GitHub: Push to main or open pull request
GitHub->>TrufflehogWorkflow: Trigger Trufflehog Secrets Scan job
TrufflehogWorkflow->>TrufflehogWorkflow: Checkout repository with pinned actions checkout
TrufflehogWorkflow->>GitHubAPI: Run trufflehog github scan with configured token
GitHubAPI-->>TrufflehogWorkflow: Scan results for verified secrets
TrufflehogWorkflow-->>GitHub: Publish CI status for commit or pull request
Generated by CodeAnt AI |
Comment on lines
+97
to
+101
| fn run_calibration_ui<B: ratatui::backend::Backend>( | ||
| terminal: &mut Terminal<B>, | ||
| camera_index: usize, | ||
| _num_points: usize, | ||
| ) -> Result<Calibrator> { |
There was a problem hiding this comment.
Suggestion: The calibration point count argument is ignored, so --points has no effect and calibration always runs with the hardcoded 9-point grid. Use the provided point count to build the point list (or validate and reject unsupported values) instead of discarding it. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Calibration CLI ignores --points and always uses 9.
- ⚠️ Users cannot request shorter or denser calibration routines.Steps of Reproduction ✅
1. Invoke the CLI calibration command with a non-default point count, e.g. `eyetracker
calibrate --points 5`, which is wired in `crates/eyetracker-cli/src/main.rs:74-83` to
populate `Commands::Calibrate { points, .. }` and then call
`calibration::run_calibration(cli.camera, *points, save.clone())` at `main.rs:115-117`.
2. In `crates/eyetracker-cli/src/calibration.rs:68-73`, `run_calibration` receives the
`num_points` argument and forwards it as `num_points as usize` into
`run_calibration_ui(&mut terminal, camera_index, num_points as usize)` at
`calibration.rs:81`.
3. Inside `run_calibration_ui` in `crates/eyetracker-cli/src/calibration.rs:97-101`, the
parameter is named `_num_points` and is never used; instead, the calibration grid is
always created from the hard-coded 9-point layout returned by
`generate_calibration_points()` at `calibration.rs:50-65`.
4. When the UI runs its calibration loop (e.g. drawing points in `draw_calibration_ui` at
`calibration.rs:227-284` and iterating over `points.len()` at `calibration.rs:150-151,
199-201`), it always iterates over the fixed 9 points regardless of the `--points`
argument, so the user sees and completes a 9‑point calibration even when requesting a
different count.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-cli/src/calibration.rs
**Line:** 97:101
**Comment:**
*Incomplete Implementation: The calibration point count argument is ignored, so `--points` has no effect and calibration always runs with the hardcoded 9-point grid. Use the provided point count to build the point list (or validate and reject unsupported values) instead of discarding it.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+163
to
+165
| eprintln!("Error listing cameras: {}", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Camera enumeration errors are logged but not returned, so the command exits successfully even when listing cameras failed. Return the error to preserve CLI failure semantics for callers and automation. [logic error]
Severity Level: Major ⚠️
- ⚠️ eyetracker list-cameras reports success despite underlying errors.
- ⚠️ Scripts relying on exit codes mis-handle camera failures.Steps of Reproduction ✅
1. Execute `eyetracker list-cameras` which parses as `Commands::ListCameras` and is
dispatched in `crates/eyetracker-cli/src/main.rs:118-120` to the local `list_cameras()`
function with `list_cameras()?`.
2. Inside `list_cameras` in `crates/eyetracker-cli/src/main.rs:147-168`, the function
calls `eyetracker_camera::list_cameras()` at line 151, which itself returns
`Result<Vec<CameraInfo>>` and can fail (see the definition at
`crates/eyetracker-camera/src/camera.rs:119-125` where devices are probed and errors
propagated).
3. When `eyetracker_camera::list_cameras()` returns `Err(e)`, the `Err(e)` arm at
`crates/eyetracker-cli/src/main.rs:163-165` runs, printing `Error listing cameras: {}` to
stderr, but does not return the error; execution falls through to `Ok(())` at
`main.rs:167-168`.
4. Because the outer `main()` uses `list_cameras()?` at `main.rs:119`, the `Ok(())` result
means `main()` still returns `Ok(())` and the process exits with status 0, so scripts or
users relying on the exit code cannot detect that camera enumeration actually failed.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-cli/src/main.rs
**Line:** 163:165
**Comment:**
*Logic Error: Camera enumeration errors are logged but not returned, so the command exits successfully even when listing cameras failed. Return the error to preserve CLI failure semantics for callers and automation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+163
to
+168
| for i in 0..(self.width * self.height) as usize { | ||
| let r = self.data[i * 3] as f32; | ||
| let g = self.data[i * 3 + 1] as f32; | ||
| let b = self.data[i * 3 + 2] as f32; | ||
| // Luminance formula | ||
| gray_data[i] = (0.299 * r + 0.587 * g + 0.114 * b) as u8; |
There was a problem hiding this comment.
Suggestion: Grayscale conversion indexes self.data as if the buffer is always complete for the declared format; malformed or truncated frames will panic on out-of-bounds access. Validate expected buffer length (or use checked indexing) before per-pixel channel reads. [possible bug]
Severity Level: Major ⚠️
- ❌ Malformed `Frame` data can trigger panics during preprocessing.
- ⚠️ External consumers must avoid passing inconsistent frame buffers.Steps of Reproduction ✅
1. Grayscale conversion is implemented in `Frame::to_grayscale()` at
`crates/eyetracker-camera/src/frame.rs:153-201`.
2. For `FrameFormat::Rgb8`, lines 162-169 loop `for i in 0..(self.width * self.height) as
usize` and index `self.data[i * 3]`, `self.data[i * 3 + 1]`, and `self.data[i * 3 + 2]`
without bounds checks.
3. `Frame::new(data, width, height, FrameFormat::Rgb8)` at line 106 does not validate that
`data.len() == width * height * 3`, so external callers can construct a `Frame` with a
truncated `data` buffer.
4. When such a malformed `Frame` is passed to any code that calls `to_grayscale()` (e.g.,
`preprocess_frame()` in `crates/eyetracker-camera/src/processing.rs:112-118`), the loop
will eventually index past `self.data.len()` and panic with an out-of-bounds access.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-camera/src/frame.rs
**Line:** 163:168
**Comment:**
*Possible Bug: Grayscale conversion indexes `self.data` as if the buffer is always complete for the declared format; malformed or truncated frames will panic on out-of-bounds access. Validate expected buffer length (or use checked indexing) before per-pixel channel reads.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+237
to
+238
| for y in (face_y..(face_y + face_h - 50)).step_by(step as usize) { | ||
| for x in (face_x..(face_x + face_w - 50)).step_by(step as usize) { |
There was a problem hiding this comment.
Suggestion: The scan bounds subtract 50 from unsigned dimensions without saturation; for small frames this underflows and creates invalid loop ranges (panic in debug or huge wrapped ranges in release). Use saturating_sub(50) as done in the inference crate. [logic error]
Severity Level: Major ⚠️
- ❌ Small-resolution frames can crash or misbehave in detection.
- ⚠️ Behavior diverges from safer inference implementation expectations.Steps of Reproduction ✅
1. The camera-side heuristic detector is `detect_eye_region(frame: &Frame)` in
`crates/eyetracker-camera/src/processing.rs:216-291`.
2. It computes face region dimensions with `face_y = height / 6` and `face_h = height / 2`
at lines 225-228, then uses `face_y + face_h - 50` and `face_x + face_w - 50` as the
exclusive upper bounds in the loops at lines 237-238.
3. For small input frames (e.g., `height < 38`), `face_y + face_h` is less than 50, so
evaluating `face_y + face_h - 50` causes unsigned underflow on `u32` (panic in debug
builds, wrapped value near `u32::MAX` in release).
4. Any future caller that passes such a small frame to `detect_eye_region()`—the function
is publicly re-exported from `crates/eyetracker-camera/src/lib.rs:12`—will see either a
panic or an enormous scan range, unlike the safer saturating logic used in the inference
crate at `crates/eyetracker-inference/src/processing.rs:31-33`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-camera/src/processing.rs
**Line:** 237:238
**Comment:**
*Logic Error: The scan bounds subtract 50 from unsigned dimensions without saturation; for small frames this underflows and creates invalid loop ranges (panic in debug or huge wrapped ranges in release). Use `saturating_sub(50)` as done in the inference crate.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+296
to
+297
| let x = region.x.min(frame.width - 1); | ||
| let y = region.y.min(frame.height - 1); |
There was a problem hiding this comment.
Suggestion: Cropping assumes non-zero frame dimensions and subtracts 1 from frame.width/frame.height; zero-sized frames will underflow and panic. Guard zero dimensions before computing clamped coordinates. [possible bug]
Severity Level: Major ⚠️
- ❌ Zero-sized frames passed to `crop_eye_region` can panic.
- ⚠️ Wrapped coordinates corrupt crop behavior in release builds.Steps of Reproduction ✅
1. Eye-region cropping is implemented as `crop_eye_region(frame: &Frame, region:
&EyeRegion)` in `crates/eyetracker-camera/src/processing.rs:295-325` and re-exported in
`crates/eyetracker-camera/src/lib.rs:12`.
2. At lines 296-300, the function clamps coordinates with `let x =
region.x.min(frame.width - 1);` and `let y = region.y.min(frame.height - 1);` before
computing `w` and `h`.
3. If a caller constructs a `Frame` with `width == 0` or `height == 0` (possible via
`Frame::new` at `crates/eyetracker-camera/src/frame.rs:106` since it performs no
validation) and then calls `crop_eye_region(&frame, ®ion)`, the expression `frame.width
- 1` or `frame.height - 1` underflows `u32`.
4. This underflow causes a panic in debug builds ("attempt to subtract with overflow") or
wraps in release, leading to incorrect cropping dimensions and potential subsequent
indexing issues in the copy loop at lines 48-57.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-camera/src/processing.rs
**Line:** 296:297
**Comment:**
*Possible Bug: Cropping assumes non-zero frame dimensions and subtracts 1 from `frame.width`/`frame.height`; zero-sized frames will underflow and panic. Guard zero dimensions before computing clamped coordinates.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+317
to
+332
| // Scale factors based on calibration | ||
| let (scale_x, scale_y) = if let Some(ref cal) = self.screen_calibration { | ||
| (cal.horizontal_scale, cal.vertical_scale) | ||
| } else { | ||
| (0.8, 0.6) // Default scaling | ||
| }; | ||
|
|
||
| // Offset from gaze direction | ||
| let offset_x = gaze.x * scale_x; | ||
| let offset_y = gaze.y * scale_y; | ||
|
|
||
| // Combine face position with gaze offset | ||
| let screen_x = (face_cx + offset_x * face_mesh.ipd() * 5.0).clamp(0.0, 1.0); | ||
| let screen_y = (face_cy - offset_y * face_mesh.ipd() * 3.0).clamp(0.0, 1.0); | ||
|
|
||
| (screen_x, screen_y) |
There was a problem hiding this comment.
Suggestion: Screen calibration is only partially applied: horizontal_offset and vertical_offset are never used during gaze-to-screen conversion, so user calibration offsets have no effect. Apply the full calibration transform (including offsets) before returning screen coordinates. [logic error]
Severity Level: Major ⚠️
- ⚠️ Screen calibration offsets ignored in gaze mapping.
- ⚠️ Future calibration workflows produce systematically biased gaze.Steps of Reproduction ✅
1. In the inference pipeline, create a pipeline instance via
`InferencePipeline::default_pipeline()` at
`crates/eyetracker-inference/src/pipeline.rs:133`, which internally constructs a
`GazeEstimation` with `screen_calibration: None` (see `GazeEstimation::new` at
`gaze_estimator.rs:125-130`).
2. Apply a non-zero screen calibration using the public API
`InferencePipeline::set_calibration()` at `pipeline.rs:147-150`, passing a
`ScreenCalibration` that sets `horizontal_offset` and/or `vertical_offset` (fields defined
at `gaze_estimator.rs:345-352`).
3. Process a frame through `InferencePipeline::process_frame()` at `pipeline.rs:159-221`
until it detects a face and calls `self.gaze_estimator.estimate(fm)` at `pipeline.rs:201`,
which in turn calls `GazeEstimation::gaze_to_screen()` at `gaze_estimator.rs:308-333`.
4. Observe in `gaze_to_screen()` (`gaze_estimator.rs:317-332`) that only
`horizontal_scale` and `vertical_scale` from `ScreenCalibration` are used; the offsets are
never applied, and the `ScreenCalibration::transform()` method that adds
`horizontal_offset`/`vertical_offset` (`gaze_estimator.rs:402-406`) is never called. Any
user-provided calibration offset therefore has no effect on the final `screen_position`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-inference/src/gaze_estimator.rs
**Line:** 317:332
**Comment:**
*Logic Error: Screen calibration is only partially applied: `horizontal_offset` and `vertical_offset` are never used during gaze-to-screen conversion, so user calibration offsets have no effect. Apply the full calibration transform (including offsets) before returning screen coordinates.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+228
to
+253
| match model_type { | ||
| ModelType::FaceDetection => { | ||
| config.input_name = "input".to_string(); | ||
| config.output_names = vec!["boxes".to_string(), "scores".to_string()]; | ||
| config.input_shape = vec![1, 128, 128, 3]; | ||
| } | ||
| ModelType::FaceMesh => { | ||
| config.input_name = "input".to_string(); | ||
| config.output_names = vec!["landmarks".to_string()]; | ||
| config.input_shape = vec![1, 256, 256, 3]; | ||
| } | ||
| ModelType::GazeEstimation => { | ||
| config.input_name = "input".to_string(); | ||
| config.output_names = vec!["gaze".to_string()]; | ||
| config.input_shape = vec![1, 224, 224, 3]; | ||
| } | ||
| ModelType::Combined => { | ||
| config.input_name = "frames".to_string(); | ||
| config.output_names = vec![ | ||
| "face_box".to_string(), | ||
| "landmarks".to_string(), | ||
| "gaze".to_string(), | ||
| ]; | ||
| config.input_shape = vec![1, 256, 256, 3]; | ||
| } | ||
| _ => {} |
There was a problem hiding this comment.
Suggestion: load_config does not handle EyeSegmentation, so that model type silently keeps default FaceMesh-style input/output settings. This breaks model contract for eye-segmentation inference. Add an explicit ModelType::EyeSegmentation branch with correct tensor names and shape. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Eye-segmentation models inherit incorrect IO configuration.
- ⚠️ Future eye-segmentation pipeline will misinterpret outputs.Steps of Reproduction ✅
1. Initialize a model loader using `ModelLoader::new()` at
`crates/eyetracker-inference/src/model.rs:136-147`, which sets up `models_dir` and
`cache_dir`.
2. Call `ModelLoader::load_config(ModelType::EyeSegmentation)` at `model.rs:224-256`,
passing the `EyeSegmentation` variant declared in `ModelType` at `model.rs:20-21`.
3. Inside `load_config`, observe that the `match model_type` at `model.rs:228-253` has
explicit branches only for `FaceDetection`, `FaceMesh`, `GazeEstimation`, and `Combined`;
`ModelType::EyeSegmentation` falls through to the `_ => {}` arm and leaves `config` as the
`ModelConfig::default()` created at `model.rs:224-227`.
4. Verify that the resulting `ModelConfig` for `EyeSegmentation` still has the default
FaceMesh-style settings (`input_name = "input"`, `output_names = ["output"]`, `input_shape
= [1, 256, 256, 3]` from `model.rs:63-75`), which will not match the contract of an actual
eye-segmentation ONNX model once such a model is wired in.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-inference/src/model.rs
**Line:** 228:253
**Comment:**
*Incomplete Implementation: `load_config` does not handle `EyeSegmentation`, so that model type silently keeps default FaceMesh-style input/output settings. This breaks model contract for eye-segmentation inference. Add an explicit `ModelType::EyeSegmentation` branch with correct tensor names and shape.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+131
to
+180
| let src_data: &[u8] = if options.to_grayscale { | ||
| &grayscale | ||
| } else { | ||
| pixels | ||
| }; | ||
|
|
||
| for dst_y in 0..dst_height { | ||
| for dst_x in 0..dst_width { | ||
| // Bilinear interpolation | ||
| let src_x = (dst_x as f32) * (src_width as f32) / (dst_width as f32); | ||
| let src_y = (dst_y as f32) * (src_height as f32) / (dst_height as f32); | ||
|
|
||
| let x0 = src_x as u32; | ||
| let y0 = src_y as u32; | ||
| let x1 = (x0 + 1).min(src_width - 1); | ||
| let y1 = (y0 + 1).min(src_height - 1); | ||
|
|
||
| let fx = src_x - x0 as f32; | ||
| let fy = src_y - y0 as f32; | ||
|
|
||
| let get_pixel = |x: u32, y: u32| -> f32 { | ||
| let idx = (y * src_width + x) as usize; | ||
| src_data.get(idx).copied().unwrap_or(0) as f32 | ||
| }; | ||
|
|
||
| // Bilinear interpolation | ||
| let p00 = get_pixel(x0, y0); | ||
| let p10 = get_pixel(x1, y0); | ||
| let p01 = get_pixel(x0, y1); | ||
| let p11 = get_pixel(x1, y1); | ||
|
|
||
| let value = p00 * (1.0 - fx) * (1.0 - fy) | ||
| + p10 * fx * (1.0 - fy) | ||
| + p01 * (1.0 - fx) * fy | ||
| + p11 * fx * fy; | ||
|
|
||
| // Apply brightness/contrast adjustments | ||
| let mut adjusted = value; | ||
| adjusted = (adjusted - 128.0) * options.contrast_factor + 128.0; | ||
| adjusted += options.brightness_offset as f32; | ||
| adjusted = adjusted.clamp(0.0, 255.0); | ||
|
|
||
| // Normalize to [0, 1] | ||
| let normalized = if options.normalize { | ||
| adjusted / 255.0 | ||
| } else { | ||
| adjusted | ||
| }; | ||
|
|
||
| output.push(normalized); |
There was a problem hiding this comment.
Suggestion: The non-grayscale path is implemented as single-channel sampling: idx is computed as if pixels are 1 byte per pixel and only one value is pushed per destination pixel. When to_grayscale is false (for example with for_onnx()), RGB input is misread and output length/content no longer matches a 3-channel tensor, causing incorrect model input. Sample each channel with RGB-aware indexing and push all channels per pixel. [type error]
Severity Level: Major ⚠️
- ⚠️ ONNX-oriented preprocessing drops two RGB channels entirely.
- ⚠️ Future RGB models receive incorrectly shaped input tensors.Steps of Reproduction ✅
1. Configure preprocessing for ONNX-style RGB input using `PreprocessOptions::for_onnx()`
at `crates/eyetracker-inference/src/processing.rs:82-94`, which sets `to_grayscale =
false` and `normalize = true`.
2. Capture a color frame in the CLI (e.g., via `Camera::capture_frame()` in
`crates/eyetracker-cli/src/app.rs:136-139`) and pass its raw RGB buffer `&frame.data`
along with `frame.width` and `frame.height` into
`eyetracker_inference::preprocess_frame()` at `processing.rs:112-190`, using the ONNX
options from step 1.
3. Inside `preprocess_frame`, observe that when `to_grayscale` is `false`, `src_data` is
set directly to `pixels` (`processing.rs:131-135`), but the bilinear sampler `get_pixel`
at `processing.rs:151-154` computes an index as if there were one byte per pixel (`(y *
src_width + x)`), ignoring the actual 3-byte-per-pixel RGB layout.
4. For each destination pixel, only a single scalar value is computed and pushed
(`output.push(normalized)` at `processing.rs:180`), so the output length is `dst_width *
dst_height` instead of the intended `dst_width * dst_height * 3` implied by the capacity
calculation at `processing.rs:118-120`. Any downstream ONNX model expecting 3-channel
tensors will receive malformed single-channel data.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-inference/src/processing.rs
**Line:** 131:180
**Comment:**
*Type Error: The non-grayscale path is implemented as single-channel sampling: `idx` is computed as if pixels are 1 byte per pixel and only one value is pushed per destination pixel. When `to_grayscale` is `false` (for example with `for_onnx()`), RGB input is misread and output length/content no longer matches a 3-channel tensor, causing incorrect model input. Sample each channel with RGB-aware indexing and push all channels per pixel.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+232
to
+240
| let cdf_min = cdf.iter().find(|&&v| v > 0).copied().unwrap_or(0); | ||
| let total = pixels.len() as u32; | ||
| let max_val = 255.0; | ||
|
|
||
| for pixel in pixels.iter_mut() { | ||
| let bin = ((*pixel * 255.0).round() as usize).clamp(0, 255); | ||
| let cdf_val = cdf[bin]; | ||
| let equalized = ((cdf_val - cdf_min) as f32 / (total - cdf_min) as f32) * max_val; | ||
| *pixel = equalized / 255.0; |
There was a problem hiding this comment.
Suggestion: Histogram equalization can divide by zero when all pixels fall into one CDF level (total == cdf_min), which produces NaN/Inf outputs and poisons downstream inference values. Guard this case and skip equalization (or return a stable constant mapping) when the denominator is zero. [logic error]
Severity Level: Major ⚠️
- ⚠️ Uniform frames cause NaN/Inf in preprocessing output.
- ⚠️ Downstream inference receives non-finite gaze input values.Steps of Reproduction ✅
1. In the CLI tracking path (`crates/eyetracker-cli/src/app.rs:136-143`), frames captured
from `Camera::capture_frame()` are preprocessed via
`eyetracker_inference::preprocess_frame(&frame.data, frame.width, frame.height,
&PreprocessOptions::default())`, where `PreprocessOptions::default()` in
`crates/eyetracker-inference/src/processing.rs:66-78` sets `equalize_histogram = true`.
2. Construct a synthetic uniform grayscale frame buffer (e.g., all zeros or all 128) of
size `width * height` and feed it through `preprocess_frame()` at `processing.rs:112-190`
with the default options, either by substituting this buffer for `frame.data` in the CLI
or via a unit test that calls `preprocess_frame` directly.
3. Because all normalized pixel values in `output` are identical after the resize and
normalization loop at `processing.rs:137-181`, the histogram built in
`equalize_histogram_inplace()` at `processing.rs:217-222` has exactly one non-zero bin,
making `cdf_min` equal to `total` (the number of pixels) in `processing.rs:232-234`.
4. In the normalization step at `processing.rs:236-240`, `equalized` is computed with a
denominator of `(total - cdf_min)`, which is zero in this uniform-frame scenario; this
division by zero produces `NaN`/`Inf` values that are written back into `pixels`,
poisoning the preprocessed buffer the CLI passes into `InferencePipeline::process_frame()`
at `app.rs:141-142`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-inference/src/processing.rs
**Line:** 232:240
**Comment:**
*Logic Error: Histogram equalization can divide by zero when all pixels fall into one CDF level (`total == cdf_min`), which produces NaN/Inf outputs and poisons downstream inference values. Guard this case and skip equalization (or return a stable constant mapping) when the denominator is zero.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
Sequence DiagramThis PR introduces a journey-gate workflow that enforces the presence and validation of journey manifests, and adds a Trufflehog-based secrets scan to CI on every push and pull request. sequenceDiagram
participant Developer
participant GitHubActions
participant JourneyGate
participant Trufflehog
Developer->>GitHubActions: Push commit or open pull request
%% Journey Gate flow
GitHubActions->>JourneyGate: Start Journey Gate workflow
JourneyGate->>JourneyGate: Search for journey manifests
alt No manifest.verified.json files
JourneyGate-->>GitHubActions: Fail job in stub mode with guidance
else Manifests found
JourneyGate->>JourneyGate: Validate manifests and run assertions
JourneyGate-->>GitHubActions: Report pass or fail
end
%% Trufflehog secrets scan flow
GitHubActions->>Trufflehog: Start Trufflehog secrets scan job
Trufflehog->>GitHubActions: Scan repository with GitHub token for verified secrets
Trufflehog-->>GitHubActions: Return findings and job status
Generated by CodeAnt AI |
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5@11bd71901bbe5b1630ceea73d27597364c9af683 |
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
The cargo-deny workflow pins actions/checkout with a double-@ ref (actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5@11bd71901bbe5b1630ceea73d27597364c9af683), which is not a valid GitHub Action reference and will cause this job (and any similarly written jobs) to fail before running their checks.
Suggestion: Update all affected workflows to use a single ref for actions/checkout (e.g., actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683) and validate the workflows run successfully in CI.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** .github/workflows/cargo-deny.yml
**Line:** 13:13
**Comment:**
*CRITICAL: The cargo-deny workflow pins `actions/checkout` with a double-@ ref (`actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5@11bd71901bbe5b1630ceea73d27597364c9af683`), which is not a valid GitHub Action reference and will cause this job (and any similarly written jobs) to fail before running their checks.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+15
to
+17
| - run: trufflehog github --only-verified --no-update | ||
| env: | ||
| GH_TOKEN: \${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The Trufflehog workflow sets GH_TOKEN as \${{ secrets.GITHUB_TOKEN }}, so GitHub does not interpolate the secret and the scan runs with a literal string instead of a valid token, breaking authenticated GitHub scanning.
Suggestion: Remove the escape and pass the secret using standard expression syntax (${{ secrets.GITHUB_TOKEN }}) so Trufflehog receives a real GitHub token for both push and PR events.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** .github/workflows/trufflehog.yml
**Line:** 15:17
**Comment:**
*HIGH: The Trufflehog workflow sets `GH_TOKEN` as `\${{ secrets.GITHUB_TOKEN }}`, so GitHub does not interpolate the secret and the scan runs with a literal string instead of a valid token, breaking authenticated GitHub scanning.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+100
to
+116
| _num_points: usize, | ||
| ) -> Result<Calibrator> { | ||
| // Create camera | ||
| let mut config = CameraConfig::eye_tracking(); | ||
| config.camera_index = Some(camera_index); | ||
| let mut camera = Camera::new(config)?; | ||
|
|
||
| // Create inference pipeline | ||
| let mut pipeline = InferencePipeline::real_time_pipeline(); | ||
|
|
||
| // Create calibrator | ||
| let calibrator = Arc::new(Mutex::new(Calibrator::new())); | ||
| let _estimator = Arc::new(Mutex::new(GazeEstimator::new(None))); | ||
|
|
||
| // Calibration state | ||
| let mut points = generate_calibration_points(); | ||
| let mut current_point_index = 0; |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The --points CLI option for calibration is passed as _num_points but never used; calibration always runs a hard-coded 9-point grid, so user-specified point counts are silently ignored.
Suggestion: Either implement calibration grid generation based on the requested num_points or drop/lock the CLI option to a fixed 9-point flow to avoid misleading users.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** crates/eyetracker-cli/src/calibration.rs
**Line:** 100:116
**Comment:**
*HIGH: The `--points` CLI option for calibration is passed as `_num_points` but never used; calibration always runs a hard-coded 9-point grid, so user-specified point counts are silently ignored.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+27
to
+33
| pub fn run_tracker( | ||
| camera_index: usize, | ||
| fps: u32, | ||
| smooth: bool, | ||
| debug: bool, | ||
| _output: crate::OutputFormat, | ||
| ) -> Result<()> { |
There was a problem hiding this comment.
Suggestion: The output format argument is accepted but explicitly ignored, so user-selected --output behavior is never applied in tracking mode. Either implement branching behavior for formats or remove the option until supported. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ `--output` flag has no effect for tracking.
- ⚠️ Users cannot access promised text/JSON/CSV streaming.Steps of Reproduction ✅
1. Note that the CLI type `OutputFormat` is exposed as a `--output` flag in `Cli` at
`crates/eyetracker-cli/src/main.rs:33-35`, with possible values Json, Text, Csv, and Tui
defined at `main.rs:45-55`.
2. Run `eyetracker track --output text` (or `--output csv` / `--output tui`), so
`Cli::parse()` populates `cli.output` and `main()` dispatches to `Commands::Track` at
`crates/eyetracker-cli/src/main.rs:111-113`.
3. Observe that `main()` passes `cli.output` into `app::run_tracker(cli.camera, *fps,
*smooth, *debug, cli.output)` at `main.rs:113`.
4. In `run_tracker`'s signature at `crates/eyetracker-cli/src/app.rs:27-33`, the parameter
is named `_output: crate::OutputFormat`, which by Rust convention and compiler behavior
indicates the value is intentionally unused.
5. Inspect `run_tracker`'s body (`app.rs:34-81`) and confirm that `_output` is never
referenced; the function always initializes the TUI (via `crossterm` and `ratatui`) and
never switches behavior based on the chosen `OutputFormat`, so user-specified formats have
no effect.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-cli/src/app.rs
**Line:** 27:33
**Comment:**
*Incomplete Implementation: The output format argument is accepted but explicitly ignored, so user-selected `--output` behavior is never applied in tracking mode. Either implement branching behavior for formats or remove the option until supported.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+99
to
+108
| if let Some(pos) = result.screen_position() { | ||
| self.gaze_x = pos.0 as f64; | ||
| self.gaze_y = pos.1 as f64; | ||
| self.confidence = result | ||
| .gaze | ||
| .as_ref() | ||
| .map(|g| g.combined.confidence as f64) | ||
| .unwrap_or(0.0); | ||
| } | ||
| self.face_detected = result.face_detected; |
There was a problem hiding this comment.
Suggestion: When inference returns no screen position, gaze and confidence are not reset, so stale values from older frames remain visible and can be interpreted as current tracking output. Explicitly clear gaze/confidence when no valid position is available. [stale reference]
Severity Level: Major ⚠️
- ⚠️ TUI shows stale gaze coordinates when tracking drops.
- ⚠️ Confidence value does not reflect current detection state.Steps of Reproduction ✅
1. Start tracking via `eyetracker` so that `Commands::Track` in
`crates/eyetracker-cli/src/main.rs:111-113` calls `app::run_tracker`, which in turn runs
the main loop `run_ui` in `crates/eyetracker-cli/src/app.rs:122-182`.
2. While a face is detected, `InferencePipeline::process_frame` in
`crates/eyetracker-inference/src/pipeline.rs:159-221` sets `face_detected = true` and
returns a `Some(gaze)` result, so `InferenceResult::screen_position()` at
`pipeline.rs:95-99` returns `Some((x, y))`.
3. `run_ui` passes each `InferenceResult` to `TrackingState::update` at
`crates/eyetracker-cli/src/app.rs:98-119`; when `screen_position()` is `Some`, the block
at lines 99-107 updates `gaze_x`, `gaze_y`, and `confidence`.
4. When tracking temporarily fails (no face in view, low confidence), `process_frame` sets
`face_detected` to `false` and `gaze` to `None` (`pipeline.rs:193-204`), so
`InferenceResult::screen_position()` returns `None`; in this case, the `if let Some(pos)`
block at `app.rs:99-107` is skipped and `gaze_x`, `gaze_y`, and `confidence` remain at
their previous values, while `self.face_detected` is still updated to `false`
(`app.rs:108`).
5. The UI continues to render `state.gaze_x`, `state.gaze_y`, and `state.confidence` in
both the footer (`draw_ui` at `app.rs:233-252`) and the main view (`draw_gaze_view` at
`app.rs:284-307`), so when `face_detected` is `false` the user sees a "NO FACE"/open-eye
status but the numerical gaze position and confidence reflect stale data from the last
successful frame.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-cli/src/app.rs
**Line:** 99:108
**Comment:**
*Stale Reference: When inference returns no screen position, gaze and confidence are not reset, so stale values from older frames remain visible and can be interpreted as current tracking output. Explicitly clear gaze/confidence when no valid position is available.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+109
to
+116
| self.frame_count += 1; | ||
| self.latency_ms = latency.as_secs_f64() * 1000.0; | ||
|
|
||
| if let Some(last) = self.last_update { | ||
| let elapsed = last.elapsed().as_secs_f64(); | ||
| if elapsed > 0.0 { | ||
| self.fps = self.frame_count as f64 / elapsed; | ||
| } |
There was a problem hiding this comment.
Suggestion: FPS is computed with cumulative frame_count divided by time since only the previous frame, which causes the displayed FPS to grow incorrectly over time. Compute FPS from frame delta over elapsed time (or total frames over total session duration) instead of cumulative count over per-frame interval. [logic error]
Severity Level: Major ⚠️
- ⚠️ TUI footer displays wildly inflated FPS values.
- ⚠️ Misleads users monitoring tracking performance.Steps of Reproduction ✅
1. Run the tracking command so `main()` in `crates/eyetracker-cli/src/main.rs:101-129`
dispatches to `Commands::Track` and calls `app::run_tracker` at line 113.
2. In `run_tracker` (`crates/eyetracker-cli/src/app.rs:27-81`), the main loop in `run_ui`
(`app.rs:136-178`) repeatedly calls `s.update(&result, result.total_latency);` at lines
146-147 for each processed frame.
3. In `TrackingState::update` (`app.rs:97-120`), on every frame `self.frame_count` is
incremented (`app.rs:109`) and `self.last_update` holds the timestamp of the previous
frame (`app.rs:118`), so `elapsed` at `app.rs:113` is the time since the last frame only.
4. The FPS is then computed as `self.fps = self.frame_count as f64 / elapsed;`
(`app.rs:115`), which divides the cumulative frame count by the single-frame delta; as the
session runs, `frame_count` grows while `elapsed` stays around the frame period, causing
the reported `fps` to increase without bound.
5. The footer UI in `draw_ui` at `crates/eyetracker-cli/src/app.rs:231-252` prints
`state.fps` (`line 249`), so the on-screen FPS value becomes unrealistically large
compared to the true frame rate.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-cli/src/app.rs
**Line:** 109:116
**Comment:**
*Logic Error: FPS is computed with cumulative `frame_count` divided by time since only the previous frame, which causes the displayed FPS to grow incorrectly over time. Compute FPS from frame delta over elapsed time (or total frames over total session duration) instead of cumulative count over per-frame interval.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
Sequence DiagramThis PR adds a journey manifest verification gate and a trufflehog-based secrets scan to the CI, enforcing documentation manifests and checking for leaked credentials on every push or pull request. sequenceDiagram
participant Developer
participant GitHubActions
participant JourneyGate
participant PhenotypeCLI
participant TrufflehogScan
Developer->>GitHubActions: Push or open pull request
GitHubActions->>JourneyGate: Start journey gate workflow
JourneyGate->>JourneyGate: Discover manifest.verified.json files
alt No manifests found
JourneyGate->>JourneyGate: Fail stub gate and emit guidance
else Manifests present
JourneyGate->>PhenotypeCLI: Validate and assert manifests
PhenotypeCLI-->>JourneyGate: All manifests and assertions pass
end
GitHubActions->>TrufflehogScan: Start trufflehog secrets scan
TrufflehogScan->>TrufflehogScan: Run repository secrets scan with configured token
TrufflehogScan-->>GitHubActions: Report secrets scan result
Generated by CodeAnt AI |
Comment on lines
+97
to
+101
| fn run_calibration_ui<B: ratatui::backend::Backend>( | ||
| terminal: &mut Terminal<B>, | ||
| camera_index: usize, | ||
| _num_points: usize, | ||
| ) -> Result<Calibrator> { |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The calibration CLI accepts a num_points argument but run_calibration_ui takes _num_points and ignores it, always running a fixed 9‑point calibration regardless of the user-specified value.
Suggestion: Either generate the calibration grid based on num_points, or validate that only 9 is supported and reject other values at the command boundary so the CLI option accurately reflects behavior.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** crates/eyetracker-cli/src/calibration.rs
**Line:** 97:101
**Comment:**
*HIGH: The calibration CLI accepts a `num_points` argument but `run_calibration_ui` takes `_num_points` and ignores it, always running a fixed 9‑point calibration regardless of the user-specified value.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4@11bd71901bbe5b1630ceea73d27597364c9af683 |
There was a problem hiding this comment.
🔴 Architect Review — CRITICAL
The Journey Gate workflow uses an invalid ref actions/checkout@v4@11bd71901bbe5b1630ceea73d27597364c9af683, which is not a valid uses: syntax and will prevent the job from starting.
Suggestion: Replace the double-@ ref with a single immutable ref such as actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 (or another single ref), consistent with the valid pinning used in other workflows like .github/workflows/codeql.yml.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** .github/workflows/journey-gate.yml
**Line:** 58:58
**Comment:**
*CRITICAL: The Journey Gate workflow uses an invalid ref `actions/checkout@v4@11bd71901bbe5b1630ceea73d27597364c9af683`, which is not a valid `uses:` syntax and will prevent the job from starting.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| - uses: trufflehog/actions/setup@main | ||
| - run: trufflehog github --only-verified --no-update | ||
| env: | ||
| GH_TOKEN: \${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
In the Trufflehog workflow GH_TOKEN is set to \${{ secrets.GITHUB_TOKEN }}, so the environment variable value is not a valid GitHub token (either the expression is not expanded or a leading backslash is included), causing Trufflehog to run without a usable credential.
Suggestion: Set GH_TOKEN to the unescaped expression GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} so the step receives a valid token and the scan can authenticate correctly.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** .github/workflows/trufflehog.yml
**Line:** 17:17
**Comment:**
*HIGH: In the Trufflehog workflow `GH_TOKEN` is set to `\${{ secrets.GITHUB_TOKEN }}`, so the environment variable value is not a valid GitHub token (either the expression is not expanded or a leading backslash is included), causing Trufflehog to run without a usable credential.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+129
to
+135
| let frame = match camera.capture_frame() { | ||
| Ok(f) => f, | ||
| Err(e) => { | ||
| tracing::warn!("Frame error: {}", e); | ||
| continue; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Suggestion: On frame-capture errors the loop immediately continues before the sleep at the end, creating a tight hot loop that can peg CPU when camera capture fails repeatedly. Add backoff/sleep in the error path before continuing. [performance]
Severity Level: Major ⚠️
- ⚠️ Calibration command can peg CPU on capture failures.
- ⚠️ Degrades system responsiveness when camera misconfigured or unplugged.Steps of Reproduction ✅
1. Run `eyetracker calibrate` from the CLI (`crates/eyetracker-cli/src/main.rs:74-83`,
dispatched at `main.rs:111-117`) using a camera index that causes `Camera::capture_frame`
to fail repeatedly (e.g., a device that is present but cannot deliver frames).
2. `run_calibration` creates the camera and calls `run_calibration_ui`
(`crates/eyetracker-cli/src/calibration.rs:68-82`), which starts the camera at
`calibration.rs:124-125` and enters the `while calibrating` loop at `calibration.rs:127`.
3. Inside the loop, the frame acquisition code (`calibration.rs:129-135`) matches on
`camera.capture_frame()`: on error it executes the `Err(e)` arm, logging
`tracing::warn!("Frame error: {}", e);` and then immediately `continue;` at
`calibration.rs:131-133`.
4. Because the `continue` jumps back to the top of the loop, the sleep throttling at
`calibration.rs:217-218` (`std::thread::sleep(Duration::from_millis(16));`) is skipped on
every failing iteration; as long as `capture_frame` keeps erroring quickly, this loop runs
as fast as possible and can peg a CPU core.
5. There is no alternate exit condition in the error path (only `calibrating` is toggled
to false after successful `cal.finalize` at `calibration.rs:201-213`), so calibration with
a persistently failing camera will spin until the user terminates the process.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/eyetracker-cli/src/calibration.rs
**Line:** 129:135
**Comment:**
*Performance: On frame-capture errors the loop immediately `continue`s before the sleep at the end, creating a tight hot loop that can peg CPU when camera capture fails repeatedly. Add backoff/sleep in the error path before continuing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
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
1 participant
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.
User description
Adds trufflehog secrets scanning workflow.
Note
High Risk
Large, cross-cutting PR that adds new workspace crates and multiple CI workflows; the new
journey-gateworkflow is in stub mode and will fail CI unlessmanifest.verified.jsonfiles exist, and several workflows changeactions/checkoutreferences in a potentially invalid way.Overview
Introduces new Rust workspace components for end-to-end eye tracking: an
eyetracker-cameracrate (webcam capture + basic frame/ROI preprocessing), aneyetracker-inferencecrate (face-mesh/gaze estimation pipeline scaffolding and preprocessing), and aneyetracker-cliTUI app with tracking and calibration flows; the workspaceCargo.tomlis expanded with shared deps to support these.CI is expanded with additional supply-chain/security automation: adds a
trufflehogsecrets-scan workflow, adds a newjourney-gateworkflow to enforce “journey manifest” validation/assertions (currently configured to hard-fail when nomanifest.verified.jsonfiles are found), updatescargo-denypolicy to ignoreRUSTSEC-2024-0436, and adjusts several workflows’actions/checkoutpinning.Reviewed by Cursor Bugbot for commit caf54d9. Bugbot is set up for automated code reviews on this repo. Configure here.
CodeAnt-AI Description
Add a command-line eye tracker and tighten CI security checks
What Changed
eyetrackercommand that can list cameras, start live tracking, run calibration, show status, and export session dataImpact
✅ Shorter setup for eye tracking✅ Clearer live tracking feedback✅ Fewer secrets slipping into CI🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.