Skip to content

hygiene(FocalPoint): preserve canonical cleanup#82

Closed
KooshaPari wants to merge 2 commits into
mainfrom
hygiene/preserve-changes
Closed

hygiene(FocalPoint): preserve canonical cleanup#82
KooshaPari wants to merge 2 commits into
mainfrom
hygiene/preserve-changes

Conversation

@KooshaPari

@KooshaPari KooshaPari commented Jun 5, 2026

Copy link
Copy Markdown
Owner

User description

Summary

Preserved hygiene work from canonical main: connector-notion/readwise model updates, focus-cli/coaching/connectors/mascot/observability cleanup, template fixes. Auto-generated artifacts and embedded gitlinks removed.

Test plan

Not run; preserved from prior session cleanup. Needs maintainer review.

Note

Medium Risk
Connector parsing changes affect sync behavior for real API payloads; template rigidity shifts example rules from semi (paid bypass) to soft (advisory), which changes demo enforcement semantics but not production code paths directly.

Overview
Connector imports now accept list responses and single-object payloads for Notion and Readwise, with shared helpers for title extraction and safer defaults on missing timestamps/URLs. Readwise articles can fall back from created_at to added_at; highlights tolerate missing document_id.

focus-cli defers pretty tracing when --json is set, emits [] for missing template dirs in JSON mode, and treats git log failures during release-notes generation as an empty commit list (with a warning) instead of aborting.

Observability uses try_init() instead of init() so repeated tracing setup does not panic. Placeholder FR tests in coaching, connectors, and mascot are marked #[ignore].

Example templates change block actions from rigidity = "semi" to "soft" in dev-flow.toml and student-canvas.toml.

GitHub Actions: cargo-audit bumps checkout, switches audit token to secrets.GITHUB_TOKEN, and drops workflow-level concurrency/permissions; cargo-deny removes top-level permissions/concurrency and updates checkout; journey-gate adds contents: read on main and stub jobs.

Reviewed by Cursor Bugbot for commit f331d96. Bugbot is set up for automated code reviews on this repo. Configure here.


CodeAnt-AI Description

Handle more API response shapes and keep CLI output stable

What Changed

  • Notion and Readwise imports now accept both list responses and single-item payloads, so more sync responses are processed successfully
  • Missing timestamps, URLs, and Readwise document IDs are now handled without dropping the whole item
  • Notion titles are pulled from more page formats, reducing “Untitled” results when the title field uses a different shape
  • focus-cli no longer adds tracing noise in JSON mode, prints an empty list when template discovery finds no directory in JSON mode, and keeps release-notes generation running when git log lookup fails in JSON mode
  • Several placeholder tests were marked as ignored, and workflow checkout/permissions settings were tightened to keep CI runs working reliably

Impact

✅ Fewer failed Notion and Readwise syncs
✅ Cleaner JSON CLI output
✅ Fewer release-notes aborts in missing-git-log cases

💡 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:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

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:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

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.

@codeant-ai

codeant-ai Bot commented Jun 5, 2026

Copy link
Copy Markdown

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 ·
Reddit ·
LinkedIn

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@KooshaPari, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 21 minutes and 27 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: a31542f5-a6ea-4140-a24b-e6a018675d17

📥 Commits

Reviewing files that changed from the base of the PR and between 652a12f and f331d96.

📒 Files selected for processing (13)
  • .claude/worktrees/agent-a00edabb7687463fa
  • .github/workflows/cargo-audit.yml
  • .github/workflows/cargo-deny.yml
  • .github/workflows/journey-gate.yml
  • crates/connector-notion/src/models.rs
  • crates/connector-readwise/src/models.rs
  • crates/focus-cli/src/main.rs
  • crates/focus-coaching/src/lib.rs
  • crates/focus-connectors/src/lib.rs
  • crates/focus-mascot/src/lib.rs
  • crates/focus-observability/src/lib.rs
  • examples/templates/dev-flow.toml
  • examples/templates/student-canvas.toml
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch hygiene/preserve-changes
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch hygiene/preserve-changes

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the Notion and Readwise connectors to handle single-object payloads and fallback defaults, skips tracing initialization in the CLI when JSON output is requested, ignores several unimplemented tests, and changes template rigidity levels from 'semi' to 'soft'. Feedback highlights a potential issue in the Readwise connector where using unwrap_or_default() for document_id could result in invalid empty strings, and notes inconsistencies in the template files where notification messages were not updated to match the new 'soft' rigidity level.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +80 to +84
document_id: h
.get("document_id")
.and_then(|d| d.as_str())
.unwrap_or_default()
.into(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using unwrap_or_default() for document_id will result in an empty string "" if the field is missing or not a string. Since document_id is a required field in the Highlight struct and likely acts as a critical relation to the parent document, allowing an empty string could lead to database constraint violations or orphaned highlights. It is safer to skip the malformed highlight by using ? (as was done previously) or handle the error explicitly.

                    document_id: h.get("document_id")?.as_str()?.into(),

trigger = { kind = "schedule", value = "0 0 9 * * 1-5" }
actions = [
{ type = "block", profile = "social", duration_seconds = 5400, rigidity = "semi" },
{ type = "block", profile = "social", duration_seconds = 5400, rigidity = "soft" },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The rigidity has been changed to "soft", but the notification message on the next line still says "Social is semi-locked". To avoid confusing the user, the message should be updated to reflect the new "soft" rigidity (or the rigidity should remain "semi" if that was intended).

]
actions = [
{ type = "block", profile = "social", duration_seconds = 86400, rigidity = "semi" },
{ type = "block", profile = "social", duration_seconds = 86400, rigidity = "soft" },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The rigidity has been changed to "soft", but the notification message on the next line still says "social locked". To prevent user confusion, the message should be updated to match the "soft" rigidity level.

@codeant-ai codeant-ai Bot added the size:L This PR changes 100-499 lines, ignoring generated files label Jun 5, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 3 potential issues.

Fix All in Cursor

Bugbot Autofix prepared fixes for all 3 issues found in the latest run.

  • ✅ Fixed: JSON release notes hide git failure
    • Removed the JSON-only error swallowing so fetch_git_log failures propagate and --json callers get a non-zero exit instead of an empty changelog.
  • ✅ Fixed: JSON template list masks missing dir
    • Removed the JSON early return that printed [] for a missing examples directory so templates list --json now errors like text mode.
  • ✅ Fixed: Template notify mismatches soft block
    • Updated the morning focus notify message to describe soft blocking without semi-lock credit bypass language.

Create PR

Or push these changes by commenting:

@cursor push df1a6b568a
Preview (df1a6b568a)
diff --git a/crates/focus-cli/src/main.rs b/crates/focus-cli/src/main.rs
--- a/crates/focus-cli/src/main.rs
+++ b/crates/focus-cli/src/main.rs
@@ -925,10 +925,6 @@
                 .or_else(|| std::env::current_dir().ok().map(|p| p.join("examples/templates")))
                 .ok_or_else(|| anyhow::anyhow!("examples/templates not found"))?;
             if !dir.is_dir() {
-                if json_output {
-                    println!("[]");
-                    return Ok(());
-                }
                 anyhow::bail!("{} is not a directory", dir.display());
             }
             let mut templates = Vec::new();
@@ -1673,14 +1669,7 @@
 fn run_release_notes(cmd: ReleaseNotesCmd, json_output: bool) -> anyhow::Result<()> {
     match cmd {
         ReleaseNotesCmd::Generate { since, format, synthesize } => {
-            let commits = match fetch_git_log(&since) {
-                Ok(commits) => commits,
-                Err(err) if json_output => {
-                    eprintln!("warn: {err}");
-                    Vec::new()
-                }
-                Err(err) => return Err(err),
-            };
+            let commits = fetch_git_log(&since)?;
             let grouped = group_commits_by_type(&commits);
 
             // If synthesize flag is set, try to call LLM endpoint; fall back to template if unavailable

diff --git a/examples/templates/dev-flow.toml b/examples/templates/dev-flow.toml
--- a/examples/templates/dev-flow.toml
+++ b/examples/templates/dev-flow.toml
@@ -61,5 +61,5 @@
 trigger = { kind = "schedule", value = "0 0 9 * * 1-5" }
 actions = [
   { type = "block", profile = "social", duration_seconds = 5400, rigidity = "soft" },
-  { type = "notify", message = "Deep focus — 90 min. Social is semi-locked, bypass costs 10 credits." },
+  { type = "notify", message = "Deep focus — 90 min. Social is softly blocked — override anytime." },
 ]

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit 21294ce. Configure here.

Comment thread crates/focus-cli/src/main.rs
Comment thread crates/focus-cli/src/main.rs
trigger = { kind = "schedule", value = "0 0 9 * * 1-5" }
actions = [
{ type = "block", profile = "social", duration_seconds = 5400, rigidity = "semi" },
{ type = "block", profile = "social", duration_seconds = 5400, rigidity = "soft" },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Template notify mismatches soft block

Low Severity

The morning focus rule’s block action uses rigidity = "soft", but the paired notify still tells users social is semi-locked with a 10-credit bypass. Installed behavior is advisory soft blocking while the message promises semi-rigid enforcement and a credit cost.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 21294ce. Configure here.

.with_line_number(true);

registry.with(fmt_layer).init();
let _ = registry.with(fmt_layer).try_init();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The tracing subscriber initialization result is discarded, so initialization failures are silently ignored and execution continues as if setup succeeded. This can leave the process running with unexpected tracing configuration while still reporting successful initialization; handle the Err case explicitly (at least log a warning/error and avoid emitting a misleading success message). [logic error]

Severity Level: Major ⚠️
- ❌ Second init_tracing call logs success despite subscriber unchanged.
- ⚠️ Library consumers can run with unexpected tracing configuration.
Steps of Reproduction ✅
1. Open `crates/focus-observability/src/lib.rs` and inspect `init_tracing` (starting at
line 68). In both the `pretty` and `json` branches, the code calls `let _ =
registry.with(fmt_layer).try_init();` (e.g., line 89) and then unconditionally logs
`"tracing initialized"` via `info!` at lines 42–47.

2. In the same file's test module (lines 72–88), observe the comment on lines 75–77:
"global tracing init can only happen once per process; subsequent calls will fail silently
(tracing-subscriber limitation)." This confirms `try_init()` returns an error when a
subscriber is already set.

3. In a single process (for example, a binary that depends on `focus-observability`),
first call `init_tracing("svc1", Some("info"))` to set up tracing successfully using the
code at `lib.rs:68–40`. Then call `init_tracing("svc2", Some("debug"))` again in the same
process.

4. On the second call, `registry.with(fmt_layer).try_init()` returns `Err` because a
global subscriber is already registered, but the `Err` is discarded (`let _ = ...`) and
the function still executes the `info!` block logging `"tracing initialized"` for `svc2`;
the subscriber remains configured from the first call, so the log message reports success
while the tracing configuration has not changed and may not match the caller's
expectations.

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/lib.rs
**Line:** 89:89
**Comment:**
	*Logic Error: The tracing subscriber initialization result is discarded, so initialization failures are silently ignored and execution continues as if setup succeeded. This can leave the process running with unexpected tracing configuration while still reporting successful initialization; handle the `Err` case explicitly (at least log a warning/error and avoid emitting a misleading success message).

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Comment on lines +48 to +52
last_edited_time: page
.get("last_edited_time")
.and_then(|t| t.as_str())
.unwrap_or_default()
.into(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Defaulting missing last_edited_time to an empty string makes downstream event mapping fall back to Utc::now(), which changes on every sync and produces unstable dedupe keys for the same page. This can generate duplicate events repeatedly; keep a stable timestamp fallback (for example created_time) or skip pages without any stable time. [logic error]

Severity Level: Major ⚠️
- ❌ Notion page_updated events get new dedupe_key each sync.
- ⚠️ Rules on notion:page_updated may fire repeatedly.
- ⚠️ Event history polluted with duplicate page update entries.
Steps of Reproduction ✅
1. Receive a Notion page payload without a `last_edited_time` field (or where it is null)
and pass it into `NotionPage::from_notion_json()` in
`crates/connector-notion/src/models.rs:17-60`.

2. In `from_notion_json`, the `last_edited_time` field is read at `models.rs:48-51` and
missing/invalid values are converted to the empty string via `.unwrap_or_default()`, so
the constructed `NotionPage.last_edited_time` is `""`.

3. Pass the resulting `NotionPage` into `NotionEventMapper::map_pages()` in
`crates/connector-notion/src/events.rs:18-55`; this calls
`chrono::DateTime::parse_from_rfc3339(&p.last_edited_time)` at `events.rs:23-25`, which
fails to parse the empty string and falls back to `Utc::now()`.

4. `EventFactory::new_dedupe_key("notion", &p.id, edited_at)` at `events.rs:27-31` uses
this `edited_at` timestamp; because it is `Utc::now()` for every run when
`last_edited_time` is missing, the dedupe key (and `NormalizedEvent.occurred_at`) changes
on each sync for the same page, allowing duplicate `notion:page_updated` events to be
emitted.

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/connector-notion/src/models.rs
**Line:** 48:52
**Comment:**
	*Logic Error: Defaulting missing `last_edited_time` to an empty string makes downstream event mapping fall back to `Utc::now()`, which changes on every sync and produces unstable dedupe keys for the same page. This can generate duplicate events repeatedly; keep a stable timestamp fallback (for example `created_time`) or skip pages without any stable time.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Comment on lines +36 to +40
updated_at: doc
.get("updated_at")
.and_then(|t| t.as_str())
.unwrap_or_default()
.into(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Defaulting missing updated_at to an empty string makes article event mapping fall back to current time, so dedupe keys vary between sync runs for identical records. This leads to duplicate article_read events instead of idempotent behavior; use a stable fallback (created_at/added_at) or drop records lacking stable temporal fields. [logic error]

Severity Level: Major ⚠️
- ❌ Readwise article_read events become non-idempotent across syncs.
- ⚠️ Article-based rules can re-trigger for same document.
- ⚠️ Analytics on Readwise articles overcount repeated events.
Steps of Reproduction ✅
1. Call `ReadwiseClient::get_articles()` in `crates/connector-readwise/src/api.rs:52-66`;
on success it deserializes the JSON and passes it to `Article::from_readwise_json(&json)`
in `crates/connector-readwise/src/models.rs:19-44`.

2. For any article JSON missing `updated_at`, `Article::from_readwise_json` initializes
`updated_at` at `models.rs:36-40` using `.get("updated_at") ... .unwrap_or_default()`, so
`Article.updated_at` becomes the empty string `""`.

3. The resulting articles are later mapped into events via
`ReadwiseEventMapper::map_articles()` in `crates/connector-readwise/src/events.rs:58-97`,
which calls `chrono::DateTime::parse_from_rfc3339(&a.updated_at)` at `events.rs:63-65`;
parsing `""` fails and the code falls back to `Utc::now()`.

4. `EventFactory::new_dedupe_key("readwise", &a.id, updated_at)` at `events.rs:67-71`
incorporates this `updated_at` timestamp into the dedupe key, so when `updated_at` was
missing in the API response the same article will receive a different dedupe key (and
`occurred_at`) on every sync run, producing duplicate `readwise:article_read` events.

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/connector-readwise/src/models.rs
**Line:** 36:40
**Comment:**
	*Logic Error: Defaulting missing `updated_at` to an empty string makes article event mapping fall back to current time, so dedupe keys vary between sync runs for identical records. This leads to duplicate `article_read` events instead of idempotent behavior; use a stable fallback (`created_at`/`added_at`) or drop records lacking stable temporal fields.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Comment on lines +86 to +90
created_at: h
.get("created_at")
.and_then(|t| t.as_str())
.unwrap_or_default()
.into(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Defaulting missing highlight created_at to an empty string causes downstream parsing to use Utc::now(), which changes every run and invalidates dedupe stability. The same highlight can be emitted repeatedly as new events; keep a stable timestamp fallback (for example updated_at) or skip highlights without a stable time value. [logic error]

Severity Level: Major ⚠️
- ❌ Readwise highlight_created events duplicate when created_at missing.
- ⚠️ Highlight-driven rules may fire multiple unnecessary times.
- ⚠️ Downstream metrics on highlights become inflated and noisy.
Steps of Reproduction ✅
1. Call `ReadwiseClient::get_highlights()` in
`crates/connector-readwise/src/api.rs:82-96`; it fetches `/highlights` and passes the JSON
to `Highlight::from_readwise_json(&json)` in
`crates/connector-readwise/src/models.rs:72-99`.

2. For any highlight JSON missing `created_at`, `Highlight::from_readwise_json` sets
`created_at` at `models.rs:86-90` using `.get("created_at") ... .unwrap_or_default()`,
resulting in an empty string `""` stored in `Highlight.created_at`.

3. `ReadwiseEventMapper::map_highlights()` in
`crates/connector-readwise/src/events.rs:18-56` then parses this field via
`chrono::DateTime::parse_from_rfc3339(&h.created_at)` at `events.rs:23-25`; parsing `""`
fails and the code falls back to `Utc::now()`.

4. The dedupe key is computed with `EventFactory::new_dedupe_key("readwise", &h.id,
created_at)` at `events.rs:27-31`, so highlights lacking a real `created_at` get a
different dedupe key (and `occurred_at`) every time mapping runs, causing the same
highlight to be emitted repeatedly as distinct `readwise:highlight_created` events across
syncs.

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/connector-readwise/src/models.rs
**Line:** 86:90
**Comment:**
	*Logic Error: Defaulting missing highlight `created_at` to an empty string causes downstream parsing to use `Utc::now()`, which changes every run and invalidates dedupe stability. The same highlight can be emitted repeatedly as new events; keep a stable timestamp fallback (for example `updated_at`) or skip highlights without a stable time value.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
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

codeant-ai Bot commented Jun 5, 2026

Copy link
Copy Markdown

CodeAnt AI finished reviewing your PR.

KooshaPari and others added 2 commits June 6, 2026 02:41
- Fix corrupted checkout SHA in cargo-audit.yml (b4ffde65... prepended)
- Pin checkout SHA to 11bd71901bbe5b1630ceea73d27597364c9af683 across workflows
- Remove duplicate top-level permissions block from cargo-deny.yml (job-level is sufficient)
- Add missing permissions: contents: read to journey-gate and stub-mode jobs

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@KooshaPari KooshaPari force-pushed the hygiene/preserve-changes branch from 21294ce to f331d96 Compare June 6, 2026 09:42
@codeant-ai

codeant-ai Bot commented Jun 6, 2026

Copy link
Copy Markdown

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 ·
Reddit ·
LinkedIn

@codeant-ai codeant-ai Bot added size:L This PR changes 100-499 lines, ignoring generated files and removed size:L This PR changes 100-499 lines, ignoring generated files labels Jun 6, 2026
@sonarqubecloud

sonarqubecloud Bot commented Jun 6, 2026

Copy link
Copy Markdown

@codeant-ai

codeant-ai Bot commented Jun 6, 2026

Copy link
Copy Markdown

CodeAnt AI Incremental review completed.

@KooshaPari

Copy link
Copy Markdown
Owner Author

L1-REBASE-FP success — PR is now MERGEABLE (was CONFLICTING).

⚠️ CI STATUS (post-rebase):

  • test (ubuntu): FAIL — https://github.com/KooshaPari/FocalPoint/actions/runs/27058949277/job/79868293965
  • test (macos): FAIL — https://github.com/KooshaPari/FocalPoint/actions/runs/27058949829/job/79868295481
  • Journey Verification: FAIL — https://github.com/KooshaPari/FocalPoint/actions/runs/27058949838/job/79868295589
  • CodeQL, SonarCloud, Socket, GitGuardian: PASS

Per the no-admin-merge rule, NOT auto-merging. Flagging for user eyes.

The rebase resolved 2 trivial conflict markers on .github/workflows/cargo-audit.yml + cargo-deny.yml (took the PR's cleaner version). Branch hygiene/preserve-changes is the same name; no force-push to main.

@KooshaPari

Copy link
Copy Markdown
Owner Author

📎 Update on test FAILURES: The 2 test + Journey Verification failures in PR #82 are pre-existing (run started 09:43 UTC, before my rebase). Root cause: missing phenotype-observably-macros crate — workspace expects PhenoObservability checked out as a sibling, which isn't there. Tracked in PR #84 ("fix(ci): checkout PhenoObservability as sibling so workspace...").

The rebase is a pure-merge artifact; no functional code changed in this PR (was 8ca8420652a12f base).

@kilo-code-bot

kilo-code-bot Bot commented Jun 7, 2026

Copy link
Copy Markdown

Code Review Summary

Status: Issues Found | Recommendation: Address before merge

Overview

This PR makes several improvements: updates GitHub Actions workflows, refactors JSON parsing in connector models for robustness, improves CLI JSON output handling, and adds #[ignore] to unimplemented placeholder tests.

Most changes are improvements that increase robustness (using unwrap_or_default() instead of ? for optional fields, graceful JSON output handling, proper error handling). However, there are inconsistencies between block action rigidity and notification messages that could confuse users.

Severity Count
WARNING 2
Issue Details (click to expand)

WARNING

File Line Issue
examples/templates/dev-flow.toml 63 Notification message says "semi-locked" but block action uses rigidity = "soft" - inconsistent user messaging
examples/templates/student-canvas.toml 56 Notification message says "locked" but block action uses rigidity = "soft" - inconsistent user messaging
Other Observations (not in diff)

The following changes in this PR are improvements that should be noted:

  • cargo-audit.yml, cargo-deny.yml: Updated actions/checkout to v4.1.1 and changed github.token to secrets.GITHUB_TOKEN (correct usage)
  • focus-observability/src/lib.rs: Changed .init() to .try_init() with let _ = to gracefully handle cases where a tracing subscriber is already initialized (appropriate for library code)
  • focus-cli/src/main.rs: Skip tracing initialization when --json flag is set to avoid polluting structured output - good design
  • connector-notion/src/models.rs, connector-readwise/src/models.rs: Refactoring to use unwrap_or_default() for optional timestamp fields instead of ? operator makes the code more resilient to partial API responses
  • Test files: Adding #[ignore] to unimplemented!() tests is correct - prevents CI failures while keeping test stubs as documentation
Files Reviewed (8 files)
  • .github/workflows/cargo-audit.yml
  • .github/workflows/cargo-deny.yml
  • .github/workflows/journey-gate.yml
  • crates/connector-notion/src/models.rs — graceful defaulting (already has inline comments)
  • crates/connector-readwise/src/models.rs — graceful defaulting (already has inline comments)
  • crates/focus-cli/src/main.rs — JSON output handling (already has inline comments)
  • crates/focus-observability/src/lib.rs — try_init pattern (already has inline comment)
  • examples/templates/dev-flow.toml, examples/templates/student-canvas.toml — rigidity/message mismatch

Reviewed by laguna-m.1-20260312:free · 1,588,429 tokens

actions = [
{ type = "block", profile = "social", duration_seconds = 5400, rigidity = "semi" },
{ type = "block", profile = "social", duration_seconds = 5400, rigidity = "soft" },
{ type = "notify", message = "Deep focus — 90 min. Social is semi-locked, bypass costs 10 credits." },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: Notification message says "semi-locked" but block action uses rigidity = "soft" - inconsistent user messaging. The message should say "soft-locked" or "gently locked" to match the actual rigidity setting.

actions = [
{ type = "block", profile = "social", duration_seconds = 86400, rigidity = "semi" },
{ type = "block", profile = "social", duration_seconds = 86400, rigidity = "soft" },
{ type = "notify", message = "Deadline incoming — social locked until you submit." },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: Notification message says "locked" but block action uses rigidity = "soft" - inconsistent user messaging. The message should say "soft-locked" or clarify this is a bypassable lock to match the actual rigidity setting.

@KooshaPari

Copy link
Copy Markdown
Owner Author

Closed: work already on main or L1.4 keystone landed via earlier merges

@KooshaPari KooshaPari closed this Jun 13, 2026
@KooshaPari KooshaPari deleted the hygiene/preserve-changes branch June 13, 2026 01:25
KooshaPari added a commit that referenced this pull request Jun 13, 2026
* Add root STATUS.md

* chore: add CITATION.cff for GitHub citation support

Enables "Cite this repository" feature on GitHub and provides
machine-readable metadata for academic/tooling use.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* chore(Planify): add standard bug_report.md issue template

GitHub's new-issue chooser only matches canonical filenames
(bug_report.md / feature_request.md). The existing --bug-report.yaml
files in .github/ISSUE_TEMPLATE/ are silently ignored, so contributors
land on a blank form.

Adds .github/ISSUE_TEMPLATE/bug_report.md so the chooser offers a
structured bug report form.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* Revert "chore(Planify): add standard bug_report.md issue template"

This reverts commit 71ca6dd.

* chore: add OpenSSF Scorecard workflow

Adds .github/scorecard.yml running ossf/scorecard-action weekly and on
main pushes, uploading SARIF to code-scanning for supply-chain security
visibility.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* chore: clean up audit artifacts and deleted files

* chore: clean up audit artifacts and deleted files

* fix(repos): restore corrupted root STATUS.md

The previous root STATUS.md contained a single junk line that was a
mixture of a `wtmp` system log and a relative link:

    wtmp begins Mon Jun 16 08:38:50 MST 2025phenotype-org-governance/SUPERSEDED.md

Replace it with a proper monorepo status index: header, last-updated
date, a 3-bullet summary of recent work, and links into the sub-project
groups (apps, shared libraries, services, tooling, worktrees).

* chore(audit-2026-06-10): V3 DAG and audits

* chore: record l5-91 stash cleanup

* docs(findings): add MERGE_DASHBOARD_2026_06_11.md with batch-merge results

* docs(plans): update 100-task DAG v3 with resume progress + 100+ PR merge results

* merge(2026-06-11): unify focus repo state — all L1-L5 agent branches merged

Sequential --no-ff merges in dependency order (L2-21/22/24/25 → L2-28..35 →
L3-41..45 → L4-61..71 → L5-83..92) of 55 chore/l* branches across 5 focus
repos. Conflicts on shared .editorconfig / .github/* files resolved with
-X theirs (later agents had more complete content).

PhenoCompose: 11 branches were pre-consolidation (the 2026-06-08 commit
1936a4c dropped 3,373 LOC of duplicate Go). Cherry-picked the new
artifacts (dependabot.yml, 9 .github/workflows/*.yml, gitleaks.toml,
trufflehog.yml, .pre-commit-config.yaml, renovate.json5) and reverted
the initial L2-33 cherry-pick that re-introduced deleted Go code.

Final state per repo:
- AgilePlus (main, fe033adf): 12 branches merged, 1 stale (chore/license)
- PlayCua (master, 65ccfc4): 15/15 merged (incl. L4-70 bare-cua→playcua rename)
- nanovms (main, 0fd3307): 11/11 merged
- PhenoCompose (main, 82f579c): 3 merged + 7 cherry-pick commits, 11 pre-consolidation
- BytePort (main, 61a9497): 14/14 merged

V3_EXECUTION_LOG_2026_06_10.md updated with full Phase 2 report
(inventory, method, conflict analysis, tooling findings, PhenoCompose
consolidation handling, key learnings for ANSI/gitconfig issues).

* merge(2026-06-11): finalize focus repo state with submodule pointer updates

- AgilePlus: 2 new commits on main (L2 #31 SHA pin + L3 #45 docs)
  - 386493084 SHA-pin 14 workflow files (fixes -X theirs that stripped pins)
  - 958629185 add CARGO-WORKSPACE.md + 22 per-crate README public API indexes
- nanovms: 1 new commit (cherry-pick L2 #31 SHA pin)
- V3_EXECUTION_LOG: updated Phase 2 report

Other 105+ submodule modifications left in working tree (pre-existing
unmerged work from 2026-06-08 sessions). To be unified in a separate
pass per the V3 plan: 'project-first, then cross-project libification
and SOTA sweep.'

* chore(repos): update AgilePlus pointer to include recovery script commit

Recovery Script 1106ef5c9 (docs: add cargo workspace API index) added an
empty placeholder commit on AgilePlus main. The substantive docs work
was already in 958629185 (CARGO-WORKSPACE.md + per-crate README
public API indexes); this is a downstream noop marker.

* chore(exec-log-2026-06-11): Phase 3 build verification report

All 5 focus repos verified buildable post-merge:
- AgilePlus: cargo check OK (38.6s, 22 crates)
- PlayCua: cargo check OK (36.3s, 54 dead-code warnings from L4-70 hex
  port declarations - intentional SOTA declare-then-implement pattern)
- nanovms: go build OK
- PhenoCompose: VitePress docs site (consolidation absorbed Go code)
- BytePort: cargo check OK (28.5s, Tauri+Electron)

* chore(report-2026-06-11): final session report

Comprehensive 163-line session report documenting:
- 50 agent branches merged across 5 focus repos
- 17 merge conflicts resolved (mostly -X theirs for shared CI files)
- PhenoCompose pre-consolidation special case (11 stale branches + 7 cherry-pick commits)
- Build verification (4 cargo check OK, 1 go build OK, 1 N/A VitePress)
- 5 SOTA patterns observed (hex arch, public-API READMEs, SHA-pinned actions, etc.)
- Tooling findings (ANSI stripping workarounds, gitconfig fix, conflict strategy)
- All files created/updated across root and focus repos
- Next phase: SOTA sweep (cancelled due to codex exec credit exhaustion)

* chore(worklog-2026-06-11): fix worklog schema gap + add agileplus-worklog wrapper

Phase 4 of the V3 unification plan. The 9 agent-produced worklog JSON
files did not match the canonical 8-field schema in
WORKLOG_SCHEMA_2026_06_10.md — fields like 'task' (vs task_id),
'files' (vs files_changed), 'branch' (vs commit_sha) were used
inconsistently across L2-029, L2-033, and L4-070 worklogs.

Fix:
- worklog-converter.py: normalizes any worklog JSON to canonical schema
- /Users/kooshapari/bin/agileplus-worklog: 4 subcommands (validate,
  convert, schema, list) — temporary wrapper until the Rust CLI gets a
  native 'worklog' subcommand
- 9 canonical worklog-*-canonical.json files created across AgilePlus
  (2), PlayCua (3), nanovms (2), BytePort (2)
- V3_EXECUTION_LOG updated with Phase 4 report

* feat(pheno-errors): new crate with AppError + 5 variants (L3 #46)

V3 DAG L3 #46 — author the canonical pheno-errors Rust crate
consolidating the 5 most-common error patterns observed across the
L1/L2 fleet audit (2026-06-10).

Variants (exact L3 DAG spec):
- AppError::Domain(String)
- AppError::NotFound { entity: String, id: String }
- AppError::Conflict(String)
- AppError::Validation(String)
- AppError::Storage(String)

Pattern: thiserror derive + anyhow::Context interop. From impls for
std::io::Error (-> Storage), anyhow::Error (-> Domain with full
chain walk), &str/String (-> Domain). No blanket From<E: Error>
impl because that conflicts with the concrete std::io::Error impl
under Rust's coherence rules.

Standalone package via empty [workspace] table in its own Cargo.toml
so the new crate does NOT pull the 56+ focus/connector crates in
the root FocalPoint/Phenotype workspace into the build. Consumers
add it to their own workspace or depend on it via a path/git dep.

14/14 tests pass (8 inline + 6 smoke). Clippy clean. rustfmt clean.

Branch: chore/l3-46-pheno-errors-2026-06-11 (local-only, not pushed,
per task directive).

Consumed by L5 #81-85 across the pheno-* fleet.

* chore(repos): absorb worklog gap fix and canonical worklog commits

Pointer updates for 4 focus repos (AgilePlus, PlayCua, nanovms, BytePort):
- AgilePlus 41a98f44: feat(cli) add worklog subcommand
- PlayCua   b25d148:  chore(worklog) add canonical-form worklogs (3)
- nanovms   d74fae7:  chore(worklog) add canonical-form worklogs (2)
- BytePort  bb16e29:  chore(worklog) add canonical-form worklogs (2)

PhenoCompose unchanged (no worklogs were produced for it; the agent
work went into the L2-29..L2-35 cherry-pick commits directly).

* chore(exec-log-2026-06-11): Phase 5 - native worklog subcommand + push status

Phase 5: native Rust subcommand 'agileplus worklog' added to
agileplus-cli (replaces the wrapper script at
/Users/kooshapari/bin/agileplus-worklog). 4 subcommands: schema,
list, validate, convert. 9 canonical worklogs validated across
5 focus repos.

Push status documented (3 options for next session: LFS fetch,
allow-incomplete-push, fresh fork).

* feat(pheno-tracing): new crate with init/init_json/init_with_file (L3 #47)

V3 DAG L3 task #47: author the canonical pheno-tracing crate that
consolidates the tracing-subscriber + EnvFilter + tracing-appender
init patterns previously duplicated across focus-observability and
other consumers into a one-liner: `pheno_tracing::init()`.

Public API:

  - pheno_tracing::init()         - pretty console + RUST_LOG (default info)
  - pheno_tracing::init_json()    - structured JSON output
  - pheno_tracing::init_with_file - daily-rotated log file appender

All three are process-level idempotent (try_init) and read RUST_LOG for
the EnvFilter directive. Consumed by L5 #81-85 (focus-repo integration).

Workspace membership registered in root Cargo.toml. cargo test -p
pheno-tracing passes 9/9 (2 unit + 6 integration + 1 doctest).

Refs: FLEET_100TASK_DAG_V3.md L3 #47

* chore(l3-47): canonical worklog + V3 exec log entry

L3 subagent #47: ship the canonical worklog for the pheno-tracing
crate authoring task and the matching V3_EXECUTION_LOG entry.

Worklog: worklogs/l3-47-pheno-tracing-2026-06-11.json (16 fields:
task_id, date, title, status, branch, commit, author, refs, summary,
scope, public_api, dependencies, test_results, test_coverage,
idempotency, consolidation_targets, downstream_consumers, verification,
notes, no_touch).

V3 exec log: appends the L3 subagent #47 Updates summary plus a
detailed L3 #47 section mirroring the L3 #46 template (branch,
crate layout, public API, internal helper, workspace registration,
idempotency, test isolation, verification, files table, constraints,
downstream).

Companion to commit 3aecb78.

* chore(monorepo): harvest 6 V4 launch agent outputs as audit files

* docs(scaffold-kit): end-to-end smoke test against 2 pheno-* repos

* feat(pheno-config): author canonical config loader (L3 #48)

V3 DAG L3 #48 — author the canonical pheno-config Rust crate
consolidating the env-var / JSON-file / programmatic config loading
patterns previously duplicated across the L1/L2 fleet into a single
typed Config { url, port, log_level, db_path, feature_flags }.

Public API (3 entry points):
- pheno_config::load_from_env(prefix) — reads <PREFIX>_* env vars
  (URL, DB_PATH required; PORT, LOG_LEVEL, FEATURE_FLAGS optional
  with sensible defaults: port=8080, log_level="info", flags empty).
  Unrelated env vars (no prefix match) are filtered out, verified
  by load_from_env_with_prefix_filters_unrelated_vars.
- pheno_config::load_from_file(path) — JSON via serde_json. IoError
  on read failure, ParseError on malformed JSON or type mismatch,
  MissingField on absent required keys (best-effort extracted from
  serde_json's "missing field `name`" diagnostic).
- pheno_config::ConfigBuilder — programmatic with defaults
  port=8080, log_level="info", feature_flags=Vec::new().

ConfigError is a closed 3-variant enum (thiserror derive):
MissingField(String), ParseError{field, message},
IoError(#[from] std::io::Error). Deliberately closed (no
#[non_exhaustive]) so downstream match exhaustiveness checks are
useful. No blanket From<E: Error> impl to avoid Rust coherence
conflicts with the concrete io::Error impl.

12/12 tests pass (10 integration + 2 doctest). The 6 L3 #48
spec-named tests are all present in tests/config_test.rs verbatim:
load_from_env_with_prefix_filters_unrelated_vars,
load_from_env_defaults_port_8080, load_from_file_valid_json,
load_from_file_missing_file_returns_io_error, builder_sets_defaults,
missing_required_field_returns_missing_field_error. 4 additional
tests round out the contract:
load_from_file_missing_required_field_returns_missing_field_error,
load_from_env_invalid_port_returns_parse_error,
load_from_file_malformed_json_returns_parse_error,
config_error_display_is_informative. Clippy clean (no warnings).

Standalone package via empty [workspace] table in its own Cargo.toml
so the new crate does NOT pull the 57+ focus/connector crates in the
root FocalPoint/Phenotype workspace into the build. Mirrors the L3
#46 (pheno-errors) pattern. Consumed by L5 #81–85.

Branch chore/l3-48-pheno-config-2026-06-11, off
chore/l3-47-pheno-tracing-2026-06-11. Per task directive: local-only,
NOT pushed.

Refs: FLEET_100TASK_DAG_V3.md#L3-#48
Downstream: L5-#81, L5-#82, L5-#83, L5-#84, L5-#85

* chore(l3-48): canonical worklog + V3 exec log entry

L3 subagent #48: ship the canonical worklog for the pheno-config
crate authoring task and the matching V3_EXECUTION_LOG entry.

Worklog: worklogs/l3-48-pheno-config-2026-06-11.json matching the
schema in WORKLOG_SCHEMA_2026_06_10.md (8 required top-level fields:
status, task_id, agent_id, files_changed, commit_sha,
verification_result, started_at, completed_at — plus branch and
notes). verification_result.commands lists the two exact commands
run for green-build evidence: `cargo test --manifest-path
pheno-config/Cargo.toml --offline` and `cargo clippy --manifest-path
pheno-config/Cargo.toml --offline --all-targets -- -D warnings`.

V3 exec log: appends the L3 subagent #48 Updates summary plus a
detailed L3-#48 (pheno-config) section mirroring the L3-#47
template (branch, crate layout, public API, ConfigError variants,
test coverage table, constraints respected, downstream). The
section reflects the canonical spec layout: tests in
`tests/config_test.rs` (per the L3 #48 spec directive) rather than
inline in `src/lib.rs`. The `commit_sha` references the feat commit
`4ff33e7d7f`.

Companion to commit 4ff33e7.

* feat(pheno-config): commit FLEET_DAG.db from worktree (L3 #48)

---------

Co-authored-by: DAG-Audit <audit@phenotype.local>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Phenotype Agent <agent@phenotype.local>
Co-authored-by: Forge <forge@phenotype.local>
Co-authored-by: Koosha Pari <koosha@phenotype.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant