Repository navigation
The harness hook judges a title argument as a title, by the keys the shim reads api fields by - #337
Conversation
…shim reads api fields by A tool call's strings were joined into one prose text, so an MCP merge's commit_title or a create-pull-request title got the invisible-character check alone, where the same text through gh pr merge --subject or gh api -f commit_title= also got the lookalike pass. The hook now notes the nearest key of each string. One under title, commit_title or subject is handed to the guards as a headline; one under squash_commit_message or merge_commit_message is a headline on its first line and prose below, as at the shim. Every string is still read whatever its key, and every checker other than the guards still reads them joined. The key list is the shim's API_TITLE_KEYS and API_MESSAGE_KEYS, shared rather than copied, with subject added. It is a closed list of which check a string gets, not a table of fields to read: a key it lacks leaves that string prose, which is how every string was judged before, so it can add a check and never remove one. Claude-Session: https://claude.ai/code/session_01QJkWXa2HxJ6q9TMNAGSNv4
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe text checker now accepts labeled headlines. The harness hook classifies selected string fields as headlines and treats other strings as prose. Commit-message fields are split into a first-line headline and remaining prose. ChangesUnicode headline checks
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant HookEvent
participant HookRun as hook::run
participant HookStrings as hook::strings
participant TextJudged as text::judged
participant Guards
HookEvent->>HookRun: tool-call values
HookRun->>HookStrings: recursively collect strings
HookStrings-->>HookRun: prose and keyed headlines
HookRun->>TextJudged: prose and labeled headlines
TextJudged->>Guards: body and headline checks
Merge Risk: ⚪ Minimal · up to No actionable issue is established that would prevent merging after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (98.91%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #337 +/- ##
==========================================
+ Coverage 94.27% 94.29% +0.01%
==========================================
Files 46 46
Lines 22566 22628 +62
==========================================
+ Hits 21275 21336 +61
- Misses 1291 1292 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A headline the harness hook judged was labelled "<tool> title" whatever key carried it, so a lookalike in commit_title, subject or squash_commit_message was reported under a field the call never had. Each headline now keeps its key, and the finding says "<tool> commit_title". The reference adds that the keys are read in every tool the hook's matcher sends it, so a matcher wider than the documented one puts a title, subject or commit_title of any other tool under the lookalike check as well. Claude-Session: https://claude.ai/code/session_01QJkWXa2HxJ6q9TMNAGSNv4
Part of #334 (item 4). Items 1-3 are in #335.
What changes
The harness (agent tool) hook joined every string of a tool call into one prose text, so an MCP
merge_pull_requestcommit_titleor acreate_pull_requesttitlegot the invisible-character check only, with no lookalike pass. The same title throughgh pr merge --subjectorgh api -f commit_title=is judged as a title.src/hook.rs:stringsnow records each string's nearest object key. A string undertitle,commit_titleorsubjectis a headline. A string undersquash_commit_messageormerge_commit_messageis split by the shim's ownmessage_subjects: the first line is a headline and the rest is prose. Every other string is prose. Every string is still read, whatever its key.src/text.rs:judgedtakesheadlines: &[Headline]. The guards get the prose (headlinefalse) and then each headline on its own (headlinetrue). Literals, prose rules and pattern rules still read all strings joined, as before.scan --textandguard --textpass&[], so they behave as before.src/shim.rs:API_TITLE_KEYSandAPI_MESSAGE_KEYS(andmessage_subjects) are nowpub(crate)and shared with the hook, not copied.subjectis added toAPI_TITLE_KEYS, sogh api -f subject=is now a title at the shim too. Each doc comment says the hook reads the list.docs/REFERENCE.md: the subject-vs-prose paragraph no longer lists the harness hook as prose. It says which tool-call keys are subjects. Thegh apikey table namessubject. "What it reads out of the call" adds a paragraph on why a closed key list is acceptable.Why a closed key list, not per-tool tables
docs/REFERENCE.md("What it reads out of the call") and thestringsdoc comment argue against a per-tool table of field names, because such a table "is missing the one a new server just added -- silently, and in the green direction". That argument is about a table of fields to READ: a missing name drops a string from every check. This list only decides which CHECK a string that is already read gets, and only towards the stricter one. A key the list lacks leaves its string as prose, which is exactly how every string was judged before this change, so the list can only add checks and never removes one. It is one closed list, keyed by field name and not by tool, and it is the same list the shim already uses forgh apiandglab apifields, so the two seams cannot drift apart.Done-when (item 4)
tests/hook_cli.rs:a_commit_title_argument_is_judged_as_a_subjectanda_title_argument_is_judged_as_a_subject: acommit_title(MCP merge) or atitle(MCP create PR) holding a Latin word with U+0430 is refused underprevent-unusual-unicode, and the report namesCYRILLIC SMALL LETTER A.the_same_word_in_a_body_is_prose: the same word underbodypasses.an_invisible_character_is_refused_under_any_key: U+202E underbody,title, and an unknown key is refused each time.an_unknown_key_stays_prose: the lookalike word under a key not on the list passes.a_merge_message_argument_is_a_subject_then_a_body: insquash_commit_message, a lookalike on the first line is refused, and one in the body passes.Also:
src/hook.rs, the unit testevery_string_is_collected_whatever_the_field_is_calledis updated, and a new unit testa_title_key_marks_a_headline_at_any_depthis added.tests/shim_cli.rs,a_title_set_through_the_api_is_judged_as_the_title_it_isadds a-f subject=form.Ran
cargo test --bin uphold, plus--test hook_cli,--test shim_cliand--test text_cli, andcargo clippy --all-targets -D warnings. The full gate has not been run here.https://claude.ai/code/session_01QJkWXa2HxJ6q9TMNAGSNv4
Summary by CodeRabbit