Skip to content

The harness hook judges a title argument as a title, by the keys the shim reads api fields by - #337

Merged
HackingGate merged 2 commits into
mainfrom
unicode-hook-titles-334
Oct 9, 2026
Merged

HackingGate merged 2 commits into
mainfrom
unicode-hook-titles-334

Conversation

@HackingGate

@HackingGate HackingGate commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

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_request commit_title or a create_pull_request title got the invisible-character check only, with no lookalike pass. The same title through gh pr merge --subject or gh api -f commit_title= is judged as a title.

  • src/hook.rs: strings now records each string's nearest object key. A string under title, commit_title or subject is a headline. A string under squash_commit_message or merge_commit_message is split by the shim's own message_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: judged takes headlines: &[Headline]. The guards get the prose (headline false) and then each headline on its own (headline true). Literals, prose rules and pattern rules still read all strings joined, as before. scan --text and guard --text pass &[], so they behave as before.
  • src/shim.rs: API_TITLE_KEYS and API_MESSAGE_KEYS (and message_subjects) are now pub(crate) and shared with the hook, not copied. subject is added to API_TITLE_KEYS, so gh 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. The gh api key table names subject. "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 the strings doc 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 for gh api and glab api fields, so the two seams cannot drift apart.

Done-when (item 4)

tests/hook_cli.rs:

  • a_commit_title_argument_is_judged_as_a_subject and a_title_argument_is_judged_as_a_subject: a commit_title (MCP merge) or a title (MCP create PR) holding a Latin word with U+0430 is refused under prevent-unusual-unicode, and the report names CYRILLIC SMALL LETTER A.
  • the_same_word_in_a_body_is_prose: the same word under body passes.
  • an_invisible_character_is_refused_under_any_key: U+202E under body, 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: in squash_commit_message, a lookalike on the first line is refused, and one in the body passes.

Also:

  • In src/hook.rs, the unit test every_string_is_collected_whatever_the_field_is_called is updated, and a new unit test a_title_key_marks_a_headline_at_any_depth is added.
  • In tests/shim_cli.rs, a_title_set_through_the_api_is_judged_as_the_title_it_is adds a -f subject= form.

Ran cargo test --bin uphold, plus --test hook_cli, --test shim_cli and --test text_cli, and cargo clippy --all-targets -D warnings. The full gate has not been run here.

https://claude.ai/code/session_01QJkWXa2HxJ6q9TMNAGSNv4

Summary by CodeRabbit

  • New Features
    • Publish-call inputs now check recognized title fields—including pull request subjects—as headlines. For full commit or merge messages, the first line is checked as a headline and the remaining lines as prose.
    • Unusual Unicode in headline fields is reported with the field name.
  • Bug Fixes
    • Invisible Unicode is refused in body text, title fields, and unrecognized fields.

…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
@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b1cf8c24-cb6b-4165-a0e5-b3919a55a61b

📥 Commits

Reviewing files that changed from the base of the PR and between ace36cc and 11af719.


📒 Files selected for processing (7)
  • docs/REFERENCE.md
  • src/hook.rs
  • src/main.rs
  • src/shim.rs
  • src/text.rs
  • tests/hook_cli.rs
  • tests/shim_cli.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.



📝 Walkthrough

Walkthrough

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

Changes

Unicode headline checks

Layer / File(s) Summary
Headline-aware text judging
src/text.rs, src/main.rs
text::judged accepts labeled headlines. It checks each headline separately and includes headline and body text in literal and prose checks. Callers pass an empty headline list when they have no headlines.
Key-based hook classification
src/shim.rs, src/hook.rs, tests/hook_cli.rs, tests/shim_cli.rs, docs/REFERENCE.md
The hook treats title, commit_title, and subject fields as headlines. It splits squash_commit_message and merge_commit_message into a first-line headline and remaining prose. Tests and documentation cover these classifications and Unicode 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
Loading

Merge Risk: ⚪ Minimal · up to 11af7

No actionable issue is established that would prevent merging after normal checks.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title is specific and related to the harness hook change. It describes classifying title arguments based on the API field keys, although it does not mention the broader headline-versus-prose handl…
Docstring Coverage Passed Docstring coverage is 81.82% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 6 files. (1 skipped: 1 …
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.


✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR

🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Comment @coderabbitai help to get the list of available commands.

@codecov-commenter

codecov-commenter commented Oct 9, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.91304% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.29%. Comparing base (ace36cc) to head (11af719).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/text.rs 97.61% 1 Missing ⚠️

❌ 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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

2 participants