Follow-ups from PR #21 review: canonicalise SessionStart hooks + harden tests#27
Merged
Merged
Conversation
…tests Follow-ups from the multi-agent review of PR #21 (merged). Three agents converged on merge_session_start_hook under-delivering its docstring. Behavior (review #1/#10): - merge_session_start_hook now canonicalises Clarion-owned hooks to exactly one running the desired command: refresh the first, remove any extras (a stale duplicate, or one pinned to a different project), and drop a Clarion-dedicated group left empty. Previously it short-circuited on a current entry and refreshed only the first stale one, so a current hook coexisting with a wrong-project pin (hand-merged settings) silently oriented the wrong project every session, and multiple stale entries left survivors. Idempotency preserved (single exact match → no-op). Tests: different-path refresh, current+stale removal, multi-stale dedup. Tests / docs: - schema.rs: migration_0002_records_its_own_version_row exercises 0002's SQL directly (bypassing apply_one's INSERT OR IGNORE fallback) so the in-transaction version-row INSERT is load-bearing — verified it fails if that line is dropped. The rollback test now also asserts the version-2 row rolls back together with the column. (review #2) - install.rs: correct the non-dir-guard comment — only --all reaches that block (--skills/--hooks alone are Components with init_clarion()==false). (review #3) - install.rs tests: cover the --force non-directory .clarion guard. (review #7) - hooks_settings.rs: shell_single_quote("") -> "''" edge case. (review #6) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups from the multi-agent review of #21 (merged). No behavior in #21 was a merge blocker; these address findings the review surfaced in the agent-orientation code.
Findings addressed
merge_session_start_hookshort-circuited on a current entry and refreshed only the first stale one → a wrong-project pin coexisting with the current hook silently oriented the wrong project every session; multiple stale entries left survivorsapply_one'sINSERT OR IGNOREmasked its absence)migration_0002_records_its_own_version_rowexercises 0002's SQL directly — verified it fails if the line is dropped. Rollback test now also asserts the version-2 row rolls back with the column.install.rsnon-dir-guard comment wrongly listed--skills/--hooks--allreaches the block (--skills/--hooksalone areComponents,init_clarion()==false).shell_single_quote("")(zero-iteration input) untested"" -> ''.--force+ non-directory.clarionguard untestedinstall_force_rejects_non_directory_clarion.Findings #5 (Components don't guard non-dir
.clarion) is a deliberate asymmetry — those flags never touch.clarion— now documented by the #3 comment fix. #8 (non-stringcommandsilently skipped) requires hand-crafted malformed settings; left as-is (YAGNI).Tests
TDD throughout. The two genuinely-broken merge cases failed first (the realistic different-path single-stale case already passed — confirming it was fine). The migration provenance test was verified to discriminate (fails without the in-SQL INSERT). Full gate green: fmt, clippy
-D warnings, nextest (316 pass), rustdoc.🤖 Generated with Claude Code