fix: Double-close fd in ManifestWatcher deinit + validation consistency

[2witstudios]

Summary

• ManifestWatcher double-close: deinit closed the file descriptor directly AND via the dispatch source's cancel handler. After the first close, the fd number can be reused by another open() call, and the second close would silently close a completely unrelated file descriptor belonging to another subsystem.
• isWatching data race: isWatching read source without queue synchronization, violating the class's own invariant that all mutable state is accessed on its serial queue.
• validate_name inconsistency: Leading-dot check used trimmed but path traversal checks used the original untrimmed name. Now consistently uses trimmed for all checks.

Test plan

• Rust validation tests pass (cargo test -p pu-core -- validation)
• Swift project builds successfully
• Manual: verify ManifestWatcher still detects file changes after atomic writes

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved validation handling for input names, now correctly detecting invalid characters when whitespace is present. Resolved potential thread-safety issues in internal monitoring systems.

[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]

Warning

Rate limit exceeded

@2witstudios has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes and 1 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d9031e5f-1634-485c-90f4-a2056713556e

📥 Commits

Reviewing files that changed from the base of the PR and between 278f249 and 525fd0b.

📒 Files selected for processing (3)

• crates/pu-cli/src/commands/bench.rs
• crates/pu-cli/src/output.rs
• crates/pu-core/src/validation.rs

📝 Walkthrough

Walkthrough

Two minor logic improvements across macOS and Rust codebases: a thread-safety enhancement to the isWatching property using queue synchronization, and a validation fix that now checks trimmed input for invalid character sequences while preserving original error messaging.

Changes

Cohort / File(s)	Summary
Thread-Safety Enhancement apps/purepoint-macos/purepoint-macos/Services/ManifestWatcher.swift	Made isWatching property thread-safe by wrapping the source != nil check with queue.sync.
Validation Logic Update crates/pu-core/src/validation.rs	Changed invalid-character validation to check trimmed input instead of original input, now detecting problematic sequences like '/', '\', or ".." after whitespace removal.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A queue-safe watch and trimmed input's grace,
Small fixes make the code run in its place,
Thread-safe reads and validation's care,
Polish and precision everywhere!
✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: fixing a double-close file descriptor issue in ManifestWatcher and addressing validation consistency across multiple checks.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch pu/fix-fd-double-close

---

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.}

[2witstudios]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

[coderabbitai]

🧹 Nitpick comments (1)

crates/pu-core/src/validation.rs (1)

37-48: Consider adding tests for whitespace-padded malicious inputs.

While the current tests cover core cases, explicit tests for whitespace-padded inputs would strengthen confidence in the trimmed validation behavior:

#[test]
fn given_whitespace_padded_traversal_should_reject() {
    assert!(validate_name(" ../evil").is_err());
    assert!(validate_name("foo/../bar ").is_err());
    assert!(validate_name(" /root").is_err());
}

🤖 Prompt for AI Agents

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

In `@crates/pu-core/src/validation.rs` around lines 37 - 48, Add unit tests
exercising validate_name with whitespace-padded inputs to ensure trimming before
validation rejects malicious values; create a test (e.g.,
given_whitespace_padded_traversal_should_reject) that calls validate_name with
strings like " ../evil", "foo/../bar " and " /root" and asserts is_err() for
each, reusing the existing test style and assertion pattern around validate_name
to confirm trimmed traversal and slash cases are rejected.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@crates/pu-core/src/validation.rs`:
- Around line 37-48: Add unit tests exercising validate_name with
whitespace-padded inputs to ensure trimming before validation rejects malicious
values; create a test (e.g., given_whitespace_padded_traversal_should_reject)
that calls validate_name with strings like " ../evil", "foo/../bar " and "
/root" and asserts is_err() for each, reusing the existing test style and
assertion pattern around validate_name to confirm trimmed traversal and slash
cases are rejected.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 52f31f54-2192-499d-bc0d-38cbcef5b607

📥 Commits

Reviewing files that changed from the base of the PR and between a33a4b8 and 278f249.

📒 Files selected for processing (2)

• apps/purepoint-macos/purepoint-macos/Services/ManifestWatcher.swift
• crates/pu-core/src/validation.rs

[2witstudios]

Addressed the CodeRabbit nitpick — added given_whitespace_padded_traversal_should_reject test in crates/pu-core/src/validation.rs covering " ../evil", "foo/../bar ", and " /root".

Also fixed 3 unused Result warnings from merged main code (bench.rs:60, bench.rs:78, output.rs:722) that were causing CI failures.

All local validation passes (clippy + tests).