refactor: Extract schedule_def tests to submodule

[2witstudios]

Summary

• Move 442 lines of tests from schedule_def.rs to schedule_def/tests.rs
• Follows the same module pattern as engine/, protocol/, types/, and output/
• Includes security fixes from CodeRabbit review

Changes

1. Extracted tests to submodule (standard Rust pattern)
2. Added name validation to find_schedule_def() to prevent path traversal
3. Fixed exclusive-after semantics in next_none_occurrence()
4. Updated tests to properly exercise global schedule directories

Before/After

Metric	Before	After
Main file	778 LoC (57% tests)	334 LoC
Tests	inline	442 LoC in tests.rs

Test plan

• cargo test -p pu-core schedule_def - 38 tests pass
• cargo clippy -p pu-core - no warnings
• CI: Format & Lint ✓, Test ✓, Build & Test ✓, Dependency Audit ✓

🤖 Generated with Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

📝 Walkthrough

Walkthrough

The single-file schedule definition implementation was replaced by a new module (mod.rs) exposing the ScheduleDef API, recurrence logic, YAML persistence, discovery (local/global), and associated helpers; an extensive test suite was added; a new scripts/churn utility script was also introduced.

Changes

Cohort / File(s)	Summary
Schedule module (new) crates/pu-core/src/schedule_def/mod.rs	Introduces the public ScheduleDef API (types: Recurrence, ScheduleTrigger, ScheduleDef), validation, YAML persistence, local/global discovery (list_schedule_defs, find_schedule_def, save_schedule_def, delete_schedule_def), and recurrence computation (next_occurrence) plus internal helper functions.
Schedule module (removed) crates/pu-core/src/schedule_def.rs	Removed previous single-file implementation (777 lines). Review for behavioral parity with new mod.rs if migrating callers.
Tests crates/pu-core/src/schedule_def/tests.rs	Adds extensive unit tests covering serialization/deserialization, defaults, validation, CRUD (local/global semantics), recurrence calculations (including edge cases like month boundaries/leap years), and filesystem interactions using temp dirs.
Utility script scripts/churn	New Bash script to compute file "hotspots" using git history + scc metrics; CLI flags (--days, --top, --min-loc, --no-tests, --json), dependency checks (scc, jq), and JSON/table output modes.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I hopped from one file into a tree,
YAML and schedules now sit with glee.
Tests in a row, I drum and hum,
New churn script finds where changes come.
Thump-thump — a tidy code bunny's plea.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the primary refactoring: extracting 442 lines of tests from schedule_def.rs into a new schedule_def/tests.rs submodule, aligning with the established module pattern.
Docstring Coverage	✅ Passed	Docstring coverage is 83.05% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch pu/refactor-schedule-def-tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/pu-core/src/schedule_def/mod.rs`:
- Around line 199-200: next_none_occurrence currently treats after == base as a
valid next occurrence, which violates the exclusive-after semantics used by
recurring branches; update the comparison in next_none_occurrence(base:
DateTime<Utc>, after: DateTime<Utc>) so it returns Some(base) only when after <
base and returns None when after >= base, ensuring a one-shot schedule is not
considered runnable again when after == base.
- Around line 122-136: find_schedule_def currently skips name validation and
allows path traversal; call the existing validate_name(name) at the start of
find_schedule_def and return None (reject) if validation fails so names like
"../other" are not used to build dir.join(format!("{name}.yaml")). Keep the rest
of the lookup logic (find_in_dir, paths::schedules_dir,
paths::global_schedules_dir) unchanged — just early-return on invalid names.

In `@crates/pu-core/src/schedule_def/tests.rs`:
- Around line 182-199: The tests only write schedule defs into
paths::schedules_dir(root) so they never exercise global schedules; update the
failing tests (e.g.,
given_local_and_global_schedule_defs_should_list_local_first and the test around
lines 273-284) to use two separate TempDir roots—one for the project root and
one to act as HOME—call isolate_home() with the HOME temp dir, create and write
a global fixture into paths::global_schedules_dir(home_root) (using
save_schedule_def) and create local fixtures in
paths::schedules_dir(project_root), and for the duplicate-name case write the
same schedule name into both global and local so that
list_schedule_defs()/find_schedule_def() must prefer the local copy and set
scope to "local".

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6da6ce74-7cc7-478b-8ba5-b56c4953c11f

📥 Commits

Reviewing files that changed from the base of the PR and between 7b72dbb and 561c3cc.

📒 Files selected for processing (3)

• crates/pu-core/src/schedule_def.rs
• crates/pu-core/src/schedule_def/mod.rs
• crates/pu-core/src/schedule_def/tests.rs

💤 Files with no reviewable changes (1)

• crates/pu-core/src/schedule_def.rs

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

crates/pu-core/src/schedule_def/mod.rs (1)

39-41: Consider reusing the existing default_agent helper.

There's already a default_agent() function in crate::serde_defaults (line 8-10 per the context snippet) that returns the same "claude".to_string(). Using it would reduce duplication:

-fn default_agent() -> String {
-    "claude".to_string()
-}

Then update line 34:

#[serde(default = "crate::serde_defaults::default_agent")]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/pu-core/src/schedule_def/mod.rs` around lines 39 - 41, Replace the
local fn default_agent() in this module with reuse of the existing
crate::serde_defaults::default_agent to avoid duplication: update the serde
attribute that currently references the local helper to #[serde(default =
"crate::serde_defaults::default_agent")] and remove the duplicate fn
default_agent() from this file so the module uses the centralized helper
instead.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@scripts/churn`:
- Around line 33-43: The options --days, --top, and --min-loc currently call
shift 2 without checking that a value exists or is valid; update the argument
parsing loop to validate $2 before consuming it: for each of DAYS, TOP, and
MIN_LOC (the cases handling "--days", "--top", "--min-loc") check that "$2" is
non-empty and does not begin with "-" (and optionally that it matches a numeric
pattern) and if invalid call usage or print a clear error and exit; only then
assign the variable and perform shift 2. Ensure flags like --no-tests, --json
and -h/--help continue to use single shifts unchanged.
- Around line 92-94: The current lookup uses grep -F "$file" against scc_file
which does substring matching and can produce false positives; replace that line
so it performs an exact match on the first TSV column instead. Modify the
scc_line assignment (the code that sets scc_line from scc_file) to use an
awk-based check that splits fields on tab and returns the first matching record
where the first column equals the value of $file (and exits after the first
match), so [[ -z "$scc_line" ]] logic remains valid; update any related variable
names if needed to preserve behavior.

---

Nitpick comments:
In `@crates/pu-core/src/schedule_def/mod.rs`:
- Around line 39-41: Replace the local fn default_agent() in this module with
reuse of the existing crate::serde_defaults::default_agent to avoid duplication:
update the serde attribute that currently references the local helper to
#[serde(default = "crate::serde_defaults::default_agent")] and remove the
duplicate fn default_agent() from this file so the module uses the centralized
helper instead.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 81eabf47-713a-4ad3-ac55-98cbb9baa431

📥 Commits

Reviewing files that changed from the base of the PR and between 561c3cc and 6d0ba6f.

📒 Files selected for processing (3)

• crates/pu-core/src/schedule_def/mod.rs
• crates/pu-core/src/schedule_def/tests.rs
• scripts/churn