Skip to content

fix: correct stripBulkToolOutputs JSDoc to match AND conditions, fix test data to satisfy both thresholds#44

Merged
ZachDreamZ merged 1 commit into
mainfrom
fix/strip-bulk-jsdoc
Jul 1, 2026
Merged

fix: correct stripBulkToolOutputs JSDoc to match AND conditions, fix test data to satisfy both thresholds#44
ZachDreamZ merged 1 commit into
mainfrom
fix/strip-bulk-jsdoc

Conversation

@ZachDreamZ

@ZachDreamZ ZachDreamZ commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Fixes

  1. JSDoc correctionstripBulkToolOutputs JSDoc said "> 100 lines OR > 5000 characters" but the implementation requires both conditions (AND). Updated doc to accurately reflect the logic and explain why both checks are needed.

  2. Test data fix — 2 microCompact tests were failing because their test data didn't satisfy both truncation thresholds (content.length >= 5000 AND lines > 100). Fixed by using longer line strings that exceed the char threshold while keeping > 100 lines.

Quality Gates

  • npm test — all 343 tests pass (10/10 files)

Open in Devin Review

@ZachDreamZ
ZachDreamZ merged commit 627d62c into main Jul 1, 2026
4 of 5 checks passed

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Open in Devin Review

Comment on lines +119 to +121
const longLine = "LongLineForTestingPaddingAndTruncationCheck "; // 47 chars
const bulkOutput = Array(150).fill(longLine).join(String.fromCharCode(10));
// 150 * 47 = 7050 chars + 149 newlines ≈ 7199 chars (>= 5000 ✓, > 100 lines ✓)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Incorrect character count in test comment overstates safety margin above threshold

The test comment claims the string is 47 characters (tests/engine-coverage.test.ts:119) but it is actually 44, making the total content 6749 chars (not 7199 as the comment on line 121 states).

Impact: A developer trusting the comment's margin when modifying the test string could unknowingly bring the total below the 5000-character threshold, breaking the test.

Verified character count mismatch via Node.js evaluation

The string "LongLineForTestingPaddingAndTruncationCheck " is evaluated by node -e to be 44 characters, not 47 as stated in the comment on tests/engine-coverage.test.ts:119. The resulting total is 150 * 44 + 149 = 6749, not 7199. While the test still passes both thresholds (>= 5000 chars and > 100 lines), the stated margin of safety is overstated by ~450 characters.

Suggested change
const longLine = "LongLineForTestingPaddingAndTruncationCheck "; // 47 chars
const bulkOutput = Array(150).fill(longLine).join(String.fromCharCode(10));
// 150 * 47 = 7050 chars + 149 newlines ≈ 7199 chars (>= 5000 ✓, > 100 lines ✓)
const longLine = "LongLineForTestingPaddingAndTruncationCheck "; // 44 chars
const bulkOutput = Array(150).fill(longLine).join(String.fromCharCode(10));
// 150 * 44 = 6600 chars + 149 newlines = 6749 chars (>= 5000 ✓, > 100 lines ✓)
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +170 to +172
const longLine = "LineContentForTruncationCheckAndTokenSavingTest "; // 51 chars
const bigTool = Array(110).fill(longLine).join(String.fromCharCode(10));
// 110 * 51 = 5610 chars + 109 newlines ≈ 5719 chars (>= 5000 ✓, > 100 lines ✓)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Incorrect character count in test comment overstates safety margin above threshold

The test comment claims the string is 51 characters (tests/engine-coverage.test.ts:170) but it is actually 48, putting the real content total at only 5389 chars — much closer to the 5000-char threshold than the stated 5719.

Impact: A developer shortening the test string by as few as 4 characters (relying on the overstated margin) would silently break the test assertion.

Verified character count mismatch via Node.js evaluation

The string "LineContentForTruncationCheckAndTokenSavingTest " is 48 characters, not 51 as stated in the comment at tests/engine-coverage.test.ts:170. The actual total is 110 * 48 + 109 = 5389 chars, not 5719. This leaves only a 389-char buffer above the 5000-char threshold instead of the 719 chars the comment implies. The test works today, but the margin is overstated by nearly half.

Suggested change
const longLine = "LineContentForTruncationCheckAndTokenSavingTest "; // 51 chars
const bigTool = Array(110).fill(longLine).join(String.fromCharCode(10));
// 110 * 51 = 5610 chars + 109 newlines ≈ 5719 chars (>= 5000 ✓, > 100 lines ✓)
const longLine = "LineContentForTruncationCheckAndTokenSavingTest "; // 48 chars
const bigTool = Array(110).fill(longLine).join(String.fromCharCode(10));
// 110 * 48 = 5280 chars + 109 newlines = 5389 chars (>= 5000 ✓, > 100 lines ✓)
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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.

1 participant