fix(FocalPoint): add serde_json::json import, align clippy MSRV#61
fix(FocalPoint): add serde_json::json import, align clippy MSRV#61KooshaPari wants to merge 35 commits into
Conversation
Issue 1: pii_scrubber test used json!() macro without importing it from serde_json. Issue 2: clippy.toml MSRV (1.75) did not match workspace rust-version (1.82). 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 · |
📝 WalkthroughWalkthroughAdds STATUS.md and expands SPEC/spec.md; updates Cargo workspace members and adds a Git-pinned workspace dependency; switches many crate manifests to workspace resolution; refactors benchmark harnesses and bench code to use helper constructors and in-memory loops; refactors Notion parsing and test fixtures; adds small test imports and test helpers; updates CI/workflows, ignores, and lint settings; and removes Cargo.toml.bak. ChangesDocumentation & Specification
Benchmarks & Cargo Harness Changes
Connector parsing refactor
Small test imports & test helpers
Repository tooling, CI, and ignore rules
Workspace backup removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 updates the project status in STATUS.md to active development, bumps the Minimum Supported Rust Version (MSRV) to 1.82 in clippy.toml, and adds a JSON utility import for telemetry tests. Feedback suggests clarifying the project's lifecycle status to avoid contradictions with previous documentation and recommends removing the redundant MSRV configuration in clippy.toml to simplify maintenance.
| # FocalPoint Status | ||
|
|
||
| FocalPoint is a focus and productivity tracking application. | ||
|
|
||
| ## Current Status | ||
|
|
||
| Active development. See README.md for project details. |
There was a problem hiding this comment.
The previous content of this file referenced SUPERSEDED.md, which contradicts the new 'Active development' status. If the project has been deprecated or replaced, the status should be updated to reflect its actual lifecycle stage rather than using a generic template.
| @@ -1,5 +1,5 @@ | |||
| # Phenotype-org standard clippy config | |||
| msrv = "1.75" | |||
| msrv = "1.82" | |||
There was a problem hiding this comment.
Clippy automatically detects the Minimum Supported Rust Version (MSRV) from the rust-version field in Cargo.toml. Since you are aligning this with the workspace version, consider removing this line from clippy.toml to avoid maintaining the same value in two places.
codeant-ai
Bot
added
the
size:XS
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
Status: No Issues Found | Recommendation: Merge
Files Reviewed (3 files)
STATUS.md— Replaced corrupted status file with proper documentationclippy.toml— Bumped MSRV from 1.75 to 1.82 to match workspace rust-versioncrates/focus-telemetry/src/pii_scrubber.rs— Added missinguse serde_json::json;import for test module
All changes are correct, minimal, and address legitimate issues:
- Missing import was causing test compilation failures
- MSRV misalignment could cause clippy inconsistencies
- STATUS.md content restored to meaningful documentation
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…backup - scorecard.yml: fix malformed checkout action (two SHAs concatenated) - cargo-deny.yml: update checkout to pinned v4 SHA (11bd71901bbe5b1630ceea73d27597364c9af683) - CLAUDE.md: correct crate count from "54+" to "56" (matches cargo metadata) - Remove Cargo.toml.bak stale backup from repo root
- Add MockFamilyControls connector (FR-MOCK-001) with deterministic event generation, polling cadence, and full event type coverage - Add RuleSuggester with 4 heuristic patterns: scheduled focus sessions, missing celebrations, missed check-ins, unlinked actions - Enable criterion benchmarks for focus-eval and focus-rules (harness=true on eval_tick, eval_batched, rule_evaluation) - Add dev-dependencies to focus-policy (criterion, anyhow) - Harden deny.toml wildcards policy: warn → deny - Restore SPEC.md and spec.md to HEAD state Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
KooshaPari
dismissed stale reviews from coderabbitai[bot] and kilo-code-bot[bot]
via
d80636c
|
CodeAnt AI is running Incremental 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:XL
|
CodeAnt AI Incremental review completed. |
Align spec.md with SPEC.md — both now contain the full implemented specification covering purpose, users, scope boundaries, architecture, connector runtime, rules engine, reward/penalty ledger, audit chain, mascot state machine, iOS app, and API design. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| pnpm-lock.yaml | ||
| # Rust | ||
| **/.cargo | ||
| **/Cargo.lock |
There was a problem hiding this comment.
Dockerignore excludes Cargo.lock preventing reproducible builds
Medium Severity
The **/Cargo.lock pattern excludes the workspace Cargo.lock from the Docker build context. For a workspace containing binary targets (CLI tools, servers), this file is essential for deterministic, reproducible builds. Without it, cargo build inside Docker will resolve dependencies to whatever latest-compatible versions are available at build time, which may differ from what was tested locally or in CI.
Reviewed by Cursor Bugbot for commit 1d28c48. Configure here.
…JSON parsing - connector-gcal: hold WATCH_LOCK for entire test body to prevent env var leaking between parallel sibling tests - connector-notion: add fallback for single-object (non-array) responses, support both plain_text and content/text.title paths Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/connector-notion/src/api.rs (2)
222-242:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
parse_task_with_different_statusesfixtures are silently broken — test always passes vacuously.Both
task_todoandtask_donefixtures are missinglast_edited_time, whichparse_single_taskrequires via?. Both calls tofrom_notion_jsonreturn an emptyVec, andassert!(true)on line 241 passes unconditionally, so this test exercises nothing and provides false coverage confidence for FR-NOTION-API-007.🐛 Proposed fix
let task_todo = serde_json::json!({ "object": "page", "id": "task_todo", + "last_edited_time": "2026-04-24T10:00:00.000Z", "properties": { - "status": {"select": {"name": "Todo"}} + "title": {"title": [{"plain_text": "Todo Task"}]}, + "Completed": {"checkbox": false}, + "status": {"select": {"name": "Todo"}} } }); let task_done = serde_json::json!({ "object": "page", "id": "task_done", + "last_edited_time": "2026-04-24T11:00:00.000Z", "properties": { - "status": {"select": {"name": "Done"}} + "title": {"title": [{"plain_text": "Done Task"}]}, + "Completed": {"checkbox": true}, + "status": {"select": {"name": "Done"}} } }); let tasks_todo = NotionTask::from_notion_json(&task_todo); let tasks_done = NotionTask::from_notion_json(&task_done); - assert!(true); + assert_eq!(tasks_todo.len(), 1); + assert!(!tasks_todo[0].completed); + assert_eq!(tasks_done.len(), 1); + assert!(tasks_done[0].completed);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/connector-notion/src/api.rs` around lines 222 - 242, The test parse_task_with_different_statuses constructs fixtures missing the required last_edited_time so NotionTask::from_notion_json (which calls parse_single_task) returns empty Vecs and the test vacuously passes; update the fixtures to include a valid last_edited_time timestamp (e.g. ISO 8601 string) in both task_todo and task_done properties so parse_single_task can succeed, then replace assert!(true) with assertions that the returned Vecs are non-empty and have the expected status values to properly exercise from_notion_json.
246-260:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
parse_paginated_responseresult items lack required fields — parsed pages are silently dropped andassert!(true)masks the issue.The inner page objects
{"object": "page", "id": "p1/p2", "properties": {}}are missingurl,created_time, andlast_edited_time.parse_single_pagereturnsNonefor each (via?), sopagesis always empty. The test claims to verify that the function handles pagination metadata (FR-NOTION-API-008) but never confirms that any pages are actually parsed.🐛 Proposed fix
let paginated = serde_json::json!({ "object": "list", "results": [ - {"object": "page", "id": "p1", "properties": {}}, - {"object": "page", "id": "p2", "properties": {}}, + { + "object": "page", + "id": "p1", + "url": "https://notion.so/p1", + "created_time": "2026-04-23T10:00:00.000Z", + "last_edited_time": "2026-04-24T10:00:00.000Z", + "properties": {"title": {"title": [{"plain_text": "Page 1"}]}} + }, + { + "object": "page", + "id": "p2", + "url": "https://notion.so/p2", + "created_time": "2026-04-23T11:00:00.000Z", + "last_edited_time": "2026-04-24T11:00:00.000Z", + "properties": {"title": {"title": [{"plain_text": "Page 2"}]}} + }, ], "next_cursor": "cursor_abc", "has_more": true }); let pages = NotionPage::from_notion_json(&paginated); - // Should parse without error even with pagination metadata - assert!(true); + assert_eq!(pages.len(), 2, "Both pages should parse successfully");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/connector-notion/src/api.rs` around lines 246 - 260, The test parse_paginated_response is creating page objects missing required fields so parse_single_page returns None and the test masks the bug with assert!(true); update the test data in parse_paginated_response to include the required fields ("url", "created_time", "last_edited_time") for each inner page and then assert that NotionPage::from_notion_json(&paginated) returns a non-empty Vec (or expected count) to validate pages are parsed despite pagination metadata; reference parse_single_page and NotionPage::from_notion_json to locate the parsing logic and ensure the test checks both parsed pages and that pagination keys (next_cursor/has_more) are ignored.crates/focus-rules/benches/rule_evaluation.rs (1)
130-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Utc::now()insideb.iter()benchmarks time-fetching, not cooldown map lookups.Line 146 calls
Utc::now()on every benchmark iteration. That is a syscall (clock_gettime) and is far more expensive than theHashMap::getbeing profiled. The expiry timestamps should be captured once beforeb.iter()so that each iteration measures only the map lookup.🐛 Proposed fix
b.iter(|| { let mut hits = 0; + let now = Utc::now(); for i in 0..1000 { let key = format!("rule_{}", i); if let Some(expires_at) = cooldowns.get(&key) { - if *expires_at > Utc::now() { + if *expires_at > now { hits += 1; } } } black_box(hits) });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/focus-rules/benches/rule_evaluation.rs` around lines 130 - 154, In bench_cooldown_map_hit_path the benchmark calls Utc::now() inside b.iter(), measuring time syscalls instead of HashMap lookup; move the time capture(s) out of the inner closure by computing the expiry timestamps once when building cooldowns (or capture a single fixed now value before b.iter()) so the loop only does cooldowns.get(&key) and compares against the precomputed expires_at values; update the variables referenced (cooldowns, expires_at, hits) accordingly so the inner b.iter() no longer calls Utc::now().crates/focus-eval/Cargo.toml (1)
18-18:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftOut-of-workspace path dependency is the root cause of all CI pipeline failures.
The path
../../../PhenoObservability/crates/phenotype-observably-macrosresolves three levels abovecrates/focus-eval/, landing outside the FocalPoint repository. This requires a sibling checkout ofPhenoObservabilityon the CI runner's filesystem. The pipeline failure confirms this:"failed to read '/home/runner/work/FocalPoint/PhenoObservability/crates/phenotype-observably-macros/Cargo.toml': No such file or directory".Options to unblock CI:
- Add a
git submoduleforPhenoObservabilityand update the CI checkout step to initialise submodules.- Publish
phenotype-observably-macrosto crates.io (or a private registry) and reference it by version.- If the crate is only needed for instrumentation, gate it behind a Cargo feature and disable that feature in CI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/focus-eval/Cargo.toml` at line 18, The Cargo.toml in the focus-eval crate references an out-of-repo path dependency phenotype-observably-macros that breaks CI; fix by replacing the path dependency with one of the three recommended approaches: (A) add PhenoObservability as a git submodule and update CI to init submodules, then keep the path but point it inside the repo tree; (B) publish phenotype-observably-macros to crates.io or a private registry and change the dependency in crates/focus-eval/Cargo.toml to use the published version string instead of the path; or (C) gate the dependency behind a Cargo feature (e.g., feature "observability" in focus-eval's Cargo.toml) and disable that feature in CI so the path/publish dependency is not required during CI builds—choose one option, update crates/focus-eval/Cargo.toml (and CI checkout or feature flags) accordingly, and ensure Cargo.lock/CI configs reflect the change.crates/focus-eval/benches/eval_tick.rs (1)
57-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
_eventon line 62 is allocated but never used in the measured closure.
make_event(0)is called once and assigned to_event, but the|b, _|closure at line 67 only usesrulesfrom the enclosing scope. The allocation is completely dead. If a per-event path is intended here (e.g., passing_eventinto the rule-check body), it needs to be captured; otherwise, remove themake_event(0)call.🐛 Proposed fix (remove dead allocation)
for rule_count in [10, 25, 50, 100] { let rules = black_box((0..rule_count).map(make_rule).collect::<Vec<_>>()); - let _event = black_box(make_event(0)); group.bench_with_input(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/focus-eval/benches/eval_tick.rs` around lines 57 - 82, The variable `_event` created by calling `make_event(0)` inside `bench_eval_tick_per_event_latency` is allocated but never used inside the benchmark closure `|b, _|`; either remove the dead allocation (delete the `_event` assignment) or actually use it in the measured loop (capture `_event` and pass it into the rule-check body) so the benchmark measures the intended per-event path—update the code around the `let _event = black_box(make_event(0));` line and the closure body accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.dockerignore:
- Around line 18-24: The dockerignore currently excludes "**/tests/" which is
too broad and can break Rust crates or other projects that rely on a tests/ tree
for integration tests or shared fixtures; update the .dockerignore to stop
excluding the entire "**/tests/" directory and instead exclude only the specific
unwanted artifacts (e.g., coverage outputs, generated test artifacts, or
temporary files) so that real test sources and fixtures are preserved while
unwanted build/coverage files remain ignored.
- Around line 7-9: The .dockerignore has duplicate exclusion rules: the
recursive globs '**/build/' and '**/target/' are redundant because 'build/' and
'target/' are already present; remove the redundant '**/build/' and '**/target/'
entries (the patterns named "**/build/" and "**/target/") so each directory
exclusion appears only once and avoid duplicate rules.
- Around line 41-42: The .dockerignore currently excludes the Cargo.lock file
via the pattern "**/Cargo.lock"; remove or comment out that pattern so
Cargo.lock is included in the Docker build context (leave other ignores like
"**/.cargo" intact). Ensure the entry "**/Cargo.lock" is deleted or disabled in
.dockerignore so cargo builds inside the image use the committed Cargo.lock for
reproducible dependency resolution.
In @.github/workflows/cargo-deny.yml:
- Line 23: Add a concise version comment next to the pinned SHA for the checkout
action: update the usages reference that currently shows
actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 to include an inline
comment noting the human-readable tag (v4.2.2) so maintainers and tools can see
the version; modify the same line (the actions/checkout reference) to append a
comment like “# v4.2.2” ensuring the SHA remains pinned and the comment only
documents the corresponding version.
In @.github/workflows/scorecard.yml:
- Line 22: Add the same human-readable version comment for the checkout action:
locate the `uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683`
entry and append the `# v4.2.2` comment (matching the `actions/checkout@v4.2.2`
SHA used elsewhere) so Dependabot and reviewers can easily see the intended
version.
In `@crates/connector-notion/src/models.rs`:
- Around line 31-46: Extract the duplicated title-extraction chain into a new
module-level helper function (e.g., fn extract_title(page: &serde_json::Value)
-> &str) and replace the inline chains in parse_single_page and
parse_single_task with calls to this helper; ensure the helper performs the same
get("properties")...unwrap_or("Untitled") logic and is placed above the
NotionPage impl so both parse_single_page and parse_single_task reference it,
preserving the existing return behavior and lifetime semantics used by those
functions.
In `@crates/focus-eval/benches/eval_batched.rs`:
- Around line 36-54: The benchmark currently only reads rule.enabled (in
bench_eval_batched_1000x50 and the related benches) which the optimizer can
constant-fold because make_rule sets enabled: true and events are unused;
replace the trivial branch with real evaluation by either invoking
BatchedRuleEvaluationPipeline::tick (or the pipeline evaluation function used
originally) for each event and rule, or by constructing rules with non-trivial
Condition objects and calling the Condition evaluation against each event so the
event data is consulted; ensure make_rule and make_event produce varying data
(e.g., some rules disabled or differing conditions) and wrap results in
black_box to prevent folding while keeping the benchmark body iterating over
events and calling the actual evaluation code rather than only accessing
rule.enabled.
- Around line 6-34: Extract the duplicated make_event(i: usize) ->
NormalizedEvent and make_rule(i: i32) -> Rule into a shared benches helper
module (e.g., a public module named bench_helpers) so both eval_batched.rs and
eval_tick.rs call the same functions; make the functions pub, add
#[allow(dead_code)] on the module or functions if needed, re-export or import
them in both bench files, and remove the local duplicates (ensure signatures and
returned types match exactly: NormalizedEvent, Rule, Trigger::Event,
Action::GrantCredit, etc.).
In `@crates/focus-eval/Cargo.toml`:
- Around line 34-40: The Cargo.toml bench entries for the Criterion benchmarks
(the [[bench]] sections with name = "eval_tick" and name = "eval_batched")
currently set harness = true which enables Cargo's test harness and conflicts
with Criterion's own harness used via criterion_group!() and criterion_main!()
in the benchmark source; change harness = true to harness = false for both bench
entries so Cargo does not inject its test harness and the Criterion macros can
provide the main function without a linker conflict.
In `@crates/focus-policy/Cargo.toml`:
- Around line 22-24: The bench configuration sets harness = true which conflicts
with Criterion's criterion_main! (duplicate main); change the bench table in
Cargo.toml (the [[bench]] stanza for name "focus_policy_benchmarks") to use
harness = false so the test harness is not injected and Criterion's generated
main is used instead.
In `@crates/focus-rules/Cargo.toml`:
- Around line 26-28: The Cargo.toml bench configuration for the benchmark named
"rule_evaluation" currently sets harness = true which prevents Criterion's own
harness from running; change the bench table for the "rule_evaluation" benchmark
(the [[bench]] entry with name = "rule_evaluation") to set harness = false so
Criterion's criterion_main!/criterion_group! harness can be used.
In `@crates/focus-templates/src/lib.rs`:
- Around line 396-423: The helper functions bytes_to_hex and base64_encode are
test-only and should be moved into the test module: cut bytes_to_hex and
base64_encode from the module root and paste them inside the existing
#[cfg(test)] mod tests { ... } block, remove the #[allow(dead_code)] attributes,
and update any test references (they already call bytes_to_hex/base64_encode) so
they resolve to the in-module helpers; keep base64_encode behavior as-is for
current tests but be aware it produces unpadded base64 if reused later.
In `@spec.md`:
- Around line 1-4: Two spec files (spec.md and SPEC.md) are identical causing
duplication; choose a canonical name (preferably SPEC.md) and remove the
duplicate file. Delete spec.md (or conversely delete SPEC.md if you prefer the
lowercase), update any references in README, docs, CI, or other code that import
or link to the removed filename to point to the chosen canonical file, and
ensure the repository root contains only the single spec file (SPEC.md) so
future edits won't diverge.
- Around line 52-100: The fenced ASCII-art block uses a bare triple-backtick
fence which triggers MD040; update the opening fence from ``` to ```text so
Markdownlint recognizes it as plain text (leave the ASCII diagram content and
the closing ``` unchanged); locate the block that begins with the triple
backticks surrounding the diagram (the ASCII box/diagram lines shown in the
diff) and add the language identifier "text" immediately after the opening
backticks.
In `@SPEC.md`:
- Around line 52-100: The fenced ASCII diagram block starting with
"┌─────────────────────────────────────────────────────────────────────────┐" is
unlabeled; update that triple-backtick fence to include the text language tag
(use ```text) so the block is treated as plain text/ASCII in rendered docs;
locate the large ASCII art block (the section showing "SwiftUI iOS App" and
"Rust Core (54 crates)" and change the opening fence to ```text (and the closing
fence remains ```).
---
Outside diff comments:
In `@crates/connector-notion/src/api.rs`:
- Around line 222-242: The test parse_task_with_different_statuses constructs
fixtures missing the required last_edited_time so NotionTask::from_notion_json
(which calls parse_single_task) returns empty Vecs and the test vacuously
passes; update the fixtures to include a valid last_edited_time timestamp (e.g.
ISO 8601 string) in both task_todo and task_done properties so parse_single_task
can succeed, then replace assert!(true) with assertions that the returned Vecs
are non-empty and have the expected status values to properly exercise
from_notion_json.
- Around line 246-260: The test parse_paginated_response is creating page
objects missing required fields so parse_single_page returns None and the test
masks the bug with assert!(true); update the test data in
parse_paginated_response to include the required fields ("url", "created_time",
"last_edited_time") for each inner page and then assert that
NotionPage::from_notion_json(&paginated) returns a non-empty Vec (or expected
count) to validate pages are parsed despite pagination metadata; reference
parse_single_page and NotionPage::from_notion_json to locate the parsing logic
and ensure the test checks both parsed pages and that pagination keys
(next_cursor/has_more) are ignored.
In `@crates/focus-eval/benches/eval_tick.rs`:
- Around line 57-82: The variable `_event` created by calling `make_event(0)`
inside `bench_eval_tick_per_event_latency` is allocated but never used inside
the benchmark closure `|b, _|`; either remove the dead allocation (delete the
`_event` assignment) or actually use it in the measured loop (capture `_event`
and pass it into the rule-check body) so the benchmark measures the intended
per-event path—update the code around the `let _event =
black_box(make_event(0));` line and the closure body accordingly.
In `@crates/focus-eval/Cargo.toml`:
- Line 18: The Cargo.toml in the focus-eval crate references an out-of-repo path
dependency phenotype-observably-macros that breaks CI; fix by replacing the path
dependency with one of the three recommended approaches: (A) add
PhenoObservability as a git submodule and update CI to init submodules, then
keep the path but point it inside the repo tree; (B) publish
phenotype-observably-macros to crates.io or a private registry and change the
dependency in crates/focus-eval/Cargo.toml to use the published version string
instead of the path; or (C) gate the dependency behind a Cargo feature (e.g.,
feature "observability" in focus-eval's Cargo.toml) and disable that feature in
CI so the path/publish dependency is not required during CI builds—choose one
option, update crates/focus-eval/Cargo.toml (and CI checkout or feature flags)
accordingly, and ensure Cargo.lock/CI configs reflect the change.
In `@crates/focus-rules/benches/rule_evaluation.rs`:
- Around line 130-154: In bench_cooldown_map_hit_path the benchmark calls
Utc::now() inside b.iter(), measuring time syscalls instead of HashMap lookup;
move the time capture(s) out of the inner closure by computing the expiry
timestamps once when building cooldowns (or capture a single fixed now value
before b.iter()) so the loop only does cooldowns.get(&key) and compares against
the precomputed expires_at values; update the variables referenced (cooldowns,
expires_at, hits) accordingly so the inner b.iter() no longer calls Utc::now().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf471ba5-3ab5-4946-955f-65ec665432be
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.dockerignore.github/workflows/cargo-deny.yml.github/workflows/scorecard.yml.gitignoreCLAUDE.mdCargo.toml.bakSPEC.mdcrates/connector-notion/src/api.rscrates/connector-notion/src/models.rscrates/focus-connectors-mock-familycontrols/src/lib.rscrates/focus-eval/Cargo.tomlcrates/focus-eval/benches/eval_batched.rscrates/focus-eval/benches/eval_tick.rscrates/focus-policy/Cargo.tomlcrates/focus-policy/benches/focus_policy_benchmarks.rscrates/focus-rule-suggester/src/lib.rscrates/focus-rules/Cargo.tomlcrates/focus-rules/benches/rule_evaluation.rscrates/focus-templates/src/lib.rsdeny.tomlspec.md
💤 Files with no reviewable changes (2)
- Cargo.toml.bak
- crates/focus-policy/benches/focus_policy_benchmarks.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: semgrep-cloud-platform/scan
🧰 Additional context used
📓 Path-based instructions (2)
deny.toml
📄 CodeRabbit inference engine (CLAUDE.md)
Use deny.toml for cargo-deny security configuration
Files:
deny.toml
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Run quality gate checks:
cargo test --workspace && cargo clippy --all -- -D warnings && cargo fmt --allFormat Rust code using
cargo fmt --allLint Rust code with
cargo clippy --all -- -D warnings(treat warnings as errors)Run tests using
cargo test --workspace
Files:
crates/connector-notion/src/api.rscrates/focus-rule-suggester/src/lib.rscrates/connector-notion/src/models.rscrates/focus-eval/benches/eval_batched.rscrates/focus-rules/benches/rule_evaluation.rscrates/focus-connectors-mock-familycontrols/src/lib.rscrates/focus-templates/src/lib.rscrates/focus-eval/benches/eval_tick.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: KooshaPari/FocalPoint
Timestamp: 2026-05-07T01:20:04.266Z
Learning: Maintain a portable Rust core with a cargo workspace structure (56 crates)
Learnt from: CR
Repo: KooshaPari/FocalPoint
Timestamp: 2026-05-07T01:20:04.266Z
Learning: Use SQLite, PostgreSQL, or SurrealDB for data persistence
🪛 GitHub Actions: cargo-deny / 0_cargo-deny.txt
crates/focus-rules/Cargo.toml
[error] 1-1: cargo-deny (command: check --all-features --manifest-path ./Cargo.toml) failed: failed to fetch crates; failed to load manifest for workspace member /github/workspace/crates/focus-always-on; failed to load manifest for dependency phenotype-observably-macros; failed to read /github/PhenoObservability/crates/phenotype-observably-macros/Cargo.toml: No such file or directory (os error 2).
crates/focus-eval/Cargo.toml
[error] 1-1: cargo-deny (command: check --all-features --manifest-path ./Cargo.toml) failed: failed to fetch crates; failed to load manifest for workspace member /github/workspace/crates/focus-always-on; failed to load manifest for dependency phenotype-observably-macros; failed to read /github/PhenoObservability/crates/phenotype-observably-macros/Cargo.toml: No such file or directory (os error 2).
crates/focus-policy/Cargo.toml
[error] 1-1: cargo-deny (command: check --all-features --manifest-path ./Cargo.toml) failed: failed to fetch crates; failed to load manifest for workspace member /github/workspace/crates/focus-always-on; failed to load manifest for dependency phenotype-observably-macros; failed to read /github/PhenoObservability/crates/phenotype-observably-macros/Cargo.toml: No such file or directory (os error 2).
🪛 GitHub Actions: CI / 0_test.txt
crates/focus-rules/Cargo.toml
[error] 1-1: Cargo test failed: failed to load manifest for workspace member '/home/runner/work/FocalPoint/FocalPoint/crates/focus-always-on'. Cause: failed to load dependency 'phenotype-observably-macros' because Cargo could not read '/home/runner/work/FocalPoint/PhenoObservability/crates/phenotype-observably-macros/Cargo.toml' (No such file or directory, os error 2).
crates/focus-eval/Cargo.toml
[error] 1-1: Cargo test failed: failed to load manifest for workspace member '/home/runner/work/FocalPoint/FocalPoint/crates/focus-always-on'. Cause: failed to load dependency 'phenotype-observably-macros' because Cargo could not read '/home/runner/work/FocalPoint/PhenoObservability/crates/phenotype-observably-macros/Cargo.toml' (No such file or directory, os error 2).
crates/focus-policy/Cargo.toml
[error] 1-1: Cargo test failed: failed to load manifest for workspace member '/home/runner/work/FocalPoint/FocalPoint/crates/focus-always-on'. Cause: failed to load dependency 'phenotype-observably-macros' because Cargo could not read '/home/runner/work/FocalPoint/PhenoObservability/crates/phenotype-observably-macros/Cargo.toml' (No such file or directory, os error 2).
🪛 GitHub Actions: CI / test
crates/focus-rules/Cargo.toml
[error] 1-1: Swatinem/rust-cache step failed: 'cargo metadata --all-features --format-version 1 --no-deps' exited with code 101. Error: failed to load manifest for workspace member '/home/runner/work/FocalPoint/FocalPoint/crates/focus-always-on' referenced by workspace at '/home/runner/work/FocalPoint/FocalPoint/Cargo.toml'.
crates/focus-eval/Cargo.toml
[error] 1-1: Swatinem/rust-cache step failed: 'cargo metadata --all-features --format-version 1 --no-deps' exited with code 101. Error: failed to load manifest for workspace member '/home/runner/work/FocalPoint/FocalPoint/crates/focus-always-on' referenced by workspace at '/home/runner/work/FocalPoint/FocalPoint/Cargo.toml'.
crates/focus-policy/Cargo.toml
[error] 1-1: Swatinem/rust-cache step failed: 'cargo metadata --all-features --format-version 1 --no-deps' exited with code 101. Error: failed to load manifest for workspace member '/home/runner/work/FocalPoint/FocalPoint/crates/focus-always-on' referenced by workspace at '/home/runner/work/FocalPoint/FocalPoint/Cargo.toml'.
🪛 markdownlint-cli2 (0.22.1)
spec.md
[warning] 52-52: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
SPEC.md
[warning] 52-52: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (6)
CLAUDE.md (2)
8-8: Accurate workspace count update.This line now reflects the current workspace size clearly and consistently.
Based on learnings: "Maintain a portable Rust core with a cargo workspace structure (56 crates)".
37-37: Key Files section is consistent with the stack section.Good sync on the crate count here; this avoids doc drift.
Based on learnings: "Maintain a portable Rust core with a cargo workspace structure (56 crates)".
crates/focus-rule-suggester/src/lib.rs (1)
480-480: Good fix: trait import is required forwith_hourin tests.
chrono::Timelikeis needed in scope forwith_hour(...)calls, so this import correctly resolves test compilation.crates/focus-connectors-mock-familycontrols/src/lib.rs (1)
205-205: LGTM: Test import addition is clean and necessary.The
EventTypeimport correctly enables type-safe event type assertions in the test functions below (lines 273, 287), allowing tests to verify that synthetic events generate the expected custom event types.deny.toml (1)
34-36: LGTM — tighteningwildcardsto"deny"is the right call.Promoting wildcard dependency references from a warning to a hard failure prevents accidental or supply-chain-risky
*version ranges from silently passing CI. As per coding guidelines,deny.tomlis the correct place to enforce this policy..gitignore (1)
4-7: LGTM.Both
/target(Rust build artifacts) and/examples/fpl(scaffold output) are appropriate gitignore additions.
| target/ | ||
| dist/ | ||
| build/ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Duplicate exclusion rules for target/ and build/.
target/ (Line 7) and build/ (Line 9) are already covered at the root level. **/build/ (Line 50) and **/target/ (Line 51) are redundant.
🔧 Proposed cleanup
# Java
*.class
**/.gradle
-**/build/
-**/target/
**/.maven/Also applies to: 50-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.dockerignore around lines 7 - 9, The .dockerignore has duplicate exclusion
rules: the recursive globs '**/build/' and '**/target/' are redundant because
'build/' and 'target/' are already present; remove the redundant '**/build/' and
'**/target/' entries (the patterns named "**/build/" and "**/target/") so each
directory exclusion appears only once and avoid duplicate rules.
| # Test/nested | ||
| **/node_modules | ||
| **/target | ||
| **/.pytest_cache | ||
| **/__pycache__ | ||
| **/*.test | ||
| **/tests/ |
There was a problem hiding this comment.
**/tests/ exclusion may break crates that use tests/ for shared fixtures.
Rust integration tests in a tests/ directory are compiled as separate binaries but may include fixture data or helper modules referenced from build.rs or example code. Excluding **/tests/ globally is broad; consider being explicit about what you want to exclude (e.g., coverage report outputs) rather than excluding entire test directories.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.dockerignore around lines 18 - 24, The dockerignore currently excludes
"**/tests/" which is too broad and can break Rust crates or other projects that
rely on a tests/ tree for integration tests or shared fixtures; update the
.dockerignore to stop excluding the entire "**/tests/" directory and instead
exclude only the specific unwanted artifacts (e.g., coverage outputs, generated
test artifacts, or temporary files) so that real test sources and fixtures are
preserved while unwanted build/coverage files remain ignored.
| **/.cargo | ||
| **/Cargo.lock |
There was a problem hiding this comment.
**/Cargo.lock must not be excluded from the Docker build context.
Cargo.lock is what makes Rust builds reproducible. Excluding it means cargo build inside the container resolves dependencies from crates.io at build time, potentially pulling different patch versions than what passed CI. This silently breaks build reproducibility and can introduce regressions or security differences between local/CI builds and the resulting Docker image.
🐛 Proposed fix
# Rust
**/.cargo
-**/Cargo.lock📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **/.cargo | |
| **/Cargo.lock | |
| **/.cargo |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.dockerignore around lines 41 - 42, The .dockerignore currently excludes the
Cargo.lock file via the pattern "**/Cargo.lock"; remove or comment out that
pattern so Cargo.lock is included in the Docker build context (leave other
ignores like "**/.cargo" intact). Ensure the entry "**/Cargo.lock" is deleted or
disabled in .dockerignore so cargo builds inside the image use the committed
Cargo.lock for reproducible dependency resolution.
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5@11bd71901bbe5b1630ceea73d27597364c9af683 | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Same missing version comment as cargo-deny.yml.
Same actions/checkout@v4.2.2 SHA — add # v4.2.2 here too for consistency and Dependabot legibility.
💡 Proposed improvement
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
+ uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/scorecard.yml at line 22, Add the same human-readable
version comment for the checkout action: locate the `uses:
actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683` entry and append the
`# v4.2.2` comment (matching the `actions/checkout@v4.2.2` SHA used elsewhere)
so Dependabot and reviewers can easily see the intended version.
| [[bench]] | ||
| name = "rule_evaluation" | ||
| harness = false | ||
| harness = true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the files exist and read the relevant sections
fd -t f "Cargo.toml" | grep -E "(focus-rules|focus-policy)"Repository: KooshaPari/FocalPoint
Length of output: 126
🏁 Script executed:
# Read the focus-rules Cargo.toml around lines 26-28
cat -n crates/focus-rules/Cargo.toml | head -35Repository: KooshaPari/FocalPoint
Length of output: 925
🏁 Script executed:
# Read the focus-policy Cargo.toml to check for comparison
cat -n crates/focus-policy/Cargo.toml | head -35Repository: KooshaPari/FocalPoint
Length of output: 768
🏁 Script executed:
# Find the rule_evaluation benchmark implementation
fd -t f "rule_evaluation" crates/focus-rules/Repository: KooshaPari/FocalPoint
Length of output: 111
🏁 Script executed:
# Read the rule_evaluation benchmark implementation
cat -n crates/focus-rules/benches/rule_evaluation.rs | head -50Repository: KooshaPari/FocalPoint
Length of output: 2133
🌐 Web query:
Criterion benchmark harness = true vs harness = false Cargo.toml
💡 Result:
In Cargo.toml, [[bench]] harness = false means “disable Cargo’s built-in (libtest) benchmark harness for this bench target, and let the benchmark crate provide its own main/harness code”. Criterion’s documentation uses harness = false for Criterion benches because Criterion supplies its own harness [1][2]. harness = true (the default) means Cargo will use the standard benchmark harness (libtest/#[bench]-style execution) for that bench target; in that case, your benches/*.rs crate is expected to follow the standard libtest benchmark harness model, not Criterion’s criterion_main!/criterion_group! model [1]. Why the distinction matters in practice (Criterion): - Criterion’s getting started guide explicitly says it’s important to disable the standard benchmark harness (i.e., harness = false) “because we'll later add our own and we don't want them to conflict” [2]. - Cargo passes --bench to the benchmark harness regardless of whether harness = false (reported behavior), so disabling the harness doesn’t change all CLI behavior, but it does change which harness is used [3]. - Common Criterion/Cargo issues arise when people override the harness and/or accidentally rely on feature flags not enabled; for example, Criterion issue #344 notes that required-features only affects compilation and you must run cargo bench --features ... so the bench is compiled [4]. (This is about required-features, but it shows how Cargo bench harness configuration affects what actually runs.) What to put in Cargo.toml for Criterion - Use: [[bench]] name = "my_benchmark" harness = false as shown in Criterion’s docs [2].
Citations:
- 1: https://doc.rust-lang.org/cargo/commands/cargo-bench.html
- 2: https://criterion-rs.github.io/book/getting_started.html
- 3: benchmark runner is passed
--bencheven whenharness = falserust-lang/cargo#3027 - 4:
[[bench]] required-features = [...]doesn't seem to work? bheisler/criterion.rs#344
Set harness = false to enable the Criterion benchmarking harness for rule_evaluation.
Criterion benchmarks require harness = false in Cargo.toml to use Criterion's own harness instead of the default libtest harness. With harness = true, Cargo invokes the standard benchmark harness, which conflicts with Criterion's criterion_main! and criterion_group! macros, breaking the benchmark.
Fix
[[bench]]
name = "rule_evaluation"
-harness = true
+harness = false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [[bench]] | |
| name = "rule_evaluation" | |
| harness = false | |
| harness = true | |
| [[bench]] | |
| name = "rule_evaluation" | |
| harness = false |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/focus-rules/Cargo.toml` around lines 26 - 28, The Cargo.toml bench
configuration for the benchmark named "rule_evaluation" currently sets harness =
true which prevents Criterion's own harness from running; change the bench table
for the "rule_evaluation" benchmark (the [[bench]] entry with name =
"rule_evaluation") to set harness = false so Criterion's
criterion_main!/criterion_group! harness can be used.
| #[allow(dead_code)] | ||
| fn bytes_to_hex(bytes: &[u8]) -> String { | ||
| bytes.iter().map(|b| format!("{:02x}", b)).collect() | ||
| } | ||
|
|
||
| // Helper: encode bytes as base64 for testing. | ||
| #[allow(dead_code)] | ||
| fn base64_encode(bytes: &[u8]) -> String { | ||
| const CHARS: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; | ||
| let mut result = String::new(); | ||
| for chunk in bytes.chunks(3) { | ||
| let b1 = chunk[0]; | ||
| let b2 = chunk.get(1).copied().unwrap_or(0); | ||
| let b3 = chunk.get(2).copied().unwrap_or(0); | ||
|
|
||
| let n = ((b1 as u32) << 16) | ((b2 as u32) << 8) | (b3 as u32); | ||
|
|
||
| result.push(CHARS[((n >> 18) & 0x3f) as usize] as char); | ||
| result.push(CHARS[((n >> 12) & 0x3f) as usize] as char); | ||
| if chunk.len() > 1 { | ||
| result.push(CHARS[((n >> 6) & 0x3f) as usize] as char); | ||
| } | ||
| if chunk.len() > 2 { | ||
| result.push(CHARS[(n & 0x3f) as usize] as char); | ||
| } | ||
| } | ||
| result | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether `base64` is a (non-dev) dependency of focus-templates and
# whether `bytes_to_hex`/`base64_encode` are used anywhere outside tests.
echo "=== Cargo.toml for focus-templates ==="
fd -i "Cargo.toml" --full-path crates/focus-templates --exec cat {}
echo ""
echo "=== All usages of bytes_to_hex and base64_encode ==="
rg -n "bytes_to_hex\|base64_encode" --type=rs -C2Repository: KooshaPari/FocalPoint
Length of output: 750
🏁 Script executed:
#!/bin/bash
# Search for usages of bytes_to_hex and base64_encode without type flag
echo "=== Searching for bytes_to_hex and base64_encode usages ==="
rg -n "bytes_to_hex|base64_encode" crates/focus-templates/src/ -A 2 -B 2
echo ""
echo "=== Check the actual lib.rs file around lines 396-423 ==="
sed -n '390,430p' crates/focus-templates/src/lib.rs
echo ""
echo "=== Check test module and usages around lines 598-599, 627, 629 ==="
sed -n '590,635p' crates/focus-templates/src/lib.rsRepository: KooshaPari/FocalPoint
Length of output: 5051
🏁 Script executed:
#!/bin/bash
echo "=== Root Cargo.toml workspace section ==="
head -100 Cargo.toml | grep -A 50 "\[workspace"
echo ""
echo "=== Search for base64 in any Cargo.toml ==="
fd Cargo.toml -x grep -l "base64" {}Repository: KooshaPari/FocalPoint
Length of output: 2582
Move test-only helpers inside #[cfg(test)] — #[allow(dead_code)] suppresses a legitimate lint.
bytes_to_hex and base64_encode (lines 396–423) are defined at module level but are only called from the test module (lines 598–599, 627, 629). This placement causes them to:
- Compile into release builds, inflating the binary unnecessarily.
- Pollute the module's namespace even though they have no production use.
- Require
#[allow(dead_code)]to suppress a valid clippy warning, which directly contradicts the-D warningsgate in the coding guidelines.
Move both functions inside the #[cfg(test)] mod tests { block to eliminate the attribute and signal their test-only purpose.
Secondary note on base64_encode: The function produces unpadded base64 (no = suffix). This is acceptable for the current use case (64-byte ed25519 signatures with the custom base64_decode function), but it is non-standard and will silently produce incorrect output if applied to variable-length inputs where length % 3 ≠ 0. If this helper is ever reused elsewhere, add proper padding (= characters).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/focus-templates/src/lib.rs` around lines 396 - 423, The helper
functions bytes_to_hex and base64_encode are test-only and should be moved into
the test module: cut bytes_to_hex and base64_encode from the module root and
paste them inside the existing #[cfg(test)] mod tests { ... } block, remove the
#[allow(dead_code)] attributes, and update any test references (they already
call bytes_to_hex/base64_encode) so they resolve to the in-module helpers; keep
base64_encode behavior as-is for current tests but be aware it produces unpadded
base64 if reused later.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| }); | ||
| } | ||
| black_box(total_matches) | ||
| }); |
There was a problem hiding this comment.
Benchmarks no longer measure actual rule evaluation
Medium Severity
All benchmark bodies were gutted to just count rule.enabled boolean flags in a loop, completely ignoring the event (note _event prefix). They no longer exercise trigger matching, condition evaluation, the rule engine, or any evaluation pipeline logic. The benchmark names (e.g., eval_batched_1000_events_50_rules) are now misleading — they measure loop iteration speed, not evaluation performance. The project's bench-guard tool would never catch real performance regressions with these benchmarks.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ca06f9b. Configure here.
tokio_tungstenite Message::text now returns Message directly instead of Result. Remove redundant Ok pattern matching. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
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 5d4f1ee. Configure here.
| @@ -1,5 +1,5 @@ | |||
| # Phenotype-org standard clippy config | |||
| msrv = "1.75" | |||
| msrv = "1.82" | |||
There was a problem hiding this comment.
clippy MSRV not aligned with workspace rust-version
Low Severity
The PR title states it aligns clippy.toml MSRV to the workspace rust-version, but clippy.toml is set to "1.82" while the workspace rust-version in Cargo.toml was bumped to "1.85" in the same commit. They remain misaligned — clippy may flag lints for APIs available since 1.82 that are fine given the actual 1.85 minimum.
Reviewed by Cursor Bugbot for commit 5d4f1ee. Configure here.
…nment, edition 2024 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
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:XL
|
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:XL
|
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:XL
|
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:XL
|
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 Incremental 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:XL
|
|
|
||
| // Main evaluation loop: for each event, evaluate all rules | ||
| for event in events.iter() { | ||
| for _event in events.iter() { |
There was a problem hiding this comment.
CRITICAL: Benchmark only measures rule.enabled guard — no rule evaluation pipeline invocation
All three benchmark functions (bench_eval_tick_100x50, bench_eval_tick_per_event_latency, and bench_eval_batched_*) measure only a for rule in rules.iter() { if rule.enabled { count += 1; } } loop. The actual BatchedRuleEvaluationPipeline::tick() call (or equivalent rule engine invocation) was removed in this refactor. The benchmark now measures nothing beyond the cost of iterating over one struct field — it is a dead benchmark with zero diagnostic value. Restore actual rule evaluation in b.iter(...).
|
|
||
| b.iter(|| { | ||
| let mut total_matches = 0; | ||
| for _event in events.iter() { |
There was a problem hiding this comment.
CRITICAL: Benchmark only measures rule.enabled guard — no rule evaluation pipeline invocation
All three benchmark functions measure only a for rule in rules.iter() { if rule.enabled { count += 1; } } loop. The actual BatchedRuleEvaluationPipeline::tick() call was removed in this refactor (alongside the same removal from eval_tick.rs and rule_evaluation.rs). The benchmark returns only events.len() × rules.len() and measures nothing about rule evaluation cost. This is a dead benchmark that provides no actionable performance data.
| payload: serde_json::json!({"reason": "test"}), | ||
| }); | ||
| let rule = black_box(make_rule()); | ||
| let _event = black_box(make_event()); |
There was a problem hiding this comment.
WARNING: Unused variable _event — outer binding is dead and shadows inner loop variable
let _event = black_box(make_event()) on line 45 is never read; the same name is re-bound inside the b.iter(|| { ... }) closure. This outer declaration wastes an eval call on every iter() pass and will trigger unused_variables under cargo clippy -- -D warnings. Drop the redundant binding and use drop(make_event()) if you must preserve the allocation cost path, or delete the line entirely.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| b.iter(|| { | ||
| let mut total_matches = 0; | ||
| for _event in events.iter() { | ||
| for rule in rules.iter() { | ||
| if rule.enabled { | ||
| total_matches += 1; | ||
| } | ||
| } | ||
|
|
||
| // Create 50 rules | ||
| let rules = black_box( | ||
| (0..50) | ||
| .map(|i| Rule { | ||
| id: Uuid::new_v4(), | ||
| name: format!("rule_{}", i), | ||
| trigger: Trigger::Event("focus_event".to_string()), | ||
| conditions: vec![], | ||
| actions: vec![Action::GrantCredit { | ||
| amount: 1, | ||
| reason: "evaluation".to_string(), | ||
| }], | ||
| priority: i as i32, | ||
| cooldown: None, | ||
| duration: None, | ||
| explanation_template: "Pipeline eval".to_string(), | ||
| enabled: true, | ||
| }) | ||
| .collect::<Vec<_>>(), | ||
| ); | ||
|
|
||
| let rule_store = Arc::new(InMemoryRuleStore::new(rules)); | ||
| let engine = Arc::new(RwLock::new(focus_rules::RuleEngine::new())); | ||
| let wallet: Arc<dyn focus_storage::ports::WalletStore> = | ||
| Arc::new(InMemoryWalletStore::new()); | ||
| let penalty: Arc<dyn focus_storage::ports::PenaltyStore> = | ||
| Arc::new(InMemoryPenaltyStore::new()); | ||
| let cursor: Arc<dyn focus_sync::CursorStore> = | ||
| Arc::new(InMemoryCursorStore::new()); | ||
| let audit: Arc<dyn focus_audit::AuditSink> = | ||
| Arc::new(focus_audit::CapturingAuditSink::new()); | ||
| let sink: Arc<dyn focus_eval::DecisionSink> = | ||
| Arc::new(focus_eval::NoopDecisionSink); | ||
|
|
||
| let pipeline = BatchedRuleEvaluationPipeline::new( | ||
| events_store as Arc<dyn focus_storage::ports::EventStore>, | ||
| rule_store, | ||
| engine, | ||
| wallet, | ||
| penalty, | ||
| cursor, | ||
| audit, | ||
| sink, | ||
| Uuid::nil(), | ||
| ); | ||
|
|
||
| let _report = pipeline.tick(now).await.unwrap(); | ||
| black_box(_report) | ||
| }); | ||
| } | ||
| black_box(total_matches) | ||
| }); |
There was a problem hiding this comment.
Suggestion: These batched benchmarks no longer evaluate rules against event content and instead only count enabled flags, so benchmark results are disconnected from real rule-engine behavior. Restore actual trigger/condition evaluation in the inner loop to avoid misleading performance numbers. [logic error]
Severity Level: Major ⚠️
- ⚠️ Batched rule-eval benchmarks ignore real condition evaluation cost.
- ⚠️ Performance regressions in RuleEngine may go undetected.
- ⚠️ Benchmark numbers diverge from production evaluation workloads.Steps of Reproduction ✅
1. Run `cargo bench -p focus-eval`, which executes the Criterion benchmark group defined
by `criterion_group!(benches, bench_eval_batched_1000x50,
bench_eval_batched_scaling_rules, bench_eval_batched_scaling_events);` at
`crates/focus-eval/benches/eval_batched.rs:116-121`.
2. In `bench_eval_batched_1000x50(c: &mut Criterion)` at
`crates/focus-eval/benches/eval_batched.rs:37-54`, the setup allocates 1000
`NormalizedEvent` instances via `make_event` and 50 `Rule` instances via `make_rule`, each
with `enabled: true` (line 32), mimicking real rule-engine inputs.
3. The hot loop being benchmarked is the closure at lines 42-52: `b.iter(|| { let mut
total_matches = 0; for _event in events.iter() { for rule in rules.iter() { if
rule.enabled { total_matches += 1; } } } black_box(total_matches) });`. This loop never
inspects event payloads (`payload` set at line 16), triggers, or conditions, and never
calls into the actual rule engine (`RuleEngine::evaluate_all`) or pipeline
(`RuleEvaluationPipeline::tick` in `crates/focus-eval/src/lib.rs:157-193`).
4. Because `make_rule` sets `enabled: true` for all rules and no real evaluation is
performed, the measured time is effectively just N×M nested iteration plus a boolean
check, independent of event content or rule complexity. This disconnects the reported
throughput from the production evaluation path (`engine.evaluate_all(&rules, event, now)`
at `src/lib.rs:191`), so the batched benchmarks no longer reflect true rule-engine
performance when developers run `cargo bench`.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/focus-eval/benches/eval_batched.rs
**Line:** 42:52
**Comment:**
*Logic Error: These batched benchmarks no longer evaluate rules against event content and instead only count `enabled` flags, so benchmark results are disconnected from real rule-engine behavior. Restore actual trigger/condition evaluation in the inner loop to avoid misleading performance numbers.
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 rule_count in [10, 25, 50, 100] { | ||
| let rules = black_box((0..rule_count).map(make_rule).collect::<Vec<_>>()); | ||
| let _event = black_box(make_event(0)); |
There was a problem hiding this comment.
Suggestion: The benchmark labeled as per-event latency creates an event but never uses it, so it only measures iterating enabled rules and misses actual event-dispatch/matching cost. Use the event inside the benchmarked loop (e.g., pass it through the same evaluation path as production) so the metric reflects real per-event latency. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Per-event latency benchmark omits event-matching and dispatch work.
- ⚠️ Developers may underestimate real tick latency under load.
- ⚠️ Rule-count scaling is measured without realistic per-event cost.Steps of Reproduction ✅
1. Run `cargo bench -p focus-eval`, which includes the Criterion group defined in
`crates/focus-eval/benches/eval_tick.rs:105-110` and executes
`bench_eval_tick_per_event_latency`.
2. In `bench_eval_tick_per_event_latency(c: &mut Criterion)` at
`crates/focus-eval/benches/eval_tick.rs:57-82`, each iteration of the outer loop over
`rule_count` builds a `rules` vector via `make_rule` (line 61) and then creates a single
`NormalizedEvent` via `let _event = black_box(make_event(0));` at line 62.
3. The benchmarked closure passed to `group.bench_with_input` at lines 64-78 ignores
`_event` completely; it only iterates `for rule in rules.iter()` and increments `matches`
when `rule.enabled` is true (lines 68-75), then `black_box(matches)` is called. No event
fields, triggers, or conditions are consulted, and there is no call into
`RuleEngine::evaluate_all` or the `RuleEvaluationPipeline::tick` implementation in
`crates/focus-eval/src/lib.rs:157-193`.
4. As a result, the benchmark labeled `eval_tick_per_event` measures the cost of scanning
enabled rules without any per-event matching or dispatch work, so developers running this
benchmark get a misleading per-event latency metric that omits the actual event evaluation
overhead present in production pipeline ticks.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/focus-eval/benches/eval_tick.rs
**Line:** 62:78
**Comment:**
*Incomplete Implementation: The benchmark labeled as per-event latency creates an event but never uses it, so it only measures iterating enabled rules and misses actual event-dispatch/matching cost. Use the event inside the benchmarked loop (e.g., pass it through the same evaluation path as production) so the metric reflects real per-event latency.
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
| unsafe { std::env::set_var("FOCALPOINT_LOG_LEVEL", "warn"); } | ||
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | ||
| unsafe { std::env::set_var("FOCALPOINT_LOG_FORMAT", "json"); } |
There was a problem hiding this comment.
Suggestion: These process-wide environment mutations are done without any synchronization, which is unsound in Rust 2024 when tests run in parallel. Concurrent env access from other test threads can cause undefined behavior; guard all env mutation/read in this module with a global mutex (or run this test serially). [race condition]
Severity Level: Major ⚠️
- ⚠️ focus-observability tests risk undefined behavior from env races.
- ⚠️ Tracing env tests may become flaky under parallel runs.Steps of Reproduction ✅
1. Run `cargo test -p focus-observability`, which executes all unit and integration tests
in a single test binary with parallel execution by default.
2. One test thread executes `test_init_tracing_respects_env_vars` in
`crates/focus-observability/src/integration_tests.rs:33-47` (file lines 162-176), calling
`unsafe { std::env::set_var("FOCALPOINT_LOG_LEVEL", "warn"); }` and `unsafe {
std::env::set_var("FOCALPOINT_LOG_FORMAT", "json"); }` at lines 165 and 167.
3. Concurrently, another test thread executes `test_init_tracing_with_pretty_format` in
`crates/focus-observability/src/lib.rs:15-24` (lines 155-162), which also performs `unsafe
{ std::env::set_var("FOCALPOINT_LOG_FORMAT", "pretty"); }` and later
`std::env::remove_var("FOCALPOINT_LOG_FORMAT")` (shown in `lib.rs` lines 18-22 via the
Read tool).
4. Because there is no global mutex or other synchronization around these
`std::env::set_var` / `std::env::remove_var` calls (confirmed by Grep for `ENV_MUTEX` in
this crate), two threads can mutate and read the same process-wide environment variables
concurrently, which is explicitly undefined behavior in Rust 2024, leading to potential
flakiness or memory-unsafe behavior during the focus-observability test suite.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/focus-observability/src/integration_tests.rs
**Line:** 165:167
**Comment:**
*Race Condition: These process-wide environment mutations are done without any synchronization, which is unsound in Rust 2024 when tests run in parallel. Concurrent env access from other test threads can cause undefined behavior; guard all env mutation/read in this module with a global mutex (or run this test serially).
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| let _lock = ENV_MUTEX.lock().expect("env lock"); | ||
| std::env::set_var(focus_coaching::KILL_SWITCH_ENV, "1"); | ||
| // SAFETY: process-wide env mutation occurs in process-local test guarded by a mutex. | ||
| unsafe { std::env::set_var(focus_coaching::KILL_SWITCH_ENV, "1") }; |
There was a problem hiding this comment.
Suggestion: The kill-switch env var is set and later removed manually, but cleanup is not panic-safe: any panic between these points (for example from unwrap) leaves global process state mutated and can corrupt subsequent tests. Use an RAII/defer-style guard so removal always runs during unwinding. [missing cleanup]
Severity Level: Major ⚠️
- ⚠️ Focus-rituals tests may leak kill-switch env state.
- ⚠️ Subsequent tests may silently short-circuit coaching behavior.Steps of Reproduction ✅
1. Run `cargo test -p focus-rituals`, which executes tests in
`crates/focus-rituals/src/lib.rs` and other modules in one process.
2. The test `no_llm_call_when_kill_switch_set` in `crates/focus-rituals/src/lib.rs:27-48`
(file lines 826-847) acquires `ENV_MUTEX`, then sets the global kill-switch with `unsafe {
std::env::set_var(focus_coaching::KILL_SWITCH_ENV, "1") };` at line 831, constructs a
Tokio runtime, builds a `RitualsEngine`, and calls
`engine.generate_morning_brief(...).unwrap();`.
3. If any operation between the `set_var` at line 831 and the `unsafe {
std::env::remove_var(focus_coaching::KILL_SWITCH_ENV) };` at line 845 panics (for example,
if `generate_morning_brief` or an `unwrap()` in that path starts failing in future
changes), stack unwinding will drop the `_lock` guard on `ENV_MUTEX` but will not execute
the `remove_var` call, leaving `FOCALPOINT_DISABLE_COACHING` set to `"1"` for the
remainder of this test process.
4. Subsequent tests in the same binary that indirectly depend on the kill-switch being
unset—e.g., weekly review tests calling `WeeklyReviewEngine::generate_weekly_review` in
`crates/focus-rituals/src/weekly.rs:60-135`, which uses
`focus_coaching::complete_guarded`, which in turn checks `kill_switch_on()` in
`crates/focus-coaching/src/lib.rs:308-322`—will observe `KILL_SWITCH_ENV=1` and
short-circuit LLM calls unexpectedly, altering behavior and potentially causing subtle
cross-test contamination; an RAII guard around the env mutation would ensure cleanup even
on panic.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/focus-rituals/src/lib.rs
**Line:** 831:831
**Comment:**
*Missing Cleanup: The kill-switch env var is set and later removed manually, but cleanup is not panic-safe: any panic between these points (for example from `unwrap`) leaves global process state mutated and can corrupt subsequent tests. Use an RAII/defer-style guard so removal always runs during unwinding.
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| let key = format!("rule_{}", i); | ||
| if let Some(expires_at) = cooldowns.get(&key) { | ||
| if *expires_at > chrono::Utc::now() { | ||
| if *expires_at > Utc::now() { |
There was a problem hiding this comment.
Suggestion: This benchmark now depends on wall-clock time inside the hot loop, so results drift as the benchmark runs (after ~60s entries stop counting as hits) and the timing mostly measures repeated Utc::now() calls rather than map lookup cost. Capture now once outside the loop (or use a fixed instant) to keep the benchmark stable and meaningful. [performance]
Severity Level: Major ⚠️
- ⚠️ Cooldown-map benchmark measures clock, not map performance.
- ⚠️ Unstable results hinder regression detection for rule engine.Steps of Reproduction ✅
1. Run the benchmarks with `cargo bench -p focus-rules --bench rule_evaluation`, which
executes `bench_cooldown_map_hit_path` in
`crates/focus-rules/benches/rule_evaluation.rs:130-154`.
2. In the benchmark setup, a `HashMap<String, DateTime<Utc>>` named `cooldowns` is
populated in a loop at lines 132-137, storing `Utc::now() + chrono::Duration::seconds(60)`
for 1000 keys, and then wrapped with `black_box(cooldowns)` at line 139.
3. Inside the hot benchmark loop (`b.iter` at lines 141-151), the code iterates 1000
times, computes `let key = format!("rule_{}", i);`, looks up `expires_at` from the map,
and then executes `if *expires_at > Utc::now() { hits += 1; }` at line 146, invoking
`Utc::now()` 1000 times per iteration.
4. This design makes the benchmark timing dominated by repeated `Utc::now()` calls and
wall-clock variation rather than pure map lookup behavior, and once more than ~60 seconds
have elapsed since map construction, the condition `*expires_at > Utc::now()` will flip
from always-true to always-false, changing `hits` from 1000 to 0 and making measured
performance depend on bench runtime rather than the cooldown-map hit-path cost; capturing
`Utc::now()` once outside the inner loop (or using a fixed timestamp) would stabilize and
clarify what the benchmark measures.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/focus-rules/benches/rule_evaluation.rs
**Line:** 146:146
**Comment:**
*Performance: This benchmark now depends on wall-clock time inside the hot loop, so results drift as the benchmark runs (after ~60s entries stop counting as hits) and the timing mostly measures repeated `Utc::now()` calls rather than map lookup cost. Capture `now` once outside the loop (or use a fixed instant) to keep the benchmark stable and meaningful.
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 Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
18-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCI gate is missing required workspace build/format checks and full clippy scope.
Please add
cargo build --workspaceandcargo fmt --allchecks, and run clippy with--all -- -D warningsto match the repo requirements.Suggested workflow adjustment
- - run: cargo test --all-features --workspace - - run: cargo clippy --all-features -- -D warnings + - run: cargo build --workspace + - run: cargo fmt --all -- --check + - run: cargo test --workspace + - run: cargo clippy --all -- -D warningsAs per coding guidelines
**/*.rs: Build the Rust workspace usingcargo build --workspace, format usingcargo fmt --all, and lint usingcargo clippy --all -- -D warnings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 18 - 20, Update the CI job to include workspace build and formatting checks and run clippy across all packages: add a step that runs "cargo build --workspace", add a step that runs "cargo fmt --all" before tests, keep "cargo test --all-features --workspace" and change the clippy invocation from "cargo clippy --all-features -- -D warnings" to "cargo clippy --all -- -D warnings" so linting covers the entire workspace; ensure step ordering is build -> fmt -> test -> clippy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/connector-gcal/src/api.rs`:
- Around line 582-583: Hold WATCH_LOCK for the entire set→async call→restore
sequence: acquire WATCH_LOCK before calling unsafe
std::env::set_var("FOCALPOINT_GCAL_WEBHOOK_URL", ...), keep the lock held across
the await of client.watch_channel_create(...) (both successful and error paths),
then perform the env restoration and release the lock only after restoration;
update the critical sections around the calls to
client.watch_channel_create(...) (both occurrences) so the lock scope starts
before set_var and ends after set_var restoration to prevent concurrent env
mutation while the async call is in-flight.
In `@crates/connector-notion/src/api.rs`:
- Around line 245-248: The test currently only checks that
NotionTask::from_notion_json(&task_todo) and (&task_done) produce non-empty
vectors (tasks_todo, tasks_done) which is insufficient; update the assertions to
validate parsed semantic fields instead: inspect the returned NotionTask entries
(e.g., first element of tasks_todo and tasks_done) and assert that their
completion-related field(s) (e.g., completed, status, or similar fields on the
NotionTask struct) match the expected values for todo vs done, and also assert
expected title/content text to ensure correct mapping; use the
NotionTask::from_notion_json result and its field names to drive these stronger
assertions.
In `@crates/focus-cli/tests/release_notes_llm.rs`:
- Around line 12-13: The tests mutate the same env var
FOCALPOINT_RELEASE_NOTES_LLM using unsafe env::set_var without a real guard; add
a real synchronization primitive (e.g., a static Mutex/OnceCell guard) and lock
it around the env::set_var / env::remove_var calls in both test functions
(referencing env::set_var and FOCALPOINT_RELEASE_NOTES_LLM in
release_notes_llm.rs) to serialize access, and remove the misleading
"process-wide ENV_MUTEX guard" comment; alternatively annotate tests to run
serially or use serial_test, but ensure all env mutations are protected by the
chosen guard.
In `@crates/focus-cli/tests/template_marketplace.rs`:
- Around line 31-32: Several tests (test_template_search_local_fallback,
test_template_show_local_fallback, test_template_registry_url_env_var,
test_template_auth_token_env_var) call unsafe { env::set_var(...) } and mutate
global process environment without restoring it; replace those direct sets with
a scoped guard pattern that captures the original env::var_os value, sets the
env var (e.g., FOCALPOINT_EXAMPLES, FOCALPOINT_TEMPLATE_REGISTRY,
FOCALPOINT_TEMPLATE_TOKEN) for the test, and restores the original value in Drop
(or use a test synchronization mechanism) so the environment is always restored
after each test finishes; locate the set calls in those test functions and
introduce a small helper (e.g., EnvVarGuard) to manage save/set/restore around
the test scope.
In `@crates/focus-observability/src/integration_tests.rs`:
- Around line 164-175: The test mutates process-wide environment unsafely (calls
to std::env::set_var, std::env::remove_var and std::env::var) and must be
serialized; introduce a global test lock (e.g., a static Mutex<()> via
once_cell::sync::Lazy or similar) and acquire the lock around the env operations
in the test before calling set_var/var/remove_var, releasing it after the checks
so parallel tests cannot race; reference the env functions set_var, remove_var,
and std::env::var in the test and wrap those calls with the lock to ensure
exclusive access.
In `@crates/focus-observability/src/lib.rs`:
- Around line 137-140: Wrap all test code that mutates process env vars
("FOCALPOINT_LOG_FORMAT" and "FOCALPOINT_LOG_LEVEL") with a global test mutex to
prevent races: introduce a static mutex (e.g. static TEST_ENV_MUTEX:
once_cell::sync::Lazy<std::sync::Mutex<()>> = once_cell::sync::Lazy::new(||
std::sync::Mutex::new(()));) and lock it (let _guard =
TEST_ENV_MUTEX.lock().unwrap();) before calling unsafe { std::env::set_var(...)
} in the tests referenced (the blocks calling std::env::set_var), releasing
automatically when the guard drops; apply the same guard around all other env
mutations in this file so tests no longer mutate process-global env
concurrently.
In `@crates/focus-rituals/src/lib.rs`:
- Around line 830-831: The test mutates the process-wide env var KILL_SWITCH_ENV
under ENV_MUTEX, but kill_switch_on() in focus-coaching reads that env var
without synchronization, causing a race; fix by ensuring both readers and
writers use the same synchronization or by avoiding env mutation: either (A)
move ENV_MUTEX (or another lock) into the focus-coaching crate and have
kill_switch_on() acquire that lock before reading KILL_SWITCH_ENV, or (B)
replace env-based signaling with a thread-safe primitive (e.g., an AtomicBool or
a focus-coaching API to set the kill switch) and update complete_guarded(),
tests that set the kill switch, and kill_switch_on() to use that primitive so
reads/writes are race-free; update references to KILL_SWITCH_ENV, ENV_MUTEX,
kill_switch_on(), and complete_guarded() accordingly.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 18-20: Update the CI job to include workspace build and formatting
checks and run clippy across all packages: add a step that runs "cargo build
--workspace", add a step that runs "cargo fmt --all" before tests, keep "cargo
test --all-features --workspace" and change the clippy invocation from "cargo
clippy --all-features -- -D warnings" to "cargo clippy --all -- -D warnings" so
linting covers the entire workspace; ensure step ordering is build -> fmt ->
test -> clippy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 093c1822-15cb-4a8c-b5bb-b7c63f0714a7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/ISSUE_TEMPLATE/config.yml.github/workflows/cargo-deny.yml.github/workflows/ci.ymlCargo.tomlcrates/connector-gcal/src/api.rscrates/connector-notion/src/api.rscrates/focus-cli/tests/release_notes_llm.rscrates/focus-cli/tests/template_marketplace.rscrates/focus-coaching/src/lib.rscrates/focus-ffi/src/lib.rscrates/focus-ffi/tests/connector_registration.rscrates/focus-icon-gen/src/bin.rscrates/focus-icon-gen/src/lib.rscrates/focus-ir/src/lib.rscrates/focus-mcp-server/src/transport/websocket.rscrates/focus-observability/src/integration_tests.rscrates/focus-observability/src/lib.rscrates/focus-rituals/src/lib.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: Kilo Code Review
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Build the Rust workspace using
cargo build --workspaceRun linting with
cargo clippy --all -- -D warningsto enforce all clippy warnings as errorsFormat Rust code using
cargo fmt --allbefore submission
Files:
crates/focus-ffi/tests/connector_registration.rscrates/focus-icon-gen/src/bin.rscrates/focus-cli/tests/release_notes_llm.rscrates/focus-ir/src/lib.rscrates/focus-coaching/src/lib.rscrates/focus-ffi/src/lib.rscrates/focus-observability/src/lib.rscrates/focus-observability/src/integration_tests.rscrates/focus-cli/tests/template_marketplace.rscrates/connector-gcal/src/api.rscrates/focus-mcp-server/src/transport/websocket.rscrates/connector-notion/src/api.rscrates/focus-icon-gen/src/lib.rscrates/focus-rituals/src/lib.rs
**/**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Run tests using
cargo test --workspacefor the entire Rust workspace
Files:
crates/focus-ffi/tests/connector_registration.rscrates/focus-cli/tests/release_notes_llm.rscrates/focus-cli/tests/template_marketplace.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: KooshaPari/FocalPoint
Timestamp: 2026-05-21T00:23:47.787Z
Learning: Run the quality gate: `cargo test --workspace && cargo clippy --all -- -D warnings && cargo fmt --all`
Learnt from: CR
Repo: KooshaPari/FocalPoint
Timestamp: 2026-05-21T00:23:47.787Z
Learning: Organize code into 56 Rust workspace crates under the `crates/` directory
Learnt from: CR
Repo: KooshaPari/FocalPoint
Timestamp: 2026-05-21T00:23:47.787Z
Learning: Place application entry points in the `apps/` directory (iOS, CLI, etc.)
Learnt from: CR
Repo: KooshaPari/FocalPoint
Timestamp: 2026-05-21T00:23:47.787Z
Learning: Place Go backend services in the `services/` directory
Learnt from: CR
Repo: KooshaPari/FocalPoint
Timestamp: 2026-05-21T00:23:47.787Z
Learning: Place build and developer tooling in the `tooling/` directory
Learnt from: CR
Repo: KooshaPari/FocalPoint
Timestamp: 2026-05-21T00:23:47.787Z
Learning: Place fuzzing harnesses in the `fuzz/` directory
Learnt from: CR
Repo: KooshaPari/FocalPoint
Timestamp: 2026-05-21T00:23:47.787Z
Learning: Maintain documentation in the `docs/` directory and published site in `docs-site/`
Learnt from: CR
Repo: KooshaPari/FocalPoint
Timestamp: 2026-05-21T00:23:47.787Z
Learning: Use SQLite, PostgreSQL, or SurrealDB as database backends
🔇 Additional comments (11)
.github/ISSUE_TEMPLATE/config.yml (1)
1-5: LGTM!crates/focus-ffi/tests/connector_registration.rs (2)
40-45: LGTM!Also applies to: 69-74
40-45: Workspace quality-gate verification could not be completed due to environment constraints.The sandbox environment experienced timeouts when executing cargo commands (
cargo build,cargo clippy,cargo test) and lacks therustfmttool, preventing the requested quality gate checks from running. The code changes themselves are syntactically correct—unsafe blocks are properly used for environment variable mutations with appropriate SAFETY comments, consistent with Rust 2024 edition requirements.Run the following locally before submission:
cargo fmt --all cargo build --workspace cargo clippy --all -- -D warnings cargo test --workspacecrates/focus-icon-gen/src/bin.rs (1)
28-28: LGTM!Also applies to: 35-37, 76-76, 87-87
crates/focus-ir/src/lib.rs (1)
1015-1016: LGTM!crates/focus-coaching/src/lib.rs (1)
375-376: LGTM!Also applies to: 380-381, 388-389
crates/focus-ffi/src/lib.rs (1)
2927-2930: LGTM!crates/focus-mcp-server/src/transport/websocket.rs (1)
121-122: LGTM!Also applies to: 138-139, 148-150, 159-160, 172-173, 199-200
.github/workflows/cargo-deny.yml (1)
33-33: LGTM!Cargo.toml (1)
28-28: LGTM!Also applies to: 66-66
crates/focus-icon-gen/src/lib.rs (1)
282-284: LGTM!Also applies to: 291-292, 326-327, 341-342
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | ||
| unsafe { std::env::set_var("FOCALPOINT_GCAL_WEBHOOK_URL", "https://webhook.local/gcal"); } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l crates/connector-gcal/src/api.rsRepository: KooshaPari/FocalPoint
Length of output: 102
🏁 Script executed:
sed -n '575,650p' crates/connector-gcal/src/api.rsRepository: KooshaPari/FocalPoint
Length of output: 3565
🏁 Script executed:
sed -n '620,650p' crates/connector-gcal/src/api.rsRepository: KooshaPari/FocalPoint
Length of output: 1554
🏁 Script executed:
sed -n '575,580p' crates/connector-gcal/src/api.rsRepository: KooshaPari/FocalPoint
Length of output: 286
🏁 Script executed:
grep -n "WATCH_LOCK\|std::env::" crates/connector-gcal/src/api.rs | head -30Repository: KooshaPari/FocalPoint
Length of output: 932
🏁 Script executed:
sed -n '575,642p' crates/connector-gcal/src/api.rsRepository: KooshaPari/FocalPoint
Length of output: 3200
Hold WATCH_LOCK for the entire duration of env mutation, async call, and restoration.
The lock is released before client.watch_channel_create(...) executes on lines 601 and 637, allowing other tests to mutate the environment variable during the critical section. The unsafe contract requires that no other thread modifies the env var while it is being read. Extend the lock scope to cover the set → async call → restore sequence.
Affected lines: 580–610, 618–641
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/connector-gcal/src/api.rs` around lines 582 - 583, Hold WATCH_LOCK for
the entire set→async call→restore sequence: acquire WATCH_LOCK before calling
unsafe std::env::set_var("FOCALPOINT_GCAL_WEBHOOK_URL", ...), keep the lock held
across the await of client.watch_channel_create(...) (both successful and error
paths), then perform the env restoration and release the lock only after
restoration; update the critical sections around the calls to
client.watch_channel_create(...) (both occurrences) so the lock scope starts
before set_var and ends after set_var restoration to prevent concurrent env
mutation while the async call is in-flight.
| let tasks_todo = NotionTask::from_notion_json(&task_todo); | ||
| let tasks_done = NotionTask::from_notion_json(&task_done); | ||
| assert!(!tasks_todo.is_empty()); | ||
| assert!(!tasks_done.is_empty()); |
There was a problem hiding this comment.
Assertions are too weak for a status-variant test.
Line 247-248 only checks non-empty output, so a broken status/completion mapping could still pass. Assert parsed semantic fields (e.g., completed false vs true, expected titles/status).
Suggested assertion upgrade
let tasks_todo = NotionTask::from_notion_json(&task_todo);
let tasks_done = NotionTask::from_notion_json(&task_done);
assert!(!tasks_todo.is_empty());
assert!(!tasks_done.is_empty());
+ assert_eq!(tasks_todo[0].completed, false);
+ assert_eq!(tasks_done[0].completed, true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let tasks_todo = NotionTask::from_notion_json(&task_todo); | |
| let tasks_done = NotionTask::from_notion_json(&task_done); | |
| assert!(!tasks_todo.is_empty()); | |
| assert!(!tasks_done.is_empty()); | |
| let tasks_todo = NotionTask::from_notion_json(&task_todo); | |
| let tasks_done = NotionTask::from_notion_json(&task_done); | |
| assert!(!tasks_todo.is_empty()); | |
| assert!(!tasks_done.is_empty()); | |
| assert_eq!(tasks_todo[0].completed, false); | |
| assert_eq!(tasks_done[0].completed, true); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/connector-notion/src/api.rs` around lines 245 - 248, The test
currently only checks that NotionTask::from_notion_json(&task_todo) and
(&task_done) produce non-empty vectors (tasks_todo, tasks_done) which is
insufficient; update the assertions to validate parsed semantic fields instead:
inspect the returned NotionTask entries (e.g., first element of tasks_todo and
tasks_done) and assert that their completion-related field(s) (e.g., completed,
status, or similar fields on the NotionTask struct) match the expected values
for todo vs done, and also assert expected title/content text to ensure correct
mapping; use the NotionTask::from_notion_json result and its field names to
drive these stronger assertions.
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | ||
| unsafe { env::set_var("FOCALPOINT_RELEASE_NOTES_LLM", "http://localhost:8000/synthesize"); } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify guard presence and env mutation callsites in this test module
rg -n 'ENV_MUTEX|set_var\(|remove_var\(' crates/focus-cli/tests/release_notes_llm.rsRepository: KooshaPari/FocalPoint
Length of output: 305
🏁 Script executed:
cat -n crates/focus-cli/tests/release_notes_llm.rsRepository: KooshaPari/FocalPoint
Length of output: 5316
Serialize env-var mutation in tests; safety comment references a non-existent guard.
The comment at line 25 claims protection by a "process-wide ENV_MUTEX guard" that does not exist in this module. Both test functions mutate the same env var without synchronization, which can cause race conditions under parallel test execution.
Proposed fix
#[cfg(test)]
mod tests {
use std::env;
+ use std::sync::Mutex;
+
+ static ENV_MUTEX: Mutex<()> = Mutex::new(());
/// Test: --synthesize flag with FOCALPOINT_RELEASE_NOTES_LLM env var set
/// Should attempt to POST grouped commits to LLM endpoint.
#[test]
fn test_release_notes_synthesize_with_env_var() {
+ let _guard = ENV_MUTEX.lock().expect("env mutex");
// SAFETY: env mutation is unsafe in Rust 2024 edition in test context
unsafe { env::set_var("FOCALPOINT_RELEASE_NOTES_LLM", "http://localhost:8000/synthesize"); }
/// Test: --synthesize flag with missing FOCALPOINT_RELEASE_NOTES_LLM env var
/// Should warn and fall back to template rendering (no crash).
#[test]
fn test_release_notes_synthesize_fallback_missing_env() {
+ let _guard = ENV_MUTEX.lock().expect("env mutex");
// SAFETY: env::remove_var is unsafe in Rust 2024 edition; this is test-only
// and protected by the process-wide ENV_MUTEX guard.
unsafe { env::remove_var("FOCALPOINT_RELEASE_NOTES_LLM") };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/focus-cli/tests/release_notes_llm.rs` around lines 12 - 13, The tests
mutate the same env var FOCALPOINT_RELEASE_NOTES_LLM using unsafe env::set_var
without a real guard; add a real synchronization primitive (e.g., a static
Mutex/OnceCell guard) and lock it around the env::set_var / env::remove_var
calls in both test functions (referencing env::set_var and
FOCALPOINT_RELEASE_NOTES_LLM in release_notes_llm.rs) to serialize access, and
remove the misleading "process-wide ENV_MUTEX guard" comment; alternatively
annotate tests to run serially or use serial_test, but ensure all env mutations
are protected by the chosen guard.
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | ||
| unsafe { env::set_var("FOCALPOINT_EXAMPLES", "./examples/templates"); } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
wc -l crates/focus-cli/tests/template_marketplace.rsRepository: KooshaPari/FocalPoint
Length of output: 116
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '25,40p' crates/focus-cli/tests/template_marketplace.rsRepository: KooshaPari/FocalPoint
Length of output: 838
🏁 Script executed:
# Check lines 73-74
sed -n '68,80p' crates/focus-cli/tests/template_marketplace.rsRepository: KooshaPari/FocalPoint
Length of output: 726
🏁 Script executed:
# Check lines 166-167 and 177-178
sed -n '160,185p' crates/focus-cli/tests/template_marketplace.rsRepository: KooshaPari/FocalPoint
Length of output: 961
🏁 Script executed:
# Look for any synchronization, mutex, or restoration patterns in the file
rg -n "mutex|Mutex|lock|restore|cleanup|setup|teardown|FOCALPOINT_EXAMPLES" crates/focus-cli/tests/template_marketplace.rsRepository: KooshaPari/FocalPoint
Length of output: 413
🏁 Script executed:
# Check the Rust edition in Cargo.toml
find crates/focus-cli -name "Cargo.toml" -type f | head -1 | xargs cat | grep -A 5 "^\[package\]"Repository: KooshaPari/FocalPoint
Length of output: 199
🏁 Script executed:
# Check workspace Cargo.toml for edition
cat Cargo.toml | grep -A 10 "^\[workspace\]"Repository: KooshaPari/FocalPoint
Length of output: 424
🏁 Script executed:
# Look for test setup/teardown or test guards in the entire test file
cat -n crates/focus-cli/tests/template_marketplace.rs | head -40Repository: KooshaPari/FocalPoint
Length of output: 1970
🏁 Script executed:
# Check if there are any #[ctor] or custom test runners
rg -n "#\[" crates/focus-cli/tests/template_marketplace.rsRepository: KooshaPari/FocalPoint
Length of output: 250
🏁 Script executed:
# Check the imports at the top of the test file
head -30 crates/focus-cli/tests/template_marketplace.rsRepository: KooshaPari/FocalPoint
Length of output: 1111
🏁 Script executed:
# Look for any test utilities or fixtures in the crate
fd -t f "\.rs$" crates/focus-cli/tests/ | head -20Repository: KooshaPari/FocalPoint
Length of output: 199
🏁 Script executed:
# Find the actual edition in workspace
cat Cargo.toml | grep "edition\s*="Repository: KooshaPari/FocalPoint
Length of output: 82
🏁 Script executed:
# Check if there's a [workspace.lints] or test config
cat Cargo.toml | grep -A 20 "\[workspace"Repository: KooshaPari/FocalPoint
Length of output: 1344
🏁 Script executed:
# Look at the complete Cargo.toml to see edition
head -50 Cargo.tomlRepository: KooshaPari/FocalPoint
Length of output: 1659
🏁 Script executed:
# Check if tests use test attributes for serial/parallel
rg -n "serial|parallel" crates/focus-cli/tests/template_marketplace.rsRepository: KooshaPari/FocalPoint
Length of output: 47
🏁 Script executed:
# Verify the actual lines where unsafe env::set_var is used - get exact line numbers
cat -n crates/focus-cli/tests/template_marketplace.rs | grep -B 2 -A 2 "unsafe.*env::set_var"Repository: KooshaPari/FocalPoint
Length of output: 1373
🏁 Script executed:
# Check if there's any documentation or comments about Rust 2024 unsafe requirements
rg -n "Rust 2024" . --type rs | head -20Repository: KooshaPari/FocalPoint
Length of output: 92
🏁 Script executed:
# Check cargo test configuration - does it run tests in parallel?
cat crates/focus-cli/Cargo.tomlRepository: KooshaPari/FocalPoint
Length of output: 1361
🏁 Script executed:
# Look for any existing env var handling patterns elsewhere in the codebase
rg -n "env::set_var" . --type rs | head -20Repository: KooshaPari/FocalPoint
Length of output: 92
🏁 Script executed:
# Check if there are any test synchronization utilities in the crate
fd -t f "\.rs$" crates/focus-cli/src | xargs grep -l "mutex\|Mutex\|lock" | head -10Repository: KooshaPari/FocalPoint
Length of output: 127
🏁 Script executed:
# Search for env::set_var usage without file type filter
rg "env::set_var" . | head -30Repository: KooshaPari/FocalPoint
Length of output: 2293
🏁 Script executed:
# Search for Rust 2024 edition safety requirements or documentation
rg "Rust 2024" . | head -20Repository: KooshaPari/FocalPoint
Length of output: 2541
🏁 Script executed:
# Check if any tests use restoration/cleanup patterns
rg -A 5 "env::set_var" crates/focus-cli/tests/template_marketplace.rsRepository: KooshaPari/FocalPoint
Length of output: 1124
🏁 Script executed:
# Look for test attributes that control execution order
rg "#\[test" crates/focus-cli/tests/template_marketplace.rs -A 2 | head -30Repository: KooshaPari/FocalPoint
Length of output: 963
🏁 Script executed:
# Search in documentation or RFC comments about why env::set_var is unsafe in Rust 2024
python3 << 'EOF'
import subprocess
result = subprocess.run(['rg', 'unsafe.*env.*set_var|SAFETY.*env|env.*mutation.*unsafe'],
capture_output=True, text=True)
print(result.stdout)
if result.stderr:
print("STDERR:", result.stderr)
EOFRepository: KooshaPari/FocalPoint
Length of output: 66
Guard env var mutations with scoped restoration to prevent test interference.
At lines 32, 74, 167, and 178, unsafe { env::set_var(...) } modifies process-global state without restoration. Since tests run in parallel by default, this creates cross-test contamination. Wrap these with either a scoped guard pattern that restores the original value after the test, or use a test synchronization mechanism.
Affected locations:
- Line 32:
FOCALPOINT_EXAMPLESintest_template_search_local_fallback - Line 74:
FOCALPOINT_EXAMPLESintest_template_show_local_fallback - Line 167:
FOCALPOINT_TEMPLATE_REGISTRYintest_template_registry_url_env_var - Line 178:
FOCALPOINT_TEMPLATE_TOKENintest_template_auth_token_env_var
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/focus-cli/tests/template_marketplace.rs` around lines 31 - 32, Several
tests (test_template_search_local_fallback, test_template_show_local_fallback,
test_template_registry_url_env_var, test_template_auth_token_env_var) call
unsafe { env::set_var(...) } and mutate global process environment without
restoring it; replace those direct sets with a scoped guard pattern that
captures the original env::var_os value, sets the env var (e.g.,
FOCALPOINT_EXAMPLES, FOCALPOINT_TEMPLATE_REGISTRY, FOCALPOINT_TEMPLATE_TOKEN)
for the test, and restores the original value in Drop (or use a test
synchronization mechanism) so the environment is always restored after each test
finishes; locate the set calls in those test functions and introduce a small
helper (e.g., EnvVarGuard) to manage save/set/restore around the test scope.
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | ||
| unsafe { std::env::set_var("FOCALPOINT_LOG_LEVEL", "warn"); } | ||
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | ||
| unsafe { std::env::set_var("FOCALPOINT_LOG_FORMAT", "json"); } | ||
| // init_tracing would honor these; second call won't re-init (subscriber singleton) | ||
| // Just verify the env vars are readable | ||
| let level = std::env::var("FOCALPOINT_LOG_LEVEL").expect("should read"); | ||
| assert_eq!(level, "warn"); | ||
| std::env::remove_var("FOCALPOINT_LOG_LEVEL"); | ||
| std::env::remove_var("FOCALPOINT_LOG_FORMAT"); | ||
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | ||
| unsafe { std::env::remove_var("FOCALPOINT_LOG_LEVEL"); } | ||
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | ||
| unsafe { std::env::remove_var("FOCALPOINT_LOG_FORMAT"); } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file
find . -name "integration_tests.rs" -path "*/focus-observability/*"Repository: KooshaPari/FocalPoint
Length of output: 119
🏁 Script executed:
# Check the file content around lines 164-175
cat -n crates/focus-observability/src/integration_tests.rs | sed -n '160,180p'Repository: KooshaPari/FocalPoint
Length of output: 1258
🏁 Script executed:
# Check the Cargo.toml to understand the Rust edition and test configuration
cat crates/focus-observability/Cargo.tomlRepository: KooshaPari/FocalPoint
Length of output: 885
🏁 Script executed:
# Search for any existing synchronization mechanisms in the test module
rg "Mutex|static.*LOCK|parking_lot" crates/focus-observability/src/integration_tests.rs -B 2 -A 2Repository: KooshaPari/FocalPoint
Length of output: 47
🏁 Script executed:
# Check if there's a test configuration that controls parallelism
rg "test-threads|parallel" crates/focus-observability/Cargo.tomlRepository: KooshaPari/FocalPoint
Length of output: 47
🏁 Script executed:
# Check workspace Cargo.toml for edition and rust-version
cat Cargo.toml | head -50Repository: KooshaPari/FocalPoint
Length of output: 1659
🏁 Script executed:
# Search for any test configuration in the workspace
rg "\[profile\]" . --type toml -A 10Repository: KooshaPari/FocalPoint
Length of output: 47
🏁 Script executed:
# Check rust-version in workspace
rg "rust-version" Cargo.tomlRepository: KooshaPari/FocalPoint
Length of output: 87
🏁 Script executed:
# Check if 2024 edition is specified in workspace Cargo.toml
cat Cargo.toml | grep -i editionRepository: KooshaPari/FocalPoint
Length of output: 195
🏁 Script executed:
# Check the focus-observability crate's Cargo.toml for explicit edition
cat crates/focus-observability/Cargo.toml | grep -i editionRepository: KooshaPari/FocalPoint
Length of output: 90
🏁 Script executed:
# Let's search for any test configuration or parallel execution settings
rg "test-threads|--test-threads" . --type tomlRepository: KooshaPari/FocalPoint
Length of output: 47
🏁 Script executed:
# Check if there are other env var mutations in the test file
rg "std::env::" crates/focus-observability/src/integration_tests.rsRepository: KooshaPari/FocalPoint
Length of output: 418
🏁 Script executed:
# Check for any static mutex or synchronization patterns in other test files in the crate
find crates/focus-observability -name "*.rs" -type f -exec grep -l "static.*Mutex\|ENV_LOCK" {} \;Repository: KooshaPari/FocalPoint
Length of output: 107
🏁 Script executed:
# Check the full test module structure to understand scope
cat -n crates/focus-observability/src/integration_tests.rs | sed -n '1,100p'Repository: KooshaPari/FocalPoint
Length of output: 4458
Unsafe env mutation requires synchronization for parallel test execution.
Lines 165, 167, 173, and 175 perform unsafe env operations without a serialization guard. In Rust 2024 edition, std::env::set_var and std::env::remove_var require proving exclusive access; parallel test execution breaks this guarantee.
Suggested fix
#[cfg(test)]
mod tests {
+ static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
+
#[test]
fn test_init_tracing_respects_env_vars() {
+ let _guard = ENV_LOCK.lock().expect("env lock");
// SAFETY: protected by ENV_LOCK for full mutation/read/restore scope
unsafe { std::env::set_var("FOCALPOINT_LOG_LEVEL", "warn"); }
// SAFETY: protected by ENV_LOCK for full mutation/read/restore scope
unsafe { std::env::set_var("FOCALPOINT_LOG_FORMAT", "json"); }
let level = std::env::var("FOCALPOINT_LOG_LEVEL").expect("should read");
assert_eq!(level, "warn");
// SAFETY: protected by ENV_LOCK
unsafe { std::env::remove_var("FOCALPOINT_LOG_LEVEL"); }
// SAFETY: protected by ENV_LOCK
unsafe { std::env::remove_var("FOCALPOINT_LOG_FORMAT"); }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | |
| unsafe { std::env::set_var("FOCALPOINT_LOG_LEVEL", "warn"); } | |
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | |
| unsafe { std::env::set_var("FOCALPOINT_LOG_FORMAT", "json"); } | |
| // init_tracing would honor these; second call won't re-init (subscriber singleton) | |
| // Just verify the env vars are readable | |
| let level = std::env::var("FOCALPOINT_LOG_LEVEL").expect("should read"); | |
| assert_eq!(level, "warn"); | |
| std::env::remove_var("FOCALPOINT_LOG_LEVEL"); | |
| std::env::remove_var("FOCALPOINT_LOG_FORMAT"); | |
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | |
| unsafe { std::env::remove_var("FOCALPOINT_LOG_LEVEL"); } | |
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | |
| unsafe { std::env::remove_var("FOCALPOINT_LOG_FORMAT"); } | |
| let _guard = ENV_LOCK.lock().expect("env lock"); | |
| // SAFETY: protected by ENV_LOCK for full mutation/read/restore scope | |
| unsafe { std::env::set_var("FOCALPOINT_LOG_LEVEL", "warn"); } | |
| // SAFETY: protected by ENV_LOCK for full mutation/read/restore scope | |
| unsafe { std::env::set_var("FOCALPOINT_LOG_FORMAT", "json"); } | |
| // init_tracing would honor these; second call won't re-init (subscriber singleton) | |
| // Just verify the env vars are readable | |
| let level = std::env::var("FOCALPOINT_LOG_LEVEL").expect("should read"); | |
| assert_eq!(level, "warn"); | |
| // SAFETY: protected by ENV_LOCK | |
| unsafe { std::env::remove_var("FOCALPOINT_LOG_LEVEL"); } | |
| // SAFETY: protected by ENV_LOCK | |
| unsafe { std::env::remove_var("FOCALPOINT_LOG_FORMAT"); } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/focus-observability/src/integration_tests.rs` around lines 164 - 175,
The test mutates process-wide environment unsafely (calls to std::env::set_var,
std::env::remove_var and std::env::var) and must be serialized; introduce a
global test lock (e.g., a static Mutex<()> via once_cell::sync::Lazy or similar)
and acquire the lock around the env operations in the test before calling
set_var/var/remove_var, releasing it after the checks so parallel tests cannot
race; reference the env functions set_var, remove_var, and std::env::var in the
test and wrap those calls with the lock to ensure exclusive access.
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | ||
| unsafe { std::env::set_var("FOCALPOINT_LOG_FORMAT", "json"); } | ||
| // SAFETY: env mutation is unsafe in Rust 2024 edition in test context | ||
| unsafe { std::env::set_var("FOCALPOINT_LOG_LEVEL", "debug"); } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether env-mutation tests are serialized in this module
rg -n 'ENV_MUTEX|set_var\(|remove_var\(' crates/focus-observability/src/lib.rsRepository: KooshaPari/FocalPoint
Length of output: 501
🏁 Script executed:
sed -n '120,170p' crates/focus-observability/src/lib.rsRepository: KooshaPari/FocalPoint
Length of output: 2434
Add a test-level mutex for env var mutation to prevent flaky parallel tests.
Tests mutate process-global env vars (FOCALPOINT_LOG_FORMAT, FOCALPOINT_LOG_LEVEL) without synchronization. When tests run in parallel (via cargo test -- --test-threads=N with N > 1), concurrent env mutations will race and cause nondeterministic test failures.
Proposed fix
#[cfg(test)]
mod tests {
use super::*;
+ use std::sync::Mutex;
+
+ static ENV_MUTEX: Mutex<()> = Mutex::new(());
#[test]
fn test_init_tracing_with_json_format() {
+ let _guard = ENV_MUTEX.lock().expect("env mutex");
// This test verifies init_tracing accepts config
// ...
#[test]
fn test_init_tracing_with_pretty_format() {
+ let _guard = ENV_MUTEX.lock().expect("env mutex");
// Set format and verify no panic
// ...Applies to tests at lines 137–140, 148–151, 157–158, 160–161.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/focus-observability/src/lib.rs` around lines 137 - 140, Wrap all test
code that mutates process env vars ("FOCALPOINT_LOG_FORMAT" and
"FOCALPOINT_LOG_LEVEL") with a global test mutex to prevent races: introduce a
static mutex (e.g. static TEST_ENV_MUTEX:
once_cell::sync::Lazy<std::sync::Mutex<()>> = once_cell::sync::Lazy::new(||
std::sync::Mutex::new(()));) and lock it (let _guard =
TEST_ENV_MUTEX.lock().unwrap();) before calling unsafe { std::env::set_var(...)
} in the tests referenced (the blocks calling std::env::set_var), releasing
automatically when the guard drops; apply the same guard around all other env
mutations in this file so tests no longer mutate process-global env
concurrently.
| // SAFETY: process-wide env mutation occurs in process-local test guarded by a mutex. | ||
| unsafe { std::env::set_var(focus_coaching::KILL_SWITCH_ENV, "1") }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all env var access sites that may race with unsafe set/remove in this crate.
set -euo pipefail
echo "== env mutation/read callsites in crates/focus-rituals =="
rg -n --type rust -C 2 'std::env::(set_var|remove_var|var|var_os)\s*\(' crates/focus-rituals
echo
echo "== async tests in crates/focus-rituals that may run concurrently =="
rg -n --type rust -C 1 '#\[tokio::test\]|#\[test\]' crates/focus-rituals/src/lib.rs
echo
echo "Expected:"
echo "- Any env access not coordinated by the same global lock indicates the unsafe precondition is not provably met."
echo "- If present, serialize these tests/processes (or avoid process env mutation in tests)."Repository: KooshaPari/FocalPoint
Length of output: 3624
🏁 Script executed:
# Search for all env var READS (var/var_os), not just writes
rg -n --type rust 'std::env::var(?:_os)?\s*\(' crates/focus-ritualsRepository: KooshaPari/FocalPoint
Length of output: 47
🏁 Script executed:
# Check if KILL_SWITCH_ENV is read anywhere in the codebase
rg -n --type rust 'KILL_SWITCH_ENV' crates/Repository: KooshaPari/FocalPoint
Length of output: 892
🏁 Script executed:
# Check the focus_coaching module to see how it reads KILL_SWITCH_ENV
fd -e rs focus_coaching | head -20Repository: KooshaPari/FocalPoint
Length of output: 47
🏁 Script executed:
# Look for Cargo.toml or test configuration that might enable parallel tests
find crates/focus-rituals -name "Cargo.toml" -o -name ".cargo" | head -5Repository: KooshaPari/FocalPoint
Length of output: 97
🏁 Script executed:
sed -n '305,315p' crates/focus-coaching/src/lib.rsRepository: KooshaPari/FocalPoint
Length of output: 482
🏁 Script executed:
# Check test parallelism configuration
cat crates/focus-rituals/Cargo.toml | grep -A 10 "\[profile\]" || echo "No profile config found"Repository: KooshaPari/FocalPoint
Length of output: 89
🏁 Script executed:
# Check if there's a test runner config
rg -n 'test-threads|RUST_TEST' . 2>/dev/null | head -20Repository: KooshaPari/FocalPoint
Length of output: 186
🏁 Script executed:
# Verify if the env read at line 309 is in production code path
sed -n '300,320p' crates/focus-coaching/src/lib.rsRepository: KooshaPari/FocalPoint
Length of output: 760
Unsafe env mutation contract is not fully guaranteed; unguarded read in production code
Lines 831, 845, and 861 write to KILL_SWITCH_ENV with ENV_MUTEX held, but the safety precondition is not met: kill_switch_on() at line 309 in crates/focus-coaching/src/lib.rs reads the same env var without any synchronization. Since complete_guarded() is public and async, concurrent calls during test execution (or in production code running parallel to tests) create a data race with the env mutations. The module-local lock protects only the writers in focus-rituals tests, not the readers in the public focus-coaching API.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/focus-rituals/src/lib.rs` around lines 830 - 831, The test mutates the
process-wide env var KILL_SWITCH_ENV under ENV_MUTEX, but kill_switch_on() in
focus-coaching reads that env var without synchronization, causing a race; fix
by ensuring both readers and writers use the same synchronization or by avoiding
env mutation: either (A) move ENV_MUTEX (or another lock) into the
focus-coaching crate and have kill_switch_on() acquire that lock before reading
KILL_SWITCH_ENV, or (B) replace env-based signaling with a thread-safe primitive
(e.g., an AtomicBool or a focus-coaching API to set the kill switch) and update
complete_guarded(), tests that set the kill switch, and kill_switch_on() to use
that primitive so reads/writes are race-free; update references to
KILL_SWITCH_ENV, ENV_MUTEX, kill_switch_on(), and complete_guarded()
accordingly.
User description
Summary
use serde_json::json;import inpii_scrubber.rstest module sojson!()macro compilesclippy.tomlMSRV to workspacerust-version(1.82)Test plan
cargo check -p focus-telemetry— passescargo test -p focus-telemetry --lib— 17 tests pass🤖 Generated with Claude Code
CodeAnt-AI Description
Handle single-item Notion responses and keep imports working in Rust 2024 tests
What Changed
Impact
✅ Fewer failed Notion imports✅ Clearer page and task names✅ Safer test runs on Rust 2024💡 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.