Skip to content

refactor: extract applescript_escape into shared escape module#379

Merged
Wirasm merged 1 commit into
mainfrom
kild/327-applescript-escape
Feb 11, 2026
Merged

refactor: extract applescript_escape into shared escape module#379
Wirasm merged 1 commit into
mainfrom
kild/327-applescript-escape

Conversation

@Wirasm

@Wirasm Wirasm commented Feb 11, 2026

Copy link
Copy Markdown
Owner

Summary

  • Move applescript_escape from terminal::common::escape to a new top-level escape module in kild-core
  • Eliminates cross-domain dependency where notify reached into terminal internals for a general-purpose string escaping function
  • Fix pre-existing unused imports in git/health.rs exposed by cargo fmt

Closes #327

Test plan

  • cargo fmt --check passes
  • cargo clippy --all -- -D warnings passes
  • cargo test --all passes (803 tests)
  • cargo build --all succeeds

Move applescript_escape from terminal::common::escape to a top-level
escape module in kild-core. This eliminates the cross-domain dependency
where notify reached into terminal internals for a general-purpose
string escaping function.

Also fix pre-existing unused imports in git/health.rs exposed by
cargo fmt (moved test-only imports into #[cfg(test)] module).

Closes #327
@Wirasm

Wirasm commented Feb 11, 2026

Copy link
Copy Markdown
Owner Author

PR Review Summary

Critical Issues (0 found)

None.

Important Issues (0 found)

None.

Suggestions (2 found)

Agent Suggestion Location
pr-test-analyzer Add explicit test for \r carriage return escaping (implemented but not tested) crates/kild-core/src/escape.rs:8
pr-test-analyzer Add test for combined escape sequences to prevent replacement-order regressions crates/kild-core/src/escape.rs:4-9

Strengths

  • Clean extraction — eliminates cross-domain dependency (notify → terminal) without changing behavior
  • Tests moved with the function, maintaining coverage
  • Unused imports in git/health.rs cleaned up properly
  • All error handling in affected code paths meets standards — no silent failures
  • No documentation updates needed (internal implementation detail)

Documentation Updates

None required — no docs reference the old module path.

Verdict

READY TO MERGE

All 4 review agents passed. The two suggestions are minor test coverage improvements (not blockers).


Reviewed by: code-reviewer, docs-impact-agent, pr-test-analyzer, silent-failure-hunter

@Wirasm Wirasm merged commit ff005e8 into main Feb 11, 2026
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: extract applescript_escape from terminal into shared utility

1 participant