Skip to content

fix: correct off-by-one error in hook template message limit enforcement (#7005)#7016

Open
vigneshakaviki wants to merge 1 commit into
docker:masterfrom
vigneshakaviki:fix/issue-7005
Open

fix: correct off-by-one error in hook template message limit enforcement (#7005)#7016
vigneshakaviki wants to merge 1 commit into
docker:masterfrom
vigneshakaviki:fix/issue-7005

Conversation

@vigneshakaviki
Copy link
Copy Markdown

Problem

The hook template message limit is incorrectly enforced by checking newline count instead of actual message count. This allows templates with 11 messages to bypass the 10-message limit.

Root Cause

Current implementation:

if n := strings.Count(out, "\n"); n > maxMessages {
    return nil, fmt.Errorf(...)
}
return strings.SplitN(out, "\n", maxMessages), nil

The check counts newlines, but:

  • 10 newlines = 11 messages (lines)
  • Condition 10 > 10 is false, so 11-message output passes
  • SplitN(..., 10) then merges the 11th message into element 10 as embedded newline

Impact

The "maximum 10 messages" guard can be bypassed by one extra rendered line, allowing hook outputs larger than intended.

Solution

Replace newline counting with actual message splitting:

  • Split output first
  • Check length of resulting slice
  • Return the split messages directly

This correctly enforces the limit on the actual number of messages.

Testing

Added comprehensive test cases:

  • Exactly 10 messages ✓
  • 10 lines with trailing newline ✓
  • 11 lines (off-by-one case) - now correctly rejected ✓
  • 12+ lines - correctly rejected ✓
  • Single message - correctly accepted ✓

Closes #7005

…ent (issue docker#7005)

The previous implementation enforced the maximum message limit by counting
newline characters (`strings.Count(out, "\n") > maxMessages`), which is
off-by-one: N newlines equals N+1 lines/messages.

This allowed templates with 11 messages (10 newlines) to pass validation,
and the 11th message would be silently merged into the 10th element as an
embedded newline.

Fix: Instead of counting newlines, split the output and count the resulting
messages. This ensures the limit is correctly enforced on the actual number
of messages, not just on newlines.

Changes:
- Replace newline counting with actual message splitting
- Updated error message to report actual message count
- Added comprehensive test cases including the off-by-one scenario

Fixes docker#7005

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
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.

cli-plugins/hooks: max-message enforcement is off by one

2 participants