[WOLIKIMCHENG]

Description

Preserve existing .vibe/hooks.toml files when Vibe cleanup finds no
Spec Kit-managed hooks to remove.

This prevents no-op installation and teardown paths from rewriting user
content, changing line endings, or deleting comment-only and whitespace-only
files. Cleanup of Spec Kit-managed hooks remains unchanged.

Testing

• Focused Vibe cleanup tests: 22 passed
• Compile and import checks: passed
• git diff --check: passed

AI Disclosure

• I did not use AI assistance for this contribution
• I did use AI assistance (fill in the disclosure below)

AI disclosure: OpenAI Codex (GPT-5.6 Sol, Terra, and Luna;
human-supervised) assisted with code and tests.

[Copilot]

Copilot review overview

🟢 Approval recommended

The focused fix is correct and has comprehensive regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Preserves unowned Vibe hook configuration during no-op cleanup.

Changes:

• Skips rewriting when no Spec Kit hook is removed.
• Adds regression coverage for content, timestamps, comments, whitespace, and forced teardown.

File	Description
src/​specify_cli/​events.py	Adds the no-op cleanup guard.
tests/​integrations/​test_events.py	Tests preservation of unowned Vibe files.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[mnriem]

The behavioral fix and regression coverage look correct, but this reinforces the wrong ownership boundary. Vibe’s native hooks format and the lifecycle of .vibe/hooks.toml should be owned entirely by VibeIntegration, not implemented through Vibe-specific branches and helpers in the shared events.py module.

Shared event code should resolve canonical events, manage the shared dispatcher, and delegate native configuration generation and cleanup to the integration. Please move the Vibe rendering, merging, and removal behavior—including this no-op preservation rule—behind integration methods implemented by VibeIntegration.

The new regression cases should likewise live in tests/integrations/test_integration_vibe.py, alongside the existing TestVibeTomlMerging coverage, rather than expanding the shared event test module with Vibe-specific behavior.

Posted on behalf of @mnriem by GitHub Copilot (model: GPT-5.6 Sol, human-supervised); comment fully AI-drafted.

[WOLIKIMCHENG]

Addressed the ownership feedback in 35c3767. Vibe-specific hook rendering and cleanup now live in VibeIntegration, while shared events retains dispatcher and manifest lifecycle orchestration. Regression coverage now follows the same boundary. Focused and broader event/integration tests pass.

[Copilot]

Copilot review overview

🔵 Needs a closer look

ASCII-only JSON escaping can produce invalid TOML for non-BMP characters.

Review effort: Balanced
Findings: None

[mnriem]

Please address Copilot feedback and fix test & lint errors

[WOLIKIMCHENG]

Addressed the Vibe review feedback in 1359d75: hook strings now use TOML-native escaping, the hooks.toml manifest key is normalized to .vibe/hooks.toml, and parser-backed Unicode/control-character regressions were added. Focused regressions (4), the Vibe/shared-events tests (195), Ruff, and git diff --check pass.

[Copilot]

🟡 Changes recommended

Verified Unicode line-splitting behavior can misidentify and delete user-owned hooks.

1 open finding

• Scanner incorrectly treats Unicode separators as TOML line breaks · New

🧠 Review effort: Balanced

[mnriem]

Please address Copilot feedback

[Copilot]

🟡 Changes recommended

A new timestamp assertion fails on Windows because it assumes nanosecond filesystem precision.

1 open issue

• Read back NTFS mtime baseline after os.utime() · New

1 resolved since last review

• Scanner incorrectly treats Unicode separators as TOML line breaks

_{Review effort: Balanced}

[mnriem]

Please address Copilot feedback

[WOLIKIMCHENG]

Thanks for flagging the Copilot feedback. I updated the Vibe cleanup test to compare against the stored filesystem mtime and am following up with the focused test results in the review thread.

[Copilot]

🔵 Needs a closer look

The new TOML scanner controls deletion of user configuration and has a confirmed preservation regression requiring correction and human validation.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Preserve standalone comments before retained user hook tables

src/​specify_cli/​integrations/​vibe/​__init__.py:439

Ending the managed span at the next table header also deletes standalone comments introducing that table. For example, a generated hook followed by # User safety hook: do not remove and an unowned [[hooks]] block loses the comment during both refresh and teardown, even though the user hook remains. An isolated comparison confirmed that the previous cleanup preserved this comment and the new scanner removes it.

Exclude trailing standalone comments from managed spans, while still including complete multiline values. Add regression coverage for refresh and teardown with a comment immediately before a retained user hook.

AI disclosure: GitHub Copilot (model unknown; automated review for the requester) generated this comment.

1 resolved since last review

• Read back NTFS mtime baseline after os.utime()

_{Review effort: Balanced}

[mnriem]

Please address Copilot feedback. If not applicable, please explain why

[Copilot]

🔵 Needs a closer look

The new TOML scanner determines which user-file content is deleted and warrants human validation beyond the reported regression checks.

No open issues 🎉

_{Review effort: Balanced}

[mnriem]

Thank you!